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

Clean up literal zero cuda_stream_view arguments #7774

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Clean up literal zero cuda_stream_view arguments #7774

merged 3 commits into from
Apr 8, 2021

Conversation

harrism
Copy link
Member

@harrism harrism commented Mar 31, 2021

Eliminates literal zero arguments to cuda_stream_view parameters in libcudf, replacing them with rmm::cuda_stream_default

Followup to rapidsai/rmm#740

@harrism harrism requested review from a team as code owners March 31, 2021 03:04
@github-actions github-actions bot added CMake CMake build issue conda libcudf Affects libcudf (C++/CUDA) code. labels Mar 31, 2021
@harrism harrism added non-breaking Non-breaking change 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond improvement Improvement / enhancement to an existing function and removed CMake CMake build issue conda labels Mar 31, 2021
@harrism harrism changed the base branch from branch-0.19 to branch-0.20 March 31, 2021 03:07
@harrism harrism removed request for a team March 31, 2021 03:07
@harrism harrism self-assigned this Mar 31, 2021
hyperbolic2346
hyperbolic2346 approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@hyperbolic2346 hyperbolic2346 left a comment

Choose a reason for hiding this comment

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

Not sure how you found all these. Looks good to me.

@hyperbolic2346
Copy link
Contributor

rerun tests

@harrism
Copy link
Member Author

harrism commented Mar 31, 2021

Not sure how you found all these. Looks good to me.

The compiler found them for me when I built with rapidsai/rmm#740. :) Otherwise that would take some pretty killer regex-fu.

@harrism
Copy link
Member Author

harrism commented Mar 31, 2021

rerun tests

@harrism harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Mar 31, 2021
@kkraus14 kkraus14 removed the 4 - Needs Review Waiting for reviewer to review or respond label Mar 31, 2021
@kkraus14
Copy link
Collaborator

kkraus14 commented Apr 1, 2021

Hmm this is failing from:

CMake Error in CMakeLists.txt:
  No known features for CUDA compiler

  ""

  version .

While building libcudf_kafka.

@harrism
Copy link
Member Author

harrism commented Apr 5, 2021

Rerun tests

@vyasr
Copy link
Contributor

vyasr commented Apr 6, 2021

I'm going to take this the last mile for @harrism. I'll aim to finish this up over the next couple of days, just holding off until CI is a bit more stable so that it doesn't have to go through multiple merge branch-0.20 -> run CI cycles.

@vyasr vyasr self-assigned this Apr 6, 2021
@codecov
Copy link

codecov bot commented Apr 8, 2021 •
edited
Loading

Codecov Report

Merging #7774 (55190cc) into branch-0.20 (599f62d) will increase coverage by 0.44%.
The diff coverage is 87.68%.

❗ Current head 55190cc differs from pull request most recent head 8c2a67a. Consider uploading reports for the commit 8c2a67a to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20    #7774      +/-   ##
===============================================
+ Coverage        82.30%   82.74%   +0.44%     
===============================================
  Files              101      103       +2     
  Lines            17053    17721     +668     
===============================================
+ Hits             14035    14663     +628     
- Misses            3018     3058      +40
Impacted Files Coverage Δ
python/cudf/cudf/utils/dtypes.py 83.44% <46.66%> (-6.45%) ⬇️
python/cudf/cudf/core/groupby/groupby.py 92.41% <78.57%> (-1.04%) ⬇️
python/cudf/cudf/core/column/lists.py 87.41% <80.00%> (+0.19%) ⬆️
python/dask_cudf/dask_cudf/backends.py 89.58% <85.71%> (-0.05%) ⬇️
python/cudf/cudf/core/column/struct.py 96.29% <86.66%> (-3.71%) ⬇️
python/cudf/cudf/core/index.py 93.04% <88.09%> (+0.01%) ⬆️
python/cudf/cudf/core/column/decimal.py 92.92% <91.48%> (-0.92%) ⬇️
python/cudf/cudf/core/column/interval.py 91.11% <92.30%> (+0.48%) ⬆️
python/cudf/cudf/core/column/column.py 87.99% <92.59%> (+0.56%) ⬆️
python/cudf/cudf/core/join/join.py 96.75% <93.75%> (+0.24%) ⬆️
... and 61 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 acf155d...8c2a67a. Read the comment docs.

@vyasr
Copy link
Contributor

vyasr commented Apr 8, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1cd2624 into rapidsai:branch-0.20 Apr 8, 2021
@karthikeyann
Copy link
Contributor

Not sure how you found all these. Looks good to me.

By this line.
constexpr cuda_stream_view(int) = delete; //< Prevent cast from 0

@galipremsagar galipremsagar mentioned this pull request Apr 15, 2021
Closed
@davidwendt davidwendt mentioned this pull request Apr 15, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Apr 15, 2021
@davidwendt
Clean up more literal zero cuda_stream_view arguments (#7968)
8a504d1 
Reference #7774 
Some more changes to files created after the previous cleanup.
This PR fixes places using the literal '0' parameter instead of `rmm::cuda_stream_default`

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Mike Wilson (https://github.com/hyperbolic2346)
  - Paul Taylor (https://github.com/trxcllnt)
  - Conor Hoekstra (https://github.com/codereport)

URL: #7968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhemstad jrhemstad jrhemstad approved these changes

@mythrocks mythrocks mythrocks approved these changes

@hyperbolic2346 hyperbolic2346 hyperbolic2346 approved these changes

Assignees

@harrism harrism

@vyasr vyasr

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@harrism @hyperbolic2346 @kkraus14 @vyasr @karthikeyann @mythrocks @jrhemstad