Repository navigation
Conversation
|
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 f7c813d |
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
Notes for reviewers:
- Problem:
NEIGHBORS_ANN_IVF_RABITQ_TESTIvfRabitq/f32_f32_i64.build_host_input_search/1(dim = 1) threw once in the 2026-09-30 nightly on13.3.0, arm64, ubuntu26.04, gh200(job) withcudaErrorInvalidValuefromcudaPeekAtLastError()atquantizer_gpu.cu:538. That is the only hit. Tracked as workstream G in #2783; the reproducer PR is #2791. - Root cause: when k-means leaves a list empty,
construct_on_gpustill quantizes it and launchessubtract_normalize_binarize_KernelwithgridDim.x = 0, which fails with exactly that error. This is deterministic for any dataset with fewer distinct rows thann_lists. The streaming build skipped empty lists instead, which left their rotated centroids uninitialized. - Fix: for an empty list, compute only its rotated centroid and skip the kernels that have no rows to process, in both build paths. The new
ivf_rabitq_empty_lists_test(dims 1, 2, 7, 64, 100 with device, host and streaming input) fails in all 10 device/host cases against the current nightlylibcuvs, and all 15 cases passed 50 repeats on each of 4 GH200 jobs with the fix (run 38073819578). - Caveat: the original nightly failure never reproduced: 0 failures in about 40,000 GH200
dim = 1builds across round 1 and round 2, and the diagnostics never showed an empty list with the test's uniform data. I don't know what emptied a list in that one nightly, so I can't give a before/after rate for the flake itself. This PR removes the only code path that raises this exception (details in the workstream G comment).
| // An empty list only needs its rotated centroid; a launch with zero blocks is invalid. | ||
| if (num_points == 0) { return; } |
There was a problem hiding this comment.
This is the launch that threw in the nightly. On main it is quantizer_gpu.cu L523-L538, and L538 is the cudaPeekAtLastError() in the error message. With num_points == 0, gridDim.x is 0, and a launch with zero blocks returns cudaErrorInvalidValue: invalid argument.
Returning here still produces the rotated centroid. gatherAndPadKernel writes the centroid as row num_points of the padded buffer, and the rotation runs on num_points + 1 rows, so steps 2-5 above still work with zero data rows. data_transformation_batch_opt_contiguous (the streaming path) gets the same guard further down.
| // An empty list has no codes or factors to compute. | ||
| if (num_points == 0) { return; } |
There was a problem hiding this comment.
The kernels after the transformation also launch one block per point: rabitq_codes_and_factors_fused uses dim3 grid(num_points) (L556-L569), and exrabitq_codes_and_factors_fused uses dim3 gridDim(num_points) (L590-L612). An empty list has no codes or factors to write, so we return once the rotated centroid is stored. quantize_batch_opt_contiguous gets the same guard below.
| // Every cluster fits into a batch (checked above), so each batch makes progress. | ||
| RAFT_EXPECTS(cluster_idx > batch_start_cluster, "IVF-RaBitQ streaming batch is empty"); |
There was a problem hiding this comment.
This used to be if (batch_vectors == 0) break;. A batch could only be empty in two cases:
- a cluster larger than the batch, which the
RAFT_EXPECTSat L680-L684 already rejects; - a batch of only empty clusters. An empty cluster always fits, so the inner loop folds empty clusters into the current batch, and every later batch starts with the non-empty cluster that didn't fit. So this case needs every list to be empty.
A silent break would have skipped the rest of the clusters. I replaced it with an assertion of the invariant and put the copies below behind batch_vectors > 0.
| // Empty clusters are quantized too, so that their rotated centroids are initialized. | ||
| size_t cluster_size = h_cluster_meta[c].num; |
There was a problem hiding this comment.
This is the streaming-path bug. if (cluster_size == 0) continue; skipped empty lists entirely, so their slots in d_rotated_centroids were never written. That buffer is allocated uninitialized at L690-L691, and initializer->AddVectors then uses those slots as the centroids for probe selection. While reproducing this, an n_probes = 1 search on a streaming-built index with empty lists returned the wrong rows for 2 of 16 queries (#2783 comment). Empty lists now go through quantize_batch_opt_contiguous, which writes the rotated centroid and returns early.
| // Probe every list, because an empty list's centroid can be closer to a query than the | ||
| // centroid of the list that holds its neighbors. | ||
| search_params sparams; | ||
| sparams.n_probes = kLists; |
There was a problem hiding this comment.
Question for reviewers: empty lists stay in the index with whatever centroid k-means gave them, so they can still be selected as probes, which wastes a probe slot. This test avoids the issue by probing every list. Should search skip empty lists (for example, by giving them an infinite coarse distance), or is that out of scope here?
Because every list is probed, the 5 host_streaming cases also pass without this fix: for an empty list, the rotated centroid only affects whether it is picked as a probe. They cover the streaming build's handling of empty clusters, but not the uninitialized-centroid bug itself.
Question for reviewers: do you want coverage for that bug too? It would need n_probes < n_lists, which brings back the question above of how empty lists rank. Or is the current coverage enough?
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: handling empty lists during IVF-RaBitQ index construction. Description check The description directly explains the empty-list failure, the fixes for device, host, and streaming builds, and the regression tests added. 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
-
🧪 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.
k-means can leave lists empty, for example when the dataset has fewer distinct rows than n_lists. Building from device data, or from host data that fits on the device, then launched the quantization kernels with zero blocks and failed with cudaErrorInvalidValue. The streaming build skipped empty lists, which left their rotated centroids uninitialized. Compute only the rotated centroid for an empty list and skip the kernels that have no rows to process. Add a regression test with fewer distinct rows than lists.
f7c813d to
a782cd0
Compare
|
/ok to test a782cd0 |
1 participant
k-means can leave IVF-RaBitQ lists empty, for example when the dataset has fewer distinct rows than
n_lists. When that happened,construct_on_gpu(device input, and host input that fits on the device) launched the quantization kernels with zero blocks and threw:The streaming build skipped empty lists instead, which left their rotated centroids uninitialized. Those garbage centroids can then be chosen as probes.
This PR computes only the rotated centroid for an empty list and skips the kernels that have no rows to process, in both build paths. It also adds a regression test that builds and searches a dataset with fewer distinct rows than lists, with device, host, and streaming input.
This is the error seen once in the 2026-09-30 nightly (
IvfRabitq/f32_f32_i64.build_host_input_search/1,dim = 1, GH200). See #2791 and #2783.