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

Update get_cucollections to use rapids-cmake #11139

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jun 22, 2022

This ensures that a consistent cuco version is used across all of RAPIDS. See rapidsai/rapids-cmake#201.

@vyasr vyasr added 3 - Ready for Review Ready for review by team CMake CMake build issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 22, 2022
@vyasr vyasr requested a review from a team as a code owner June 22, 2022 23:24
@vyasr vyasr self-assigned this Jun 22, 2022
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 22, 2022
@PointKernel
Copy link
Member

I have a question about how to smoothly update cuco git tag in the future: if there is a breaking change coming with the new git tag, should the person open PRs in all related repos like cudf, raft and cugraph to fix the breaking conflicts? Also, CI tests will break if the git tag gets updated but the fixing PRs are not merged. Though the CI failures are temporary, do we have a way to avoid them?

@robertmaynard
Copy link
Contributor

For breaking changes you can use an override in a PR for each project first to roll out the change, and verify everything works.

Before merging the PR's we update rapids-CMake and remove the overrides and merge the PRs.

@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@6362fbe). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.08   #11139   +/-   ##
===============================================
  Coverage                ?   86.34%           
===============================================
  Files                   ?      144           
  Lines                   ?    22729           
  Branches                ?        0           
===============================================
  Hits                    ?    19625           
  Misses                  ?     3104           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6362fbe...0bc6fee. Read the comment docs.

@PointKernel
Copy link
Member

For breaking changes you can use an override in a PR for each project first to roll out the change, and verify everything works.

Got it. I'm still a bit concerned about this since it's mainly me updating cuco. Currently, I only need to take care of cudf whenever made the change. And in the future, I need to make 3 PRs for cudf, raft and cugraph (actually 4, for rapids-cmake also). To get all PRs merged in good timing without breaking CI or blocking other developers requires some synchronization effort. This becomes especially challenging if the person (like me) doesn't have write access to all 4 repos mentioned above.

@vyasr
Copy link
Contributor Author

vyasr commented Jun 23, 2022

The current solution just kicks the can down the road, though. The way that things currently work, if we update cuco in cudf and don't do so in raft, everything is fine until we hit some sort of release build where all the packages are compiled into the same environment and we start hitting conflicts. By centralizing the dependency, yes it increases the possibility of breaking CI, but at least it will fail fast which is usually preferable. I do agree that it can make it trickier to synchronize the updates without sufficient permissions on all the repos, though. I see this as the lesser of two evils (and it seems like from our Slack conversations others felt the same), but I'm certainly happy to discuss it further.

This is only an issue for breaking changes, right?

@PointKernel
Copy link
Member

it will fail fast which is usually preferable

Agreed

This is only an issue for breaking changes, right?

Yes, breaking changes only. There will be one major breaking change (NVIDIA/cuCollections#157) coming soon so it makes me think of how to do the git update properly. If I have write access to all repos, I still need to get all PRs approved before merging the one in rapids-cmake. After merging that, I need to update all downstream PRs to remove the local overwrite which will result in another round of CI tests. In the best situation, I can merge those PRs once CI checks are finished so there will be a one-hour slot that CI constantly fails. Does it sound right?

@robertmaynard
Copy link
Contributor

Yes, breaking changes only. There will be one major breaking change (NVIDIA/cuCollections#157) coming soon so it makes me think of how to do the git update properly. If I have write access to all repos, I still need to get all PRs approved before merging the one in rapids-cmake. After merging that, I need to update all downstream PRs to remove the local overwrite which will result in another round of CI tests. In the best situation, I can merge those PRs once CI checks are finished so there will be a one-hour slot that CI constantly fails. Does it sound right?

That timeline sounds correct to me.

@vyasr
Copy link
Contributor Author

vyasr commented Jun 23, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 6cc498f into rapidsai:branch-22.08 Jun 23, 2022
@vyasr vyasr deleted the feature/use_rapids_cpm_cuco branch June 23, 2022 16:13
rapids-bot bot pushed a commit to rapidsai/raft that referenced this pull request Jun 27, 2022
This ensures that a consistent cuco version is used across all of RAPIDS. See rapidsai/rapids-cmake#201 and rapidsai/cudf#11139

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

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

URL: #722
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants