Skip to content

Remove inertia_check Param from KMeans#8033

Merged
rapids-bot[bot] merged 4 commits intorapidsai:mainfrom
tarang-jain:rm-inertia-check
Apr 30, 2026
Merged

Remove inertia_check Param from KMeans#8033
rapids-bot[bot] merged 4 commits intorapidsai:mainfrom
tarang-jain:rm-inertia-check

Conversation

@tarang-jain
Copy link
Copy Markdown
Contributor

@tarang-jain tarang-jain commented Apr 29, 2026

Depends on rapidsai/cuvs#2015. Inertia checking is being made mandatory and rapidsai/cuvs#2015 is a breaking change. This PR is needed to prevent compilation failures.

@tarang-jain tarang-jain requested review from a team as code owners April 29, 2026 20:40
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Apr 29, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added Cython / Python Cython or Python issue CUDA/C++ labels Apr 29, 2026
@tarang-jain tarang-jain self-assigned this Apr 29, 2026
@tarang-jain tarang-jain added breaking Breaking change improvement Improvement / enhancement to an existing function and removed Cython / Python Cython or Python issue CUDA/C++ labels Apr 29, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

📝 Walkthrough

Walkthrough

Removed the inertia_check parameter from KMeans across C++ headers, parameter conversion, Python Cython bindings, and benchmark setup; benchmark now relies on the KMeansParams default for inertia behavior.

Changes

Cohort / File(s) Summary
Core Parameter Definition
cpp/include/cuml/cluster/kmeans_params.hpp, cpp/src/kmeans/kmeans_params.cpp
Deleted the bool inertia_check field from ML::kmeans::KMeansParams and removed its assignment in the to_cuvs() conversion. SPDX year range updated in the source file.
Benchmark Code
cpp/bench/sg/kmeans.cu
Removed explicit setting of kmeans.inertia_check when constructing benchmark parameters; now uses the params' default behavior.
Python Bindings (Cython)
python/cuml/cuml/cluster/cpp/kmeans.pxd
Updated the extern C++ struct declaration to remove the inertia_check member; batch_centroids is now the final declared field.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Remove inertia_check Param from KMeans' directly and concisely describes the main change across all modified files in the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description accurately explains the purpose of the changes: removing the inertia_check parameter from KMeans because inertia checking is being made mandatory in the cuvs dependency, and this PR prevents compilation failures from that breaking change.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
python/cuml/cuml/cluster/cpp/kmeans.pxd (1)

1-3: ⚠️ Potential issue | 🟡 Minor

Update the SPDX year range.

This file is modified in 2026, but the header still ends at 2025. Please bump it to include 2026.

As per coding guidelines, "Ensure copyright headers of files are up-to-date and in the correct format".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/cuml/cuml/cluster/cpp/kmeans.pxd` around lines 1 - 3, The SPDX
copyright year range in the file header using the SPDX-FileCopyrightText token
is out of date (ends at 2025); update the header string from "2019-2025" to
include 2026 (e.g., "2019-2026") so the SPDX-FileCopyrightText line reflects the
current modification year while leaving SPDX-License-Identifier unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@python/cuml/cuml/cluster/cpp/kmeans.pxd`:
- Around line 1-3: The SPDX copyright year range in the file header using the
SPDX-FileCopyrightText token is out of date (ends at 2025); update the header
string from "2019-2025" to include 2026 (e.g., "2019-2026") so the
SPDX-FileCopyrightText line reflects the current modification year while leaving
SPDX-License-Identifier unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 95b0b327-48f3-4972-b699-10878d413a2e

📥 Commits

Reviewing files that changed from the base of the PR and between c811e80 and e72fc35.

📒 Files selected for processing (4)
  • cpp/bench/sg/kmeans.cu
  • cpp/include/cuml/cluster/kmeans_params.hpp
  • cpp/src/kmeans/kmeans_params.cpp
  • python/cuml/cuml/cluster/cpp/kmeans.pxd
💤 Files with no reviewable changes (3)
  • cpp/src/kmeans/kmeans_params.cpp
  • cpp/bench/sg/kmeans.cu
  • cpp/include/cuml/cluster/kmeans_params.hpp

Copy link
Copy Markdown
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

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

LGTM! I think we might have some tests fail until the cuvs PR is merged, since we want inertia checking enabled and dropping it here would lead to the default false.

This is a breaking change because I have removed the inertia_check param from the cuML API too. So downstream users of cuML Python / C++ API will be affected.

Note that we don't consider the C++ API public in any way. And since the python API hasn't changed at all, this isn't a breaking change. I've updated the label.

@jcrist jcrist added non-breaking Non-breaking change and removed breaking Breaking change labels Apr 29, 2026
@github-actions github-actions Bot added Cython / Python Cython or Python issue CUDA/C++ labels Apr 29, 2026
@tarang-jain
Copy link
Copy Markdown
Contributor Author

Thanks! @jcrist I have also updated the PR description and removed the comment that it is a breaking change.

@tarang-jain
Copy link
Copy Markdown
Contributor Author

/ok to test 535edd8

@tarang-jain
Copy link
Copy Markdown
Contributor Author

@jcrist @csadorf looks like this did not break and all checks are passing. So we can merge this even now i.e. before rapidsai/cuvs#2015

@tarang-jain
Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot rapids-bot Bot merged commit 016840c into rapidsai:main Apr 30, 2026
101 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants