Migrate from rmm::cuda_stream_view to cuda::stream_ref - #5014
Conversation
Greptile SummaryThe PR migrates native CUDA stream handling from
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains in the reviewed follow-up scope. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[JNI and native callers] --> B[cuda::stream_ref]
B --> C[cuDF and RMM operations]
B --> D[Raw CUDA APIs via stream.get]
B --> E[Host synchronization via stream.sync]
F[cuDF submodule and dependency pins] --> C
F --> D
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
Signed-off-by: Bradley Dice <bdice@bradleydice.com>
|
Build |
|
For the record, the immediate problem of the build-error in All that said, switching to the new |
Resolve the stream_ref conflicts to match d4f603d9027baa31556a2522baa6ca2e0231d5a0, including thirdparty/cudf at 5316f3696226.
Signed-off-by: MithunR <mithunr@nvidia.com>
|
Build |
|
I have resolved conflicts, and updated to latest. |
|
Now that I've laid a hand on it, I'm not sure if I can approve this PR. 🤦 |
|
pre-commit.ci autofix |
| "always_download" : true, | ||
| "git_shallow" : false, | ||
| "git_tag" : "9af39e27161d680083b8c309b9040f8c944d68ad", | ||
| "git_tag" : "26bd4831585ce34294bce6bb3324d648cfd31747", |
There was a problem hiding this comment.
why is kvikio tag updated?
There was a problem hiding this comment.
That should not have been. That's a rogue edit. I'm testing with a corrected version.
There was a problem hiding this comment.
Thank you for catching that, @pmattione-nvidia.
|
Looks like there are still merge conflicts?.. |
Yep. It's a little surprising. I'm sorting them out now. |
|
It appears the CI failure results from NVIDIA/cudf#23803, and is being addressed in NVIDIA/cudf#23862. I'll update the |
1. Moved thirdparty/cudf to exclude NVIDIA/cudf/pull/23803. 2. Corrected kvikio version tag. Signed-off-by: MithunR <mithunr@nvidia.com>
|
Build |
|
Sorry this took so long. @igorpeshansky / @pmattione-nvidia: Would either of you mind having another look? |
| void generate_buckets(GeneratorFunc generator, | ||
| cudf::mutable_column_view output, | ||
| rmm::cuda_stream_view stream) | ||
| cuda::stream_ref stream) |
There was a problem hiding this comment.
[Optional] Do we want to #include <cuda/stream> (IWYU)? Or do we not add one if we pick it up transitively through our own header?
Also in iceberg/iceberg_datetime_util.cu, iceberg/iceberg_truncate.cu, protobuf/protobuf.cu, protobuf/protobuf_builders.cu, protobuf/protobuf_kernels.cu, aggregation64_utils.cu, case_when.cu, cast_string.cu, cast_string_to_float.cu, charset_decode.cu, datetime_rebase.cu, decimal_utils.cu, exception_with_row_index_utilities.cu, get_json_object.cu, histogram.cu, map.cu, map_zip_with_utils.cu, multiply.cu, number_converter.cu, uuid.cu.
There was a problem hiding this comment.
This is behind main. I realize you wanted to rewind past NVIDIA/cudf#23803 (per #5014 (comment)), but since the submodule-sync bot advanced it there already, nothing stops it from doing that again, so it won't protect main from breakage. But I guess this is independent of the NVIDIA/cudf#23862 fix, so I won't hold the PR on that.
Ditto for the rmm downgrade in thirdparty/cudf-pins/versions.json…
| int const start, | ||
| int const end, | ||
| rmm::cuda_stream_view stream = rmm::cuda_stream_default, | ||
| cuda::stream_ref stream = rmm::cuda_stream_default, |
There was a problem hiding this comment.
Do we also want to switch from rmm::cuda_stream_default to cudf::get_default_stream() here?
| cuda::stream_ref stream = rmm::cuda_stream_default, | |
| cuda::stream_ref stream = cudf::get_default_stream(), |
There was a problem hiding this comment.
Yeah, I can fix this too.
|
JFYI I'm working on migrating |
|
My agent opened #5053 as part of getting NVIDIA/cudf#23770 working, feel free to grab any changes you need from there or just switch to that PR if you would like. |
Summary
rmm::cuda_stream_viewtocuda::stream_ref.<cuda/stream>and usestream.get()andstream.sync()with the new API.Closes NVIDIA/cudf-spark#15685
Related to NVIDIA/cudf#23636