fix dim() (embedding dimensions) method for qdrant cloud dense encoders#3079
Open
fix dim() (embedding dimensions) method for qdrant cloud dense encoders#3079
Conversation
* unify key generation for point ids * fix tests * adding platform to vector key * fix tests * fixing other methods requiring point key * fix point key * fixing test * account for platform=None
* adding sparse encoder util * adding sparse encoder setting * add sparse enc * adding sparse hash encoder * adding scikit-learn * fix sparse encoder * fix topic embedding' * fix default vectorizer name * adding cloud inference capability * adding openai api key to options dict * fix limits * docstring updates * adding test * some optimizations * fixing limit for prefetch queries * hide hybrid search behind posthog feature flag * scale prefetch with offset * fix yield return * fix sparse hash threshold calculation * switching hybrid search to be a url param * remove search params from groupby * adding cache decorator to sparse encoder * fix test * fix test * add default encoding name * fix tests * fix stop_words param * adding test for hybrid flag and group_by * pinning tokenizer to None for tests * fix sparse embedding when searching
for more information, see https://pre-commit.ci
shanbady
changed the title
for more information, see https://pre-commit.ci
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a runtime error when using the Qdrant Cloud encoder with dense embedding models by making dim() return the correct embedding vector size (needed for Qdrant collection configuration).
Changes:
- Update
QdrantCloudEncoderto compute embedding dimensions vialitellm.get_model_info(...). - Adjust tiktoken model lookup to use the encoder’s
model_short_name()(supports provider-prefixed model names likeopenai/...). - Minor whitespace tweak in
vector_search().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
vector_search/utils.py |
Minor formatting change (blank line) in vector_search() flow. |
vector_search/encoders/qdrant_cloud.py |
Fixes model token encoding lookup and adds a dim() implementation for Qdrant Cloud dense encoders. |
Comments suppressed due to low confidence (1)
vector_search/encoders/qdrant_cloud.py:55
- Add a unit test for
QdrantCloudEncoder.dim()that mockslitellm.get_model_infoand verifies it returns the expected embedding dimension (especially for provider-prefixed model names likeopenai/text-embedding-3-small). This will prevent regressions in collection creation whereencoder_dense.dim()is required.
def dim(self):
"""
Return the dimension of the embeddings
"""
info = litellm.get_model_info(self.model_short_name())
return info["output_vector_size"]
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mbertrand
approved these changes
Mar 27, 2026
Comment on lines
+52
to
+79
| def dim(self): | ||
| """ | ||
| Return the dimension of the embeddings | ||
| """ | ||
| info = litellm.get_model_info(self.model_short_name()) | ||
| if not isinstance(info, dict): | ||
| msg = ( | ||
| f"Could not determine embedding dimension: litellm.get_model_info(" | ||
| f"{self.model_short_name()!r}) returned {type(info).__name__}, " | ||
| "expected a dict with an 'output_vector_size' field." | ||
| ) | ||
| raise TypeError(msg) | ||
| if "output_vector_size" not in info: | ||
| msg = ( | ||
| "Could not determine embedding dimension: 'output_vector_size' " | ||
| f"missing from litellm.get_model_info({self.model_short_name()!r}) " | ||
| "response." | ||
| ) | ||
| raise ValueError(msg) | ||
| dim = info["output_vector_size"] | ||
| if not isinstance(dim, int): | ||
| msg = ( | ||
| "Could not determine embedding dimension: 'output_vector_size' " | ||
| f"from litellm.get_model_info({self.model_short_name()!r}) is of " | ||
| f"type {type(dim).__name__}, expected int." | ||
| ) | ||
| raise TypeError(msg) | ||
| return dim |
Member
There was a problem hiding this comment.
Would be good to have a parametrized unit test for this function but otherwise everything works, LGTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/10629
Description (What does it do?)
This PR resolves a bug that happens when attempting to use the new qdrant cloud encoder with a dense model
How can this be tested?
Additional Context
We have the option to use the cloud encoder for openai embeddings once we are migrated but will keep the legacy encoder for now until we have a chance to fully test it (seems to be working without issues afaik)