-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Issue #17219] MilvusVectorStore Parse "milvus_search_config" out of "vector_store_kwargs" #17221
Conversation
…ilvusVectorStore.query
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.
PR makes sense to me, one question: should we go with search_config
instead of milvus_search_config
for the parameter name? This way it might be more obvious that we're overriding the value of self.search_config
at query time.
@@ -619,7 +619,7 @@ def query(self, query: VectorStoreQuery, **kwargs: Any) -> VectorStoreQueryResul | |||
filter=string_expr, | |||
limit=query.similarity_top_k, | |||
output_fields=output_fields, | |||
search_params=self.search_config, | |||
search_params=kwargs["milvus_search_config"] if "milvus_search_config" in kwargs else self.search_config, |
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.
You can do search_params=kwargs.get("milvus_search_config", self.search_config)
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.
I haven't looked at other vector db's and how they might handle (or whether they even support) these search parameters Since this was an adjustment only for the MilvusVectorStore I figured it would be in line with the "milvus_scalar_filters" "vector_store_kwargs" key to differentiate it as specifically Milvus.
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.
kwargs.get("milvus_search_config", self.search_config)
You are right. I changed it to this to follow the pattern used in the rest of the class.
Description
Currently, the MilvusVectorStore saves "search_config" used by ALL queries during 'init' and we cannot change it at query runtime. This change allows us to override this default init configuration for uses where we need a different search criteria, like in "range queries". We already have a **kwargs parameter in the "query" function parameters we use to pass milvus specific configuration like the "milvus_scalar_filters", and we could use this to pass in a "milvus_search_config".
Fixes # (issue)
17209
Type of Change
How Has This Been Tested?
Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.
Suggested Checklist: