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 to CCCL 2.2.0. #1315

Merged
merged 8 commits into from
Dec 20, 2023
Merged

Update to CCCL 2.2.0. #1315

merged 8 commits into from
Dec 20, 2023

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Dec 16, 2023

Description

This PR updates cuspatial to CCCL 2.2.0. Do not merge until all of RAPIDS is ready to update.

Follow-up to #1314.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@bdice bdice added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Dec 16, 2023
@bdice bdice self-assigned this Dec 16, 2023
@bdice bdice mentioned this pull request Dec 16, 2023
Closed
26 tasks
@github-actions github-actions bot added the ci label Dec 16, 2023
@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Dec 16, 2023
@github-actions github-actions bot added the cmake Related to CMake code or build configuration label Dec 18, 2023
@bdice bdice marked this pull request as ready for review December 18, 2023 16:06
@bdice bdice requested review from a team as code owners December 18, 2023 16:06
bdice
bdice commented Dec 18, 2023
View reviewed changes
ci/build_cpp.sh Outdated Show resolved Hide resolved
bdice
bdice commented Dec 18, 2023
View reviewed changes
fetch_rapids.cmake Outdated Show resolved Hide resolved
vyasr
vyasr approved these changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

It would be nice to split this PR in two since every other repo has the CCCL code changes separate from CMake, but at this point given how far along we are with everything and the impending holidays I'll go ahead and approve to get this done.

@bdice bdice mentioned this pull request Dec 19, 2023
Merged
7 tasks
rapids-bot bot pushed a commit to rapidsai/rapids-cmake that referenced this pull request Dec 19, 2023
@bdice
Add patch reverting CCCL PR 211. (#511)
ef3388f 
Something is going wrong in `thrust::copy`, so I am adding a patch to revert NVIDIA/cccl#211. This will fix the failing tests for rapidsai/cuspatial#1315.

This is where it's failing, with `thrust::copy`:
https://github.com/rapidsai/cuspatial/blob/8e8fa0689339c0288746b01a8af66fb599f5c770/cpp/tests/range/multipolygon_range_test.cu#L800

CI log: https://github.com/rapidsai/cuspatial/actions/runs/7229405991/job/19700477218?pr=1315#step:7:3280

```
[ RUN      ] MultipolygonRangeOneTest/1.OneMultipolygonRange
unknown file: Failure
C++ exception with description "__copy:: D->D: failed: cudaErrorInvalidValue: invalid argument" thrown in the test body.

[  FAILED  ] MultipolygonRangeOneTest/1.OneMultipolygonRange, where TypeParam = double (0 ms)
```

I traced it down to this CCCL PR - note that `thrust::transform(..., thrust::identity{})` works, but the new `thrust::copy` code path in NVIDIA/cccl#211 that calls `cudaMemcpyAsync` doesn't work for some reason. I was not able to identify the exact reason why that PR causes the failure. Patching this out will unblock RAPIDS and let us migrate forward with CCCL 2.2.0.

This helps with #502.

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

Approvers:
  - Michael Schellenberger Costa (https://github.com/miscco)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #511
@bdice bdice added the improvement Improvement / enhancement to an existing function label Dec 19, 2023
@bdice bdice added the breaking Breaking change label Dec 19, 2023
@bdice
Copy link
Contributor Author

bdice commented Dec 19, 2023 •
edited
Loading

FYI: @robertmaynard created rapidsai/cudf#14655 to help fix the failing devcontainer build.

edit: That PR did not fix the issue. The issue was solved by shipping a new cuml conda package with CCCL 2.2.0. The CCCL 2.2.0 files were being overwritten by the CCCL in the cuml conda package.

@bdice bdice removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Dec 19, 2023
@bdice
Copy link
Contributor Author

bdice commented Dec 20, 2023

/merge

@rapids-bot rapids-bot bot merged commit 9501982 into rapidsai:branch-24.02 Dec 20, 2023
69 checks passed
@vyasr vyasr mentioned this pull request Dec 20, 2023
Merged
3 tasks
@vyasr
Copy link
Contributor

vyasr commented Dec 20, 2023

rapidsai/cudf#14656 should resolve the underlying issue we were observing (that was patched by getting an updated copy of cuml).

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertmaynard robertmaynard robertmaynard approved these changes

@harrism harrism harrism approved these changes

@vyasr vyasr vyasr approved these changes

@raydouglass raydouglass raydouglass approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt trxcllnt is a code owner automatically assigned from rapidsai/cuspatial-cpp-codeowners

Assignees

@bdice bdice

Labels
breaking Breaking change ci cmake Related to CMake code or build configuration improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library
Projects
cuSpatial
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bdice @vyasr @robertmaynard @harrism @raydouglass