-
Notifications
You must be signed in to change notification settings - Fork 111
cuvs-bench-cpu: avoid 'mkl' dependency on aarch64 #750
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
cuvs-bench-cpu: avoid 'mkl' dependency on aarch64 #750
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
docs/source/cuvs_bench/index.rst
Outdated
@@ -120,7 +120,7 @@ The steps below demonstrate how to download, install, and run benchmarks on a su | |||
python -m cuvs_bench.run --dataset deep-image-96-inner --algorithms cuvs_cagra --batch-size 10 -k 10 | |||
|
|||
# (3) export data | |||
python -m cuvs_bench.data_export --dataset deep-image-96-inner | |||
python -m cuvs_bench.run.data_export --dataset deep-image-96-inner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these tests indirectly caught a mistake in the docs! When I set up the import tests, I did something like this to find which modules were referenced in documentation:
git grep -E 'cuvs_bench\.'
That showed me these references to cuvs_bench.data_export
.
However, the import tests failed:
Traceback (most recent call last):
File "/opt/conda/conda-bld/test_tmp/run_test.py", line 5, in <module>
import cuvs_bench.data_export
ModuleNotFoundError: No module named 'cuvs_bench.data_export'
...
CondaBuildUserError: TESTS FAILED: cuvs-bench-cpu-25.04.00a95-py310_250305_g0208914_95.conda
...
import: 'cuvs_bench'
import: 'cuvs_bench.data_export'
And it really does look to me like this needs to be cuvs_bench.run.data_export
... there's no top-level __init__.py
doing remapping in https://github.com/rapidsai/cuvs/tree/branch-25.04/python/cuvs_bench/cuvs_bench, and data_export
is down in cuvs_bench/run/
: https://github.com/rapidsai/cuvs/blob/89b03493b487910d2125fde6680590adde8e2a95/python/cuvs_bench/cuvs_bench/run/data_export.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now an option that is true
by default. However, for this PR please just update the line to cuvs_bench.run --data-export
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, sure! Thanks for the explanation.
Just pushed 669e169 with that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on the import, @jameslamb ! Yay tests!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved pending doc update
docs/source/cuvs_bench/index.rst
Outdated
@@ -120,7 +120,7 @@ The steps below demonstrate how to download, install, and run benchmarks on a su | |||
python -m cuvs_bench.run --dataset deep-image-96-inner --algorithms cuvs_cagra --batch-size 10 -k 10 | |||
|
|||
# (3) export data | |||
python -m cuvs_bench.data_export --dataset deep-image-96-inner | |||
python -m cuvs_bench.run.data_export --dataset deep-image-96-inner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now an option that is true
by default. However, for this PR please just update the line to cuvs_bench.run --data-export
Saw one test failure for wheels:
Strongly suspect that that's a flaky test. I just triggered a re-run. |
/merge |
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
Fixes rapidsai/docker#739 rapidsai#260 introduced a runtime dependency on `mkl` for the `cuvs-bench-cpu` conda package. There are not aarch64 packages for `mkl` on conda-forge, so this makes `cuvs-bench-cpu` impossible to install on aarch64. This fixes that, by applying the same "only add on x86_64" guard used for `mkl` everywhere else in this project, e.g. like this: https://github.com/rapidsai/cuvs/blob/89b03493b487910d2125fde6680590adde8e2a95/conda/recipes/cuvs-bench-cpu/meta.yaml#L51 It also proposes adding import tests to the `cuvs-bench-cpu` conda recipe, so issues like this can be caught in CI in the future. ## Notes for Reviewers I searched for references like this ```shell git grep -i mkl ``` ### How I tested this Saw lines like this in `conda-python-build` logs: ```text BUILD START: ['cuvs-bench-cpu-25.04.00a96-py312_250305_g94340bc_96.conda'] ... TEST START: /tmp/conda-bld-output/linux-aarch64/cuvs-bench-cpu-25.04.00a96-py312_250305_g94340bc_96.conda ... import: 'cuvs_bench' import: 'cuvs_bench.generate_groundtruth' import: 'cuvs_bench.get_dataset' import: 'cuvs_bench.plot' import: 'cuvs_bench.run' import: 'cuvs_bench.run.data_export' import: 'cuvs_bench.split_groundtruth' import: 'cuvs_bench' import: 'cuvs_bench.generate_groundtruth' import: 'cuvs_bench.get_dataset' import: 'cuvs_bench.plot' import: 'cuvs_bench.run' import: 'cuvs_bench.run.data_export' import: 'cuvs_bench.split_groundtruth' ... TEST END: /tmp/conda-bld-output/linux-aarch64/cuvs-bench-cpu-25.04.00a96-py312_250305_g94340bc_96.conda ``` ([build link](https://github.com/rapidsai/cuvs/actions/runs/13687659537/job/38276160494?pr=750#step:9:5961)) Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Gil Forsyth (https://github.com/gforsyth) - Divye Gala (https://github.com/divyegala) URL: rapidsai#750
Fixes rapidsai/docker#739
#260 introduced a runtime dependency on
mkl
for thecuvs-bench-cpu
conda package. There are not aarch64 packages formkl
on conda-forge, so this makescuvs-bench-cpu
impossible to install on aarch64.This fixes that, by applying the same "only add on x86_64" guard used for
mkl
everywhere else in this project, e.g. like this:cuvs/conda/recipes/cuvs-bench-cpu/meta.yaml
Line 51 in 89b0349
It also proposes adding import tests to the
cuvs-bench-cpu
conda recipe, so issues like this can be caught in CI in the future.Notes for Reviewers
I searched for references like this
How I tested this
Saw lines like this in
conda-python-build
logs:(build link)