Repository navigation
Use a recall-based check in TestCuVSRandomizedHNSWVectorSearch - #2824
Conversation
The test required every hit of a single query to be among the closest 3 * topK vectors. The search is approximate, and on this test's data (uniform random vectors with up to 2048 dimensions) a top-1 to top-3 search occasionally returns a valid neighbor just outside that window. Check all queries instead, allow 2 such misses in total, and keep hard checks that every result is a distinct document with a vector, scored by its own vector, in score order, with min(topK, vectors) results. Also raise TOP_K_LIMIT to 128: the search does not fail beyond 64.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
bdice
added
bug
|
/ok to test 37f4477 |
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
This is a test-only change for TestCuVSRandomizedHNSWVectorSearch.testVectorSearch, which failed with Result returned was not in topK*3: 229 (CAGRA-built HNSW graph, searched on the CPU). It is workstream O of #2783. The combined stress run found it once. In the reproducer #2819, the unmodified test looped with random seeds failed 3 times in 1586 runs, and seed 2A18DA9484B33896 failed in 3 of 12 replays across amd64 and arm64 (run).
- Root cause: a legitimate approximate-search miss, not a format bug (analysis). The failing hit was a valid document at rank 8 for topK = 2, only 1.2% farther away than the true nearest neighbor. The graph was healthy (every node reachable, no invalid ids). Lucene searches each segment with a beam as wide as topK, and on uniform random vectors with up to 2048 dimensions that beam is nearly greedy for topK <= 3. The old assertion required every hit of a single query to be in the closest
3 * topK. - Fix: check all the queries with the shared
assertSearchFindsNearestNeighborshelper from #2775, and allow 2 hits outside3 * topKin total. It also adds hard checks for the invariants that would catch a real format bug. - Evidence (after run, summary): looped with random seeds, the test failed 0 times in 2166 runs, and both failing seeds passed 20/20 replays. On 16,553 random indexes, the new check failed 0 times where the old one would have failed 7 times, and the full cuvs-lucene suite passed on all 4 jobs.
| // The search is approximate. Lucene searches each segment's HNSW graph with a beam as wide as | ||
| // topK, and in this data (uniform random vectors with up to 2048 dimensions, so all distances | ||
| // are within about 10% of each other) a narrow beam sometimes stops just short of the true | ||
| // neighbors. Over 27,003 random indexes from this test's distribution, with 10 queries each, a | ||
| // result fell outside the closest 3 * topK in 1.7% of queries at topK = 1, 0.21% at topK = 2, | ||
| // 0.03% at topK = 3, 3 times in 270,000 queries at topK = 5, and never at topK >= 8. Those | ||
| // results were at most rank 17, and typically only 1.3% farther away than the nearest | ||
| // neighbor. On the same segments, Lucene's own HNSW format missed in 22% of queries at | ||
| // topK = 1. Allowing 2 misses across all the queries keeps the false failure rate below 1e-5 | ||
| // per run. | ||
| int allowedMisses = 2; | ||
| assertSearchFindsNearestNeighbors( | ||
| searcher, "vector", "id", dataset, hasVector, queries, topK, allowedMisses); |
There was a problem hiding this comment.
This replaces the per-hit check on query 0 only (main) with a budget over all the queries. The numbers in the comment come from the #2819 sweeps (before, after). The new check didn't fail on any of the 16,553 sweep indexes, but it still failed on Lucene's own Lucene99HnswVectorsFormat built on the same segments (32 times with default parameters, 29 with maxConn = beamWidth = 32). So it can still tell a weaker graph from this one. #2775 uses the same helper with allowedMisses = 4 and 10 in the two TestCagraToHnswSerializationAndSearch* tests (example).
Question for reviewers: is this recall threshold appropriate? It allows 2 hits outside the closest 3 * topK, counted across all 1-10 queries of a run, no matter what topK is. I calibrated the budget for the small-topK cases where misses happen. A fixed count is lenient at large topK, where there are up to 128 results per query and the sweep never saw a miss. The alternatives I see are a mean-recall floor, or a budget that scales with numQueries * topK. The "below 1e-5 per run" figure is extrapolated from the per-query miss rates above. The direct measurement is 0 failures in 16,553 sweep indexes and 2166 test runs.
| // A wider search is more accurate, not less: in the measurements described in testVectorSearch, | ||
| // no result was outside the closest 3 * topK for any topK from 8 to 200. | ||
| static int TOP_K_LIMIT = 128; |
There was a problem hiding this comment.
The old TODO said "This fails beyond 64" (main). I couldn't find a failure there: in the #2819 sweep, topK = 89, 128 and 200 had 0 hits outside 3 * topK, mean recall was 1.000, and the worst per-query recall was 0.989 (comment). testVectorSearchWithFilter also uses this limit, but its filter matches a single document, so a larger topK doesn't change what it checks (L163-L177).
Question for reviewers: the TODO doesn't say what failed beyond 64. If anyone remembers the original failure, please check whether this change would bring it back.
| public static void assertSearchFindsNearestNeighbors( | ||
| IndexSearcher searcher, | ||
| String vectorField, | ||
| String idField, | ||
| float[][] dataset, | ||
| boolean[] indexed, | ||
| float[][] queries, | ||
| int topK, | ||
| int allowedMisses) | ||
| throws IOException { | ||
| int indexedCount = dataset.length; | ||
| if (indexed != null) { | ||
| indexedCount = 0; | ||
| for (boolean isIndexed : indexed) { | ||
| if (isIndexed) indexedCount++; | ||
| } | ||
| } | ||
| int expectedResults = Math.min(topK, indexedCount); |
There was a problem hiding this comment.
This test leaves the vector out of about 40% of the documents when the dataset has 100 or more rows (L88-L96), but the existing helper assumes every row is indexed. This overload takes the hasVector mask and ranks only the rows that have a vector, as the old generateExpectedResults did (main). The existing 7-argument overload delegates with null, so its callers in the TestCagraToHnswSerializationAndSearch* tests behave as before.
The expected count becomes min(topK, indexed rows) instead of topK, because a small dataset can have fewer indexed rows than topK. The old test didn't check the count at all.
| assertTrue( | ||
| "id " + ids[i] + " for query " + q + " has no vector", | ||
| indexed == null || indexed[ids[i]]); | ||
| assertTrue("id " + ids[i] + " returned twice for query " + q, seen.add(ids[i])); | ||
| if (i > 0) { | ||
| assertTrue( | ||
| "results for query " + q + " are not sorted by score at position " + i, | ||
| results.scoreDocs[i].score <= results.scoreDocs[i - 1].score); | ||
| } | ||
| float expectedScore = | ||
| VectorSimilarityFunction.EUCLIDEAN.compare(queries[q], dataset[ids[i]]); | ||
| assertEquals( |
There was a problem hiding this comment.
These are the hard checks, and they don't count against the miss budget: every hit has a vector, there are no duplicates, scores don't increase, and each score equals the exact distance to that document's own vector. An ordinal-to-docID mapping error or a corrupted graph would break one of them, while a legitimate ANN miss wouldn't. All of them held in every #2819 sweep (0 violations, comment).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to This PR only changes test assertions and a test top-k limit, with no production code affected. No concrete merge-blocking risk was found. The remaining open questions concern how strict the new recall threshold should be. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: replacing the randomized HNSW test with a recall-based check. Description check The description directly explains the recall-based validation, allowed misses, retained checks, helper changes, and increased TOP_K_LIMIT. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
imotov
left a comment
imotov
left a comment
There was a problem hiding this comment.
LGTM, thanks for running this!
|
/merge |
github-project-automation
Bot
moved this from Approved
to Done
in Unstructured Data Processing
2 participants
TestCuVSRandomizedHNSWVectorSearch.testVectorSearchrequired every hit of one query to be among the closest3 * topKvectors. The search is approximate: on this test's data (uniform random vectors with up to 2048 dimensions), a topK = 1 to 3 search sometimes returns a valid neighbor just outside that window, typically about 1% farther away than the nearest neighbor.The test now checks all the queries and allows 2 such misses in total. It keeps hard checks that every result is a distinct document with a vector, scored by its own vector, in score order, with
min(topK, vectors)results. This reuses theassertSearchFindsNearestNeighborshelper from #2775, extended for indexes where some documents have no vector.TOP_K_LIMITis raised to 128, since the search does not fail beyond 64.Measurements and the reproducer are in #2819. Part of #2783.