Deprecate probability parameter on SVC and LinearSVC - #8089
Conversation
|
@switch527 Thank you for the contribution! Would you mind cleaning up the PR description a bit? |
|
Sorry for the original PR body. It had template-y ### Changes / ### Verification sections that I should have caught before opening. I pushed a rewrite a few minutes ago, and the live body should be much cleaner now if you reload. One label note, the PR is currently tagged breaking + improvement, but the deprecation should be non-breaking since the sentinel default preserves prior behavior (existing code just gets a warning). Could you swap when you have a moment? |
|
Thanks for updating, but the description is still overly verbose, just focus on the high-level changes we don't need a detailed prose of all changes. We label PRs that introduce a deprecation also as breaking. |
|
Got it, cut it down. New body is up. The breaking tag is noted. Thanks for the clarification. |
|
/ok to test 9a758ee |
a4ac617 to
5646958
Compare
5646958 to
8a903af
Compare
e23d5dd to
04abb77
Compare
|
@jcrist, Heads-up: |
|
@jcrist , code currently says 26.10. Did you mean the next release (now 26.10 after the 26.08 bump) or literally 26.08? |
I've updated it. |
|
Thanks. So removal stays at 26.10 (one cycle after the 26.08 deprecation), code already reads that way, leaving the docstrings as-is. |
|
Actually, I think we should get this into the 26.06 release. Please rebase this branch on release/26.06 and update the relevant deprecation notices. |
9efe249 to
44f43bb
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDeprecates ChangesSVC/LinearSVC probability parameter deprecation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks @switch527! This mostly looks good-to-go! I'm going to fix up the merge conflict, push up a few small tweaks, and try to shepherd this through CI to get it in for the upcoming release. |
Mirrors the sklearn 1.9 deprecation (sklearn PR #32050). cuml's SVC and LinearSVC now default `probability="deprecated"`; fit() emits a FutureWarning when the user passed an explicit value. Internal call sites read the resolved value through a new `_effective_probability` property. The recommended replacement is `CalibratedClassifierCV(estimator(), ensemble=False)` from `sklearn.calibration`. Closes NVIDIA#7982.
Per review, this lands in 26.06 rather than 26.08-dev. Shift the deprecation notices accordingly: deprecated-in 26.08 to 26.06, removed-in 26.10 to 26.08, across the SVC and LinearSVC docstrings, the fit-time FutureWarning text, and the accel upstream pytest.ini filter-removal reminder.
The deprecation retarget shifted removal from 26.10 to 26.08; the filter cleanup reminders in the test tree still pointed at 26.10.
The accel integration test falls back to native sklearn, whose own FutureWarning says "was deprecated"; widen that one filter to match both. Anchor the direct-path matchers on the cuml-specific tail so they cannot accidentally pass on an unrelated FutureWarning.
44f43bb to
1ae8806
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/cuml-accel/limitations.rst`:
- Around line 510-514: Replace the incorrect reference to
sklearn.calibration.CalibratedClassifier with the correct public class
CalibratedClassifierCV in the documentation text; update the sentence that
currently mentions "wrapping SVC with sklearn.calibration.CalibratedClassifier"
to instead say "wrapping SVC with CalibratedClassifierCV" and ensure the example
remains CalibratedClassifierCV(SVC(), ensemble=False) so the class name is
consistent with the example and public API.
In `@python/cuml/cuml/svm/svc.py`:
- Around line 114-117: The docstring and user-facing deprecation warning for the
SVC `probability` parameter are inconsistent with the PR timeline; update all
occurrences that mention removal in "26.08" or the deprecated directive
"26.06"/"26.08" to the agreed removal version "26.10". Specifically edit the
deprecated directive and message strings in python/cuml/cuml/svm/svc.py (the SVC
class/docstring and any warnings triggered for the `probability` parameter) and
also change the matching strings around lines referenced (e.g., the other
instance at lines ~461-463) so both the `.. deprecated::` directive and the
warning text use "26.10".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c77bb940-48d8-40a1-bcc1-c1e20368a019
📒 Files selected for processing (14)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/svm.pypython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml_accel_tests/integration/test_svc.pypython/cuml/cuml_accel_tests/upstream/pytest.inipython/cuml/tests/explainer/test_explainer_kernel_shap.pypython/cuml/tests/explainer/test_explainer_permutation_shap.pypython/cuml/tests/test_base.pypython/cuml/tests/test_linear_svm.pypython/cuml/tests/test_pickle.pypython/cuml/tests/test_sklearn_import_export.pypython/cuml/tests/test_svm.py
🚧 Files skipped from review as they are similar to previous changes (12)
- python/cuml/tests/test_pickle.py
- python/cuml/cuml/svm/svm_base.pyx
- python/cuml/tests/test_linear_svm.py
- python/cuml/tests/explainer/test_explainer_permutation_shap.py
- python/cuml/cuml_accel_tests/integration/test_svc.py
- python/cuml/tests/test_svm.py
- python/cuml/cuml/svm/linear_svc.py
- python/cuml/tests/test_base.py
- python/cuml/cuml/accel/_overrides/sklearn/svm.py
- python/cuml/cuml_accel_tests/upstream/pytest.ini
- python/cuml/tests/test_sklearn_import_export.py
- python/cuml/tests/explainer/test_explainer_kernel_shap.py
|
/ok to test 553f48e |
|
/ok to test 9c43ad3 |
|
/merge |
1df5fa9
into
NVIDIA:release/26.06
|
Thanks a lot to both of you for bringing this over the finish line! |
|
Happy to help! |

Closes #7982
Mirrors sklearn PR #32050 on cuml.SVC and cuml.LinearSVC. Sentinel + _effective_X property pattern, same as PR #7958. FutureWarning fires from fit when the user passes an explicit value.
Also fixes a latent bug in the accel proxy where _gpu_fit was reading self.probability truthily. Default SVC() on small data would have routed through the probability code path since the sentinel string is truthy.