kernel-memory icon indicating copy to clipboard operation
kernel-memory copied to clipboard

Added very first spike for NOT filtering

Open alkampfergit opened this issue 1 year ago • 2 comments

This is NOT ready to be merged, because I've not updated all the Memories, but it is a possible implementation for NOT filter as for discussion #688.

We need to discuss if the interface for memory filter is ok (I've tried to maintain binary compatibility in the case someone serialized the structure somewhere). I've added a simple function to grab all the filters that are configured, you can see how you can use the helper in Redis Memory that was the only memory I've updated in this PR (if everything is good I'll proceed in implementing in other basic providers as well.

I've also modified the configuration for Redis test adding an optional setting that allows using Azure Openai for testing. Actually it is more common for organizations to have Azure OpenAI than Direct OpenAI

alkampfergit avatar Jul 09 '24 07:07 alkampfergit

@dluc I've implemented for almost all provider, except elasticsearch. The elasticsesarch provider has some test that fails outside the scope of this PR, but it uses an approac to save tag into nested object where, if we have tags with multiple values, like multiple users, it saves multiple nested object with the same name but different value. This makes quite difficult to generate the query, I'll need probably more work.

In the meanwhile you can validate the overall PR, after all elasticsearch is the only provider missing up to now.

alkampfergit avatar Jul 15 '24 16:07 alkampfergit

@dluc if you want this PR is ready for review.

alkampfergit avatar Aug 05 '24 07:08 alkampfergit

hi @alkampfergit would you have time to merge the latest changes from main? I was trying to fix SqlServerMemory conflicts but I'm not 100% confident about my changes. Please let me know if there's anything I can do to help. This feature would be really useful, hope we can find a way to complete it.

dluc avatar Nov 19 '24 21:11 dluc

hi @alkampfergit would you have time to merge the latest changes from main? I was trying to fix SqlServerMemory conflicts but I'm not 100% confident about my changes. Please let me know if there's anything I can do to help. This feature would be really useful, hope we can find a way to complete it.

I'll retry in the weekend, I've tried this morning but I saw that the implementation of SQL memory changed, so I carefully need to understand what is really changed and replay my modifications.

I'll keep you informed.

alkampfergit avatar Nov 21 '24 08:11 alkampfergit

FYI: we've ported some of the filtering logic from KM connectors to SK connector, so we plan on refactoring KM connectors to use SK vector interfaces. SK filtering logic is still missing some feature such as AND/OR, however, soon it will support all of that, plus "NOT" rules.

Considering this PR has been stale for a while and that SK will eventually offer what's needed, we could close this, and focus on refactoring KM to leverage SK, once ready.

dluc avatar Jan 15 '25 14:01 dluc

Note: once https://github.com/microsoft/semantic-kernel/pull/10273 is merged into SK and released, we can start refactoring all storage connectors to use SK, reducing the code to maintain here and having also support for NOT.

dluc avatar Feb 11 '25 18:02 dluc

Update: SK VectorStore is almost ready and will provide this feature for free.

Thanks to LINQ expressions, we'll be able to support queries such as

(isTest eq false) and (tags/any(t: t eq 'user:4') or count gt 3) and not(tests2/any(t: t eq true))

(or similar equivalent syntax), and we'll drop the current approach based on arrays.

dluc avatar Mar 22 '25 22:03 dluc