Repository navigation
Conversation
With one row, check_graph_degree reduced both the intermediate and the final graph degree to 0, and the kNN graph build (NN-descent or IVF-PQ) then hit an illegal memory access, which aborted the process from a stream synchronization in a destructor. Throw a clear error up front instead, and keep HnswRandomizedIT from generating single-row datasets.
The test required the first 5 results of every query to be among the exact 2k+10 nearest neighbors. On uniformly random data in hundreds of dimensions the nearest neighbors are nearly equidistant, so CAGRA can legitimately miss that window now and then (NVIDIA#631). The other reported failures (NVIDIA#1048, NVIDIA#1468) were filtered-out rows leaking into the results through the prefilter buffer that BitSet.toLongArray() truncated, fixed in NVIDIA#2375. Check what must always hold instead: every query returns min(k, passing rows) distinct, valid rows that pass the prefilter, sorted, with their exact distances, and a copy of a row finds that row first. Check recall only as a mean over all queries of each dataset memory kind, with bounds set from the distribution measured in CI. Also attach the padded device dataset that CAGRA search now requires.
|
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. |
CagraIndexImpl.search built the filter bitmap through Util.concatenate, which treats a BitSet with no bits set like a missing filter and sets every bit. A prefilter that excludes all rows therefore returned the excluded rows. CAGRA takes a single filter for all queries, so use its bits directly. The re-enabled CagraRandomizedIT hit this on tiny datasets whose random prefilter happened to exclude every row.
bdice
added
bug
|
/ok to test 9af2f6b |
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
This builds on #2805. This PR's diff includes #2805's commit (cagra_build.cuh, CagraSingleRowIT, HnswRandomizedIT, and the + 2 in CagraRandomizedIT); please review those parts there. The comments here only cover the two commits unique to this PR: the CagraRandomizedIT rewrite and the CagraIndexImpl prefilter fix.
Workstream J of the flaky-CI sweep (#2783). CagraRandomizedIT has been @Ignored since #1469 because of intermittent Not found in expected list failures (#631, #1048, #1468). The root-cause comment found two causes:
- #1048 and #1468 were a real bindings bug, already fixed by #2375: filtered-out rows leaked through the prefilter. Restoring the old buffer sizing reproduces it, including
Not found in expected list: 192(run 38114759255). - #631 was a legitimate approximate miss: on uniform random high-dimensional data, a correct CAGRA result can fall just outside the exact
2k + 10window.
Fix: the test now checks the invariants every query must satisfy, and checks recall only as a mean. The re-enabled test then found a third bug: a prefilter that excludes every row was ignored. It hit 7 times in about 560 runs (run 38130033033), and this PR fixes it.
Verification (run 38138513521, 4x amd64 / 13.3.0 / rtxpro6000): 400 runs of the re-enabled test (~120,000 random cases) had 0 failures, and the full cuvs-java IT suite passed 4/4.
| // CAGRA takes a single filter for all queries. Use its bits as they are: concatenate() | ||
| // would read a BitSet with no bits set as "no filter" and return excluded rows. | ||
| long[] filters = query.getPrefilter().get(0, query.getNumDocs()).toLongArray(); |
There was a problem hiding this comment.
This is the bindings fix. Util.concatenate treats a null or empty BitSet as "no filter" and sets every bit (Util.java). So withPrefilter(new BitSet(n), n), which should exclude every row, became accept-all and returned excluded rows. CAGRA only ever had a single filter here, so the BitSet[] wrapper did nothing useful, and the bits are now used directly. get(0, numDocs) keeps the old truncation to numDocs. buildMemorySegment(..., (len + 63) / 64) zero-pads the trailing words that toLongArray() drops (Util.java, the #2375 fix), so an all-clear filter really is all zeros on the device.
Question for reviewers: BruteForceIndexImpl (L188) and TieredIndexImpl (L245) still go through concatenate, so an all-clear filter has the same problem there. For brute force, with per-query filters, "empty means no filter for this query" may be intended. Should I change concatenate so that only null means "no filter" and fix all three together, or keep this PR to CAGRA and do the others as a follow-up?
| .withPrefilter(new BitSet(dataset.length), dataset.length) | ||
| .build(); | ||
|
|
||
| List<Map<Integer, Float>> results = index.search(query).getResults(); | ||
| assertEquals(algo.toString(), List.of(Map.of(), Map.of()), results); |
There was a problem hiding this comment.
Regression test for the fix above: an empty BitSet as the prefilter must return no results, with both SINGLE_CTA and MULTI_CTA. Before the CagraIndexImpl change, such a filter returned the excluded rows; the randomized test hit this on tiny datasets where every random prefilter bit came out false.
| // Lower bounds for the mean recall@k over the queries of each dataset memory kind, without and | ||
| // with a prefilter. In CI (RTX PRO 6000, CUDA 13), about 300 runs of this test measured means of | ||
| // 0.83 (standard deviation 0.022, lowest 0.76) without a prefilter and 0.94 (standard deviation | ||
| // 0.011, lowest 0.90) with one. The bounds sit 8 and 12 standard deviations below the means, so | ||
| // they only fail when results are wrong rather than approximate. | ||
| private static final double MIN_MEAN_RECALL_UNFILTERED = 0.65; | ||
| private static final double MIN_MEAN_RECALL_FILTERED = 0.8; | ||
| // Minimum fraction of the searches for a copy of a row that return that row first. All of more | ||
| // than 100,000 such searches did in CI. | ||
| private static final double MIN_SELF_MATCH_RATE = 0.99; | ||
| // Returned distances are computed in float on the GPU; allow for the difference in rounding | ||
| // from the double-precision distances computed here (at most 2.4e-7 in CI). | ||
| private static final double DISTANCE_RELATIVE_TOLERANCE = 1e-4; |
There was a problem hiding this comment.
The thresholds come from about 300 CI runs on one config (RTX PRO 6000, CUDA 13). Measured means were 0.83 unfiltered (sd 0.022, lowest 0.76) and 0.94 filtered (sd 0.011, lowest 0.90), so the bounds sit 8 and 12 sd below them. Recall is averaged per dataset memory kind over the 100 iterations (L78-L112), not checked per query. A single query on uniform random data in up to 2048 dims can legitimately miss its exact neighbors, and that was the #631 failure mode.
Question for reviewers: Are these bounds acceptable for the whole java test matrix? I only measured them on one GPU and CUDA version. Would you rather loosen them further, or derive them from a smaller, fixed-seed workload?
| // The results map holds one entry per distinct row, so the count also rules out duplicates. | ||
| Map<Integer, Float> result = resultMaps.get(q); | ||
| assertEquals(where + ": number of distinct results", expectedCount, result.size()); |
There was a problem hiding this comment.
This replaces the old "first 5 results within the exact top 2k + 10" check (CuVSTestCase.java) with per-query invariants: an exact result count, valid ids, no rows excluded by the prefilter, results sorted by distance, and distances that match the exact ones (relative tolerance 1e-4; the largest error measured was 2.4e-7).
Question for reviewers: Is it right to require exactly min(k, passing rows) results for every query? CAGRA doesn't strictly guarantee k results under a filter (#2799 fixes a case where a selective filter came back short). Here the prefilter keeps ~50% of rows and itopk is the default 64, and none of ~120,000 cases came back short. But if reviewers consider fewer than k results with a filter acceptable, I'd relax this to <=.
| assertTrue( | ||
| where + ": row " + id + " is excluded by the prefilter", | ||
| prefilter == null || prefilter.get(id)); |
There was a problem hiding this comment.
This is the check that reports the #1048 / #1468 bug class directly. Before, a leaked filtered-out row only showed up indirectly, as Not found in expected list. With the pre-#2375 buffer sizing restored, every leaked row was a filtered-out row at the start of a dropped 64-bit word (ids 192, 706, 1216, 1984, 9408) (run 38114759255).
| /** | ||
| * Gives the index the device-padded vectors CAGRA search needs. A device matrix whose rows are | ||
| * already padded can only be viewed, not copied. | ||
| */ | ||
| private static AutoCloseable attachPaddedDataset(CagraIndex index, CuVSMatrix deviceVectors) | ||
| throws Throwable { | ||
| if (CagraIndex.isPaddedDataset(deviceVectors)) { | ||
| var view = index.makePaddedDatasetView(deviceVectors); | ||
| index.updateDataset(view); | ||
| return view; | ||
| } | ||
| var dataset = index.makePaddedDataset(deviceVectors); | ||
| index.updateDataset(dataset); | ||
| return dataset; | ||
| } |
There was a problem hiding this comment.
The test had bit-rotted while it was @Ignored: CAGRA search now needs the padded device dataset attached to the index, and the old test never attached it. This helper attaches it for all three memory kinds (L210-L214). It uses a view when the device matrix is already padded, because such a matrix can only be viewed, not copied.
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 59 minutes. View limit details
Comment |
1 participant
Re-enables
CagraRandomizedIT, which was disabled in #1469, and replaces its per-query top-k containment check with checks that do not fail on legitimate approximate results.Root cause of the historical failures (#631, #1048, #1468):
CagraRandomizedIT.testResultsTopKWithRandomValues#1048 (Not found in expected list: 192) and [BUG] [Java]CagraRandomizedIT.testResultsTopKWithRandomValuesfails randomly #1468 (448) were filtered-out rows leaking into the results.BitSet.toLongArray()drops trailing all-zero words, so the prefilter copy read past the end of the host buffer, and stray bits let rows in the first dropped 64-bit word through. Fix prefilter bitmap host buffer under-allocation in Java bindings #2375 fixed this. Restoring the old buffer sizing in CI reproduces the same failure, includingNot found in expected list: 192at n=195, and every leaked row sits at the start of a dropped word.1583, no prefilter) is a legitimate approximate miss. On uniformly random data in hundreds or thousands of dimensions the nearest neighbors are nearly equidistant, so the first results can now and then fall outside the exact top2k + 10, both on current main and in releases back to 25.02.The re-enabled test also found a third bug, fixed here.
CagraIndexImpl.searchbuilt the filter throughUtil.concatenate, which treats aBitSetwith no bits set as a missing filter. A prefilter that excludes every row therefore returned the excluded rows. The search now uses the filter's bits as they are, andtestPrefilterExcludingEveryRowReturnsNothingcovers this. The bindings forBruteForceQuery.withPrefiltersandTieredIndexQuery.withPrefilterstill go throughconcatenate, so they are left for a follow-up.The test now checks, for every query:
min(k, passing rows)distinct valid row ids, no rows excluded by the prefilter, results sorted with their exact distances, and a copy of a dataset row returning that row first. Recall@k is checked only as a mean per dataset memory kind, with bounds set from about 300 runs measured in CI. Without a prefilter the measured mean is 0.83 (sd 0.022) against a bound of 0.65. With a prefilter it is 0.94 (sd 0.011) against a bound of 0.8. The test also attaches the padded device dataset that CAGRA search now requires.Builds on #2805 (single-row CAGRA build fix). Part of #2783.