Adding cudf streaming dist tests - #22864
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Signed-off-by: niranda perera <niranda.perera@gmail.com>
7f7e3dd to
7a6bb81
Compare
|
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 extends CMake discovery for ChangesTest environment setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@cpp/libcudf_streaming/cmake/thirdparty/get_cudf.cmake`:
- Around line 19-20: The current guard only skips find_package when
cudf::cudftestutil exists, but tests also need cudf::cudftestutil_impl; update
the logic in get_cudf.cmake so the testing-component guard covers both imported
targets (check NOT TARGET cudf::cudftestutil AND NOT TARGET
cudf::cudftestutil_impl before skipping) or alternatively always call
find_package(cudf ... COMPONENTS testing) when BUILD_TESTS is true and then
validate that both cudf::cudftestutil and cudf::cudftestutil_impl are available
after the call, erroring (or re-invoking find_package) if either is missing.
In `@cpp/libcudf_streaming/tests/main/mpi.cpp`:
- Around line 48-68: split_comm() currently creates and returns a fresh
shared_ptr but never stores it into the member split_comm_, causing repeated
MPI_Comm_split calls and losing ownership for teardown; fix by assigning the
newly created shared_ptr to split_comm_ (i.e. split_comm_ =
std::shared_ptr<rapidsmpf::MPI>(...)) before returning it, and ensure the custom
deleter captures the split MPI_Comm handle you created so MPI_Comm_free is
invoked exactly once when split_comm_ is destroyed (use the local split_comm
variable captured by value in the deleter). Ensure the early-return check in
split_comm() still returns the cached split_comm_ when present.
In `@cpp/libcudf_streaming/tests/main/ucxx.cpp`:
- Around line 31-32: The call to cudaFree(nullptr) used to force CUDA context
creation must check and handle the CUDA API return status; update the
initialization code that currently calls cudaFree(nullptr) so it captures the
returned cudaError_t, tests for cudaSuccess, and on failure logs the CUDA error
string (cudaGetErrorString) and aborts/returns non-zero before proceeding to
UCX/MPI initialization (i.e., change the unchecked cudaFree(nullptr) call in
ucxx.cpp to check the result and exit with an error message if initialization
fails).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 47aa4303-d984-42fe-bf6d-2df22cebb835
📒 Files selected for processing (7)
cpp/libcudf_streaming/cmake/ConfigureOptionalCommunication.cmakecpp/libcudf_streaming/cmake/thirdparty/get_cudf.cmakecpp/libcudf_streaming/tests/CMakeLists.txtcpp/libcudf_streaming/tests/environment.hppcpp/libcudf_streaming/tests/main/mpi.cppcpp/libcudf_streaming/tests/main/single.cppcpp/libcudf_streaming/tests/main/ucxx.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/libcudf_streaming/tests/environment.hpp (1)
38-38: 📐 Maintainability & Code Quality | 💤 Low valueConsider adding
[[nodiscard]]tooptions().The accessor is side-effect-free and returns a reference; discarding it is almost certainly a bug.
Suggested change
- rapidsmpf::config::Options& options() { return options_; } + [[nodiscard]] rapidsmpf::config::Options& options() { return options_; }🤖 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/tests/environment.hpp` at line 38, The accessor options() returns a reference and is side-effect-free, so add the [[nodiscard]] attribute to its declaration to prevent callers from accidentally discarding the returned reference; update the signature from "rapidsmpf::config::Options& options() { return options_; }" to include the attribute (e.g. "[[nodiscard]] rapidsmpf::config::Options& options() { return options_; }") so the compiler will warn when the result is ignored.
🤖 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/tests/environment.hpp`:
- Line 38: The accessor options() returns a reference and is side-effect-free,
so add the [[nodiscard]] attribute to its declaration to prevent callers from
accidentally discarding the returned reference; update the signature from
"rapidsmpf::config::Options& options() { return options_; }" to include the
attribute (e.g. "[[nodiscard]] rapidsmpf::config::Options& options() { return
options_; }") so the compiler will warn when the result is ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 606505fb-30fb-463d-8809-becece25af85
📒 Files selected for processing (1)
cpp/libcudf_streaming/tests/environment.hpp
Updated comment to clarify file structure for communicator-specific environments.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
pentschev
left a comment
There was a problem hiding this comment.
One nit about binary naming, otherwise LGTM.
| # These executables reuse cudf_streaming_test_sources but provide a different test environment main | ||
| # (main/mpi.cpp / main/ucxx.cpp), and are launched via mpirun for multiple ranks. | ||
| if(CUDF_STREAMING_HAVE_MPI) | ||
| add_executable(cudf_streaming_mpi_tests main/mpi.cpp) |
There was a problem hiding this comment.
| add_executable(cudf_streaming_mpi_tests main/mpi.cpp) | |
| add_executable(libcudf_streaming_mpi_tests main/mpi.cpp) |
There was a problem hiding this comment.
Nit: let's make test names complete and matching the package/library name. This would also match the same prefix being proposed for benchmarks here.
|
/ok to test b851330 |
This reverts commit c2f7ecb. Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
/ok to test 41bb34e |
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
Description
Adds multi-rank C++ test executables for
cudf_streamingthat exercise the test suite under MPI and UCXX communicators, mirroring rapidsmpf'smpi_tests/ucxx_tests.Changes
cmake/ConfigureOptionalCommunication.cmake: Enable MPI/UCXX detection underBUILD_TESTS(previously onlyBUILD_BENCHMARKS/BUILD_EXAMPLES), soCUDF_STREAMING_HAVE_MPI/CUDF_STREAMING_HAVE_UCXXare set for test builds.cmake/thirdparty/get_cudf.cmake: Request cudf's optionaltestingcomponent whenBUILD_TESTSis on, socudf::cudftestutil{,_impl}resolve from the installed (conda) cudf package without a local cudf build.tests/environment.hpp: RefactorEnvironmentinto an abstract base exposing a polymorphic communicator interface; addMPI/UCXXtoTestEnvironmentType. Communicator-specific state (e.g.mpi_comm_) moves into the concrete subclasses.tests/main/mpi.cpp(new):MPIEnvironmentthat initializes MPI, builds arapidsmpf::MPIcommunicator, and providesbarrier()/split_comm().tests/main/ucxx.cpp(new):UCXXEnvironmentthat bootstraps UCXX over MPI (init_using_mpi) with the required CUDA-context/thread-level setup.tests/main/single.cpp: Convert to aSingleEnvironmentsubclass of the new abstract base.tests/CMakeLists.txt: Add acudf_streaming_mpirun_test_addhelper and buildcudf_streaming_mpi_tests/cudf_streaming_ucxx_tests(gated onCUDF_STREAMING_HAVE_MPI/UCXX), registering per-rankmpirunctest cases; applyCUDF_CUDA_FLAGSto the shared test sources so the injected cudf testutil.cufiles compile.depends on #22860
Checklist