Increase scikit-learn compatibility to 1.5+#6666
Increase scikit-learn compatibility to 1.5+#6666rapids-bot[bot] merged 17 commits intorapidsai:branch-25.06from
Conversation
Removes conditional import logic that is no longer needed and bumps the version from 1.5 to 1.6
|
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. |
github-actions
Bot
added
conda
csadorf
added
improvement
|
/ok to test eecf4f6 |
|
/ok to test cac0fd7 |
csadorf
changed the title
csadorf
changed the title
csadorf
changed the title
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Overall LGTM! Just a couple small questions/comments.
| - reason: Test is failing with cuml.accel | ||
| - reason: Test should fail with cuml.accel (scikit-learn 1.5) | ||
| marker: cuml_accel_bugs | ||
| condition: "scikit-learn<1.6" |
There was a problem hiding this comment.
Question around this condition - do these tests pass in scikit-learn 1.6 (and if so, why?)?
Or do the tests not exist in scikit-learn 1.6? If it's the latter, would removing the condition cause the test run to fail in some way (so typos/stale test markers are detectable)? Or is the condition technically not necessary in that case?
Same with the >= 1.6 case below.
|
|
||
| ## Scikit-learn Compatibility | ||
|
|
||
| cuML is compatible with scikit-learn version 1.5 or higher. |
There was a problem hiding this comment.
"or higher" reads odd to me, but maybe it's just Monday morning. "and higher"? "and up"? Maybe just a version constraint? Feel free to ignore, maybe I'm just overthinking.
| cuML is compatible with scikit-learn version 1.5 or higher. | |
| cuML is compatible with scikit-learn >= 1.5. |
| specific: | ||
| - output_types: [conda, requirements] | ||
| matrices: | ||
| # Package versions to user for the "oldest-dep" CI run | ||
| - matrix: {dependencies: "oldest"} | ||
| packages: | ||
| - scikit-learn==1.5.2 | ||
| - matrix: | ||
| packages: |
There was a problem hiding this comment.
Nitpick: we usually put specific after common.
|
Thanks a lot for the review comments. I intent to address all questions and suggestions in a follow-up. |
|
/merge |
This PR makes scikit-learn a required dependency with a compatibility range matching that of the test range. It is a follow-up to #6666, which added compatibility with scikit-learn versions 1.5+. All conditional import logic for scikit-learn is removed. Closes #6426. Authors: - Tim Head (https://github.com/betatim) - Simon Adorf (https://github.com/csadorf) - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Simon Adorf (https://github.com/csadorf) - Kyle Edwards (https://github.com/KyleFromNVIDIA) URL: #6608
5 participants
This PR updates the scikit-learn dependency requirements and adds dependency constraints for testing:
==1.5.*to>=1.5==1.5.2Based on #6608 .