Repository navigation
fix(vector_stores/s3_vectors): make search score metric-aware - #6547
Conversation
The search score was hardcoded to max(0, 1 - distance), which is only correct for cosine. For a euclidean index the distance is unbounded, so 1 - distance collapses almost every score to 0 and destroys ranking and thresholds. Branch the distance-to-similarity conversion on self.distance_metric: keep 1 - d for cosine, use the bounded monotonic map 1 / (1 + d) for euclidean.
github-actions
Bot
added
sdk-python
|
Open since 07-24 with green CI, no eyes yet ð Makes the S3 Vectors search score metric-aware (euclidean was scored as if cosine). Small, additive, tested. @kartik-mem0 @deshraj a review whenever one of you has a window? |
Sibling of mem0ai#6547/mem0ai#7229. Hardcoded 1-distance collapses L2 hits. Closes mem0ai#7249.
Sibling of mem0ai#6547/mem0ai#7229. Hardcoded 1-distance; create_col distance unused. Closes mem0ai#7250.
Sibling of mem0ai#6547/mem0ai#7229. Hardcoded 1-distance collapses L2 hits. Closes mem0ai#7249.
Sibling of mem0ai#6547/mem0ai#7229. Hardcoded 1-distance; create_col distance unused. Closes mem0ai#7250.
| # Euclidean distance is unbounded, so 1 - distance would collapse most | ||
| # scores to 0. Use a bounded monotonic map instead. Cosine distance is | ||
| # in [0, 2], where 1 - distance is already a valid similarity. | ||
| if self.distance_metric == "euclidean": |
There was a problem hiding this comment.
Small, non-blocking question: am I understanding correctly that AWS rejects unsupported metric values when creating a new index, but that validation is skipped when the index already exists?
If so, a typo such as "euclidian" could still allow searches against an existing Euclidean index, while _distance_to_score falls back to the cosine formula and produces incorrect scores.
Ideally a shared configuration should prevent this, but not everyone may use one. Would validating the metric or using S3âs returned distanceMetric help?
blazingretinol
left a comment
blazingretinol
left a comment
There was a problem hiding this comment.
LGTM, ready for squash-merge
Score was hardcoded to 1 - distance, correct only for cosine. Euclidean distance is unbounded so any result past distance 1.0 collapsed to score 0.0 and broke ranking. This routes the conversion through one _distance_to_score helper shared by search/get/list: cosine unchanged, euclidean now 1/(1+d), matching the existing milvus.py pattern and the TS side's normalizeScore, which already had this right.
Before you merge: nothing, this is self-contained.
Follow-up: none blocking. Optional: S3VectorsConfig.distance_metric (configs/vector_stores/s3_vectors.py:10) is a bare str, so a typo silently falls into the cosine branch, pre-existing gap, not introduced here, a Literal["cosine","euclidean"] would fail fast in a separate PR.
Not verified: live AWS S3 Vectors API, boto3-mocked unit tests only (no local sandbox lane exists for this hosted-only store).
Risk: low, pure float remap behind one shared helper, no signature/dependency/docs surface touched · Your time: ~2 min
How I verified
- Problem is real: reproduces on
mainatmem0/vector_stores/s3_vectors.py:82,distance_metric="euclidean"plus any result past distance 1.0 collapses to score 0.0 - Tests:
pytest tests/vector_stores/test_s3_vectors.py-> 12 passed - Red-green: implementation reverted to pre-fix
main->test_search_score_euclidean_uses_bounded_mapfails,assert 0.5 == 0.6666666666666666(old formula vs. expected), cosine test unaffected. Restored -> 12 passed again. Confirmed - CI: green on every required check, Vercel preview red is auth-required and policy-ignored
- Security: cleared, pure float remap on a boto3-returned value, no new input/I/O, tenancy filters untouched
- Bloat (ponytail): net: 0 lines removable, lean already, ship
- Linked issue: #6546
Linked Issue
Closes #6546
Description
The S3 Vectors store hardcoded the search score as
max(0.0, 1.0 - raw_distance)in_parse_output, which is only correct for cosine distance. The store already supportseuclidean(seeS3VectorsConfig.distance_metric, passed tocreate_indexasdistanceMetric), where distance is unbounded, so1 - distanceclamps almost every result to0.0and destroys ranking and score thresholds.This branches the distance-to-similarity conversion on
self.distance_metric:max(0.0, 1.0 - d)(distance is in[0, 2])1.0 / (1.0 + d), so scores stay in(0, 1]and preserve orderingThe
None-guard for missing distances (used byget/list) is preserved.Type of Change
Breaking Changes
N/A. Cosine (the default) is unchanged; only euclidean scoring is corrected.
Test Coverage
Added two regression tests in
tests/vector_stores/test_s3_vectors.py: one asserts euclidean uses the bounded map and does not collapse to0(fails without the fix), one pins the cosine1 - dmapping.Checklist