-
Notifications
You must be signed in to change notification settings - Fork 550
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
Test updates of CCCL (thrust, cub, libcudacxx) to 2.1.0. #5388
Conversation
Currently wheel builds are failing because of changes in libcudacxx 2.1.0, but conda C++ builds should be failing too. This is because libcuml doesn't express that it has a direct dependency on CCCL, and instead inherits that dependency from the The correct way is shown here in raft for thrust/rmm. We need to do this same thing for thrust/rmm/raft in order: https://github.com/rapidsai/raft/blob/082be6ecd4437d180bf34d5ba5d691a27b21141f/cpp/CMakeLists.txt#L153-L155 |
include(cmake/thirdparty/get_thrust.cmake) | ||
include(cmake/thirdparty/get_rmm.cmake) |
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.
Should we have a flag or something to enable this conditionally and then be able to do these test PRs without adding the get_dependency.cmake files every time>?
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 discussed with @robertmaynard about this, since I was missing some historical context here. It sounds like this was tried before in #4643 which was closed without much discussion. I think I would argue that cuML should explicitly depend on thrust and rmm. As I understand it, the only reason not to do so (and to instead get those dependencies via raft) was because of patches that raft needed on thrust/rmm. However, it doesn't look like raft has patches for thrust/rmm at present (see raft's patches directory).
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.
In other words, the historical reasons for the current state (no explicit dependencies) may be outdated and we may want to add explicit dependencies on thrust/rmm so that rapids-cmake is able to properly control these versions.
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.
Other context from @robertmaynard:
The original PR that added thrust and rmm to cuml: #3844
PR that yanked thrust/rmm/etc in 22.02: #4256
e06f987
to
cd79dff
Compare
This PR is ready to pass off to a cuML C++ dev for completion. We are planning to ship CCCL 2.1.0 support (possibly CCCL 2.2.0) in RAPIDS 23.12. The corresponding rapids-cmake changes will be merged early in the development cycle for 23.12. We are aiming for the PRs to every RAPIDS library to be ready to merge changes needed for CCCL 2 support when 23.10 burndown begins and the 23.12 branch is created. The general readiness of RAPIDS libraries for this CCCL major version bump is being tracked here: rapidsai/rapids-cmake#399 (comment) The most common changes that are needed are to wrap device lambdas where the return type is needed on the host with Be aware that some build errors might be due to other API changes in CCCL 2.1.0, so there may be additional changes necessary besides device lambda return types. For instance, the error below might be due to a CUB API change, but I wasn't immediately able to identify what was wrong:
Consult the changelogs below for more information: |
cd79dff
to
5d9311e
Compare
Closed in favor of #5623. |
This PR tests a rapids-cmake branch with CCCL (thrust, cub, libcudacxx) updated to 2.1.0. Do not merge this PR. The changes will be merged upstream in rapids-cmake after all libraries pass CI.