Clean up stale cuML multi-GPU build options - #8137
Conversation
|
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 removes the MPI-comms build option, updates CMake/test gating for multi-GPU builds, migrates KNN multi-GPU tests to RAFT device-vector-based storage and newer APIs, simplifies the PCA test matrix, and refreshes build documentation to match the new test and dependency flow. ChangesMulti-GPU Test Infrastructure Refactoring
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🧹 Nitpick comments (2)
BUILD.md (1)
111-111: ⚡ Quick winRemove
$prompts in command blocks to satisfy markdownlint MD014.These lines use shell prompts without output, which triggers the current markdownlint rule. Please remove the leading
$(or adjust the rule config if prompts are intentional).Also applies to: 116-116, 201-201, 206-206, 211-211, 212-212
🤖 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 `@BUILD.md` at line 111, Remove the leading "$" shell prompt from standalone command lines in BUILD.md (e.g., change "$ ctest --test-dir cpp/build --output-on-failure" and the other occurrences at the same command blocks on lines showing "$ ...") so the command blocks contain only the raw commands; update each occurrence (including the instances corresponding to the other reported lines) to remove the prompt prefix to satisfy markdownlint MD014.cpp/tests/mg/knn_test_helper.cuh (1)
144-164: ⚡ Quick winSkip allocating unused KNN neighbor buffers here.
For the classify/regress paths in this PR,
out_i_part_storageandout_d_part_storageare no longer consumed, butgenerate_data()still allocates them for every query partition. That is avoidable multi-GPU device-memory pressure with no coverage benefit. Consider making those buffers optional or splitting the helper setup for raw-KNN vs classify/regress tests.As per coding guidelines, "Avoid excessive memory allocations in hot paths".
🤖 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/tests/mg/knn_test_helper.cuh` around lines 144 - 164, generate_data() is allocating out_d_part_storage and out_i_part_storage (and creating Matrix::floatData_t / Matrix::Data<int64_t> and pushing to out_d_parts/out_i_parts) even for classify/regress tests that don't consume KNN neighbors; change the logic to allocate and create these device buffers and the corresponding Matrix outputs only when they are actually needed (e.g., when the test mode requires raw KNN neighbors or when params.k > 0), or add a boolean flag like use_knn_buffers to gate the emplace_back and new Matrix::floatData_t / Matrix::Data<int64_t> creation; if skipping allocation, ensure you also skip pushing to out_d_parts/out_i_parts (or push nullptr consistently) so the rest of the code handles the absence safely and avoids unnecessary multi-GPU memory pressure.
🤖 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/tests/mg/knn_classify.cu`:
- Around line 87-89: The test currently returns a hardcoded success via
MLCommon::CompareApprox<int>(1)(actual, expected); replace this sentinel with a
real assertion that compares the classifier output (knn_th.out_parts) to the
expected labels or deterministic invariants derived from the generated blobs:
locate the block where int actual/expected and the return are set, compute
expected labels from the same blob-generation logic used in the test (or
hardcode the deterministic expected vector), build an appropriate comparison
(element-wise equality or tolerance-based check) between knn_th.out_parts and
the expected labels, and return the result of that comparison instead of the
constant true value.
In `@cpp/tests/mg/knn_regress.cu`:
- Around line 65-67: The test currently compares two literals (1 vs 1) instead
of the regression output; change the comparison to assert knn_regress's actual
output against the expected value(s). Locate the knn_regress invocation and the
variables actual and expected, set actual to the predicted value(s) returned or
written by knn_regress (or the output buffer it fills), set expected to the
known ground-truth regression result(s), and call MLCommon::CompareApprox<T>
with those actual and expected values (or iterate/compare element-wise if the
prediction is an array) so the test fails when knn_regress returns incorrect
predictions.
In `@cpp/tests/mg/knn.cu`:
- Around line 208-210: The test currently uses a hardcoded placeholder
(actual/expected = 1) and MLCommon::CompareApprox<int> which masks failures;
replace this with real validation of knn() outputs by extracting the computed
neighbor indices and distances returned by the knn() call (e.g., the arrays or
device buffers produced by knn()) and comparing them to the known ground-truth
neighbors/distances for the test dataset: compute actual values from the knn()
outputs (neighbor indices and/or distances), set expected values to the
precomputed ground-truth vectors for this test case, and call the appropriate
MLCommon::CompareApprox (use CompareApprox<int> for indices and
CompareApprox<float/double> for distances) with a sensible tolerance to assert
equality; update the return to use those comparisons instead of the placeholder
CompareApprox<int>(1)(1,1).
---
Nitpick comments:
In `@BUILD.md`:
- Line 111: Remove the leading "$" shell prompt from standalone command lines in
BUILD.md (e.g., change "$ ctest --test-dir cpp/build --output-on-failure" and
the other occurrences at the same command blocks on lines showing "$ ...") so
the command blocks contain only the raw commands; update each occurrence
(including the instances corresponding to the other reported lines) to remove
the prompt prefix to satisfy markdownlint MD014.
In `@cpp/tests/mg/knn_test_helper.cuh`:
- Around line 144-164: generate_data() is allocating out_d_part_storage and
out_i_part_storage (and creating Matrix::floatData_t / Matrix::Data<int64_t> and
pushing to out_d_parts/out_i_parts) even for classify/regress tests that don't
consume KNN neighbors; change the logic to allocate and create these device
buffers and the corresponding Matrix outputs only when they are actually needed
(e.g., when the test mode requires raw KNN neighbors or when params.k > 0), or
add a boolean flag like use_knn_buffers to gate the emplace_back and new
Matrix::floatData_t / Matrix::Data<int64_t> creation; if skipping allocation,
ensure you also skip pushing to out_d_parts/out_i_parts (or push nullptr
consistently) so the rest of the code handles the absence safely and avoids
unnecessary multi-GPU memory pressure.
🪄 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: a219478d-fa4f-4306-b3bf-cae7ecec7e44
📒 Files selected for processing (10)
BUILD.mdbuild.shcpp/CMakeLists.txtcpp/README.mdcpp/tests/CMakeLists.txtcpp/tests/mg/knn.cucpp/tests/mg/knn_classify.cucpp/tests/mg/knn_regress.cucpp/tests/mg/knn_test_helper.cuhcpp/tests/mg/pca.cu
| int actual = 1; | ||
| int expected = 1; | ||
| return raft::CompareApprox<int>(1)(actual, expected); | ||
| return MLCommon::CompareApprox<int>(1)(actual, expected); |
There was a problem hiding this comment.
Assert the classifier output instead of a sentinel value.
This always returns true, so changes in knn_classify() output will not fail the test. Please validate knn_th.out_parts against expected labels or deterministic invariants from the generated blobs.
🤖 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/tests/mg/knn_classify.cu` around lines 87 - 89, The test currently
returns a hardcoded success via MLCommon::CompareApprox<int>(1)(actual,
expected); replace this sentinel with a real assertion that compares the
classifier output (knn_th.out_parts) to the expected labels or deterministic
invariants derived from the generated blobs: locate the block where int
actual/expected and the return are set, compute expected labels from the same
blob-generation logic used in the test (or hardcode the deterministic expected
vector), build an appropriate comparison (element-wise equality or
tolerance-based check) between knn_th.out_parts and the expected labels, and
return the result of that comparison instead of the constant true value.
| int actual = 1; | ||
| int expected = 1; | ||
| return raft::CompareApprox<int>(1)(actual, expected); | ||
| return MLCommon::CompareApprox<int>(1)(actual, expected); |
There was a problem hiding this comment.
Assert the regression output instead of a sentinel value.
This still compares 1 to 1, so the test passes even if knn_regress() returns incorrect predictions. That makes the API migration untested for the actual regression results.
🤖 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/tests/mg/knn_regress.cu` around lines 65 - 67, The test currently
compares two literals (1 vs 1) instead of the regression output; change the
comparison to assert knn_regress's actual output against the expected value(s).
Locate the knn_regress invocation and the variables actual and expected, set
actual to the predicted value(s) returned or written by knn_regress (or the
output buffer it fills), set expected to the known ground-truth regression
result(s), and call MLCommon::CompareApprox<T> with those actual and expected
values (or iterate/compare element-wise if the prediction is an array) so the
test fails when knn_regress returns incorrect predictions.
| int actual = 1; | ||
| int expected = 1; | ||
| return raft::CompareApprox<int>(1)(actual, expected); | ||
| return MLCommon::CompareApprox<int>(1)(actual, expected); |
There was a problem hiding this comment.
Replace the placeholder assertion with a real KNN result check.
This still returns CompareApprox(1)(1, 1), so the test passes even if knn() produces wrong neighbors or distances. That leaves the RAFT/API migration effectively unvalidated on this path.
🤖 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/tests/mg/knn.cu` around lines 208 - 210, The test currently uses a
hardcoded placeholder (actual/expected = 1) and MLCommon::CompareApprox<int>
which masks failures; replace this with real validation of knn() outputs by
extracting the computed neighbor indices and distances returned by the knn()
call (e.g., the arrays or device buffers produced by knn()) and comparing them
to the known ground-truth neighbors/distances for the test dataset: compute
actual values from the knn() outputs (neighbor indices and/or distances), set
expected values to the precomputed ground-truth vectors for this test case, and
call the appropriate MLCommon::CompareApprox (use CompareApprox<int> for indices
and CompareApprox<float/double> for distances) with a sensible tolerance to
assert equality; update the return to use those comparisons instead of the
placeholder CompareApprox<int>(1)(1,1).
chyunsu3
left a comment
chyunsu3
left a comment
There was a problem hiding this comment.
Some questions about the CMake changes
| # ################################################################################################## | ||
| # * build ml_test executable ------------------------------------------------- | ||
| if(all_algo OR dbscan_algo) | ||
| if(BUILD_CUML_TESTS AND (all_algo OR dbscan_algo)) |
There was a problem hiding this comment.
Is it not possible to simply skip add_subdirectory(cpp/tests) when BUILD_CUML_TESTS is false?
There was a problem hiding this comment.
The cpp/tests directory defines the SG, MG and prims tests.
Historically, --nolibcumltest (BUILD_CUML_TESTS=OFF) means remove the SG tests and cpp-mgtests (BUILD_CUML_MG_TESTS=ON) means add the MG tests.
We could either make --nolibcumltest disable all the tests but this would change the expected behavior. We could also have the SG, MG and prims tests be defined in three distinct directories, but this is a bit outside of the scope of this PR. Even though this may be something to look into.
| $<TARGET_NAME_IF_EXISTS:GPUTreeShap::GPUTreeShap> | ||
| $<$<BOOL:${LINK_CUFFT}>:CUDA::cufft${_ctk_fft_static_suffix}> | ||
| ${OpenMP_CXX_LIB_NAMES} | ||
| $<$<OR:$<BOOL:${BUILD_CUML_STD_COMMS}>,$<BOOL:${BUILD_CUML_MPI_COMMS}>>:NCCL::NCCL> |
There was a problem hiding this comment.
Doesn't multi-GPU functionalities of cuML use NCCL?
There was a problem hiding this comment.
cuML’s own code calls the abstract raft::comms::comms_t interface. It does not include nccl.h or call NCCL functions directly. The MG tests link raft::distributed, whose CMake target links NCCL::NCCL.
github-actions
Bot
added
conda
|
Let's try to merge this pull request soon, since #8394 requires it. |
|
/ok to test 508cb5d |
|
/ok to test 5e8bd4b |
|
Re-targeting this PR to 26.10. |
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
explanations all make sense to me, changes look good from a packaging-codeowners perspective
|
/merge |
5 participants
Addressing #7845.
This PR :
BUILD_CUML_MG_TESTSthe single switch for C++ multi-GPU tests