-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Python: Introducing Keyword Hybrid Search and Lambda Filters for Azure AI Search #11482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
e4baa12
to
9e4b565
Compare
Python Unit Test Overview
|
590746f
to
bb5d361
Compare
a1924ac
to
b0f60ae
Compare
python/semantic_kernel/connectors/memory/azure_ai_search/azure_ai_search_collection.py
Outdated
Show resolved
Hide resolved
If I understand correctly, there aren't any integration tests for this, right? I'm seeing what looks like unit tests only for now? |
@roji indeed int tests still to be done, made a note |
a8fc179
to
32aa44c
Compare
b201298
to
0b08684
Compare
3548400
to
0aa68d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for keyword hybrid search and adds lambda filter functionality across multiple memory connectors while updating filter property names. Key changes include:
- Adding support for Callable filters (lambda filters) in methods such as _build_filter with corresponding parameter type updates.
- Renaming data field properties from is_filterable/is_full_text_searchable to is_indexed/is_full_text_indexed in connectors and samples.
- Removing obsolete Azure AI Search connector files to consolidate functionality.
Reviewed Changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
python/semantic_kernel/connectors/memory/pinecone/_pinecone.py | Updated filter handling with lambda support and renaming of filter properties. |
python/semantic_kernel/connectors/memory/mongodb_atlas/* | Updated field property checks and filter dictionary creation. |
python/semantic_kernel/connectors/memory/in_memory/in_memory_collection.py | Added support for Callable filters and updated error handling for get without keys. |
python/semantic_kernel/connectors/memory/chroma/chroma.py | Introduced lambda filter support and updated type imports. |
python/semantic_kernel/connectors/memory/azure_cosmos_db/* | Updated filter property usage and query building logic. |
python/semantic_kernel/connectors/memory/azure_ai_search/* | Removed obsolete Azure AI Search connector files as part of consolidation. |
python/samples/concepts/memory/* | Updated sample data model definitions and search/upsert methods to reflect rename changes. |
python/samples/concepts/chat_history/, python/samples/concepts/caching/ | Updated field attribute names for filtering and indexing. |
Comments suppressed due to low confidence (2)
python/semantic_kernel/connectors/memory/pinecone/_pinecone.py:465
- [nitpick] The variable name 'filter' shadows the built-in filter function. Consider renaming it (e.g. 'flt' or 'built_filter') to avoid potential confusion.
if options.filter and (filter := self._build_filter(options.filter)):
python/semantic_kernel/connectors/memory/azure_ai_search/utils.py:1
- The entire Azure AI Search connector has been removed. Please ensure that any dependent modules or documentation are updated accordingly and that this change is intentional.
# File removed entirely
632489d
to
4605301
Compare
else: | ||
raise VectorStoreOperationException("No lambda expression found in the filter.") | ||
|
||
def _lambda_parser(self, node: ast.AST) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be used in the future by other memory connectors? Should this live in a common module? Given the complexity of the logic, it may be beneficial to move it to its own module for test purposes.
) -> Sequence[Any] | None: | ||
if not keys: | ||
if options is not None: | ||
raise NotImplementedError("Get without keys is not yet implemented.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only raised when options
is not None and key is empty, but the error message doesn't give the full context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens in other connectors too.
Motivation and Context
Hybrid
Adds a new abstraction for keyword hybrid search
Implements this for Azure AI Search
Adds search tests for Azure AI Search
Filters
Adds support for a Callable (lambda func) as a filter in VectorSearchOptions
Implements a ast walker to go from a lambda to a string for the filter
Adds a set of test parameter that can be extended
This further does some work on the vector abstractions:
is_filterable
tois_indexed
andis_full_text_searchable
tois_full_text_indexed
: Closes Port dotnet feature: .Net: Rename IsFilterable to IsIndexed and IsFullTextSearchable to IsFullTextIndexed #11598local_embedding
from embedding field, addshas_local_embedding
to data field, this allows one to create a data model without local embedding, in the old model that required a vector field anyway, now that can be set on the data field that needs to be embedded by the service.does_collection_exists
anddelete_collection
methods to VectorStore: Closes Port dotnet feature: .Net: Add delete and exists methods for IVectorStore #11597Relates to #10561
Description
Contribution Checklist