Skip to content

rename 'sklearn' conda dependency to 'scikit-learn' #743

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 4, 2025

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Mar 3, 2025

#695 introduced a conda dependency on "sklearn". There is no sklearn... the package is called scikit-learn.

This fixes that.

Notes for Reviewers

How this fixes CI

On #738, @rhdong was facing the following problem. The conda-cpp-build jobs were producing packages with version 25.04.00a94:

BUILD START: ['libcuvs-25.04.00a94-cuda11_250303_g2ffe160_94.conda',
'libcuvs-static-25.04.00a94-cuda11_250303_g2ffe160_94.conda',
'libcuvs-examples-25.04.00a94-cuda11_250303_g2ffe160_94.conda',
'libcuvs-tests-25.04.00a94-cuda11_250303_g2ffe160_94.conda']

(logs link)

But at test time, they were getting 25.05.00a84:

...
libcuvs                   25.04.00a84     cuda11_250224_ga1e0cc0_84    rapidsai-nightly

(logs link)

It looks like this sklearn dependency was the root cause, as explained in #743 (comment)

Copy link

copy-pr-bot bot commented Mar 3, 2025

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.

@jameslamb
Copy link
Member Author

/ok to test

@jameslamb
Copy link
Member Author

aha!! Think this would explain it!

LibMambaUnsatisfiableError: Encountered problems while solving:
  - nothing provides sklearn >=1.5 needed by cuvs-bench-25.04.00a95-cuda11_py310_250303_g6ba6666_95

