Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis set of changes introduces support for Maximal Marginal Relevance (MMR) re-ranking in search queries across the codebase. New MMR-related types and schema definitions are added to both REST and gRPC interfaces, including protobuf messages and Python models. The Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/congruence_tests/test_sparse_search.py (1)
8-8: Remove unused import.The
sort_sparse_vectorimport is not used anywhere in the file.-from qdrant_client.local.sparse import sort_sparse_vectorqdrant_client/local/local_collection.py (1)
2154-2191: Consider adding validation for distance consistencyThe distance calculation logic correctly handles all vector types. However, it would be beneficial to validate that the distance metric is consistent across the calculations.
Consider adding a validation to ensure the distance parameter is properly set for sparse vectors (which always use DOT product) to avoid potential confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
qdrant_client/conversions/common_types.py(1 hunks)qdrant_client/conversions/conversion.py(3 hunks)qdrant_client/embed/_inspection_cache.py(4 hunks)qdrant_client/grpc/points_pb2.pyi(4 hunks)qdrant_client/http/models/models.py(5 hunks)qdrant_client/local/local_collection.py(6 hunks)qdrant_client/proto/points.proto(2 hunks)tests/congruence_tests/test_multivector_search_queries.py(2 hunks)tests/congruence_tests/test_query.py(2 hunks)tests/congruence_tests/test_sparse_search.py(3 hunks)tests/conversions/fixtures.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
qdrant_client/conversions/common_types.py (1)
qdrant_client/http/models/models.py (1)
Mmr(1540-1552)
qdrant_client/conversions/conversion.py (2)
qdrant_client/http/models/models.py (2)
Mmr(1540-1552)NearestQuery(1614-1619)qdrant_client/grpc/points_pb2.pyi (7)
Mmr(2702-2735)nearest_with_mmr(2774-2775)nearest(2684-2685)nearest(2752-2753)mmr(2687-2690)Query(2739-2791)NearestInputWithMmr(2678-2698)
qdrant_client/http/models/models.py (1)
qdrant_client/grpc/points_pb2.pyi (2)
Mmr(2702-2735)mmr(2687-2690)
🪛 Ruff (0.12.2)
tests/congruence_tests/test_sparse_search.py
8-8: qdrant_client.local.sparse.sort_sparse_vector imported but unused
Remove unused import: qdrant_client.local.sparse.sort_sparse_vector
(F401)
🔇 Additional comments (42)
qdrant_client/conversions/common_types.py (1)
127-127: LGTM: Clean type alias additionThe
Mmrtype alias follows the established pattern in the file and correctly referencesrest.Mmr. The placement is appropriate and consistent with the codebase structure.tests/conversions/fixtures.py (4)
533-533: LGTM: Language casing correctionThe change from "English" to "english" aligns with the lowercase convention mentioned in the AI summary for consistency across protobuf, HTTP models, and inspection cache schemas.
1474-1475: LGTM: Comprehensive MMR test fixturesGood test coverage with both parameterized (
diversity=0.6,candidates_limit=100) and default MMR instances. This ensures both explicit configuration and default behavior are tested.
1488-1493: LGTM: MMR query fixtures for testingThe
nearest_with_mmrquery fixtures properly combine vector input with MMR instances, providing comprehensive test coverage for the new query variant. Using both parameterized and default MMR instances ensures thorough testing.
1718-1719: LGTM: Fixtures properly registeredThe new MMR queries are correctly added to the "Query" fixtures list, making them available for congruence testing across different client implementations.
qdrant_client/proto/points.proto (2)
643-650: LGTM: Well-structured MMR input messageThe
NearestInputWithMmrmessage is properly defined with clear field names, appropriate types, and good documentation. The field numbering (1, 2) is correct and the structure logically combines nearest neighbor search with MMR parameters.
680-680: LGTM: Proper Query message extensionThe
nearest_with_mmrfield is correctly added to the Query message's oneof variant with appropriate tag numbering (9) and clear documentation. This properly extends the existing query system with MMR functionality.tests/congruence_tests/test_sparse_search.py (4)
144-153: LGTM! Default MMR query implementation looks correct.The method properly uses the
query_pointsAPI with a default MMR configuration and follows the existing pattern in the class.
155-164: LGTM! Parametrized MMR query implementation is well-configured.The MMR parameters are appropriate: diversity=0.3 provides a reasonable balance between relevance and diversity, and candidates_limit=30 (3x the final limit) ensures sufficient candidates for re-ranking.
166-176: LGTM! MMR query with score threshold implementation is correct.The implementation properly combines MMR parameters with score threshold filtering. The score threshold value of 3.3 appears high for typical normalized similarity scores, but it's appropriate for testing consistency between local and remote implementations with sparse vector scoring.
210-227: LGTM! Comprehensive MMR test implementation.The test function properly validates MMR functionality by:
- Using 100 fixture points for adequate diversity testing
- Comparing local and remote client results for consistency
- Testing all three MMR query variants (default, parametrized, and with score threshold)
- Following the established testing pattern in the file
qdrant_client/embed/_inspection_cache.py (4)
84-84: LGTM! Correct addition of MMR cache path entry.The addition of "Mmr" with an empty array follows the consistent pattern used for other schema definitions that don't have vector fields requiring string path tracking.
2988-3007: Well-designed MMR schema definition.The schema correctly defines the Maximal Marginal Relevance parameters:
diversity: Optional float in [0,1] range with clear documentation about balancing diversity vs relevancecandidates_limit: Optional integer for limiting candidate consideration- Proper validation constraints and default values
- Clear, comprehensive documentation explaining the algorithm's purpose
The schema follows established patterns in the codebase and provides appropriate flexibility for MMR configuration.
3284-3288: Properly integrated MMR into NearestQuery schema.The addition of the optional
mmrproperty to theNearestQueryschema is correctly implemented:
- Uses proper schema reference to the
Mmrdefinition- Maintains backward compatibility with nullable type
- Includes clear documentation explaining the MMR reranking functionality
- Follows the established pattern for optional query enhancements
This integration allows nearest neighbor queries to optionally apply MMR reranking using the same query vector.
2373-2391: SnowballLanguage Casing Change VerifiedAll occurrences of SnowballLanguage values have been updated to lowercase and are consistent across the codebase:
- qdrant_client/http/models/models.py: Enum definition uses lowercase values
- qdrant_client/embed/_inspection_cache.py: JSON schema enum lists lowercase values
- qdrant_client/conversions/conversion.py: Conversion logic passes lowercase model.language
- tests/embed_tests/test_local_inference.py & tests/conversions/test_validate_conversions.py: References to SnowballParams.language use lowercase
- No remaining
"Arabic","English", etc., found in code or testsNo breaking changes detected; the casing update is safely applied everywhere.
tests/congruence_tests/test_query.py (2)
799-883: Well-structured MMR query methods with comprehensive test coverage.The MMR query methods are well-implemented and follow the existing patterns in the class. They provide good test coverage across different vector types (text, image, code), distance metrics (cosine, dot, euclidean), and MMR configurations (default and parameterized). The score thresholds are appropriately set for different distance metrics.
1731-1760: Comprehensive MMR congruence test with proper client coverage.The test function properly validates MMR functionality across all client implementations (local, HTTP, gRPC). It systematically tests all the MMR query methods, ensuring consistent behavior across different client types. The test follows the established patterns in the file and provides thorough coverage of the MMR feature.
qdrant_client/conversions/conversion.py (4)
1307-1314: LGTM! Clean MMR conversion implementation.The method correctly follows the established pattern for handling optional gRPC fields using
HasField()and properly maps to the REST model structure.
1347-1351: LGTM! Proper MMR query variant handling.The implementation correctly handles the
nearest_with_mmrvariant by extracting both the vector input and MMR configuration, then creating the appropriate RESTNearestQuerystructure.
3590-3592: Confirm asymmetric optional field handling is intentional.The
RestToGrpc.convert_mmrmethod directly passes field values without checking forNone, whileGrpcToRest.convert_mmrusesHasField()checks. This asymmetry appears consistent with other conversion methods in the file, but please verify this is the intended pattern for MMR fields.
3597-3602: LGTM! Proper conditional MMR handling.The implementation correctly checks for MMR presence and creates the appropriate gRPC structure. The conditional logic properly handles both MMR and non-MMR cases, falling back to the standard
nearestquery when MMR is not present.qdrant_client/grpc/points_pb2.pyi (1)
2789-2792: Guard against simultaneousnearestandnearest_with_mmrusage.
nearest_with_mmrwas added to the existingvariantone-of, but client code may now accidentally set bothnearestandnearest_with_mmr.
Ensure the high-level API (builder/helpers) forbids this or overwrites the earlier value, otherwise the generated message will be invalid and raise at send-time.qdrant_client/local/local_collection.py (7)
42-43: LGTM!The imports for
calculate_distance_coreandcalculate_multi_distance_coreare correctly added to support the MMR implementation.Also applies to: 55-56
732-733: LGTM!The validation ensures that a query is provided when merging prefetch sources, preventing potential runtime errors.
772-773: LGTM!The validation and default limit handling are consistent with the overall error handling strategy.
Also applies to: 779-779
896-908: LGTM!The MMR query handling is correctly integrated into the query collection flow, maintaining backward compatibility for non-MMR queries.
2075-2107: LGTM!The MMR search wrapper correctly handles the candidate search and delegates to the MMR algorithm with appropriate parameters.
2192-2217: LGTM!The MMR selection loop correctly implements the iterative selection process, properly balancing relevance and diversity according to the MMR algorithm.
2108-2217: Fix the type check condition for query_vectorThe MMR implementation looks correct overall, but there's a potential issue with the type checking:
At line 2157, the condition checks
candidate_vectorsbut should likely checkquery_vectorsince it's being used in thecalculate_distance_corecall at line 2163.Apply this fix:
- if not isinstance(candidate_vectors, np.ndarray): - candidate_vectors = np.array(candidate_vectors) + if not isinstance(query_vector, np.ndarray): + query_vector = np.array(query_vector)Additionally, consider using a different variable name to avoid confusion when converting the candidate_vectors list to numpy array.
Likely an incorrect or invalid review comment.
qdrant_client/http/models/models.py (5)
1540-1553: LGTM: Well-structured MMR model with proper validation.The
Mmrmodel is correctly implemented with appropriate field types and comprehensive docstrings. The field constraints (diversity in [0, 1] range) are clearly documented, and the optional nature of both fields provides good flexibility.
1616-1619: LGTM: Clean integration of MMR into NearestQuery.The MMR field is properly integrated as an optional parameter with a clear description that explains its purpose for re-ranking after search. The field name and type are consistent with the newly defined
Mmrmodel.
767-793: LGTM: Enhanced FeatureFlags documentation and new migration flag.The changes improve documentation clarity by:
- Adding version information for when features were implemented/enabled
- Providing more detailed descriptions of RocksDB-related flags
- Adding a new
migrate_rocksdb_payload_indicesflag for payload index migrationThe descriptions are informative and help users understand the impact of each flag.
1685-1685: LGTM: Corrected default value in OptimizersConfig description.The description update corrects the default value from "20,000" to "10,000" kilobytes, which aligns with the stated "based on experiments and observations" rationale. This provides users with accurate information about the default indexing threshold.
2795-2813: LGTM: Standardized SnowballLanguage enum values to lowercase.The change from capitalized to lowercase enum values (e.g., "Arabic" → "arabic") follows common conventions for language identifiers and improves consistency. This aligns with typical language code standards used in text processing libraries.
tests/congruence_tests/test_multivector_search_queries.py (8)
65-74: LGTM! Good implementation of default MMR query.The method correctly implements a default MMR query using the
models.NearestQuerywith a defaultmodels.Mmr()configuration. The structure is consistent with other query methods in the class.
76-84: LGTM! Proper parametrized MMR query implementation.The method correctly implements a parametrized MMR query with diversity=0.3 and candidates_limit=30, which are reasonable test values for validating MMR functionality.
86-95: LGTM! MMR query with score threshold is correctly implemented.The addition of score_threshold=7.0 provides good test coverage for MMR queries combined with score filtering. The parameters are consistent with the previous parametrized query.
97-105: LGTM! Good coverage for different vector types with MMR.The method correctly tests MMR functionality on the "multi-image" vector type using the image query vector. This provides good coverage across different vector types.
120-128: LGTM! Euclidean distance MMR query is correctly implemented.The method properly tests MMR functionality with Euclidean distance using the "multi-code" vector type, providing good coverage across different distance metrics.
130-141: LGTM! Complete coverage with Euclidean distance and score threshold.The method correctly combines MMR with Euclidean distance and score thresholding. The score_threshold=10.0 appears reasonable for Euclidean distance metrics.
160-183: LGTM! Comprehensive MMR test with good structure.The test function follows the established pattern and provides excellent coverage by testing all MMR query variants. The use of a smaller fixture dataset (10 points) is appropriate for MMR testing as it focuses on the re-ranking functionality rather than large-scale search performance.
107-118: Confirm score_threshold aligns with dot-product scaleThe
score_threshold=260.0in
tests/congruence_tests/test_multivector_search_queries.py:113–118
may exceed the maximum possible dot-product for your image vectors. For a vector of dimensionimage_vector_size, the absolute max dot product is that size (all ones), and random vectors drawn from [0,1) will average around 0.25 × D. Please verify the configuredimage_vector_sizeintests/fixtures/config.pyand adjust the threshold to reflect your expected similarity distribution.• File to update:
tests/congruence_tests/test_multivector_search_queries.py (methodmmr_query_parametrized_dot_score_threshold)
• Config to check:
tests/fixtures/config.py →multi_vector_config→image_vector_size
| // Maximal Marginal Relevance (MMR) algorithm for re-ranking the points. | ||
| message Mmr { | ||
| // Tunable parameter for the MMR algorithm. | ||
| // Determines the balance between diversity and relevance. | ||
| // | ||
| // A higher value favors diversity (dissimilarity to selected results), | ||
| // while a lower value favors relevance (similarity to the query vector). | ||
| // | ||
| // Must be in the range [0, 1]. | ||
| // Default value is 0.5. | ||
| optional float diversity = 2; | ||
|
|
||
| // The maximum number of candidates to consider for re-ranking. | ||
| // | ||
| // If not specified, the `limit` value is used. | ||
| optional uint32 candidates_limit = 3; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review field numbering in Mmr message
The Mmr message definition is well-documented with clear parameter descriptions and constraints. However, there's a gap in field numbering - diversity uses tag 2 and candidates_limit uses tag 3, but tag 1 is missing.
This could indicate:
- A removed field (which would be fine for backward compatibility)
- An unintentional gap in numbering
- Reserved field tag 1 for future use
If this is intentional, consider adding a comment or reserved field declaration for clarity.
🏁 Script executed:
#!/bin/bash
# Check if field tag 1 was used elsewhere or if this is intentional
rg -n "= 1;" qdrant_client/proto/points.proto | grep -E "(message Mmr|field.*= 1)"Length of output: 285
Add a reserved field tag for Mmr message
To follow Protobuf best practices and avoid confusion when skipping tag 1, explicitly reserve it in the Mmr message.
Locations to update:
- File:
qdrant_client/proto/points.proto - Message:
Mmr(around lines 652–668)
Suggested diff:
message Mmr {
+ // Reserve tag 1 for backward compatibility/future use.
+ reserved 1;
// Tunable parameter for the MMR algorithm.
// Determines the balance between diversity and relevance.
@@
optional float diversity = 2;
// The maximum number of candidates to consider for re-ranking.
optional uint32 candidates_limit = 3;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Maximal Marginal Relevance (MMR) algorithm for re-ranking the points. | |
| message Mmr { | |
| // Tunable parameter for the MMR algorithm. | |
| // Determines the balance between diversity and relevance. | |
| // | |
| // A higher value favors diversity (dissimilarity to selected results), | |
| // while a lower value favors relevance (similarity to the query vector). | |
| // | |
| // Must be in the range [0, 1]. | |
| // Default value is 0.5. | |
| optional float diversity = 2; | |
| // The maximum number of candidates to consider for re-ranking. | |
| // | |
| // If not specified, the `limit` value is used. | |
| optional uint32 candidates_limit = 3; | |
| } | |
| // Maximal Marginal Relevance (MMR) algorithm for re-ranking the points. | |
| message Mmr { | |
| // Reserve tag 1 for backward compatibility/future use. | |
| reserved 1; | |
| // Tunable parameter for the MMR algorithm. | |
| // Determines the balance between diversity and relevance. | |
| // | |
| // A higher value favors diversity (dissimilarity to selected results), | |
| // while a lower value favors relevance (similarity to the query vector). | |
| // | |
| // Must be in the range [0, 1]. | |
| // Default value is 0.5. | |
| optional float diversity = 2; | |
| // The maximum number of candidates to consider for re-ranking. | |
| // | |
| // If not specified, the `limit` value is used. | |
| optional uint32 candidates_limit = 3; | |
| } |
🤖 Prompt for AI Agents
In qdrant_client/proto/points.proto around lines 652 to 668, the Mmr message
uses tag numbers starting from 2 and 3 but skips tag 1 without reserving it. To
follow Protobuf best practices and prevent accidental reuse of tag 1, add a
reserved field tag for number 1 in the Mmr message by including a reserved
statement reserving tag 1.
| DIVERSITY_FIELD_NUMBER: builtins.int | ||
| CANDIDATES_LIMIT_FIELD_NUMBER: builtins.int | ||
| diversity: builtins.float | ||
| """Tunable parameter for the MMR algorithm. | ||
| Determines the balance between diversity and relevance. | ||
|
|
||
| A higher value favors diversity (dissimilarity to selected results), | ||
| while a lower value favors relevance (similarity to the query vector). | ||
|
|
||
| Must be in the range [0, 1]. | ||
| Default value is 0.5. | ||
| """ | ||
| candidates_limit: builtins.int | ||
| """The maximum number of candidates to consider for re-ranking. | ||
|
|
||
| If not specified, the `limit` value is used. | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for diversity & candidates_limit.
The docstring states that diversity must be within [0, 1] and that a missing candidates_limit should fall back to limit, yet nothing enforces either invariant.
Consider adding validation in the Python → protobuf conversion layer (e.g. conversions/conversion.py) to:
if not (0.0 <= mmr.diversity <= 1.0):
raise ValueError("MMR.diversity must be in [0, 1]")
if mmr.candidates_limit is not None and mmr.candidates_limit <= 0:
raise ValueError("MMR.candidates_limit must be a positive integer")Failing early avoids silent mis-tuning or server-side errors.
🤖 Prompt for AI Agents
In qdrant_client/grpc/points_pb2.pyi around lines 2707 to 2723, the fields
diversity and candidates_limit lack validation despite docstrings specifying
constraints. Add validation checks in the Python to protobuf conversion layer
(e.g., conversions/conversion.py) to ensure diversity is between 0 and 1
inclusive, raising a ValueError if not, and to ensure candidates_limit, if set,
is a positive integer, raising a ValueError otherwise. This enforces the
documented invariants and prevents silent misconfiguration.
* generate rest client * gen grpc client * conversions * inpection cache * wip implement mmr * upd openapi models * tests: add mmr tests * wip: implement mmr * new: use postprocessed scores * fix maxsim asymmetry and empty sparse intersection * fix: fix conversions * fix: fix type hints * chore: update inspection cache * fix: revert changes to add points and update named vectors --------- Co-authored-by: generall <[email protected]> Co-authored-by: George Panchuk <[email protected]>
3 participants
WIP
TODO: