Build libcudf_streaming comm benchmarks with partial MPI/UCXX availability - #22860
Merged
rapids-bot[bot] merged 9 commits intoJun 12, 2026
Merged
Conversation
pentschev
added
3 - Ready for Review
…ptional-mpi-ucxx-build
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR makes MPI and UCXX optional for libcudf_streaming: it adds a CMake module that detects available backends and exports an interface target, introduces communicator availability helpers, gates benchmark/example targets on those detections, and adds compile-time guards plus runtime validation in benchmark and NDSH code. ChangesOptional Communicator Support Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
libcudf
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/libcudf_streaming/benchmarks/streaming/bench_streaming_shuffle.cpp (1)
388-394: 💤 Low valueConsider using
RAPIDSMPF_EXPECTSfor consistency withbench_shuffle.cpp.The null check pattern here differs from
bench_shuffle.cpp(lines 246-249) which usesRAPIDSMPF_EXPECTS. While both approaches work correctly, using the same pattern would improve consistency across the benchmark codebase.♻️ Suggested change for consistency
`#ifdef` CUDF_STREAMING_HAVE_UCXX auto ucxx = std::dynamic_pointer_cast<rapidsmpf::ucxx::UCXX>(comm); - if (ucxx == nullptr) { - log.print("Expected UCXX communicator when using bootstrap mode"); - throw std::runtime_error{"Expected UCXX communicator when using bootstrap mode"}; - } + RAPIDSMPF_EXPECTS( + ucxx != nullptr, "Expected UCXX communicator when using bootstrap mode", std::runtime_error); ucxx->barrier();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/libcudf_streaming/benchmarks/streaming/bench_streaming_shuffle.cpp` around lines 388 - 394, Replace the manual null-check and throw in the bootstrap UCXX communicator block with the RAPIDSMPF_EXPECTS macro for consistency: locate the dynamic_pointer_cast to rapidsmpf::ucxx::UCXX (variable ucxx) and the subsequent null-check/throw, and change it to use RAPIDSMPF_EXPECTS(ucxx != nullptr, "Expected UCXX communicator when using bootstrap mode") (or equivalent macro signature used in bench_shuffle.cpp) and then call ucxx->barrier(); so behavior remains identical but matches the pattern used in bench_shuffle.cpp.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/libcudf_streaming/benchmarks/streaming/bench_streaming_shuffle.cpp`:
- Around line 388-394: Replace the manual null-check and throw in the bootstrap
UCXX communicator block with the RAPIDSMPF_EXPECTS macro for consistency: locate
the dynamic_pointer_cast to rapidsmpf::ucxx::UCXX (variable ucxx) and the
subsequent null-check/throw, and change it to use RAPIDSMPF_EXPECTS(ucxx !=
nullptr, "Expected UCXX communicator when using bootstrap mode") (or equivalent
macro signature used in bench_shuffle.cpp) and then call ucxx->barrier(); so
behavior remains identical but matches the pattern used in bench_shuffle.cpp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 09cb6ce0-6734-416f-b107-f3a9cae7028e
📒 Files selected for processing (17)
cpp/libcudf_streaming/CMakeLists.txtcpp/libcudf_streaming/benchmarks/CMakeLists.txtcpp/libcudf_streaming/benchmarks/bench_shuffle.cppcpp/libcudf_streaming/benchmarks/streaming/CMakeLists.txtcpp/libcudf_streaming/benchmarks/streaming/bench_streaming_shuffle.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/CMakeLists.txtcpp/libcudf_streaming/benchmarks/streaming/ndsh/bench_read.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q01.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/q09.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/utils.cppcpp/libcudf_streaming/benchmarks/streaming/ndsh/utils.hppcpp/libcudf_streaming/benchmarks/utils/comm.hppcpp/libcudf_streaming/cmake/ConfigureOptionalCommunication.cmakecpp/libcudf_streaming/cmake/thirdparty/get_rapidsmpf.cmakecpp/libcudf_streaming/examples/CMakeLists.txt
💤 Files with no reviewable changes (4)
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q09.cpp
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q01.cpp
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q04.cpp
- cpp/libcudf_streaming/benchmarks/streaming/ndsh/q03.cpp
nirandaperera
left a comment
nirandaperera
left a comment
Contributor
There was a problem hiding this comment.
Had some comments and questions
Comment on lines
+137
to
+144
| std::optional<CommType> parse_comm_type(std::string_view name) | ||
| { | ||
| auto const names = comm_type_names(); | ||
| for (std::size_t i = 0; i < names.size(); ++i) { | ||
| if (name == names[i]) { return static_cast<CommType>(i); } | ||
| } | ||
| return std::nullopt; | ||
| } |
Contributor
There was a problem hiding this comment.
nit. optional is not really needed it seems.
Suggested change
| std::optional<CommType> parse_comm_type(std::string_view name) | |
| { | |
| auto const names = comm_type_names(); | |
| for (std::size_t i = 0; i < names.size(); ++i) { | |
| if (name == names[i]) { return static_cast<CommType>(i); } | |
| } | |
| return std::nullopt; | |
| } | |
| CommType parse_comm_type(std::string_view name) | |
| { | |
| auto const names = comm_type_names(); | |
| auto it = std::ranges::find(names, name); | |
| return static_cast<CommType>(std::distance(names.begin(), it)); | |
| } |
Contributor
Author
There was a problem hiding this comment.
It is used to catch cases where the CommType is invalid, e.g., return (name == a_valid_name) ? name : std::nullopt;.
Comment on lines
+103
to
+120
| bool is_comm_type_available(CommType comm_type) | ||
| { | ||
| switch (comm_type) { | ||
| case CommType::SINGLE: return true; | ||
| #ifdef CUDF_STREAMING_HAVE_MPI | ||
| case CommType::MPI: return true; | ||
| #else | ||
| case CommType::MPI: return false; | ||
| #endif | ||
| #ifdef CUDF_STREAMING_HAVE_UCXX | ||
| case CommType::UCXX: return true; | ||
| #else | ||
| case CommType::UCXX: return false; | ||
| #endif | ||
| case CommType::MAX: return false; | ||
| } | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Nit. Bit clunky.
Suggested change
| bool is_comm_type_available(CommType comm_type) | |
| { | |
| switch (comm_type) { | |
| case CommType::SINGLE: return true; | |
| #ifdef CUDF_STREAMING_HAVE_MPI | |
| case CommType::MPI: return true; | |
| #else | |
| case CommType::MPI: return false; | |
| #endif | |
| #ifdef CUDF_STREAMING_HAVE_UCXX | |
| case CommType::UCXX: return true; | |
| #else | |
| case CommType::UCXX: return false; | |
| #endif | |
| case CommType::MAX: return false; | |
| } | |
| return false; | |
| } | |
| bool is_comm_type_available(CommType comm_type) | |
| { | |
| switch (comm_type) { | |
| case CommType::SINGLE: | |
| return true; | |
| case CommType::MPI: | |
| #ifdef CUDF_STREAMING_HAVE_MPI | |
| return true; | |
| #endif | |
| break; | |
| case CommType::UCXX: | |
| #ifdef CUDF_STREAMING_HAVE_UCXX | |
| return true; | |
| #endif | |
| break; | |
| case CommType::MAX: | |
| break; | |
| } | |
| return false; // Universal fallback | |
| } |
Comment on lines
+126
to
+133
| bool first = true; | ||
| for (std::size_t i = 0; i < static_cast<std::size_t>(CommType::MAX); ++i) { | ||
| auto const comm_type = static_cast<CommType>(i); | ||
| if (!is_comm_type_available(comm_type)) { continue; } | ||
| if (!first) { out << ", "; } | ||
| out << names[i]; | ||
| first = false; | ||
| } |
Contributor
There was a problem hiding this comment.
Nit.
Suggested change
| bool first = true; | |
| for (std::size_t i = 0; i < static_cast<std::size_t>(CommType::MAX); ++i) { | |
| auto const comm_type = static_cast<CommType>(i); | |
| if (!is_comm_type_available(comm_type)) { continue; } | |
| if (!first) { out << ", "; } | |
| out << names[i]; | |
| first = false; | |
| } | |
| for (std::size_t i = 0; i < static_cast<std::size_t>(CommType::MAX); ++i) { | |
| auto const comm_type = static_cast<CommType>(i); | |
| if (!is_comm_type_available(comm_type)) { continue; } | |
| out << names[i] << ", "; | |
| } |
Contributor
Author
There was a problem hiding this comment.
This would then return something like "mpi, ucxx, ". No reason to make that sloppy-looking to save 3 lines of code.
Comment on lines
+39
to
+45
| #ifdef CUDF_STREAMING_HAVE_MPI | ||
| return "mpi"; | ||
| #elif defined(CUDF_STREAMING_HAVE_UCXX) | ||
| return "ucxx"; | ||
| #else | ||
| return {}; | ||
| #endif |
Contributor
There was a problem hiding this comment.
What if CUDF_STREAMING_HAVE_MPI & CUDF_STREAMING_HAVE_UCXX both true? This defaults to mpi, but I feel like it should be,
Suggested change
| #ifdef CUDF_STREAMING_HAVE_MPI | |
| return "mpi"; | |
| #elif defined(CUDF_STREAMING_HAVE_UCXX) | |
| return "ucxx"; | |
| #else | |
| return {}; | |
| #endif | |
| #ifdef CUDF_STREAMING_HAVE_MPI | |
| #ifdef CUDF_STREAMING_HAVE_UCXX | |
| return "ucxx"; | |
| #else | |
| return "mpi"; | |
| #endif | |
| #endif | |
| return {}; |
Contributor
Author
There was a problem hiding this comment.
I'd agree, because UCXX is our main use case. However, I chose to keep the status quo for now because we may have scripts relying on the default communicator to mean mpi. I think it's best to punt this to another PR if we want to make changes to defaults.
Comment on lines
+27
to
+28
| OPTIONS "BUILD_MPI_SUPPORT ${rapidsmpf_build_mpi_support}" | ||
| "BUILD_UCXX_SUPPORT ${rapidsmpf_build_ucxx_support}" |
Contributor
There was a problem hiding this comment.
If the cmake variables dont match with the released rapidsmpf distribution, would this build rapidsmpf locally?
Contributor
Author
There was a problem hiding this comment.
I don't understand the question, we are just matching what the RapidsMPF distribution delivers. Why would we rebuild?
3 tasks
…ptional-mpi-ucxx-build # Conflicts: # cpp/libcudf_streaming/CMakeLists.txt # cpp/libcudf_streaming/benchmarks/CMakeLists.txt # cpp/libcudf_streaming/benchmarks/streaming/CMakeLists.txt
nirandaperera
approved these changes
Jun 12, 2026
Contributor
|
/merge |
10 tasks
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jun 23, 2026
Adds multi-rank C++ test executables for `cudf_streaming` that exercise the test suite under MPI and UCXX communicators, mirroring rapidsmpf's `mpi_tests`/`ucxx_tests`.
### Changes
- **`cmake/ConfigureOptionalCommunication.cmake`**: Enable MPI/UCXX detection under `BUILD_TESTS` (previously only `BUILD_BENCHMARKS`/`BUILD_EXAMPLES`), so `CUDF_STREAMING_HAVE_MPI`/`CUDF_STREAMING_HAVE_UCXX` are set for test builds.
- **`cmake/thirdparty/get_cudf.cmake`**: Request cudf's optional `testing` component when `BUILD_TESTS` is on, so `cudf::cudftestutil{,_impl}` resolve from the installed (conda) cudf package without a local cudf build.
- **`tests/environment.hpp`**: Refactor `Environment` into an abstract base exposing a polymorphic communicator interface; add `MPI`/`UCXX` to `TestEnvironmentType`. Communicator-specific state (e.g. `mpi_comm_`) moves into the concrete subclasses.
- **`tests/main/mpi.cpp`** (new): `MPIEnvironment` that initializes MPI, builds a `rapidsmpf::MPI` communicator, and provides `barrier()`/`split_comm()`.
- **`tests/main/ucxx.cpp`** (new): `UCXXEnvironment` that bootstraps UCXX over MPI (`init_using_mpi`) with the required CUDA-context/thread-level setup.
- **`tests/main/single.cpp`**: Convert to a `SingleEnvironment` subclass of the new abstract base.
- **`tests/CMakeLists.txt`**: Add a `cudf_streaming_mpirun_test_add` helper and build `cudf_streaming_mpi_tests`/`cudf_streaming_ucxx_tests` (gated on `CUDF_STREAMING_HAVE_MPI`/`UCXX`), registering per-rank `mpirun` ctest cases; apply `CUDF_CUDA_FLAGS` to the shared test sources so the injected cudf testutil `.cu` files compile.
depends on #22860
Authors:
- Niranda Perera (https://github.com/nirandaperera)
- Vyas Ramasubramani (https://github.com/vyasr)
Approvers:
- Vyas Ramasubramani (https://github.com/vyasr)
- Peter Andreas Entschev (https://github.com/pentschev)
URL: #22864
3 tasks
mhaseeb123
pushed a commit
to mhaseeb123/cudf
that referenced
this pull request
Jun 24, 2026
Adds multi-rank C++ test executables for `cudf_streaming` that exercise the test suite under MPI and UCXX communicators, mirroring rapidsmpf's `mpi_tests`/`ucxx_tests`.
### Changes
- **`cmake/ConfigureOptionalCommunication.cmake`**: Enable MPI/UCXX detection under `BUILD_TESTS` (previously only `BUILD_BENCHMARKS`/`BUILD_EXAMPLES`), so `CUDF_STREAMING_HAVE_MPI`/`CUDF_STREAMING_HAVE_UCXX` are set for test builds.
- **`cmake/thirdparty/get_cudf.cmake`**: Request cudf's optional `testing` component when `BUILD_TESTS` is on, so `cudf::cudftestutil{,_impl}` resolve from the installed (conda) cudf package without a local cudf build.
- **`tests/environment.hpp`**: Refactor `Environment` into an abstract base exposing a polymorphic communicator interface; add `MPI`/`UCXX` to `TestEnvironmentType`. Communicator-specific state (e.g. `mpi_comm_`) moves into the concrete subclasses.
- **`tests/main/mpi.cpp`** (new): `MPIEnvironment` that initializes MPI, builds a `rapidsmpf::MPI` communicator, and provides `barrier()`/`split_comm()`.
- **`tests/main/ucxx.cpp`** (new): `UCXXEnvironment` that bootstraps UCXX over MPI (`init_using_mpi`) with the required CUDA-context/thread-level setup.
- **`tests/main/single.cpp`**: Convert to a `SingleEnvironment` subclass of the new abstract base.
- **`tests/CMakeLists.txt`**: Add a `cudf_streaming_mpirun_test_add` helper and build `cudf_streaming_mpi_tests`/`cudf_streaming_ucxx_tests` (gated on `CUDF_STREAMING_HAVE_MPI`/`UCXX`), registering per-rank `mpirun` ctest cases; apply `CUDF_CUDA_FLAGS` to the shared test sources so the injected cudf testutil `.cu` files compile.
depends on NVIDIA#22860
Authors:
- Niranda Perera (https://github.com/nirandaperera)
- Vyas Ramasubramani (https://github.com/vyasr)
Approvers:
- Vyas Ramasubramani (https://github.com/vyasr)
- Peter Andreas Entschev (https://github.com/pentschev)
URL: NVIDIA#22864
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This updates
libcudf_streamingso communication-dependent examples and benchmarks no longer require both MPI and UCXX to be present at configure time.Changes:
libcudf_streaming.singlecommunicator.example_shufflewhen MPI is unavailable.example_shuffleCUDA compilation by adding the required extended-lambda flag.