Skip to content

Use batched unfused 1-NN for KMeans on Blackwell - #2346

Open
viclafargue wants to merge 4 commits into
NVIDIA:release/26.08from
viclafargue:fused-1-nn-kmeans-blackwell
Open

Use batched unfused 1-NN for KMeans on Blackwell#2346
viclafargue wants to merge 4 commits into
NVIDIA:release/26.08from
viclafargue:fused-1-nn-kmeans-blackwell

Conversation

@viclafargue

@viclafargue viclafargue commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

An investigation of large-scale KMeans failures on Blackwell revealed that the unfused 1-NN path bypasses KMeans distance batching and materializes the complete n_samples × n_clusters distance matrix. This can require hundreds of gigabytes of temporary workspace and may overflow int32 indexing.

This change batches the unfused 1-NN implementation on Blackwell, restoring bounded workspace usage. It also renames the dispatch booleans to make the metric-capability and implementation-selection decisions clearer.

@viclafargue
viclafargue requested a review from a team as a code owner July 22, 2026 13:17
@viclafargue viclafargue added bug Something isn't working non-breaking Introduces a non-breaking change labels Jul 22, 2026

@tfeher tfeher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is not the right solution. Missing the tiling logic was an oversight in #1768. It was actually discussed, and was thought to be addressed, but apparently there is another call path where this is not working.

I think the correct solution would add a simple 1D tiling along n_samples. Wouldn't that solve the problem?

@tarang-jain

tarang-jain commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

I think the correct solution would add a simple 1D tiling along n_samples. Wouldn't that solve the problem?

A 1D tiling would also have to be done on the centroids right?

@tfeher

tfeher commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

I think the correct solution would add a simple 1D tiling along n_samples

Having said that, this code path will be reworked in #2249, and what we do here will be deprecated. Therefore we should not develop anything complicated to fix this.

@tfeher

tfeher commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

A 1D tiling would also have to be done on the centroids right?

Depends on what are typical parameter ranges we expect this to work on. If the target use case would not work with tiling only on n_samples, then I would just go for the simpler solution proposed by Victor.

@tarang-jain

Copy link
Copy Markdown
Contributor

Depends on what are typical parameter ranges we expect this to work on. If the target use case would not work with tiling only on n_samples, then I would just go for the simpler solution proposed by Victor.

The behabior of the API is to acknowledge tile parameters on both the centroids and the samples. That said, kmeans_balanced might have different requirements. Changing the use_fused function could mean affecting the behavior there as well, which may not be intended for balanced kmeans.

// On Blackwell onwards, use unfused
return false;
// On Blackwell onwards, use fused
return true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we should think about the effect on balanced kmeans. Avoidance of tiling there may have be intentional. Then if balanced and regular kmeans are expected to behave differently, we must solve this problem in another way -- kmeans should be able to override this function and force the use of fused kernel always.

@viclafargue

Copy link
Copy Markdown
Contributor Author

Balanced KMeans uses the same assignment function but does not pass the regular KMeans tiling parameters. Instead, it bounds memory through its own outer minibatching and calls the assignment with batch_samples = 0 and batch_centroids = 0. Its use of the unfused Blackwell path was therefore intentional (normally executes as a single inner tile).

My latest commit fix restores Blackwell to the shared unfused heuristic and makes regular KMeans’s unfused assignment honor batch_samples and batch_centroids, avoiding the full distance-matrix allocation.

@viclafargue
viclafargue force-pushed the fused-1-nn-kmeans-blackwell branch from 1a40699 to a24db1f Compare July 28, 2026 10:06
@viclafargue
viclafargue changed the base branch from main to release/26.08 July 28, 2026 10:07

@achirkin achirkin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The allocation/batch sizing seems right. I'd suggest to add some tests to cover specifically this codepath - it's not clear whether it is ever tested in the CI with the current test suite.

@viclafargue viclafargue changed the title Use fused 1-NN for KMeans on Blackwell Fix unfused 1-NN for KMeans on Blackwell Jul 28, 2026
@viclafargue viclafargue changed the title Fix unfused 1-NN for KMeans on Blackwell Use batched unfused 1-NN for KMeans on Blackwell Jul 28, 2026
@viclafargue
viclafargue force-pushed the fused-1-nn-kmeans-blackwell branch from a24db1f to 711b3f6 Compare July 28, 2026 15:00
@viclafargue
viclafargue requested a review from a team as a code owner July 28, 2026 15:00
@viclafargue

Copy link
Copy Markdown
Contributor Author

Added a test that exercises no batching, sample-only batching, centroid-only batching, and combined batching for both 32-bit and 64-bit indices. It validates labels and inertia on every GPU and verifies that batching reduces device-memory allocations.
We use predict because minClusterAndDistanceCompute is an internal, non-exported template and cannot be linked directly from the test executable.

@viclafargue
viclafargue requested a review from achirkin July 28, 2026 16:16

@achirkin achirkin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank for the new tests! LGTM

@tarang-jain tarang-jain left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. LGTM except one small comment.

Comment thread cpp/tests/cluster/kmeans_predict_batching.cu Outdated

@tfeher tfeher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Victor, LGTM!

Comment thread cpp/src/cluster/detail/minClusterDistanceCompute.cu
@viclafargue

Copy link
Copy Markdown
Contributor Author

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants