Enable cudf_streaming distributed (MPI/UCXX) tests in CI - #22954
Enable cudf_streaming distributed (MPI/UCXX) tests in CI#22954rapids-bot[bot] merged 11 commits into
Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
github-actions
Bot
added
libcudf
nirandaperera
added
bug
|
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:
📝 WalkthroughWalkthroughThe PR updates libcudf_streaming test registration and install logic in CMake, caches the UCXX split communicator in tests, adds OpenMPI setup for CI ctest runs, expands the streaming pytest runner to multiple MPI rank counts, and removes a pre-commit clean step from the style check script. ChangesCMake and UCXX test updates
CI Script Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/libcudf_streaming/tests/CMakeLists.txt (1)
259-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelocate script ignores
DESTDIR, unlike the rest of the install.The
install(TARGETS ...)at Line 236 honors$ENV{DESTDIR}(CMake prepends it automatically for staged installs), but the hand-writtenfile(WRITE "${CMAKE_INSTALL_PREFIX}/...")here does not. Under aDESTDIR-staged install the executables land in$DESTDIR/$PREFIX/bin/gtests/libcudf_streaming/, whileCTestTestfile.cmakeis written to the real$PREFIX, splitting the two apart (and potentially writing outside the staging root).♻️ Honor DESTDIR in the relocation target path
file(READ "${_src_ctestfile}" _ctest_contents) string(REPLACE "\"${_build_gtests_prefix}" "\"./" _ctest_contents "${_ctest_contents}") -file(WRITE "${CMAKE_INSTALL_PREFIX}/bin/gtests/libcudf_streaming/CTestTestfile.cmake" +file(WRITE "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/gtests/libcudf_streaming/CTestTestfile.cmake" "${_ctest_contents}" )
$ENV{DESTDIR}is written literally inside the[=[ ]=]block and expands when the script runs at install time, matching CMake's standard staged-install behavior.Please confirm whether the conda/CI install for the
testingcomponent ever runs withDESTDIRset; if it always installs directly to$PREFIX, this is purely defensive.🤖 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/CMakeLists.txt` around lines 259 - 263, The file(WRITE) operation for CTestTestfile.cmake does not respect the DESTDIR environment variable, unlike the install(TARGETS) command which honors it automatically. To fix this, modify the file(WRITE) call that targets "${CMAKE_INSTALL_PREFIX}/bin/gtests/libcudf_streaming/CTestTestfile.cmake" to prepend "$ENV{DESTDIR}" to the path, changing it to "$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/gtests/libcudf_streaming/CTestTestfile.cmake" to ensure the CTestTestfile.cmake is written to the correct location during staged installs with DESTDIR set.
🤖 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 `@ci/run_cudf_streaming_pytests.sh`:
- Around line 20-27: The mpirun command in the run_mpirun_test function uses -np
with values up to 5, which can exceed available MPI slots on smaller CI runners
and cause failures. Add the --oversubscribe flag to the mpirun command
invocation (the line starting with "mpirun --map-by node") to allow the MPI
processes to oversubscribe the available slots, ensuring the test runs
successfully regardless of the runner's slot availability.
---
Nitpick comments:
In `@cpp/libcudf_streaming/tests/CMakeLists.txt`:
- Around line 259-263: The file(WRITE) operation for CTestTestfile.cmake does
not respect the DESTDIR environment variable, unlike the install(TARGETS)
command which honors it automatically. To fix this, modify the file(WRITE) call
that targets
"${CMAKE_INSTALL_PREFIX}/bin/gtests/libcudf_streaming/CTestTestfile.cmake" to
prepend "$ENV{DESTDIR}" to the path, changing it to
"$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/bin/gtests/libcudf_streaming/CTestTestfile.cmake"
to ensure the CTestTestfile.cmake is written to the correct location during
staged installs with DESTDIR set.
🪄 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: a7fc3129-79ff-4bd2-867f-527aabf2c851
📒 Files selected for processing (2)
ci/run_cudf_streaming_pytests.shcpp/libcudf_streaming/tests/CMakeLists.txt
Signed-off-by: niranda perera <niranda.perera@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ci/run_cudf_streaming_ctests.sh`:
- Around line 29-33: Add a GPU preflight check in run_cudf_streaming_ctests.sh
before the ctest invocation so the job fails fast when no NVIDIA device is
visible. Use the existing script flow around the OpenMPI environment setup and
the ctest call to verify GPU availability first, and exit with a clear,
actionable error message if the check fails. Keep the check close to the ctest
entrypoint so misconfigured CI workers are caught early instead of producing
opaque test failures.
🪄 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: 6c1da75e-2b40-450b-8008-59cb13b892a2
📒 Files selected for processing (3)
ci/run_cudf_streaming_ctests.shci/run_cudf_streaming_pytests.shcpp/libcudf_streaming/tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- ci/run_cudf_streaming_pytests.sh
- cpp/libcudf_streaming/tests/CMakeLists.txt
Signed-off-by: niranda perera <niranda.perera@gmail.com>
c7e1a0e to
d8294ea
Compare
Signed-off-by: niranda perera <niranda.perera@gmail.com>
There was a problem hiding this comment.
I'm a little concerned by some of the changes in this file. Why did we need to write custom relocation logic? Is it because using mpirun requires something specific from the choice of paths?
There was a problem hiding this comment.
@vyasr
Oh okay. IINM, rapids_test_add + rapids_test_install_relocatable handles relocation/ paths, by recreating the CTestTestFile.txt file based on resource specs. But, if I pass a test command like this, mpirun --map-by node ... -np <nranks> ... <test_exec>, the installation gets messed up (recreated ctest test file contains malformed paths). This was the original question I posted in rapids build channel.
So, changes in this file, adds test with a new component tag testing and then installs them to the bin path. Then installs the ctesttestfile with paths adjusted.
|
/merge |
|
Thanks @vyasr |
2 participants
Description
The installed/CI test path for
libcudf_streamingpreviously only includedlibcudf_streaming_single_tests— thempirun-launched MPI and UCXX gtest executables were registered with plainadd_test()and never installed, so they silently did not run in CI. On the Python side, the pytest suite only ran single-rank. This PR makes the distributed tests first-class in CI.Changes
cpp/libcudf_streaming/tests/CMakeLists.txt— unify test registration/installation (rapidsmpf-style):rapids-testmachinery (rapids_test_init/rapids_test_add/rapids_test_install_relocatable), sincerapids_test_install_relocatablecannot faithfully round-trip a launcher-prefixed command (mpirun ... <exe>) into the installedCTestTestfile, which is why the MPI/UCXX tests had been left out.add_test()using each executable's absolute build path, and every test target is tagged with a customCOMPONENT testingmarker property.CTestTestfile.cmakeis then re-rooted at install time (absolute build path →./) and written next to the installed executables. Thetestingcomponent remainsEXCLUDE_FROM_ALL.cmake --install . --component testingnow installs thesingle,mpi, anducxxtest executables plus aCTestTestfile.cmakethat runs all of them; dropped the rapids GPU resource-spec scheduling (not needed — we don't run ctests in parallel across GPUs).ci/run_cudf_streaming_pytests.sh— run the Python tests undermpirunacross multiple ranks (1 2 3 4 5 8), matchingrapidsmpf/ci/run_pytests.sh.Checklist