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

Move libcudacxx to use rapids_cpm and use newer versions #9539

Merged
merged 8 commits into from
Nov 1, 2021
Merged

Move libcudacxx to use rapids_cpm and use newer versions #9539

merged 8 commits into from
Nov 1, 2021

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Oct 27, 2021 •
edited
Loading

Updates cudf to use the newest versions of nvbench and libcudacxx to allow compilation with both CUDA 11.4 and 11.5.

@robertmaynard robertmaynard added feature request New feature or request non-breaking Non-breaking change labels Oct 27, 2021
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Oct 27, 2021
@robertmaynard robertmaynard force-pushed the fea/move_some_deps_forward branch from fd104f6 to 03ccc33 Compare October 28, 2021 15:42
@robertmaynard robertmaynard changed the title Move nvbench and libcudacxx to newer versions Move libcudacxx to use rapids_cpm and use newer versions Oct 28, 2021
@robertmaynard robertmaynard force-pushed the fea/move_some_deps_forward branch from f927c61 to 3088f44 Compare October 28, 2021 19:14
@robertmaynard
Copy link
Contributor Author

rerun tests

@robertmaynard robertmaynard mentioned this pull request Oct 29, 2021
Closed
@robertmaynard robertmaynard marked this pull request as ready for review October 29, 2021 15:58
@robertmaynard robertmaynard requested a review from a team as a code owner October 29, 2021 15:58
@github-actions github-actions bot added the Python Affects Python cuDF API. label Oct 29, 2021
vyasr
vyasr reviewed Oct 29, 2021
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.

The changes all look good to me. My only question is, do we need to be doing anything about the libcudacxx libraries as well anywhere? The logic that I see is almost exclusively related to including the headers for compilation, is rapids_cpm_libcudacxx handling making the libraries available for linking?

@robertmaynard
Copy link
Contributor Author

The changes all look good to me. My only question is, do we need to be doing anything about the libcudacxx libraries as well anywhere? The logic that I see is almost exclusively related to including the headers for compilation, is rapids_cpm_libcudacxx handling making the libraries available for linking?

rapids_cpm_libcudacxx only offers the header only bits of libcudacxx, which is the exact components that cudf requires.

@vyasr vyasr self-requested a review October 29, 2021 17:02
@robertmaynard robertmaynard requested a review from a team as a code owner October 29, 2021 17:54
@robertmaynard robertmaynard requested a review from bdice October 29, 2021 17:54
@robertmaynard
Copy link
Contributor Author

rerun tests

1 similar comment
@ajschmidt8
Copy link
Member

rerun tests

bdice
bdice approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

All looks fine to me. Thanks @robertmaynard!

@codecov
Copy link

codecov bot commented Oct 29, 2021 •
edited
Loading

Codecov Report

Merging #9539 (5cdcc4a) into branch-21.12 (ab4bfaa) will decrease coverage by 0.12%.
The diff coverage is n/a.

❗ Current head 5cdcc4a differs from pull request most recent head c8095ad. Consider uploading reports for the commit c8095ad to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9539      +/-   ##
================================================
- Coverage         10.79%   10.66%   -0.13%     
================================================
  Files               116      117       +1     
  Lines             18869    19730     +861     
================================================
+ Hits               2036     2104      +68     
- Misses            16833    17626     +793
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/sorting.py 92.90% <0.00%> (-1.21%) ⬇️
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
... and 66 more

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 ca347ff...c8095ad. Read the comment docs.

@github-actions github-actions bot added the Java Affects Java cuDF API. label Oct 29, 2021
@robertmaynard
Copy link
Contributor Author

rerun tests

@robertmaynard robertmaynard force-pushed the fea/move_some_deps_forward branch from 3d97d9a to c8095ad Compare November 1, 2021 13:06
@robertmaynard robertmaynard requested a review from a team as a code owner November 1, 2021 13:06
@revans2
Copy link
Contributor

revans2 commented Nov 1, 2021

@jbrennan333 Could you take a look at this from the java side of things. I think it is fine, but I just don't have much experience with the compression libraries

jbrennan333
jbrennan333 approved these changes Nov 1, 2021
View reviewed changes
Copy link
Contributor

@jbrennan333 jbrennan333 left a comment

Choose a reason for hiding this comment

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

+1 the nvcomp-related changes look good to me.

@robertmaynard
Copy link
Contributor Author

rerun tests

@codereport codereport mentioned this pull request Nov 1, 2021
Merged
@robertmaynard
Copy link
Contributor Author

rerun tests

@robertmaynard
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 237b0ce into rapidsai:branch-21.12 Nov 1, 2021
@robertmaynard robertmaynard deleted the fea/move_some_deps_forward branch November 1, 2021 16:25
@jakirkham jakirkham mentioned this pull request May 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrennan333 jbrennan333 jbrennan333 approved these changes

@bdice bdice bdice approved these changes

@vyasr vyasr vyasr approved these changes

@jlowe jlowe jlowe approved these changes

Assignees
No one assigned
Labels
CMake CMake build issue feature request New feature or request Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@robertmaynard @ajschmidt8 @revans2 @jlowe @vyasr @bdice @jbrennan333