fix(retrieval): honor score_threshold in hybrid search (was silently dropped)#22
Open
yuzushi-dev wants to merge 2 commits into
Open
fix(retrieval): honor score_threshold in hybrid search (was silently dropped)#22yuzushi-dev wants to merge 2 commits into
yuzushi-dev wants to merge 2 commits into
Conversation
…dropped) MilvusVectorStore.hybrid_search() accepted no score_threshold and never applied one, while the dense-only search() did. Since hybrid (dense+SPLADE) is the default active path, the relevance threshold was silently ignored there: low-relevance chunks passed through and could produce misleading citations. - milvus.py / vector_store_port.py: hybrid_search() now accepts score_threshold and filters candidates below it, mirroring the dense path. - vector.py: forward score_threshold to the store (it was dropped before); the dense fallback does NOT forward it (fusion scale != cosine scale). - retrieval_service.py: the hybrid call site no longer passes the tenant's cosine similarity_threshold into hybrid (left None). Hybrid fusion scores are ~0.01-0.03, not cosine 0-1; reusing a cosine 0.7 there would zero out every result. The param is now correctly plumbed and opt-in, to be tuned on the fusion scale separately. No behaviour change in prod (call site resolves to None today, as before, so the ranking is unchanged); this removes the silent-drop and makes a hybrid-scale threshold usable without a regression. Verified: diff reviewed end-to-end (no prod ranking change); unit test in tests/unit/test_hybrid_score_threshold.py (None = no filter; explicit threshold drops below-threshold candidates).
Both the dense (search) and the new hybrid (hybrid_search) filters used `if score_threshold and hit.score < score_threshold`, so an explicit 0.0 — a valid fusion-scale threshold (RRF scores are tiny positives) — was falsy and silently disabled filtering instead of keeping scores >= 0. Use `is not None` on both paths. Regression test asserts 0.0 drops a negative-score hit.
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
1 participant
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.
Problem
MilvusVectorStore.hybrid_search()accepted noscore_thresholdand never applied one, while the dense-onlysearch()did. Since hybrid (dense+SPLADE) is the default active path in prod, the configured relevance threshold was silently ignored there — low-relevance chunks passed through and could feed misleading citations.Surfaced while diagnosing a negative-feedback retrieval miss: a cited chunk with cosine ~0.4751 (below the tenant's
similarity_threshold) was returned anyway, because the threshold is dead code in the hybrid path.Change
milvus.py/vector_store_port.py:hybrid_search()now acceptsscore_thresholdand filters candidates below it, mirroring the dense path.vector.py: forwardsscore_thresholdto the store (previously dropped — only used in the dense-fallback). The dense fallback does not forward it (fusion scale ≠ cosine scale).retrieval_service.py: the hybrid call site no longer passes the tenant's cosinesimilarity_thresholdinto hybrid (leftNone).Why the call site stays
None(anti-regression)Hybrid fusion (RRF) scores are ~0.01–0.03, not cosine 0–1. Wiring a cosine
0.7into the now-honored hybrid filter would zero out every result. So the param is correctly plumbed and opt-in, to be tuned on the fusion scale separately.No behaviour change in prod: the call site resolves to
Nonetoday (as it did before, where the value was silently dropped), so hybrid ranking is unchanged. This PR removes the silent-drop and makes a hybrid-scale threshold usable without a regression.Review
Diff reviewed end-to-end by a separate model (Fable): confirmed no prod ranking change (traced
RetrievalConfig.score_thresholddefaultNone, never set by any prod constructor), filter logic matches the dense path, protocol in sync, test is non-tautological. Verdict: approve.Test
tests/unit/test_hybrid_score_threshold.py(stubs the pymilvus Collection, no real Milvus):score_threshold=None→ all candidates; explicit threshold → below-threshold dropped.Known follow-up (pre-existing, out of scope)
The dense fallback inside
hybrid_search()(milvus.py) dropsdocument_ids/collection_name, i.e. the document ACL filter is lost if hybrid throws and falls back to dense. Not introduced here — tracked separately.