Vector Search: hybrid search#3060
Conversation
OpenAPI ChangesShow/hide 2 changes: 0 error, 0 warning, 2 infoUnexpected changes? Ensure your branch is up-to-date with |
| """ | ||
| Return the sparse encoder based on settings | ||
| """ | ||
| Encoder = import_string(settings.QDRANT_SPARSE_ENCODER) |
There was a problem hiding this comment.
what does import_string do here?
There was a problem hiding this comment.
settings.QDRANT_SPARSE_ENCODER specifies the encoder class to instantiate (this is also how the dense encoder works)
There was a problem hiding this comment.
oh i get it! That makes sense
| collection_name=search_collection, | ||
| count_filter=search_filter, | ||
| exact=True, | ||
| exact=False, |
There was a problem hiding this comment.
Will this make the counts incorrect for non hybrid queries? Will that be a problem for paging?
There was a problem hiding this comment.
It could be an issue for collections with very large number of points (the contentfile chunks collection) however I don't that is not a collection i expect we will ever need to accurately paginate through (or rely on this count number). The performance tradeoff in having it inexact I think outweighs the need for an accurate count of chunks
|
|
||
|
|
||
| def vector_search( | ||
| def vector_search( # noqa: PLR0913 |
There was a problem hiding this comment.
Can you add a test for vector_search for hybrid_search=True or just group_by= null in general?
Also, this does not need to be addressed in this pr but vector_search shouldn't be in utils and should be in it's own file since it's only called by the views and is not a utility function
abeglova
left a comment
There was a problem hiding this comment.
Works great locally. I'm having trouble joining the group Tobias set up to test the qdrant cloud encoder
| try: | ||
| self.token_encoding_name = tiktoken.encoding_name_for_model(model_name) | ||
| except KeyError: | ||
| msg = f"Model {model_name} not found in tiktoken. defaulting to None" | ||
| log.warning(msg) | ||
|
|
There was a problem hiding this comment.
Bug: In QdrantCloudEncoder.__init__, a KeyError exception logs a fallback to None but doesn't assign self.token_encoding_name, leading to a potential AttributeError.
Severity: MEDIUM
Suggested Fix
In the except KeyError block of the QdrantCloudEncoder.__init__ method, add the line self.token_encoding_name = None after the log warning. This will ensure the attribute is set as intended by the log message and prevent subsequent AttributeError exceptions.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: vector_search/encoders/qdrant_cloud.py#L21-L26
Potential issue: In the `QdrantCloudEncoder.__init__` method, if
`tiktoken.encoding_name_for_model(model_name)` raises a `KeyError`, the `except` block
logs a warning message stating it is "defaulting to None" but fails to actually assign
`self.token_encoding_name = None`. Unlike `LiteLLMEncoder`, which has a class-level
fallback, `QdrantCloudEncoder` has no such safety net. Consequently, any downstream code
attempting to access the `token_encoding_name` attribute on the instance will trigger an
`AttributeError`, causing a runtime crash.
| else: | ||
| # fallback to dense only search | ||
| search_params["using"] = encoder_dense.model_short_name() | ||
| search_params["query"] = encoder_dense.embed_query(query_string) | ||
|
|
||
| if "group_by" in params: | ||
| search_params.pop("search_params", None) | ||
| search_params["group_by"] = params.get("group_by") | ||
| search_params["group_size"] = params.get("group_size", 1) | ||
| group_result = client.query_points_groups(**search_params) |
There was a problem hiding this comment.
Bug: The vector_search function incorrectly passes with_payload and with_vectors parameters to client.query_points_groups() during grouped searches, which will cause a TypeError.
Severity: HIGH
Suggested Fix
Before calling client.query_points_groups within the if "group_by" in params: block, remove the with_payload and with_vectors keys from the search_params dictionary, similar to how search_params is removed. Add search_params.pop("with_payload", None) and search_params.pop("with_vectors", None).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: vector_search/utils.py#L929-L938
Potential issue: In the `vector_search` function, when a `group_by` parameter is
present, the code correctly removes the `search_params` key before calling
`client.query_points_groups`. However, it fails to also remove the `with_payload` and
`with_vectors` keys from the parameters dictionary. These keys are not intended for
grouped queries in this context. Passing these extraneous keyword arguments will cause
`client.query_points_groups` to raise a `TypeError` at runtime, causing grouped searches
to fail.
|
works now! |
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/10380
Description (What does it do?)
This PR integrates and enables the following:
How can this be tested?
testing local hybrid search
./manage.py generate_embeddings --alltesting deployed/clound inferenced hybrid search
./manage.py generate_embeddings --allAdditional Context