Skip to content
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

simplify wheel CI scripts, other small packaging changes #6190

Merged
merged 10 commits into from
Dec 31, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Dec 18, 2024

Fixes #6023

Proposes some miscellaneous packaging cleanup:

  • updates rapids-dependency-file-generator to its latest version (1.17.0) in pre-commit config
  • removes unused PACKAGE_CUDA_SUFFIX in build_wheel.sh
  • removes unnecessary pip install cmake in test_wheel.sh
  • CMake option cleanup:
  • dependencies.yaml changes:
    • breaks some dependencies out into depends_on_* groups to reduce duplication, and for consistency with other RAPIDS projects (docs explaining this)
    • alphabetizes dependency lists

@jameslamb jameslamb added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Dec 18, 2024

This comment was marked as resolved.

@jameslamb
Copy link
Member Author

/ok to test

@jameslamb jameslamb changed the title WIP: re-organize dependencies.yaml WIP: simplify wheel CI scripts, other small packaging changes Dec 23, 2024
@jameslamb
Copy link
Member Author

/ok to test

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Dec 23, 2024
@jameslamb jameslamb changed the title WIP: simplify wheel CI scripts, other small packaging changes simplify wheel CI scripts, other small packaging changes Dec 23, 2024
@jameslamb jameslamb marked this pull request as ready for review December 23, 2024 22:24
@jameslamb jameslamb requested review from a team as code owners December 23, 2024 22:24
@@ -142,9 +173,7 @@ dependencies:
- cxx-compiler
- fmt>=11.0.2,<12
- libcumlprims==25.2.*,>=0.0.0a0
- libcuvs==25.2.*,>=0.0.0a0
- libraft-headers==25.2.*,>=0.0.0a0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move/remove libcumlprims and libraft-headers into their own depends_on_X lists. Or maybe we can remove libraft-headers here? We did that in cugraph, iirc. This is not a blocker, I defer to your judgment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to remove libraft-headers here. That was fine in rapidsai/cugraph#4805 (comment) because there, it was coming through transitively via libraft.

cuml's environments and conda recipes don't depend on libraft, so I think it's still needed here.

I just pushed b9a0bb0 moving libcumlprims and libraft-headers into depends_on lists. They were only used once so it doesn't remove any duplication today, but it's helpful for applying this depends_on convention consistently across RAPIDS, and keeps the diff of a future change using those packages in more places smaller.

I'll merge this once CI passes.

@jameslamb jameslamb force-pushed the dependencies-cleanup branch from 4d51566 to b9a0bb0 Compare December 24, 2024 16:14
@jameslamb
Copy link
Member Author

CI here is failing with this dask-related issue:

E ImportError: cannot import name 'meta_nonempty' from 'dask.dataframe.core' (/opt/conda/envs/test/lib/python3.12/site-packages/dask/dataframe/core.py)

(build link)

That should have been fixed by rapidsai/rapids-dask-dependency#77.

But for some reason CI here is not picking up the newest rapids-dask-dependency ... it's getting 25.02.00a8 when the latest (the one with that new pinning) is 25.02.00a9. (ref: https://anaconda.org/rapidsai-nightly/rapids-dask-dependency/files).

Trying one more re-run of CI. If that fails, I'll investigate why conda isn't choosing that newer rapids-dask-dependency.

@bdice
Copy link
Contributor

bdice commented Dec 30, 2024

@jameslamb We have had to purge old alpha builds of rapids-dask-dependency several times in the past when the dask pinnings have changed. In those situations, the solver was preferring an older rapids-dask-dependency build with a different dask pinning for unclear reasons. We may need to do that again.

@jameslamb
Copy link
Member Author

Ok yeah sounds good @bdice , I'll go ask for that.

By the way, I think CI here will now also be blocked until #6187 is merged. Seeing this in wheel builds:

/__w/cuml/cuml/python/cuml/build/cp310-cp310-linux_aarch64/_deps/cuvs-src/cpp/include/cuvs/cluster/kmeans.hpp(88): error: identifier "RAFT_LEVEL_INFO" is undefined

(build link)

Which I think is a result of rapidsai/raft#2530 being merged about an hour ago.

@vyasr
Copy link
Contributor

vyasr commented Dec 30, 2024

Ah yes unfortunately that is correct. I figured today was a fairly low-impact day to start merging those since most people are off. I'm currently waiting for the latest raft builds to deploy, then I'll merge the corresponding cuvs PR, then the cuml one.

@jameslamb
Copy link
Member Author

Yep all good! Thanks for keeping that rapids-logger stuff moving, I definitely think the bumps along the way will be worth it for the end state it gets us to.

@jameslamb
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 91496b3 into rapidsai:branch-25.02 Dec 31, 2024
65 checks passed
@jameslamb jameslamb deleted the dependencies-cleanup branch December 31, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci CMake CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropping CMake install in ARM wheel tests?
3 participants