Fixes for sklearn 1.9 release - #8208
Conversation
We previously exact pinned the `intermediate` and `oldest` dep runs, but left the `latest` run to be unbounded. This can cause CI to fail when sklearn releases a new version. For _normal_ test runs I think we want this. We're pretty flexible with sklearn compat in cuml core, so we don't anticipate large breakage with each sklearn release. The upstream sklearn tests run with `cuml.accel` are a different story. Our test infra is tightly coupled to the tests shipped with each version of sklearn. Since tests are (rightfully) not a public interface, these can change wildly between sklearn versions, leading to more effort to update our xfails and CI setup with each release. This change: - Updates our CI setup to pin the sklearn version used in all upstream sklearn test runs (oldest, intermediate, and latest) - Applies this to both the `conda-python-scikit-learn-accel-tests` runs and the `conda-python-sklearn-example-tests` runs. - Drops the `intermediate` pinnings from `test_python`. This wasn't necessary, intermediate deps are only used for sklearn upstream runs and not any other test run. It's cleaner to keep this list specific to the upstream test setup.
|
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: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a pinned scikit-learn dependency entry and exposes it to CI test scripts, and implements callback-aware ProxyBase changes that route callbacks to CPU, enforce GPU rejection when callbacks exist, sync state on partial_fit fallback, plus gated tests and docs noting CPU fallbacks. ChangesScikit-learn dependency management
Proxy callback routing, fallback, and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/cuml/cuml/accel/estimator_proxy.py`:
- Around line 389-392: The current guard that checks getattr(self._cpu,
"_skl_callbacks", ()) is too broad and forces all methods
(predict/transform/score) to fall back to CPU; change the logic so the
UnsupportedOnGPU is raised only inside fit and partial_fit paths (methods named
fit and partial_fit) when callbacks are present, and in shared dispatch
code/checks (the place referencing reason) short-circuit GPU dispatch only if a
dispatch "reason" is already set (i.e., skip re-checking/forcing CPU when
self._reason exists). Apply the same targeted change where the duplicate guard
appears (the other block around lines 467-472) so callback rejection is limited
to fit-like methods and other methods respect an existing reason flag rather
than unconditionally falling back.
🪄 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: 7e8cd233-0434-4ad4-9649-12158a847c91
📒 Files selected for processing (3)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml_accel_tests/test_estimator_proxy.py
49bf40b to
03c1683
Compare
We don't support these configured directly on any of our proxy estimators. This plumbing just ensures that things don't error. For example, configuring a `ProgressBar(0)` on a pipeline won't cause CPU fallback, but a `ProgressBar(None)` (recurses into all sub-estimators) will. In future releases once the callbacks API has stabilized we might add direct support.
03c1683 to
287a27a
Compare
jameslamb
left a comment
There was a problem hiding this comment.
Left one very small suggestion for improvement, but overall I'm supportive of this. Do anything you want with that comment. Thanks for the clear description!
| # machine, but :shrug:. All we care about is that things are plumbed | ||
| # properly, so ignoring this error here for now. This is repeated twice | ||
| # below as well. | ||
| # See https://github.com/rapidsai/cuml/issues/8212. |
There was a problem hiding this comment.
We're seeing a new issue where the _repr_mimebundle_ call (which is fully sklearn code) now fails on one CI run. I suspect a misconfigured CI machine. For now we're skipping the failure mode that occurs here to move on, opened an issue to track: #8212
This does 2 things to provide minimal but adequate support for the just released sklearn 1.9:
Pins sklearn in our upstream test runs
We previously exact pinned the
intermediateandoldestdep runs, but left thelatestrun to be unbounded. This can cause CI to fail when sklearn releases a new version.For normal test runs I think we want this. We're pretty flexible with sklearn compat in cuml core, so we don't anticipate large breakage with each sklearn release.
The upstream sklearn tests run with
cuml.accelare a different story. Our test infra is tightly coupled to the tests shipped with each version of sklearn. Since tests are (rightfully) not a public interface, these can change wildly between sklearn versions, leading to more effort to update our xfails and CI setup with each release.This change:
conda-python-scikit-learn-accel-testsruns and theconda-python-sklearn-example-testsruns.intermediatepinnings fromtest_python. This wasn't necessary, intermediate deps are only used for sklearn upstream runs and not any other test run. It's cleaner to keep this list specific to the upstream test setup.Fixes #8206.
Adds support for
sklearn.callbacksincuml.accelThis adds plumbing support for
sklearn.callbacksincuml.accel. Only 2 non-meta-estimators currently support this in sklearn (LogisticRegressionandStandardScaler). In cases where a callback is configured on either of these we now fallback to CPU. For other cases where accelerated estimators are wrapped in meta-estimators that have a callback configured we don't fall back and everything works appropriately.Fixes #8209.
With this PR we are assured that:
cumlitself still pass with the newsklearn 1.9cuml_accel_testsforcuml.accelpass withsklearn 1.9sklearn 1.9since this requires more effort than seems feasible for today given our imminent release. I don't anticipate major incompatibilities within this release though, so this should be enough to say we have compatibility with sklearn 1.9.