Add scikit-learn as a dependency #6438

betatim · 2025-03-14T15:06:57Z

This PR both adds scikit-learn as a dependency of cuml and sets up the CI so that we test with scikit-learn 1.5.x as the "oldest" dependency.

For now we use scikit-learn>=1.5 as the dependency for cuml itself. We might change the version based on #6437.

copy-pr-bot · 2025-03-14T15:07:01Z

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.

betatim · 2025-03-14T15:07:06Z

/ok to test

betatim · 2025-03-14T16:38:44Z

/ok to test

betatim · 2025-03-14T17:39:43Z

/ok to test

betatim · 2025-03-17T09:21:18Z

@bdice or @jameslamb for the conda based jobs the setup using oldest-deps in dependencies.yaml works. In the oldest deps job 1.5 is installed, in the other jobs 1.6.1 (the current newest version) is installed.

However in the wheels jobs we always get 1.6.1. Even in the oldest-deps job. Is there a way to have the oldest version installed? Currently the wheel based test doesn't install any dependencies directly, only via installing the cuml wheel. Is there a pattern in another repo that I can re-use that uses rapids-dependency-file-generator to install the dependencies in an explicit step (as is done for the conda jobs) and then install the wheel?

betatim · 2025-03-17T11:10:26Z

Needs #6444 to be resolved

csadorf · 2025-03-17T13:55:46Z

In addition to keeping this PR in draft status, I've applied the "DO NOT MERGE" label since this PR should not be merged into the current default branch (25.04).

jameslamb · 2025-03-17T17:30:03Z

Thanks for @. I personally think it'd be worth removing the try-catching of scikit-learn imports within this PR:

cuml/python/cuml/cuml/internals/import_utils.py

Lines 163 to 169 in 9c17b09

    
           def has_sklearn(): 
        
               try: 
        
                   import sklearn  # NOQA 
        
                   return True 
        
               except ImportError: 
        
                   return False

To be more confident that this change and the proposed floor is working the way you expect it to. But up to you all if you think that makes the PR too large.

This PR both adds scikit-learn as a dependency of cuml

The conda recipes need to be updated as well.

For cuml, scikit-learn needs to be added to the run: dependencies here:

cuml/conda/recipes/cuml/meta.yaml

Line 75 in 9c17b09

run:

If @jcrist 's PR (#6431) is merged before this one, then you should also remove scikit-learn from run_constrained: in the cuml recipe on this branch.

For cuml-cpu, the pin needs to be updated from an == to a >=, as you've done elsewhere in this PR.

cuml/conda/recipes/cuml-cpu/meta.yaml

Line 37 in 9c17b09

- scikit-learn==1.5.*

However in the wheels jobs we always get 1.6.1. Even in the oldest-deps job. Is there a way to have the oldest version installed?

In ci/test_wheel.sh, you should generate a constraints.txt file and include it in this pip install call:

cuml/ci/test_wheel.sh

Lines 12 to 14 in 9c17b09

    
           rapids-pip-retry install \ 
        
             ./dist/libcuml*.whl \ 
        
             "$(echo ./dist/cuml*.whl)[test]"

Here's an example from cuDF: https://github.com/rapidsai/cudf/blob/5638caacc01d8bc00803e3309310482ff58f73c8/ci/test_wheel_cudf.sh#L15-L24

betatim · 2025-03-18T14:42:33Z

/ok to test

betatim · 2025-03-19T12:29:08Z

Based on #6444 (comment) we now instantiate the classes and pass them to the constructor in SVC. This fixes a bug where we passed the wrong type to the constructor which was discovered by using scikit-learn 1.6. We don't really care about how exactly the class is configured, so we can pass more or less any constructor arguments (as far as I understand from Victor).

betatim · 2025-03-20T09:18:39Z

/ok to test

@viclafargue

…6471) The `estimator` attribute is used in the scikit-learn tags machinery to figure out what tags the meta estimator has. We pass a default constructed instance as it isn't actually used. This was found as part of #6438 but can be fixed standalone. The problem isn't with the new scikit-learn version, but we discovered it with it. cc @viclafargue Authors: - Tim Head (https://github.com/betatim) Approvers: - Jim Crist-Harif (https://github.com/jcrist) - Victor Lafargue (https://github.com/viclafargue) URL: #6471

This also makes it so that we test scikit-learn 1.5.x as "oldest" dependency.

This means that the version of scikit-learn that is installed via the wheel install is constrained to the oldest version cuml supports.

We can't pass a class, we need to pass an instance so that the tags infrastructure works.

copy-pr-bot · 2025-04-07T11:15:23Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

betatim · 2025-04-07T11:18:57Z

/ok to test

…-as-dependency

betatim · 2025-04-07T11:27:28Z

/ok to test

betatim · 2025-04-07T15:40:48Z

/ok to test

betatim · 2025-04-08T06:13:52Z

/ok to test

With the minimum version of dask that we require numpy 1.23 can't be installed because it is too old for dask

betatim · 2025-04-08T11:50:04Z

/ok to test

Ridge.solver_ was only added in scikit-learn v1.5, so we need to conditionally set it in the accelerator.

betatim · 2025-04-09T07:00:52Z

/ok to test

Different versions of scikit-learn have different default values for the constructor. Try and handle those cases gracefully

github-actions bot added conda conda issue Cython / Python Cython or Python issue labels Mar 14, 2025

github-actions bot added the ci label Mar 14, 2025

csadorf linked an issue Mar 14, 2025 that may be closed by this pull request

Make sklearn a required dependency #6426

Open

csadorf added the DO NOT MERGE Hold off on merging; see PR for details label Mar 17, 2025

csadorf mentioned this pull request Mar 17, 2025

[BUG] classes instead of instances are passed to meta estimators in SVC #6444

Closed

betatim mentioned this pull request Mar 20, 2025

FIX Pass instances to the constructor of the meta-estimator in SVC #6471

Merged

csadorf assigned betatim Apr 3, 2025

betatim changed the base branch from branch-25.04 to branch-25.06 April 7, 2025 09:08

betatim changed the base branch from branch-25.06 to branch-25.04 April 7, 2025 09:12

betatim changed the base branch from branch-25.04 to branch-25.06 April 7, 2025 09:13

betatim added 7 commits April 7, 2025 11:17

Add scikit-learn as a dependency

41cd558

This also makes it so that we test scikit-learn 1.5.x as "oldest" dependency.

No stars

ab297c4

Pass dependency selector on

c1fa901

Use constraints when installing the wheel

0679576

This means that the version of scikit-learn that is installed via the wheel install is constrained to the oldest version cuml supports.

Make scikit-learn imports unconditional

4ece755

Fix constructor argument type

de2f4af

We can't pass a class, we need to pass an instance so that the tags infrastructure works.

Fix dependency generation key

6fadf51

betatim added 2 commits April 7, 2025 11:19

Trigger CI

7f34bfb

Go down to scikit-learn 1.3.2

cd22e27

betatim force-pushed the add-sklearn-as-dependency branch from 6635720 to cd22e27 Compare April 7, 2025 11:15

Add more "oldest" deps

32adc0b

Merge remote-tracking branch 'upstream/branch-25.06' into add-sklearn…

6854625

…-as-dependency

Fix styling

dc49669

Bump Numpy minimum version

0dbe68e

With the minimum version of dask that we require numpy 1.23 can't be installed because it is too old for dask

betatim added 3 commits April 8, 2025 16:25

Less subsampling

fb14e75

Make solver_ attribute conditional

327f340

Ridge.solver_ was only added in scikit-learn v1.5, so we need to conditionally set it in the accelerator.

Include cudf fixes

db21757

Handle default parameter translations

eefbafa

Different versions of scikit-learn have different default values for the constructor. Try and handle those cases gracefully

betatim mentioned this pull request Apr 10, 2025

Check that changing hyper-parameter defaults are handled #6526

Draft

csadorf linked an issue Apr 25, 2025 that may be closed by this pull request

Support scikit-learn version 1.6.* #6504

Open

2 tasks

betatim mentioned this pull request Apr 30, 2025

Make scikit-learn 1.6 a required dependency #6608

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scikit-learn as a dependency #6438

Add scikit-learn as a dependency #6438

betatim commented Mar 14, 2025 •

edited

Loading

copy-pr-bot bot commented Mar 14, 2025

betatim commented Mar 14, 2025

betatim commented Mar 14, 2025

betatim commented Mar 14, 2025

betatim commented Mar 17, 2025

betatim commented Mar 17, 2025

csadorf commented Mar 17, 2025

jameslamb commented Mar 17, 2025

betatim commented Mar 18, 2025

betatim commented Mar 19, 2025

betatim commented Mar 20, 2025

copy-pr-bot bot commented Apr 7, 2025

betatim commented Apr 7, 2025

betatim commented Apr 7, 2025

betatim commented Apr 7, 2025

betatim commented Apr 8, 2025

betatim commented Apr 8, 2025

betatim commented Apr 9, 2025

Add scikit-learn as a dependency #6438

Are you sure you want to change the base?

Add scikit-learn as a dependency #6438

Conversation

betatim commented Mar 14, 2025 • edited Loading

copy-pr-bot bot commented Mar 14, 2025

betatim commented Mar 14, 2025

betatim commented Mar 14, 2025

betatim commented Mar 14, 2025

betatim commented Mar 17, 2025

betatim commented Mar 17, 2025

csadorf commented Mar 17, 2025

jameslamb commented Mar 17, 2025

betatim commented Mar 18, 2025

betatim commented Mar 19, 2025

betatim commented Mar 20, 2025

copy-pr-bot bot commented Apr 7, 2025

betatim commented Apr 7, 2025

betatim commented Apr 7, 2025

betatim commented Apr 7, 2025

betatim commented Apr 8, 2025

betatim commented Apr 8, 2025

betatim commented Apr 9, 2025

betatim commented Mar 14, 2025 •

edited

Loading