new: support multimodal embeddings#908
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for a new embedding model type, In the model embedder, batching logic is improved to distinguish between text and image data for multimodal models, and model validation is streamlined. Explicit error handling is added for unsupported inference object usage with the new multimodal models. The common fastembed module now includes the new model type and its supported models dictionary. Testing is expanded with a new fixture and test function to mock and verify multimodal embedding behavior, including batching, vector shapes, and embedding values. Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pyproject.toml (1)
30-30: Consider pinning the Git dependency to a specific commit or tag.
Relying on the HEAD of a repository can introduce unpredictability if changes occur upstream. Pinning ensures consistent builds across environments.tests/embed_tests/test_local_inference.py (2)
1719-1722: Consider suppressing or replacing the unused import.
Static analysis flagsimport fastembedas unused. You can either suppress it (e.g.,# noqa: F401) or useimportlib.util.find_spec('fastembed')for availability checks and skip.🧰 Tools
🪛 Ruff (0.8.2)
1722-1722:
fastembedimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
1726-1753: Mock class is well-structured.
The counters for text and image calls facilitate easy assertions. If reused in multiple tests, consider resetting them in a setup to avoid cross-test pollution.qdrant_client/embed/embedder.py (1)
158-190: Constructor follows established pattern.
get_or_init_late_interaction_multimodal_modelmirrors existingget_or_init_*methods. Consider supporting thedeprecatedflag if needed for future or backward compatibility.qdrant_client/embed/model_embedder.py (2)
314-317: Improve type checking comment clarity.The comment about IDE type checker behavior could be more clear.
- if not isinstance( - data, get_args(INFERENCE_OBJECT_TYPES) - ): # ide type checker ignores `not` and scolds - return data # type: ignore + # Some IDEs incorrectly flag type issues with negated isinstance checks + if not isinstance(data, get_args(INFERENCE_OBJECT_TYPES)): + return data # type: ignore
384-390: Ensure proper error handling for multimodal inference objects.The code correctly validates supported models and raises an appropriate error for unsupported
InferenceObjectusage with multimodal models. However, consider adding more descriptive error messages to guide users.- raise ValueError(f"{model_name} does not support `InferenceObject` interface") + raise ValueError( + f"{model_name} does not support `InferenceObject` interface. " + "Use Document or Image objects directly for multimodal embeddings." + )Also applies to: 428-438
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
pyproject.toml(1 hunks)qdrant_client/embed/embedder.py(5 hunks)qdrant_client/embed/model_embedder.py(7 hunks)qdrant_client/fastembed_common.py(2 hunks)tests/embed_tests/test_local_inference.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/embed_tests/test_local_inference.py
1722-1722: fastembed imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
- GitHub Check: Python 3.13 test
🔇 Additional comments (14)
qdrant_client/fastembed_common.py (3)
14-14: Confirm the availability ofLateInteractionMultimodalEmbedding.
The import is correct, but please verify that the newly switched Git-based dependency reliably provides this attribute.
23-23: Fallback assignment looks good.
SettingLateInteractionMultimodalEmbedding = Nonefor the import failure scenario aligns with how other classes are handled in the except block.
81-88: Dictionary declaration is consistent with other embedding model dictionaries.
The_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSstructure matches the existing pattern and should remain maintainable.tests/embed_tests/test_local_inference.py (3)
6-6: Direct embedder import is straightforward.
The explicit import fromqdrant_client.embed.embedderis clear and avoids any confusion about which functions are being tested.
1708-1717: Batch size checks appear consistent.
Ensuring thatparam_storage["batch_size"]matchesinference_batch_sizeafter each embedding call confirms correct option propagation. No issues found.
1755-1818: Test coverage for multimodal embedding is effective.
The assertions for text and image calls, as well as final scroll checks, confirm that embeddings are applied and stored properly. This test is thorough for verifying the new multimodal logic.qdrant_client/embed/embedder.py (5)
11-11: Import statement is consistent with other imports.
IncludingLateInteractionMultimodalEmbeddingmaintains parity with the newly supported multimodal embedding type.
20-20: Importing_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSis logical.
This keeps the embedder aligned with the newly declared dictionary infastembed_common.py.
45-47: New dictionary for multimodal embedding models is consistent.
Usingdefaultdict(list)mirrors the approach taken for other embedding model caches, helping to keep the embedder interface uniform.
236-265: Multimodal check inembedis well-structured.
Raising an error if both texts and images are provided prevents conflicting usage, and the new branch is integrated cleanly among other embedding checks.
266-375: Implementation of_embed_late_interaction_multimodalis robust.
It correctly enforces exclusive text-versus-image input. Additionally, reusing the shared pattern for model retrieval maintains code consistency.qdrant_client/embed/model_embedder.py (3)
13-13: LGTM! Import changes align with multimodal support.The addition of
NumericVectorStructand_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSimports support the new multimodal embedding functionality.Also applies to: 21-21
283-296: Return type change improves type safety.The return type change from
models.VectorStructtoNumericVectorStructprovides better type safety and aligns with the actual return values in the implementation.
334-396:Details
❓ Verification inconclusive
Verify the handling of mixed text and image inputs.
The embed method has been updated to handle both text and image inputs by tracking their types separately. However, we should verify that this works correctly with mixed inputs.
🏁 Script executed:
#!/bin/bash # Search for test cases that verify mixed text/image input handling rg -A 5 "def test.*mixed.*input" tests/ ast-grep --pattern 'def test_$_($_) { $$$ texts = $$$ images = $$$ $$$ }'Length of output: 125
Mixed Input Handling Requires Manual Verification
The updated embed method distinguishes between text and image inputs by grouping them based on type. However, our automated search did not yield any test cases specifically verifying mixed input handling. Please manually verify that the logic preserves the ordering and correctly processes a batch with both text and image objects. If possible, consider adding test cases to cover this mixed scenario.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.toml(1 hunks)qdrant_client/embed/embedder.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code Definitions (1)
qdrant_client/embed/embedder.py (2)
qdrant_client/embed/model_embedder.py (1)
embed(334-381)tests/embed_tests/test_local_inference.py (2)
embed_text(1737-1740)embed_image(1742-1745)
🪛 GitHub Actions: type-checkers
qdrant_client/embed/embedder.py
[error] 258-258: Argument 1 to '_embed_dense_image' of 'Embedder' has incompatible type 'list[Any] | None'; expected 'list[Any]'
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
🔇 Additional comments (8)
qdrant_client/embed/embedder.py (8)
10-11: LGTM on the new imports.The code correctly imports the new
LateInteractionMultimodalEmbeddingclass and the_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSconstant needed for the multimodal embedding support.Also applies to: 20-20
45-47: LGTM on the new instance variable.The new dictionary to store instances of multimodal embedding models follows the same pattern as other model dictionaries in the class, maintaining consistency.
158-190: LGTM on the new model initialization method.The
get_or_init_late_interaction_multimodal_modelmethod follows the same pattern as other model initialization methods in the class, maintaining consistency and reusing the same approach for model caching and initialization.
268-294: LGTM on the extracted text embedding method.The
_embed_dense_textmethod extracts the dense text embedding logic from the mainembedmethod, improving code organization and readability.
295-324: LGTM on the extracted sparse text embedding method.The
_embed_sparse_textmethod extracts the sparse text embedding logic from the mainembedmethod, improving code organization and readability.
326-346: LGTM on the extracted late interaction text embedding method.The
_embed_late_interaction_textmethod extracts the late interaction text embedding logic from the mainembedmethod, improving code organization.
348-376: LGTM on the new multimodal embedding method.The
_embed_late_interaction_multimodalmethod correctly handles the new multimodal embedding functionality, following the same pattern as other embedding methods. It properly checks that only one input type (text or images) is provided.
378-390: LGTM on the extracted image embedding method.The
_embed_dense_imagemethod extracts the image embedding logic from the mainembedmethod, improving code organization.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qdrant_client/embed/embedder.py(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
qdrant_client/embed/embedder.py (4)
qdrant_client/embed/model_embedder.py (1)
embed(334-381)qdrant_client/async_qdrant_fastembed.py (1)
query(580-654)qdrant_client/qdrant_fastembed.py (1)
query(626-710)tests/embed_tests/test_local_inference.py (2)
embed_text(1737-1740)embed_image(1742-1745)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13 test
🔇 Additional comments (6)
qdrant_client/embed/embedder.py (6)
11-11: Good addition of the new embedding typesThe imports have been correctly expanded to include the new
LateInteractionMultimodalEmbeddingclass and the_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSconstant. This enables the multimodal embedding functionality.Also applies to: 20-20
45-47: Consistent implementation of model dictionary for multimodal embeddingsThe instance variable for storing multimodal embedding models follows the same pattern as the other model dictionaries in the class, which is good for consistency and maintainability.
158-190: Well-structured initialization method for multimodal embeddingsThe
get_or_init_late_interaction_multimodal_modelmethod correctly implements the model initialization pattern consistent with other similar methods in the class. The validation of supported models and caching mechanism is properly implemented.
236-268: Improved code organization with specialized embedding methodsBreaking down the embedding logic into specialized private methods improves code organization and readability. The branching logic for different model types is clear and well-structured.
The assertion on line 258 correctly ensures type safety for the
imagesparameter, addressing the issue that was previously flagged in the pipeline.
381-393: Good implementation of image embedding methodThe
_embed_dense_imagemethod is well-implemented and follows the same pattern as the other embedding methods, maintaining consistency throughout the codebase.
271-349: Excellent refactoring of embedding methodsThe extraction of embedding logic into separate methods for each embedding type is a significant improvement. This approach:
- Makes the code more modular and easier to maintain
- Improves readability by grouping related functionality
- Facilitates testing each embedding type in isolation
- Simplifies future extensions to support additional embedding types
This is a great example of the Single Responsibility Principle in action.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
qdrant_client/embed/embedder.py (1)
257-259: Good mypy type checking assistanceThe assertion helps the static type checker understand that images is definitely not None in this code path, preventing false positive errors.
Consider adding an explanatory message to the assertion for better debugging:
- assert ( - images is not None - ) # just to satisfy mypy which can't infer it from the previous conditions + assert images is not None, "Images must be provided at this point - this assertion helps mypy"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qdrant_client/embed/embedder.py(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
qdrant_client/embed/embedder.py (2)
qdrant_client/http/models/models.py (1)
SparseVector(2580-2586)tests/embed_tests/test_local_inference.py (2)
embed_text(1737-1740)embed_image(1742-1745)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13 test
🔇 Additional comments (7)
qdrant_client/embed/embedder.py (7)
11-11: Well-organized import additions for multimodal supportThe imports for
LateInteractionMultimodalEmbeddingand_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSare correctly added to support the new multimodal embedding functionality.Also applies to: 20-20
45-47: Good implementation of multimodal embeddings storageThe dictionary for storing multimodal embedding models follows the same pattern as the other model dictionaries, maintaining consistency throughout the codebase.
158-191: LGTM: get_or_init_late_interaction_multimodal_model implementationThe implementation follows the established pattern of other model initialization methods in the class:
- Validates the model name against supported models
- Creates an options dictionary with sensible defaults
- Checks for existing model instances before creating new ones
- Properly stores and returns the model instance
This approach maintains consistency and prevents duplicate model instantiation.
233-269: Excellent refactoring of the embed methodThe embed method has been significantly improved by:
- Using a clear validation check at the beginning
- Breaking down embedding logic into specialized private methods
- Adding proper type hints and assertions for static type checking
- Extending support for multimodal embeddings
The code now better handles the different embedding scenarios while maintaining clean separation of concerns.
351-367: Appropriately implemented multimodal text embedding methodThe implementation properly handles embedding text inputs for multimodal models by:
- Getting or initializing the appropriate model
- Using a specialized
embed_textmethod rather than the genericembed- Correctly converting the embeddings to the expected format
368-382: Appropriately implemented multimodal image embedding methodThe implementation correctly handles embedding image inputs for multimodal models by:
- Getting or initializing the appropriate model
- Using a specialized
embed_imagemethod- Properly converting the embeddings to the expected format
271-297: Well-structured private helper methodsThe refactoring into specialized private helper methods is excellent and follows good software engineering practices:
- Each method has a single responsibility
- Consistent patterns are maintained across all methods
- Type annotations are properly used
- The code is more maintainable and easier to understand
This approach will make it easier to add more embedding types in the future.
Also applies to: 298-328, 329-350, 383-395
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/embed_tests/test_local_inference.py (3)
1722-1725: Consider using importlib to check for the availability of fastembed.The static analyzer flagged that
fastembedis imported but not used directly in the code. Consider usingimportlib.util.find_specfor a cleaner way to check if a module is available.- try: - import fastembed - except ImportError: - pytest.skip("FastEmbed is not installed, skipping") + import importlib.util + if importlib.util.find_spec("fastembed") is None: + pytest.skip("FastEmbed is not installed, skipping")🧰 Tools
🪛 Ruff (0.8.2)
1722-1722:
fastembedimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
1726-1746: Good mock design with tracking counters.The mock implementation properly tracks call counts and provides predefined embeddings. However, using class variables for tracking calls could potentially lead to issues if multiple tests use this fixture and run in parallel or if tests depend on the order of execution.
class LateInteractionMultimodalEmbeddingMock: - text_calls = 0 - image_calls = 0 num_image_columns = 10 num_text_columns = 20 text_embedding = np.array([[0.1, 0.2, 0.3]] * num_text_columns) image_embedding = np.array([[0.1, 0.2, 0.3]] * num_image_columns) def __init__(self, *args, **kwargs): - pass + self.text_calls = 0 + self.image_calls = 0 def embed_text(self, documents, *args, **kwargs): - LateInteractionMultimodalEmbeddingMock.text_calls += 1 + self.text_calls += 1 for _ in documents: yield self.text_embedding def embed_image(self, images, *args, **kwargs): - LateInteractionMultimodalEmbeddingMock.image_calls += 1 + self.image_calls += 1 for _ in images: yield self.image_embedding
1755-1817: Well-structured test for multimodal embedding.The test thoroughly validates the multimodal embedding functionality by checking:
- Batching behavior (via call counts)
- Result shapes and dimensions
- Expected embedding values
The vectors configuration with
multivector_configmatches the expected multimodal embedding architecture. However, the test assertions currently rely on class variables, which should be adjusted if you change to instance variables as suggested above.If you change to instance variables in the mock:
- assert mock_cls.text_calls == 1 # assert that text records were assembled into batches - assert mock_cls.image_calls == 1 + # Get the mock instance from the embedder + mock_instance = local_client._model_embedder.embedder.late_interaction_embedding_models["Qdrant/colpali-v1.3-fp16"][0] + assert mock_instance.text_calls == 1 # assert that text records were assembled into batches + assert mock_instance.image_calls == 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
pyproject.toml(1 hunks)qdrant_client/embed/embedder.py(4 hunks)qdrant_client/embed/model_embedder.py(7 hunks)qdrant_client/fastembed_common.py(2 hunks)tests/embed_tests/test_local_inference.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pyproject.toml
- qdrant_client/fastembed_common.py
- qdrant_client/embed/embedder.py
- qdrant_client/embed/model_embedder.py
🧰 Additional context used
🧬 Code Definitions (1)
tests/embed_tests/test_local_inference.py (12)
qdrant_client/embed/model_embedder.py (1)
embed(334-381)qdrant_client/embed/embedder.py (1)
embed(225-270)tests/congruence_tests/test_discovery.py (1)
local_client(38-42)tests/congruence_tests/test_facet.py (1)
local_client(35-38)tests/congruence_tests/test_search_distance_matrix.py (1)
local_client(27-30)tests/congruence_tests/test_sparse_discovery.py (1)
local_client(37-48)qdrant_client/local/qdrant_local.py (3)
upsert(773-778)create_collection(988-1039)scroll(730-749)qdrant_client/local/local_collection.py (2)
upsert(2292-2328)scroll(1870-1904)qdrant_client/async_qdrant_client.py (3)
upsert(1513-1583)create_collection(2232-2319)scroll(1354-1421)qdrant_client/qdrant_remote.py (3)
upsert(1840-1918)create_collection(2703-2822)scroll(1648-1736)qdrant_client/qdrant_client.py (4)
upsert(1561-1638)QdrantClient(28-3163)create_collection(2311-2399)scroll(1399-1467)qdrant_client/async_qdrant_remote.py (3)
upsert(1616-1685)create_collection(2446-2548)scroll(1443-1519)
🪛 Ruff (0.8.2)
tests/embed_tests/test_local_inference.py
1722-1722: fastembed imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
- GitHub Check: Python 3.13 test
🔇 Additional comments (2)
tests/embed_tests/test_local_inference.py (2)
1718-1753: LGTM! Well-structured fixture for mocking multimodal embeddings.The fixture properly mocks the
LateInteractionMultimodalEmbeddingclass with tracking capabilities for both text and image embeddings, and correctly restores the original class after the test. The mock implementation provides fixed embedding outputs which enables deterministic testing.🧰 Tools
🪛 Ruff (0.8.2)
1722-1722:
fastembedimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
1708-1708: Code formatting change to single-line upsert.This is a minor formatting change that consolidates the upsert method call into a single line, which improves readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/embed_tests/test_local_inference.py (3)
1722-1725: Consider using importlib.util.find_spec for package availability checkThe static analysis tool flagged the
fastembedimport as unused. While your approach works, usingimportlib.util.find_specis the recommended way to check for package availability.- try: - import fastembed - except ImportError: - pytest.skip("FastEmbed is not installed, skipping") + from importlib.util import find_spec + if find_spec("fastembed") is None: + pytest.skip("FastEmbed is not installed, skipping")🧰 Tools
🪛 Ruff (0.8.2)
1722-1722:
fastembedimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
1726-1746: Consider resetting class variables between testsThe mock class uses class variables to track call counts, which could lead to test failures if multiple tests run sequentially or in parallel. Consider adding a reset method or using instance variables instead.
class LateInteractionMultimodalEmbeddingMock: - text_calls = 0 - image_calls = 0 num_image_columns = 10 num_text_columns = 20 text_embedding = np.array([[0.1, 0.2, 0.3]] * num_text_columns) image_embedding = np.array([[0.1, 0.2, 0.3]] * num_image_columns) def __init__(self, *args, **kwargs): - pass + self.text_calls = 0 + self.image_calls = 0 def embed_text(self, documents, *args, **kwargs): - LateInteractionMultimodalEmbeddingMock.text_calls += 1 + self.text_calls += 1 for _ in documents: yield self.text_embedding def embed_image(self, images, *args, **kwargs): - LateInteractionMultimodalEmbeddingMock.image_calls += 1 + self.image_calls += 1 for _ in images: yield self.image_embedding
1801-1802: Update assertion to handle instance variablesIf you implement the suggested change to use instance variables in the mock, you'll need to update these assertions as well.
- assert mock_cls.text_calls == 1 # assert that text records were assembled into batches - assert mock_cls.image_calls == 1 + # Get the instance used inside the embedder + mock_instance = local_client._model_embedder.embedder.late_interaction_multimodal_embedding_models["Qdrant/colpali-v1.3-fp16"][0].model + assert mock_instance.text_calls == 1 # assert that text records were assembled into batches + assert mock_instance.image_calls == 1qdrant_client/embed/embedder.py (2)
258-261: Consider simplifying the assertion for type checkingThe assertion is used solely for type checking purposes. Consider using a simpler check or relying on the conditional logic.
- assert ( - images is not None - ) # just to satisfy mypy which can't infer it from the previous conditions + # At this point, images must be not None due to the previous conditions
352-383: Consider adding is_query parameter for consistencyUnlike the other embedding methods, the multimodal methods don't have an
is_queryparameter. If the multimodal embedding doesn't need different behavior for queries vs. documents, it would be good to add a comment explaining this.def _embed_late_interaction_multimodal_text( self, texts: list[str], model_name: str, options: Optional[dict[str, Any]], batch_size: int, + is_query: bool = False, # Unused, but added for API consistency ) -> list[list[list[float]]]: + # Multimodal embeddings don't have different behavior for queries embedding_model_inst = self.get_or_init_late_interaction_multimodal_model( model_name=model_name, **options or {} ) return [ embedding.tolist() for embedding in embedding_model_inst.embed_text( documents=texts, batch_size=batch_size ) ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
pyproject.toml(1 hunks)qdrant_client/embed/embedder.py(4 hunks)qdrant_client/embed/model_embedder.py(7 hunks)qdrant_client/fastembed_common.py(2 hunks)tests/embed_tests/test_local_inference.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pyproject.toml
- qdrant_client/fastembed_common.py
- qdrant_client/embed/model_embedder.py
🧰 Additional context used
🧬 Code Definitions (1)
tests/embed_tests/test_local_inference.py (12)
qdrant_client/embed/model_embedder.py (1)
embed(334-382)qdrant_client/embed/embedder.py (1)
embed(225-270)tests/congruence_tests/test_discovery.py (1)
local_client(38-42)tests/congruence_tests/test_facet.py (1)
local_client(35-38)tests/congruence_tests/test_search_distance_matrix.py (1)
local_client(27-30)tests/congruence_tests/test_sparse_discovery.py (1)
local_client(37-48)qdrant_client/local/qdrant_local.py (3)
upsert(773-778)create_collection(988-1039)scroll(730-749)qdrant_client/local/local_collection.py (2)
upsert(2292-2328)scroll(1870-1904)qdrant_client/async_qdrant_client.py (3)
upsert(1513-1583)create_collection(2232-2319)scroll(1354-1421)qdrant_client/async_qdrant_remote.py (3)
upsert(1616-1685)create_collection(2446-2548)scroll(1443-1519)qdrant_client/qdrant_client.py (4)
upsert(1561-1638)QdrantClient(28-3163)create_collection(2311-2399)scroll(1399-1467)qdrant_client/qdrant_remote.py (3)
upsert(1840-1918)create_collection(2703-2822)scroll(1648-1736)
🪛 Ruff (0.8.2)
tests/embed_tests/test_local_inference.py
1722-1722: fastembed imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
- GitHub Check: Python 3.13 test
🔇 Additional comments (7)
tests/embed_tests/test_local_inference.py (3)
1719-1752: Good test fixture implementation for multimodal embeddingThe mock class properly simulates the behavior of the
LateInteractionMultimodalEmbeddingclass by tracking calls to text and image embedding methods and returning consistent embeddings.🧰 Tools
🪛 Ruff (0.8.2)
1722-1722:
fastembedimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
1755-1817: Comprehensive test for multimodal embedding functionalityThe test effectively validates the integration of multimodal embedding by:
- Creating points with both text and image vectors
- Verifying embedding method calls
- Checking embedding shapes and values
- Testing with points containing only text vectors
This provides good coverage for the new functionality.
1708-1708: LGTM - Minor method call format fixThe simplified method call format improves readability.
qdrant_client/embed/embedder.py (4)
46-48: Appropriate addition of multimodal embedding model storageThe dictionary for storing multimodal embedding model instances follows the same pattern as the other model types, maintaining consistency in the codebase.
159-192: Well-implemented model initialization methodThe
get_or_init_late_interaction_multimodal_modelmethod follows the established pattern of the other model initialization methods, checking for existing instances with matching options before creating new ones, which is efficient.
237-269: Good refactoring of embedding logicThe refactoring of the
embedmethod improves code organization by separating different embedding types into specialized private methods while maintaining the same interface.
272-351: Well-structured private methods for different embedding typesThe extracted private methods for different embedding types follow a consistent pattern and preserve the original functionality, making the code more modular and easier to maintain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
qdrant_client/fastembed_common.py (1)
16-16: Remove unused_MultitaskTextEmbeddingimport
Ruff flags this import as unused. Consider removing if not needed.- from fastembed.text.multitask_embedding import JinaEmbeddingV3 as _MultitaskTextEmbedding🧰 Tools
🪛 Ruff (0.8.2)
16-16:
fastembed.text.multitask_embedding.JinaEmbeddingV3imported but unused; consider usingimportlib.util.find_specto test for availability(F401)
tests/embed_tests/test_local_inference.py (1)
1718-1752: Mock fixture forLateInteractionMultimodalEmbedding
Providing a mock class is a good approach for focused testing. However, the import offastembedin line 1721 is not otherwise referenced, prompting Ruff to flag it as unused. Also, note that the static counters (text_callsandimage_calls) might introduce concurrency issues if tests run in parallel.Consider removing or conditionally importing
fastembedusingimportlib.util.find_spec, and either resetting the static counters or using instance-level counters if concurrency becomes a concern:- import fastembed + # Optionally use importlib.util.find_spec to check for module availability if needed🧰 Tools
🪛 Ruff (0.8.2)
1721-1721:
fastembedimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
qdrant_client/embed/embedder.py (3)
157-189: Well-implemented model initialization method for multimodal embeddings.This method follows the established pattern from other model initialization methods:
- Validates the model name against supported models
- Creates a consistent options dictionary
- Checks for an existing model in cache
- Creates and caches a new model instance when needed
Consider adding a docstring to explain the purpose and behavior of this method, especially since it handles a new model type.
341-356: New method for multimodal text embedding correctly implemented.The implementation properly initializes the multimodal model and calls the appropriate
embed_textmethod. The return type annotation correctly indicates the expected structure of embeddings.This method doesn't handle the
is_queryparameter unlike the other text embedding methods. If this is by design, consider adding a comment explaining this difference.
270-385: Good separation of embedding logic into specialized methods.Breaking down the embedding logic into specialized private methods improves code organization and readability. Each method follows a consistent pattern and handles its specific embedding scenario appropriately.
Consider adding docstrings to these private methods to document their purpose, parameters, and return values. This would be especially helpful for understanding the differences between the various embedding types.
def _embed_dense_text( self, texts: list[str], model_name: str, options: Optional[dict[str, Any]], is_query: bool, batch_size: int, ) -> list[list[float]]: + """ + Embed text documents using a dense embedding model. + + Args: + texts: List of text documents to embed + model_name: Name of the embedding model to use + options: Optional model initialization options + is_query: Whether the documents are queries (for retrieval) + batch_size: Batch size for embedding + + Returns: + List of dense vector embeddings + """ embedding_model_inst = self.get_or_init_model(model_name=model_name, **options or {}) if not is_query:Similar docstrings should be added to all other private embedding methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
qdrant_client/embed/embedder.py(4 hunks)qdrant_client/embed/model_embedder.py(7 hunks)qdrant_client/fastembed_common.py(2 hunks)tests/embed_tests/test_local_inference.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
qdrant_client/embed/embedder.py (3)
qdrant_client/embed/model_embedder.py (1)
embed(334-382)qdrant_client/http/models/models.py (1)
SparseVector(2687-2693)tests/embed_tests/test_local_inference.py (2)
embed_text(1736-1739)embed_image(1741-1744)
qdrant_client/fastembed_common.py (1)
qdrant_client/http/models/models.py (1)
Distance(650-661)
🪛 Ruff (0.8.2)
qdrant_client/fastembed_common.py
16-16: fastembed.text.multitask_embedding.JinaEmbeddingV3 imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
tests/embed_tests/test_local_inference.py
1721-1721: fastembed imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
- GitHub Check: Python 3.13.x on ubuntu-latest test
- GitHub Check: Python 3.12.x on ubuntu-latest test
- GitHub Check: Python 3.11.x on ubuntu-latest test
- GitHub Check: Python 3.10.x on ubuntu-latest test
- GitHub Check: Python 3.9.x on ubuntu-latest test
🔇 Additional comments (18)
qdrant_client/fastembed_common.py (3)
14-14: Added import for LateInteractionMultimodalEmbedding
The new import enhances the file to recognize and support multimodal embedding.
23-23: Consistent fallback on import error
AssigningLateInteractionMultimodalEmbedding = Nonemirrors the existing pattern for optional imports and maintains graceful handling when the library is unavailable.
71-78: Initialization of_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELS
This dictionary correctly mirrors the pattern used by other embedding model dictionaries and appropriately defaults to{}when the class is unavailable.tests/embed_tests/test_local_inference.py (2)
6-6: New import ofqdrant_client.embed.embedder
This import is necessary for mocking and testing the new multimodal embedding behavior.
1754-1817: Newtest_embed_multimodaltest coverage
This test thoroughly validates text and image embeddings for the new multimodal embedding model. The logic to verify vector shapes and calls is appropriate and aids in confirming correct behavior and ordering.qdrant_client/embed/model_embedder.py (8)
13-13: Additional imports for numeric vector types
ImportingNumericVectorandNumericVectorStructclarifies the codebase’s type usage.
21-21: Added_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSto imports
Extends the embedding logic to handle late interaction multimodal support in the model embedder.
284-285: Return type changed toNumericVectorStruct
Switching frommodels.VectorStructtoNumericVectorStructprovides a more precise type signature and aligns with the rest of the numeric vector workflow.
314-316: Explicit check for inference object types
Guarding against non-inference objects here prevents accidental attempts to drain non-inference embeddings.
335-339: Refined batching logic and docstring
Separating text and image data in the same method is clearly explained in the docstring. The approach efficiently handles multimodal objects while preserving ordering.
386-390: Addition of_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSto supported sets
This ensures that the model embedder recognizes the new multimodal models as valid.
431-433: Extended model sets forInferenceObjectresolution
Unpacking the new model references in the conditional broadens the coverage for recognized inference objects.
437-438: Explicit error for Late Interaction Multimodal +InferenceObject
Raising an exception for unsupported scenarios avoids silent failures and communicates expected usage constraints.qdrant_client/embed/embedder.py (5)
8-19: Imports added for multimodal embedding support.The new imports for
LateInteractionMultimodalEmbeddingand_LATE_INTERACTION_MULTIMODAL_EMBEDDING_MODELSare correctly included to support the multimodal embedding functionality.
44-46: Added cache for multimodal embedding models.The new cache implementation is well-structured and follows the same pattern as the existing model caches, using the appropriate type annotations.
358-371: New method for multimodal image embedding correctly implemented.The implementation correctly handles multimodal image embedding by initializing the appropriate model and calling the
embed_imagemethod. The return value is properly formatted as expected.
256-258: Good type checking support with assert statement.The assert statement helps mypy understand that
imagescannot beNoneat this point, avoiding a type error. This shows good attention to static type checking requirements.
235-268:Details
❓ Verification inconclusive
Good refactoring of the embed method with clear delegation.
The
embedmethod has been nicely refactored to delegate to specialized private methods based on model type and input modality. This improves code organization and maintainability.Note that the
is_queryparameter isn't passed to the multimodal embedding methods. Is this intentional? If multimodal models don't support query mode, consider adding a comment to clarify this design decision.
🏁 Script executed:
#!/bin/bash # Check for query_embed or similar methods in LateInteractionMultimodalEmbedding # Look for class definition and methods ast-grep --pattern $'class LateInteractionMultimodalEmbedding { $$$ }' # Specifically look for query embedding methods ast-grep --pattern $'def query_embed($$$) { $$$ }' # Look for usages or tests of is_query with multimodal embeddings rg -A 5 "is_query.*multimodal" -g "*.py"Length of output: 170
Action Required: Verify Multimodal Query Behavior
Good refactoring overall—the delegation to specialized methods is clean. However, the
is_queryparameter isn’t forwarded when processing multimodal inputs. Our initial verification (searching for query-specific implementations in the multimodal embedding methods) did not reveal any handling ofis_query. Please verify that this omission is intentional (i.e. multimodal models don’t support query mode) and, if so, add a comment clarifying the design decision. If query mode should be supported, then the methods might need updating.
* new: support multimodal embeddings * fix: fix embed when model name is present in both text and image supported models * fix: fix type checkers * refactor: split multimodal embed into embed text and embed image * chore: comment batch separation * chore: rollback formatting
2 participants
No description provided.