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

compile libnvcomp with PTDS if requested #9540

Merged
merged 3 commits into from
Nov 1, 2021
Merged

compile libnvcomp with PTDS if requested #9540

merged 3 commits into from
Nov 1, 2021

Conversation

jbrennan333
Copy link
Contributor

closes #9534

Change get_nvcomp.cmake to compile with CUDA_API_PER_THREAD_DEFAULT_STREAM is PER_THREAD_DEFAULT_STREAM is defined.

I did not add unit tests for this, but I tested it manually by building CUDF and then running a query to verify that PTDS was being used.

@jbrennan333 jbrennan333 requested a review from a team as a code owner October 27, 2021 21:25
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Oct 27, 2021
@jbrennan333 jbrennan333 self-assigned this Oct 27, 2021
@jbrennan333 jbrennan333 added 3 - Ready for Review Ready for review by team bug Something isn't working Performance Performance related issue non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. labels Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9540      +/-   ##
================================================
- Coverage         10.79%   10.66%   -0.13%     
================================================
  Files               116      117       +1     
  Lines             18869    19726     +857     
================================================
+ Hits               2036     2104      +68     
- Misses            16833    17622     +789
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 f41e05f...e48d27d. Read the comment docs.

@jbrennan333
Copy link
Contributor Author

@robertmaynard, @vyasr it looks like I need a Cmake reviewer for this. Can you take a look?

robertmaynard
robertmaynard requested changes Oct 29, 2021
View reviewed changes
@jbrennan333
Copy link
Contributor Author

Thanks for the review @robertmaynard! I will update the patch after retesting with these changes.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Oct 29, 2021
@jbrennan333 jbrennan333 added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Oct 29, 2021
@jbrennan333
Copy link
Contributor Author

I believe this is ready to merge. The 11.5 build issues are unrelated.

@jbrennan333
Copy link
Contributor Author

@robertmaynard do I need another review to get this merged?

@robertmaynard
Copy link
Contributor

@robertmaynard do I need another review to get this merged?

In general we required two reviews before merging. Maybe @vyasr or @jrhemstad could provide the second review.

vyasr
vyasr approved these changes Nov 1, 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.

LGTM

@vyasr
Copy link
Contributor

vyasr commented Nov 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 83f605c into rapidsai:branch-21.12 Nov 1, 2021
@jbrennan333
Copy link
Contributor Author

Thanks @robertmaynard and @vyasr!

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

@vyasr vyasr vyasr approved these changes

Assignees

@jbrennan333 jbrennan333

Labels
3 - Ready for Review Ready for review by team 5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] libnvcomp is not compiled with PTDS
3 participants
@jbrennan333 @robertmaynard @vyasr