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

Replace remaining instances of rmm::cuda_stream_default with cudf::default_stream_value #11082

Merged

Conversation

jbrennan333
Copy link
Contributor

Closes #10864.
Also closes #9614.

This PR is a follow-up to #10877. It replaces all of the remaining instances of rmm::cuda_stream_default with cudf::default_stream_value.

There are a lot of replacements and addition of includes, along with some reformatting due to clang-format, but like #10877, there should be no noticeable functional change here.

@jbrennan333 jbrennan333 self-assigned this Jun 8, 2022
@github-actions github-actions bot added Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Jun 8, 2022
@jbrennan333 jbrennan333 added feature request New feature or request 4 - Needs Review Waiting for reviewer to review or respond not a bug Performance Performance related issue non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. Java Affects Java cuDF API. labels Jun 8, 2022
@jbrennan333
Copy link
Contributor Author

I should clarify that when cuDF is compiled with LIBCUDF_USE_PER_THREAD_DEFAULT_STREAM, this does ensure that any calls to outside libraries that might not have been compiled for PTDS will be called with the per thread default stream instead of the default stream, so in that respect it is a functional change. All known instances where we needed that change were addressed in #10877, specifically code that called into nvcomp-2.3.

@github-actions github-actions bot added Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Jun 8, 2022
@jbrennan333
Copy link
Contributor Author

It looks like the gpuCI/cudf/build/cuda-arm64/11.5 build has been broken for a while?

02:04:20 Version of installed CUDA didn't match package
02:04:20 Build step 'Execute shell' marked build as failure
02:04:20 [Set GitHub commit status (universal)] ERROR on repos [GHRepository@25f3499f[nodeId=MDEwOlJlcG9zaXRvcnk5MDUwNjkxOA==,description=cuDF - GPU DataFrame Library ,homepage=[http://rapids.ai,name=cudf,fork=false,archived=false,visibility=public,size=100218,milestones={},language=C++,commits={},source](http://rapids.ai%2Cname%3Dcudf%2Cfork%3Dfalse%2Carchived%3Dfalse%2Cvisibility%3Dpublic%2Csize%3D100218%2Cmilestones%3D%7B%7D%2Clanguage%3Dc++%2Ccommits%3D%7B%7D%2Csource/)=<null>,parent=<null>,isTemplate=false,compareUsePaginatedCommits=false,url=https://api.github.com/repos/rapidsai/cudf,id=90506918,nodeId=<null>,createdAt=2017-05-07T03:43:37Z,updatedAt=2022-06-07T20:05:49Z]] (sha:af5df59) with context:gpuCI/cudf/build/cuda-arm64/11.5

@jbrennan333
Copy link
Contributor Author

I believe the CI failures are unrelated to my changes, so I am going to open this up for review.

@jbrennan333 jbrennan333 marked this pull request as ready for review June 15, 2022 15:34
@jbrennan333 jbrennan333 requested review from a team as code owners June 15, 2022 15:34
@jbrennan333 jbrennan333 requested review from vuule and nvdbaranec June 15, 2022 15:34
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

Here are few instances of potential replacement.

cpp/src/column/column.cu:185:  rmm::cuda_stream_view stream{};
cpp/src/io/utilities/hostdevice_vector.hpp:164:  rmm::cuda_stream_view stream{};
cpp/src/io/csv/reader_impl.cu:91:  selected_rows_offsets(rmm::cuda_stream_view stream) : all{0, stream}, selected{all} {}
cpp/tests/column/column_device_view_test.cu:39:  rmm::cuda_stream_view stream{};
cpp/tests/column/column_device_view_test.cu:57:  rmm::cuda_stream_view stream{};
cpp/tests/table/experimental_row_operator_tests.cu:53:  rmm::cuda_stream_view stream{};
cpp/tests/table/experimental_row_operator_tests.cu:76:  rmm::cuda_stream_view stream{};
cpp/tests/table/experimental_row_operator_tests.cu:101:  rmm::cuda_stream_view stream{};
cpp/tests/table/experimental_row_operator_tests.cu:125:  rmm::cuda_stream_view stream{};
cpp/tests/table/table_view_tests.cu:45:  rmm::cuda_stream_view stream{};
cpp/include/cudf/column/column.hpp:182:                     rmm::cuda_stream_view stream = rmm::cuda_stream_view{});
cpp/benchmarks/synchronization/synchronization.hpp:36:        rmm::cuda_stream_view stream{}; // default stream, could be another stream

Also, unused stream member in cpp/src/transform/row_bit_count.cu:164

cpp/include/cudf/column/column.hpp Show resolved Hide resolved
@vuule
Copy link
Contributor

vuule commented Jun 16, 2022

Could we patch rmm locally to throw if a stream view of a default stream is created and run tests? Looks like it's easy to miss a few when checking manually.

@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

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

@@               Coverage Diff               @@
##             branch-22.08   #11082   +/-   ##
===============================================
  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 ef6a390...186a4ef. Read the comment docs.

@ttnghia
Copy link
Contributor

ttnghia commented Jun 17, 2022

This change is huge. Assuming that there are a lot of other PRs that are under review, I wonder if after merging this PR then after the pending PRs are merged some will reverse back this result.

It is ideal to merge this lastly, after all other PRs, right before code freeze.

@jbrennan333
Copy link
Contributor Author

This change is huge. Assuming that there are a lot of other PRs that are under review, I wonder if after merging this PR then after the pending PRs are merged some will reverse back this result.
It is ideal to merge this lastly, after all other PRs, right before code freeze.

Thanks for the comment @ttnghia. We actually put off doing this fully at the end of the 22.06 sprint, and instead only put in the changes needed to fix nvcomp (#10877). I didn't want to make such a huge change at the end of the sprint. Even though the change is low risk, there is still a chance to introduce bugs.
I would prefer to go ahead with this one now. I will take the action to check again closer to the end of the 22.08 and put up another PR if needed.

@jbrennan333
Copy link
Contributor Author

Could we patch rmm locally to throw if a stream view of a default stream is created and run tests? Looks like it's easy to miss a few when checking manually.

Thanks for the suggestion @vuule. I will try this.

@jbrennan333
Copy link
Contributor Author

rerun tests

@ttnghia
Copy link
Contributor

ttnghia commented Jun 21, 2022

Rerun tests.

@jbrennan333
Copy link
Contributor Author

@karthikeyann I made the changes you requested. Thanks again for the review!

@jbrennan333
Copy link
Contributor Author

Thanks @karthikeyann, @nvdbaranec, @ttnghia and @vuule for the reviews! Can someone please merge this for me?

@ttnghia
Copy link
Contributor

ttnghia commented Jun 22, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit f4f3428 into rapidsai:branch-22.08 Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond feature request New feature or request Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue
Projects
None yet
6 participants