Use new input validation in cuml.linear_models/cuml.solvers - #7978
Merged
rapids-bot[bot] merged 16 commits intoApr 23, 2026
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. |
jcrist
force-pushed
the
input-validation-linear-models
branch
from
April 22, 2026 16:19
8a209b6 to
870f66c
Compare
jcrist
added
improvement
jcrist
marked this pull request as ready for review
April 22, 2026 16:19
jcrist
changed the title
cuml.linear_models/cuml.solverscuml.linear_models/cuml.solvers
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughConsolidates input/target validation using new check_* helpers across solvers and estimators, centralizes ElasticNet/Lasso GPU-fit logic into a new _ElasticNetMixin, updates solver APIs to accept estimator context and optionally return classes, and changes many 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)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuml/cuml/solvers/sgd.pyx (1)
447-460:⚠️ Potential issue | 🟠 MajorCall
check_is_fitted()before touchingself.coef_inpredict().This path reads
self.coef_.dtypeimmediately, so an unfitted estimator raises an attribute error instead of the standard not-fitted error.As per coding guidelines, "predict/transform methods must call check_is_fitted(), validate input dimensions against fitted dimensions, handle input type correctly (cuDF, pandas, NumPy), and ensure output type is consistent with input type."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/solvers/sgd.pyx` around lines 447 - 460, predict() reads self.coef_.dtype before verifying the estimator is fitted, causing AttributeError for unfitted models; add a call to check_is_fitted(self) at the start of predict() (before accessing self.coef_) and then proceed to call check_inputs(self, X, dtype=self.coef_.dtype, ...). Also ensure after check_inputs you validate input feature dimension against the fitted coef_ shape (e.g., compare X.shape[1] to self.coef_.shape[0] or similar) so predict follows the expected fitted/validation flow.python/cuml/cuml/solvers/qn.pyx (1)
558-587:⚠️ Potential issue | 🟠 MajorDon't drop the decoded class labels returned by
fit_qn().In classifier mode,
fit_qn(..., return_classes=True)gives you the original label set, butQN.fit()only storesn_classes_.QN.predict()still emits0/1orargmaxindices, so labels like{-1, 1}or strings will no longer round-trip correctly after fit.As per coding guidelines, "Silent data corruption from type coercion, incorrect handling of cuDF vs pandas vs NumPy inputs, or missing validation causing crashes on invalid input must be addressed."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/solvers/qn.pyx` around lines 558 - 587, The classifier branch of QN.fit currently discards the decoded labels returned by fit_qn(..., return_classes=True); update QN.fit so that when is_classifier is True you capture and persist the returned classes (e.g., assign the returned classes to self.classes_ or similar) instead of only storing n_classes_, and wrap them in the appropriate container (CumlArray or native array consistent with other estimator attributes) to preserve original label types for QN.predict and downstream round-trip; update any existing code that expects n_classes_ only to continue to set n_classes_ from len(self.classes_) when present.
🤖 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/linear_model/linear_regression.pyx`:
- Around line 344-345: Replace the cupyx-only sparse check in
linear_regression.pyx: import is_sparse from cuml.common.sparse_utils and change
the predicate that currently uses sp.issparse(X) (the branch setting solver =
"lsmr") to use is_sparse(X) instead so host scipy.sparse.spmatrix and device
cupyx sparse types are both detected before calling _fit_libcuml() or
cp.asarray(X); keep the existing solver assignment and surrounding logic
unchanged.
In `@python/cuml/cuml/solvers/cd.pyx`:
- Around line 19-20: Restore the deprecated alias fit_coordinate_descent to
point to the new fit_cd and include it in the module exports: add either
fit_coordinate_descent = fit_cd (or a thin wrapper that calls fit_cd and issues
a DeprecationWarning) and update __all__ to contain "fit_coordinate_descent"
alongside "fit_cd" and "CD"; also ensure the same alias/wrapper is provided for
the helper/function defined around lines 78-92 so downstream imports keep
working for one release cycle.
In `@python/cuml/cuml/solvers/qn.pyx`:
- Around line 243-245: The call to check_array in qn.pyx incorrectly uses the
keyword convert_to_dtype; change it to convert_dtype in the coef =
check_array(...) call (so it becomes convert_dtype=convert_dtype) to match
check_array's signature, and update the docstring around the parameter
description (currently labeled convert_to_dtype at line ~158) to use
convert_dtype for consistency; locate the call in the QN warm-start path and the
docstring in the same module to apply these two renames.
- Around line 184-195: The call to check_inputs inside fit_qn is missing the
convert_dtype argument, causing implicit dtype coercion; update the check_inputs
invocation in fit_qn to pass convert_dtype=convert_dtype (preserving the
existing y_dtype/return_classes logic) so callers that request strict dtype
checking are honored; ensure the symbol names match the surrounding scope
(fit_qn and convert_dtype) and run tests for QN.fit and linear-model paths after
the change.
In `@python/cuml/cuml/solvers/sgd.pyx`:
- Around line 174-182: The call to check_inputs in fit_sgd ignores the
convert_dtype parameter so callers like SGD.fit(..., convert_dtype=False) are
silently coerced; update the check_inputs invocation in fit_sgd to forward
convert_dtype (e.g., check_inputs(..., convert_dtype=convert_dtype, ...)) and
ensure the fit_sgd function signature exposes convert_dtype (and any callers
such as MBSGDClassifier.fit that call fit_sgd propagate it) so dtype conversion
behavior honors the original argument.
---
Outside diff comments:
In `@python/cuml/cuml/solvers/qn.pyx`:
- Around line 558-587: The classifier branch of QN.fit currently discards the
decoded labels returned by fit_qn(..., return_classes=True); update QN.fit so
that when is_classifier is True you capture and persist the returned classes
(e.g., assign the returned classes to self.classes_ or similar) instead of only
storing n_classes_, and wrap them in the appropriate container (CumlArray or
native array consistent with other estimator attributes) to preserve original
label types for QN.predict and downstream round-trip; update any existing code
that expects n_classes_ only to continue to set n_classes_ from
len(self.classes_) when present.
In `@python/cuml/cuml/solvers/sgd.pyx`:
- Around line 447-460: predict() reads self.coef_.dtype before verifying the
estimator is fitted, causing AttributeError for unfitted models; add a call to
check_is_fitted(self) at the start of predict() (before accessing self.coef_)
and then proceed to call check_inputs(self, X, dtype=self.coef_.dtype, ...).
Also ensure after check_inputs you validate input feature dimension against the
fitted coef_ shape (e.g., compare X.shape[1] to self.coef_.shape[0] or similar)
so predict follows the expected fitted/validation flow.
🪄 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: Pro Plus
Run ID: 010d7871-5f1b-4687-b480-6b139e249576
📒 Files selected for processing (16)
python/cuml/cuml/accel/_overrides/sklearn/linear_model.pypython/cuml/cuml/common/classification.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/linear_model/mbsgd_regressor.pypython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/solvers/cd.pyxpython/cuml/cuml/solvers/qn.pyxpython/cuml/cuml/solvers/sgd.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_mbsgd_classifier.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
csadorf
self-requested a review
April 22, 2026 18:38
Member
Author
|
Gah, a commit got lost somehow in a rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/test_input_estimators.py`:
- Around line 114-122: The call to make_dataset is passing a 1-tuple instead of
a boolean due to the trailing comma in (is_classifier(model) or
isinstance(model, cuml.QN),); remove the trailing comma so the third argument is
the boolean expression is_classifier(model) or isinstance(model, cuml.QN) (leave
the rest of the call intact) to ensure make_dataset receives a proper True/False
classifier flag for model.
🪄 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: Pro Plus
Run ID: 5b21d294-8a44-4d93-80d1-50cf37221252
📒 Files selected for processing (4)
python/cuml/cuml/linear_model/base.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_input_estimators.pypython/cuml/tests/test_pickle.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
csadorf
approved these changes
Apr 22, 2026
Member
Author
|
/merge |
This was referenced Apr 28, 2026
This was referenced May 6, 2026
This was referenced Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This applies the new input validation utilities added in #7973 to
cuml.linear_modelsandcuml.solvers.Doing this fixed ~70 failing sklearn compatibility tests for cuml proper, and at least 60 upstream tests for
cuml.accel.Fixes #7986
Fixes #7987
Part of #7428