Could not solve for environment specs
The following packages are incompatible
├─ cuvs-bench 25.4.*,>=0.0.0a0  is installable with the potential options
│  ├─ cuvs-bench [25.04.00a86|25.04.00a87|...|25.04.00a95] would require
│  │  └─ sklearn >=1.5 , which does not exist (perhaps a missing channel);
│  ├─ cuvs-bench 25.04.00a84 would require
│  │  └─ cuvs 25.04.00a84.*  with the potential options
│  │     ├─ cuvs 25.04.00a84 would require
│  │     │  └─ libcuvs 25.04.00a84.* , which can be installed;
│  │     ├─ cuvs 25.04.00a84 would require
│  │     │  └─ python >=3.11,<3.12.0a0 , which can be installed;
│  │     └─ cuvs 25.04.00a[84](https://github.com/rapidsai/cuvs/actions/runs/13640201652/job/38132073495?pr=743#step:9:85) would require
│  │        └─ python >=3.12,<3.13.0a0 , which can be installed;
│  ├─ cuvs-bench 25.04.00a84 would require
│  │  ├─ python >=3.11,<3.12.0a0 , which can be installed;
│  │  └─ python_abi 3.11.* *_cp311, which can be installed;
│  └─ cuvs-bench 25.04.00a84 would require
│     └─ python_abi 3.12.* *_cp312, which can be installed;
├─ libcuvs 25.4.*,>=25.4.00a[91](https://github.com/rapidsai/cuvs/actions/runs/13640201652/job/38132073495?pr=743#step:9:92)  is not installable because it conflicts with any installable versions previously reported;
└─ python 3.10**  is not installable because there are no viable options
   ├─ python 3.10.8 conflicts with any installable versions previously reported;
   └─ python [3.10.0|3.10.1|...|3.10.9] would require
      └─ python_abi 3.10.* *_cp310, which conflicts with any installable versions previously reported.

(build link)

There is no package called sklearn... it's called scikit-learn. So the solver is falling back to the last package before #695 added an sklearn dependency.

@jameslamb
Copy link
Member Author

/ok to test

@jameslamb jameslamb added improvement Improves an existing functionality non-breaking Introduces a non-breaking change and removed DO NOT MERGE labels Mar 3, 2025
@jameslamb jameslamb changed the title [NEVER MERGE] test CI rename 'sklearn' conda dependency to 'scikit-learn' Mar 3, 2025
@jameslamb jameslamb marked this pull request as ready for review March 3, 2025 23:21
@jameslamb jameslamb requested a review from a team as a code owner March 3, 2025 23:21
@jameslamb jameslamb requested a review from KyleFromNVIDIA March 3, 2025 23:21
@jameslamb
Copy link
Member Author

Looks like this worked... conda is now installing the package built in this same CI run.

libcuvs                   25.04.00a92     cuda11_250303_g7fbe487_92    file:///tmp/cpp_channel

(build link)

@betatim
Copy link
Member

betatim commented Mar 4, 2025

For my curiosity/education: is there a way to tell the tool used to build conda packages that if one of the dependencies doesn't exist, it should error/warn (very loudly)? It seems hard to imagine a situation where I add a dependency to my package's dependencies list and I am Ok with it not existing.

Basically, I was wondering how it happened that it was hard to spot that sklearn as a dependency was a mistake in #695

(For PyPI there is a sklearn that will error if you install it. I guess for conda-forge no one has bothered to create a similar package, also not clear if it would be smart)

@jameslamb
Copy link
Member Author

It seems hard to imagine a situation where I add a dependency to my package's dependencies list and I am Ok with it not existing.

"not existing" means "not found in the channels conda is configured to look at during the build", not "could never be found in other channels".

There are cases where that's helpful. For example, cugraph-dgl has a runtime (note: not build-time) dependency on dgl.

https://github.com/rapidsai/cugraph-gnn/blob/8074120bd99992aa033632871c039136bf49d26c/conda/recipes/cugraph-dgl/meta.yaml#L28

The CUDA dgl packages are distributed via the dglteam channel (not conda-forge), and we don't make that channel available at build time in CI. That makes builds a bit faster.

is there a way to tell the tool used to build conda packages that if one of the dependencies doesn't exist, it should error/warn (very loudly)?

This is one of many failure modes that are intended to be caught by the test: section of the conda recipe, which will do things like run python -c "import {module}". Something like that would have had a good chance of catching this mistake.

And we have that in many recipes across RAPIDS...

tests:
requirements:
- cuda-version ={{ cuda_version }}
imports:
- cuvs

... but skip it by passing --no-test ...

cuvs/ci/build_python.sh

Lines 28 to 31 in af81a61

rapids-conda-retry build \
--no-test \
--channel "${CPP_CHANNEL}" \
conda/recipes/cuvs

... because conda builds run on CPU-only runners, and so many RAPIDS packages cannot be import'd without a GPU in the environment.

The most reliable way to catch problems like this given those constraints is to force conda to install exactly the package built in CI at test-time. If we had that, the builds in #695 would have failed with a big loud solver error like #743 (comment)

You may want to follow conversation about that at these places:

@bdice
Copy link
Contributor

bdice commented Mar 4, 2025

Thanks, this fix makes sense. Strict channel priority will help us a lot...

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 89b0349 into rapidsai:branch-25.04 Mar 4, 2025
64 checks passed
@jameslamb jameslamb deleted the test-ci branch March 4, 2025 15:58
rapids-bot bot pushed a commit to rapidsai/cuml that referenced this pull request Mar 19, 2025
Fixes #6403 

This project publishes a conda package, `cuml-cpu`, which does what it sounds like... allows the use of cuML on systems without a GPU.

This proposes some updates to packaging for `cuml-cpu`:

* fixes importing in CPU-only environment (broken in 25.04, see #6403)
* enables import tests during conda builds, to reduce the risk of such issues going undetected in the future

## Notes for Reviewers

### Why all these changes in Python code?

See some of the challenges I faced documented in #6400 (comment). 

In short, `import cuml` when it was installed via `cuml-cpu` will break at import time whenever modules imported with `cuml.internals.safe_imports.gpu_only_import()` are used in any of the following ways:

* type hints
* decorators
* any other module-level direct use

Like this:

```text
cuml.internals.safe_imports.UnavailableError: cudf is not installed in non GPU-enabled installations
```

### How long has this been broken? What's the root cause?

It seems like something changed within 25.04... earlier versions of cuML are not affected by these issues: #6403 (comment)

I don't know what the root cause is. Maybe some changes to `cuml`'s top-level imports in 25.04 is now pulling in the modules with these problems at runtime, when previously it wasn't? I'm really not sure.

### Benefits of these Changes

This adds a bit of test coverage in CI, minimally verifying that `cuml-cpu` is installable and that `import cuml` works in an environment without a GPU.

Inspired by:

* similar changes in `cuvs`: rapidsai/cuvs#750
* this conversation I recently had with @betatim : rapidsai/cuvs#743 (comment)

### How I tested this

Saw stuff like this in `conda-python-build` jobs, confirming that the import tests were running and passing:

```text
BUILD START: ['cuml-cpu-25.04.00a137-py310_250312_g153b21870_137.conda']
...
import: 'cuml'
...
Resource usage statistics from testing cuml-cpu:
...
   Time elapsed: 0:00:10.0
...
TEST END: /tmp/conda-bld-output/linux-64/cuml-cpu-25.04.00a137-py310_250312_g153b21870_137.conda
```

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Gil Forsyth (https://github.com/gforsyth)
  - Simon Adorf (https://github.com/csadorf)
  - Tim Head (https://github.com/betatim)

URL: #6400
jiangyinzuo pushed a commit to jiangyinzuo/cuvs that referenced this pull request Mar 27, 2025
rapidsai#695 introduced a conda dependency on `"sklearn"`. There is no `sklearn`... the package is called `scikit-learn`.

This fixes that.

## Notes for Reviewers

### How this fixes CI

On rapidsai#738, @rhdong was facing the following problem. The `conda-cpp-build` jobs were producing packages with version `25.04.00a94`:

```text
BUILD START: ['libcuvs-25.04.00a94-cuda11_250303_g2ffe160_94.conda',
'libcuvs-static-25.04.00a94-cuda11_250303_g2ffe160_94.conda',
'libcuvs-examples-25.04.00a94-cuda11_250303_g2ffe160_94.conda',
'libcuvs-tests-25.04.00a94-cuda11_250303_g2ffe160_94.conda']
```

([logs link](https://github.com/rapidsai/cuvs/actions/runs/13636442011/job/38116419908?pr=738#step:9:489))

But at test time, they were getting `25.05.00a84`:

```text
...
libcuvs                   25.04.00a84     cuda11_250224_ga1e0cc0_84    rapidsai-nightly
```

([logs link](https://github.com/rapidsai/cuvs/actions/runs/13636442011/job/38125502858?pr=738#step:9:4583))

It looks like this `sklearn` dependency was the root cause, as explained in rapidsai#743 (comment)

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Ben Frederickson (https://github.com/benfred)
  - Bradley Dice (https://github.com/bdice)

URL: rapidsai#743
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change Python
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants