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.

Sign up for GitHub

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

Jump to bottom

Update CCCL to 2.1.0. #399

Closed
wants to merge 20 commits into from
Closed

Update CCCL to 2.1.0. #399

wants to merge 20 commits into from

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Apr 14, 2023 •
edited
Loading

Description

This PR updates rapids-cmake to use CCCL version 2.1.0 (thrust, cub, libcudacxx). I will test this downstream with other repositories before opening this draft for review.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@bdice bdice mentioned this pull request Apr 14, 2023
Merged
4 tasks
@bdice
Copy link
Contributor Author

bdice commented Apr 15, 2023 •
edited
Loading

Downstream testing PRs (will edit this comment as I test more repos). I will check the box when the downstream PR passes CI.

@bdice bdice mentioned this pull request Apr 19, 2023
Closed
robertmaynard
robertmaynard reviewed Apr 19, 2023
View reviewed changes
RAPIDS.cmake Outdated Show resolved Hide resolved
robertmaynard
robertmaynard requested changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Given that Thrust now brings in a copy of libcudacxx we will need to update rapids-cmake/cpm/thrust.cmake to first call rapids-cmake/cpm/libcudacxx.cmake so that we always use our version of libcudacxx.

@sleeepyjack sleeepyjack mentioned this pull request Apr 25, 2023
Merged
@bdice bdice self-assigned this Apr 26, 2023
@bdice bdice added breaking Introduces a breaking change improvement Improves an existing functionality labels Apr 26, 2023
@bdice
Copy link
Contributor Author

bdice commented Apr 26, 2023

I have now updated libcudacxx and it appears that builds/tests are passing for rmm. I verified in the logs that Thrust/CUB/libcudacxx versions 2.1.0 are being used.

I will open testing PRs for other RAPIDS repositories.

(cross-posted from rapidsai/rmm#1247 (comment))

@bdice
Copy link
Contributor Author

bdice commented Apr 27, 2023

Several packages (cuDF, cuML, cuGraph) are complaining about problems with device lambdas. I will need to investigate this further. We'll likely need to write proper functors/callable structs for these lambdas.

ajschmidt8
ajschmidt8 previously approved these changes Apr 27, 2023
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

EDIT---

whoops, didn't realize this was still draft. I'll probably have to re-approve when it's ready, but that's fine.

@bdice
Copy link
Contributor Author

bdice commented May 10, 2023

Update: there was a minor issue with cuda::proclaim_return_type which is fixed in NVIDIA/libcudacxx#448. I have a smaller patch in this PR that I'll update once that upstream PR is merged. We could use an untagged commit hash instead of a patch once that is merged... not sure which approach we tend to prefer in versions.json.

@robertmaynard
Copy link
Contributor

Update: there was a minor issue with cuda::proclaim_return_type which is fixed in NVIDIA/libcudacxx#448. I have a smaller patch in this PR that I'll update once that upstream PR is merged. We could use an untagged commit hash instead of a patch once that is merged... not sure which approach we tend to prefer in versions.json.

Go with a tag and a versions.json patch entry.

@bdice bdice mentioned this pull request May 15, 2023
Closed
This was referenced Jul 10, 2023
Closed
Open
Open
@bdice bdice changed the base branch from branch-23.06 to branch-23.10 July 21, 2023 17:47
@bdice bdice mentioned this pull request Jul 24, 2023
Open
This was referenced Aug 2, 2023
Merged
Closed
PointKernel pushed a commit to NVIDIA/cuCollections that referenced this pull request Aug 2, 2023
@bdice
Use rapids-cmake 23.10. (#342)
e29b25b 
This PR updates cuCollections to rapids-cmake 23.10. This helps prepare
for CCCL 2.1.0 updates
(rapidsai/rapids-cmake#399).
@marcinz marcinz mentioned this pull request Aug 23, 2023
Closed
@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 30, 2023

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.

@bdice
Copy link
Contributor Author

bdice commented Aug 30, 2023

/ok to test

bdice and others added 15 commits August 30, 2023 18:38
@bdice
Use libcudacxx from rapids-cmake rather than Thrust bundled version.
ded488a
@bdice
Fix git_tag for libcudacxx.
231bc64
@bdice
Don't install libcudacxx twice in rapids_core_dependencies.
9f674bb
@bdice
Remove outdated Thrust patches.
ed4257c
@bdice
Add patch for libcudacxx: "Use pragma to disable execution checks in …
227ccfe 
…cuda::proclaim_return_type."
@bdice
Update rapids-cmake/rapids-version.cmake
c5fb154
@bdice
Update reroot_support patch.
ac26379
@bdice
Update install_rules patch.
5f0e475
@bdice
Fix reroot support to apply after install_rules.diff.
f5a0c19
@bdice
Apply suggested changes from Rob.
ed46757
@robertmaynard @bdice
Ensure that patches with install rules execute as expected
dc7c323
@bdice
Expect libcudacxx 2.1.0 instead of 1.8.0 in Thrust config.
1c05b50
@robertmaynard @bdice
Ensure that thrust sees where our version of libcudacxx is
3e51227 
This will make sure that thrust uses our version of libcudacxx
instead of the version packaged inside of it.
@robertmaynard @bdice
Ensure that thrust finds the rapids-cmake provided libcudacxx
24f443c
@bdice
Mark patch as fixed in 2.2.
db19079
@bdice
Copy link
Contributor Author

bdice commented Aug 31, 2023

I rebased and force-pushed this PR so that all commits are signed. Now, future changes should not require /ok to test for CI to run.

@bdice bdice mentioned this pull request Sep 12, 2023
Closed
This was referenced Sep 14, 2023
Closed
Closed
@bdice bdice changed the base branch from branch-23.10 to branch-23.12 September 22, 2023 17:48
@bdice bdice mentioned this pull request Sep 22, 2023
Merged
7 tasks
rapids-bot bot pushed a commit that referenced this pull request Oct 16, 2023
@bdice
Update libcudacxx to 2.1.0 (#464)
e73fef6 
This PR separates out the libcudacxx update from #399. I am proposing to update only libcudacxx to 2.1.0, and leave thrust/cub pinned at 1.17.2 until all of RAPIDS is ready to update. Then we can move forward with #399 next.

Separating the update for libcudacxx should allow RAPIDS to use some of the new features we want while giving more time to RAPIDS libraries to migrate to CCCL 2.1.0 (particularly for breaking changes in Thrust/CUB).

**Immediate benefits of bumping only libcudacxx to 2.1.0:**
- Enables migration to Thrust/CUB 2.1.0 to be done more incrementally, because we could merge PRs using `cuda::proclaim_return_type` into cudf/etc. which would reduce the amount of unmerged code we're maintaining in the "testing PRs" while waiting for all RAPIDS repos to be ready for Thrust/CUB 2.1.0.
- Unblocks work in rmm (rapidsai/rmm#1095) and quite a few planned changes for cuCollections (such as NVIDIA/cuCollections#332, NVIDIA/cuCollections#331, NVIDIA/cuCollections#289)

**Risk Assessment:**
This should be fairly low risk because libcudacxx 2.1.0 is similar to our current pinning of 1.9.1 -- the major version bump was meant to align with Thrust/CUB and isn't indicative of major breaking changes.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #464
@bdice
Copy link
Contributor Author

bdice commented Dec 7, 2023

Closing this, now that #495 is merged. We will leave rapids_cpm_thrust and rapids_cpm_libcudacxx in place pinned to their current versions, and eventually deprecate them in favor of rapids_cpm_cccl.

@bdice bdice closed this Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertmaynard robertmaynard robertmaynard requested changes

@ajschmidt8 ajschmidt8 ajschmidt8 left review comments

Assignees

@bdice bdice

Labels
breaking Introduces a breaking change improvement Improves an existing functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bdice @robertmaynard @ajschmidt8