Fast CAGRA Index Merge - #2352
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. |
|
/ok to test 2c54392 |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/ok to test |
dantegd
left a comment
There was a problem hiding this comment.
Overall, I really like the direction and PR! Reusing the partition graphs, adding a lightweight cross-partition scaffold, and then running the existing CAGRA optimization is such a great way to make merge substantially cheaper than a rebuild. The split between preflight, consolidation, scaffold construction, and graph assembly also makes the implementation fairly easy to follow.
One API concern: the C API still calls the existing C++ overload without merge_params, so it needs updating, right?
| RAFT_EXPECTS(range.start == covered && range.end > range.start && | ||
| range.end <= static_cast<int64_t>(partitions.memberships.size()), | ||
| "Fastener partition ranges must compactly cover all memberships"); | ||
| for (int64_t start = range.start; start < range.end; start += leaf_size) { |
There was a problem hiding this comment.
I think there’s a connectivity edge case here. Since the sorts are stable, rows within a partition can remain grouped by their source index. Slicing an oversized partition into consecutive chunks can then produce leaves containing rows from only one input, and the leaf kernel skips every same-origin pair.
For example, identical inputs whose row counts are multiples of leaf_size can end up with every chunk aligned to a single origin, leaving the original graphs disconnected. Could we either keep splitting or deterministically mix origins before slicing? A regression test for that case would also be helpful.
There was a problem hiding this comment.
| Dataset | Arm | Merge ms | Δ | Recall@12 | Δ | Rep spread |
|---|---|---|---|---|---|---|
| wiki-1m | baseline | 545.6 | — | 0.991683 | — | 1.1e-4 |
| wiki-1m | roundrobin | 546.6 | +0.18% | 0.991875 | +0.000192 | 4.1e-5 |
| wiki-1m | resplit | 857.7 | +57.2% | 0.992083 | +0.000400 | 2.5e-5 |
| openai-2m | baseline | 2271.5 | — | 0.958383 | — | 7.5e-5 |
| openai-2m | roundrobin | 2272.1 | +0.03% | 0.958758 | +0.000375 | 8.4e-5 |
| openai-2m | resplit | 3292.8 | +45.0% | 0.958158 | −0.000225 | 1.2e-4 |
| yfcc-10m | baseline | 2744.5 | — | 0.981367 | — | 7.5e-5 |
| yfcc-10m | roundrobin | 2734.6 | −0.36% | 0.983583 | +0.002216 | 3.4e-5 |
| yfcc-10m | resplit | 3915.5 | +42.7% | 0.982783 | +0.001416 | 4.2e-5 |
Good catch here, the interaction between concatenating the datasets and stable sorting here didn't occur to me. Experiments show that:
- 6% of leaves are single origin in the current design on wiki-1M, so this is happening, although even with round-robin you get like 5-10% of those still just because some single origin leaves happen for organic geometric reasons
- round-robin slightly bumps recall with an impact on build time that seems to be within noise
- further geometric splitting until you have leaves all at or below the max leaf size is expensive and doesn't seem amazing for recall.
Also trying not splitting them further and just treating them as big leaves, which I don't think will work but want to rule out before committing to round robin.
There was a problem hiding this comment.
| Dataset | Arm | Merge ms | Δ | Recall@12 | vs baseline | vs roundrobin | Spread |
|---|---|---|---|---|---|---|---|
| wiki-1m | baseline | 514.5 | — | 0.991775 | — | +0.000008 | 2.5e-5 |
| wiki-1m | roundrobin | 516.2 | +0.35% | 0.991767 | −0.000008 | — | 1.0e-4 |
| wiki-1m | big512 | 542.1 | +5.37% | 0.992825 | +0.001050 | +0.001058 | 8e-6 |
| wiki-1m | big1024 | 554.0 | +7.68% | 0.993075 | +0.001300 | +0.001308 | 1.7e-5 |
| openai-2m | baseline | 2203.4 | — | 0.958417 | — | −0.000308 | 6.7e-5 |
| openai-2m | roundrobin | 2206.6 | +0.15% | 0.958725 | +0.000308 | — | 1.6e-5 |
| openai-2m | big512 | 2273.0 | +3.16% | 0.960408 | +0.001991 | +0.001683 | 1.0e-4 |
| openai-2m | big1024 | 2287.2 | +3.80% | 0.960958 | +0.002541 | +0.002233 | 4.2e-5 |
| yfcc-10m | baseline | 2419.1 | — | 0.981433 | — | −0.002125 | 5.9e-5 |
| yfcc-10m | roundrobin | 2430.8 | +0.49% | 0.983558 | +0.002125 | — | 1.2e-4 |
| yfcc-10m | big512 | 2528.8 | +4.54% | 0.984625 | +0.003192 | +0.001067 | 1.7e-5 |
| yfcc-10m | big1024 | 2594.4 | +7.25% | 0.985058 | +0.003625 | +0.001500 | 6.7e-5 |
Treating leaves of size up to 512 (default max leaf size is 256) as normal leaves that don't need round robin gives slightly higher recall but has some added complexity and takes slightly longer to do. For now, I'll stick to round robin for simplicity but let me know if you think this is a worthwhile tradeoff.
| if (params.graph_degree == 0 || static_cast<uint64_t>(params.graph_degree) >= rows) { | ||
| return reject("graph_degree must be positive and smaller than the combined row count"); | ||
| } | ||
| if (static_cast<uint64_t>(params.graph_degree) > max_input_degree + scaffold_degree) { |
There was a problem hiding this comment.
I think one limit is missing from preflight: the graph passed to sort_knn_graph_device_inplace() can’t exceed kMaxSortDegree (1024). For example, an input degree of 1024 plus any scaffold currently passes preflight and then fails later in the sorter. In AUTO mode, that also means we miss the rebuild fallback.
Could we include max_input_degree + scaffold_degree <= kMaxSortDegree in the eligibility check?
| } // namespace | ||
|
|
||
| typedef AnnCagraIndexMergeTest<float, float, std::uint32_t> AnnCagraFastenerMergeRecallTest; | ||
| TEST_P(AnnCagraFastenerMergeRecallTest, DefaultFloatMergeSearchRecallAgainstBruteForce) |
There was a problem hiding this comment.
Could we add recall coverage for at least one non-float dtype and one merge with more than two inputs? The current dtype tests show that the output graph is structurally valid, but only the small two-part float case verifies that the merged graph is actually useful for search. That feels important if AUTO is going to become the default.
| auto dim = dataset.extent(1); | ||
|
|
||
| // Cut every parent in the bucket into fixed-height tiles | ||
| int tile_rows = context.assignment_tile_rows; |
There was a problem hiding this comment.
Could we derive tile_rows from the dimension and padded leader count instead of always using 2048?
Preflight only checks the leaf-GEMM shape, while assignment needs roughly (tile_rows + padded_leaders) * dim + tile_rows * padded_leaders float elements. Some configurations therefore pass preflight and later fail the workspace assertion here if I'm not mistaken. Could we reduce the tile height per bucket and reject only when the leader matrix plus even one point row cannot fit?
| dataset, | ||
| parents, | ||
| buckets[bucket_index], | ||
| 1 << bucket_index, |
There was a problem hiding this comment.
Curious, have you measured how much work is going to padded leaders here? A parent just over a power-of-two boundary could nearly doubles the gathered leader storage, dot-product GEMM, distance materialization, and select_k width.
There was a problem hiding this comment.
I compared against always doing non-batched GEMM, which was substantially slower, and batching with individual sizes, which was moderately faster but iirc added more LoC than felt worth it.
| RAFT_CUDA_TRY(cudaGetLastError()); | ||
|
|
||
| int leader_blocks = strided_grid_size(static_cast<int64_t>(batch_size * leader_elements)); | ||
| manyway_gather_tile_leaders_kernel<<<leader_blocks, THREADS_PER_BLOCK, 0, stream>>>( |
There was a problem hiding this comment.
It looks like every tile of the same parent gathers the same leader matrix again. For large parents split across many 2048-row tiles, that can add substantial conversion and memory traffic, especially when the leader count is close to the tile height. Could we gather each parent’s leaders once and reuse them across its tiles, or did benchmarking show that the added bookkeeping costs more?
There was a problem hiding this comment.
This seems to perform ~the same but adds a small amount of complexity to do the caching.
| Dataset | Arm | Merge ms (median) | Range | Δ |
|---|---|---|---|---|
| wiki-1m | baseline | 517.0 | 506.0–518.8 | — |
| wiki-1m | cached | 515.7 | 509.5–518.7 | −0.25% |
| openai-2m | baseline | 2214.8 | 2199.9–2216.9 | — |
| openai-2m | cached | 2220.0 | 2208.8–2221.4 | +0.23% |
| yfcc-10m | baseline | 2422.0 | 2405.9–2429.0 | — |
| yfcc-10m | cached | 2427.8 | 2417.4–2432.1 | +0.24% |
| int64_t total = static_cast<int64_t>(batch_size) * tile_rows * padded_leaders; | ||
| for (; linear < total; linear += stride) { | ||
| int leader = static_cast<int>(linear % padded_leaders); | ||
| int row = static_cast<int>((linear / padded_leaders) % tile_rows); |
There was a problem hiding this comment.
This is a small thing, but wanted to ask: for dimensions that aren’t naturally 16-byte aligned, this allocates and copies a second full consolidated dataset while all input datasets are still alive. Would it be practical to consolidate directly into aligned storage and pass a row stride through the scaffold/sort kernels? That could materially lower peak memory for int8/uint8 and other unaligned dimensions.
|
The Java API is still lagging, but is a bit more to convert. Could add to this PR or defer, up to @dantegd. |
|
/ok to test |
Brings the branch up to date with main (f72199e), which resolves the check-c-abi failure: the checker was reporting cuvsResourcesSetWorkspacePool, cuvsRMMAsyncMemoryResourceEnable, cuvsCagraSearchMultiPartition and cuvsSelectK as removed functions, when in fact they were added upstream after this branch's base and were simply missing here. Only cpp/src/neighbors/cagra.cuh conflicted, and only positionally: both sides append a new function immediately after the existing merge() overload. Kept both -- this branch's merge() taking merge_params, and main's multi-partition search() overload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Currently failing because the wheel builds are too big. Not sure there's much I can do about this. |
This PR implements the Fastener graph merge operation. This PR supports merging for
float,half,int8, anduint8dtypes, and euclidean distances.There was originally specialization to use int8 GEMM for the integer types, but I ran into portability issues on Ada and it turns out that using the same f32 path for both is simpler and not substantially slower. Currently investigating switching the unified path to TF32 to use tensor cores.
The core logic resides in
cagra_merge_scaffold.cuh.This PR adds 7.64 MiB to
libcuvs.so, a 2.94% increase.