Apply new validation to cuml.svm - #8029
Conversation
jcrist
added
non-breaking
jcrist
added
sklearn-api-compat
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughCentralizes input validation via check_inputs/check_y; refactors linear SVM internals to accept an estimator and is_classifier, return classes for classifiers, switch to raw CuPy allocations/pointers, wrap outputs at API boundaries, update reflect reset semantics, and narrow scikit-learn xfail expectations for SVM estimators. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cuml/cuml/svm/linear_svr.py (1)
203-203: Consider usingreset=Trueinstead ofreset="type"for fit method.According to the context snippets,
reset="type"only resets the reflected type (input type tracking via_set_output_type), but does NOT reset feature validation attributes (n_features_in_,feature_names_in_). Forfit()methods, typicallyreset=Trueshould be used to ensure both the output type and feature metadata are reset.However, looking at the implementation,
cuml.svm.linear.fitinternally callscheck_inputs(..., reset=True), which handles the feature reset. This appears to be intentional to centralize the reset logic in the shared fit function.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/svm/linear_svr.py` at line 203, Change the decorator on the fit method from `@reflect`(reset="type") to `@reflect`(reset=True) so fitting resets both the reflected output type and feature metadata (n_features_in_, feature_names_in_); locate the decorator in linear_svr.py where fit is defined and update it to reset=True, and then run tests or ensure the existing cuml.svm.linear.fit call that uses check_inputs(..., reset=True) remains compatible (no additional code changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml/svm/linear_svr.py`:
- Line 4: Replace the incorrect "import numpy as cp" with "import cupy as cp" so
that all cp.* usages (e.g., cp.float64 in the type casts and cp.isscalar checks)
refer to CuPy rather than NumPy; update the import at the top of linear_svr.py
to import cupy as cp to match the codebase convention and ensure cp.float64 and
cp.isscalar are the CuPy implementations.
---
Nitpick comments:
In `@python/cuml/cuml/svm/linear_svr.py`:
- Line 203: Change the decorator on the fit method from `@reflect`(reset="type")
to `@reflect`(reset=True) so fitting resets both the reflected output type and
feature metadata (n_features_in_, feature_names_in_); locate the decorator in
linear_svr.py where fit is defined and update it to reset=True, and then run
tests or ensure the existing cuml.svm.linear.fit call that uses
check_inputs(..., reset=True) remains compatible (no additional code changes
required).
🪄 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: 811ead37-9fad-4fe4-b64b-80a9086028b3
📒 Files selected for processing (8)
python/cuml/cuml/svm/linear.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
1b6f8e6 to
4e8ce89
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/svm/linear_svr.py (1)
208-224:⚠️ Potential issue | 🟠 MajorForward
convert_dtypeintocuml.svm.linear.fit().
LinearSVR.fit()still exposesconvert_dtype, but this call no longer passes it through. As written,convert_dtype=Falseis ignored and the centralized validator always uses its default coercion behavior. That's a regression in the public fit API.Suggested fix
coef, intercept, n_iter, _, _ = cuml.svm.linear.fit( self, X, y, sample_weight=sample_weight, + convert_dtype=convert_dtype, loss=self.loss, penalty=self.penalty, fit_intercept=self.fit_intercept, penalized_intercept=self.penalized_intercept, max_iter=self.max_iter,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/svm/linear_svr.py` around lines 208 - 224, The call to cuml.svm.linear.fit inside LinearSVR.fit is not forwarding the public convert_dtype flag, so convert_dtype set on LinearSVR is ignored; update the cuml.svm.linear.fit(...) call (the invocation in linear_svr.py that currently passes loss, penalty, C, tol, epsilon, etc.) to include convert_dtype=self.convert_dtype (or the local convert_dtype parameter) so the centralized validator receives the intended value from LinearSVR.fit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml`:
- Around line 793-796: The current xfail entries for the four SVM validation
checks
("sklearn.tests.test_common::test_estimators[LinearSVC()-check_classifier_data_not_an_array]",
"sklearn.tests.test_common::test_estimators[LinearSVR()-check_regressor_data_not_an_array]",
"sklearn.tests.test_common::test_estimators[SVC()-check_classifier_data_not_an_array]",
"sklearn.tests.test_common::test_estimators[SVR()-check_regressor_data_not_an_array]")
are incorrectly placed in the flaky bucket; remove these four lines from
cuml_accel_flaky and add them to a deterministic xfail bucket (e.g., the
invalid-sklearn-tests or an SVM-specific xfail group) so they are treated as
persistent validation gaps rather than flaky tests. Ensure the new bucket uses
the deterministic xfail configuration (not strict: false) and keep the exact
test IDs as shown so the test runner matches them.
---
Outside diff comments:
In `@python/cuml/cuml/svm/linear_svr.py`:
- Around line 208-224: The call to cuml.svm.linear.fit inside LinearSVR.fit is
not forwarding the public convert_dtype flag, so convert_dtype set on LinearSVR
is ignored; update the cuml.svm.linear.fit(...) call (the invocation in
linear_svr.py that currently passes loss, penalty, C, tol, epsilon, etc.) to
include convert_dtype=self.convert_dtype (or the local convert_dtype parameter)
so the centralized validator receives the intended value from LinearSVR.fit.
🪄 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: 3bca8710-2925-4ccf-876c-d4c09c7ddcea
📒 Files selected for processing (8)
python/cuml/cuml/svm/linear.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_sklearn_compatibility.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml/svm/svr.py
|
I like it. Do I want to know why the |
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks! LGTM. Just one minor comment.
This one is annoying. I've not actually seen the flakiness from the classifier versions ( The intent of the In our case, we sometimes see differences in output large enough to fail the test. I suspect this is due to some non-determinism in svm module, as well as differences between float32 and float64 (the non-array-but-array-like-input is coerced to float32 in our case, while the numpy array input remains at float64). This difference in dtype treatment is consistent with how sklearn's Since the actual point of the test ("does this estimator correctly handle array-like inputs") is now resolved, I'm not worried about marking this test as flaky. We do the right thing as far as this use case is concerned. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/cuml/svm/linear_svc.py (1)
243-273: ⚡ Quick winAdd a
fit_intercept=Falseregression test for the scalar-intercept path.
python/cuml/cuml/svm/linear.pyx:268-279makes the scalarintercept_branch reachable here, and this wrapper now flipsintercept_between a scalar andCumlArray. A focused test forfit_intercept=Falseon bothLinearSVCandLinearSVRwould lock down the new descriptor/interop path.As per coding guidelines, "Update unit tests when making code changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/svm/linear_svc.py` around lines 243 - 273, Add regression tests that exercise the scalar-intercept path by training models with fit_intercept=False for both LinearSVC and LinearSVR (the code path calling cuml.svm.linear.fit and assigning self.intercept_ via the scalar check cp.isscalar(intercept)). For each model, call fit with fit_intercept=False and assert that intercept_ is a plain scalar (not a CumlArray) and that coef_ remains a CumlArray; include a paired test with fit_intercept=True to confirm the intercept_ is a CumlArray in that case. Ensure tests are deterministic with a small synthetic dataset and cover both classifier and regressor to lock down the descriptor/interop path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/cuml/svm/linear_svc.py`:
- Around line 243-273: Add regression tests that exercise the scalar-intercept
path by training models with fit_intercept=False for both LinearSVC and
LinearSVR (the code path calling cuml.svm.linear.fit and assigning
self.intercept_ via the scalar check cp.isscalar(intercept)). For each model,
call fit with fit_intercept=False and assert that intercept_ is a plain scalar
(not a CumlArray) and that coef_ remains a CumlArray; include a paired test with
fit_intercept=True to confirm the intercept_ is a CumlArray in that case. Ensure
tests are deterministic with a small synthetic dataset and cover both classifier
and regressor to lock down the descriptor/interop path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 05489ca7-334a-441b-b7f0-a7ec280e8197
📒 Files selected for processing (8)
python/cuml/cuml/svm/linear.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_sklearn_compatibility.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml/svm/linear.pyx
|
/merge |
4 participants
This applies the new validation utilities to
cuml.svm, affectingSVC,SVR,LinearSVC, andLinearSVR. The most effective way to do this required some code movement to rearrange where validation occurred. Beyond that (and some increased validation checks) there should be no behavior changes.Fixes #8004.