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 nvcomp to 2.3.3 #221

Merged
merged 5 commits into from
Aug 5, 2022
Merged

Update nvcomp to 2.3.3 #221

merged 5 commits into from
Aug 5, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jul 21, 2022 •
edited
Loading

This PR updates nvcomp to version 2.3.3. Resolves #219

@vyasr vyasr added improvement Improves an existing functionality non-breaking Introduces a non-breaking change 2 - In Progress Currenty a work in progress 5 - DO NOT MERGE Hold off on merging; see PR for details labels Jul 21, 2022
@vyasr vyasr self-assigned this Jul 21, 2022
@vyasr vyasr requested a review from a team as a code owner July 21, 2022 21:34
@vyasr
Copy link
Contributor Author

vyasr commented Jul 21, 2022

Once I've tested against cudf we can make a final call on whether or not this gets into 22.08

@vyasr vyasr mentioned this pull request Jul 21, 2022
Closed
robertmaynard
robertmaynard reviewed Jul 21, 2022
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.

We should only activate this conditional for 2.3.2 :
https://github.com/robertmaynard/rapids-cmake/blob/1b29b2e4192f0c6e46416c711de4f7608f6f65e7/rapids-cmake/cpm/nvcomp.cmake#L98

@vyasr
Copy link
Contributor Author

vyasr commented Jul 22, 2022

@robertmaynard I think that I've done this correctly, but I'm not entirely familiar with how JSON parses into strings in CMake so let me know if there's an alternative comparison that would make sense here. Also, should we be doing something like > 2.3.0 AND < 2.3.3 instead?

@robertmaynard
Copy link
Contributor

robertmaynard commented Jul 29, 2022 •
edited
Loading

Please re-target branch-22.10

vyasr
vyasr commented Aug 4, 2022
View reviewed changes
rapids-cmake/cpm/nvcomp.cmake Outdated Show resolved Hide resolved
@vyasr vyasr changed the base branch from branch-22.08 to branch-22.10 August 4, 2022 22:04
@vyasr vyasr added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Aug 4, 2022
@vyasr vyasr removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Aug 4, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Aug 4, 2022

@robertmaynard I think we are good to merge this now. rapidsai/cudf#11327 was the test that this upgrade would be safe.

@robertmaynard
Copy link
Contributor

Drop the updates to RAPIDS.cmake and you can merge away!

:shipit:

vyasr
vyasr commented Aug 5, 2022
View reviewed changes
RAPIDS.cmake Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Aug 5, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 910b4f2 into rapidsai:branch-22.10 Aug 5, 2022
@vyasr vyasr deleted the feature/nvcomp_2.3.3 branch August 5, 2022 15:58
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

Assignees

@vyasr vyasr

Labels
3 - Ready for Review Ready for review by team improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Update nvcomp version to 2.3.3
2 participants
@vyasr @robertmaynard