Support row-major RandomForest training inputs - #8324
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. |
RAMitchell
added
improvement
There was a problem hiding this comment.
Pull request overview
Adds an opt-in runtime switch to train cuML RandomForest directly from row-major feature matrices (keeping column-major as the default), reducing peak memory usage by avoiding an explicit transpose/copy for callers that already have row-major data.
Changes:
- Extend C++ RandomForest
fitAPIs withinput_row_majorand thread it through quantile computation and DecisionTree training. - Introduce
Dataset::value(row, col)plusn_rows/n_colsstored asint64_t, and update split/partition kernels to use the accessor instead of hard-coded column-major indexing. - Update single-GPU RF benchmarks to exercise both
/coland/rowlayouts.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/src/randomforest/randomforest.cuh | Adds input_row_major to RandomForest::fit and threads it into quantile computation and DT training. |
| cpp/src/randomforest/randomforest.cu | Extends exported fit overloads to accept and forward input_row_major. |
| cpp/src/decisiontree/decisiontree.cuh | Adds row_major plumbing into DecisionTree training entrypoint. |
| cpp/src/decisiontree/batched-levelalgo/quantiles.cuh | Enables quantile sampling from either row-major or column-major inputs. |
| cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh | Switches kernel element access to dataset.value(row, col). |
| cpp/src/decisiontree/batched-levelalgo/dataset.h | Adds Dataset::value(row, col), n_rows/n_cols as int64_t, and a row_major flag. |
| cpp/src/decisiontree/batched-levelalgo/builder.cuh | Threads row_major into Dataset construction and updates column-count references. |
| cpp/include/cuml/ensemble/randomforest.hpp | Updates public C++ API declarations with input_row_major defaulting to false. |
| cpp/bench/sg/rf_regressor.cu | Updates benchmark to run in both layouts and clear trees between runs (but needs an argument fix). |
| cpp/bench/sg/rf_classifier.cu | Updates benchmark to run in both layouts and clear trees between runs (but needs an argument fix). |
| cpp/bench/sg/dataset.cuh | Adds /row vs /col suffix to benchmark dataset naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f94a1c9 to
f421885
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds row-major input support to Random Forest training, threads the layout flag through C++ and Python APIs, updates decision-tree and quantile internals, and expands benchmarks and tests to exercise both memory layouts. ChangesRow-major training support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp/src/decisiontree/batched-levelalgo/quantiles.cuh (1)
123-152: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMEDIUM: Document the
row_majordata contract consistently.The doc now says both layouts are supported, but
@param datastill says column-major only and the newrow_majorparameter is missing. This leaves the row-major shape/stride contract ambiguous.Proposed fix
- * `@param` data Column-major input matrix with shape `[n_cols, n_rows]`. + * `@param` data Input matrix. When `row_major` is false, data is column-major with shape + * `[n_cols, n_rows]`; when `row_major` is true, data is row-major with shape + * `[n_rows, n_cols]`. @@ - * `@param` seed User seed for deterministic sampling. + * `@param` seed User seed for deterministic sampling. + * `@param` row_major Whether `data` is row-major instead of column-major.As per coding guidelines, “Function parameters with ambiguous data format (row-major or column-major) must be explicitly documented or validated at the function entry point.”
🤖 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/src/decisiontree/batched-levelalgo/quantiles.cuh` around lines 123 - 152, The computeQuantiles documentation is inconsistent about input layout: the function now supports both column-major and row-major, but the existing `@param` data text still describes only column-major and does not document row_major. Update the computeQuantiles comment to explicitly describe the layout contract for data, add a `@param` row_major entry, and clarify the expected shape/stride semantics when row_major is true versus false so the API contract is unambiguous.Source: Coding guidelines
cpp/src/randomforest/randomforest.cuh (1)
265-266: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSame stale "column major format" doc issue as
randomforest.cu.The
inputdoc doesn't reflect the newinput_row_majorswitch added below.Based on learnings, treat headers under cpp/include/cuml/ as internal implementation details backing the Python API, but ensure clear, minimal notes to aid future maintainers — this internal
.cuhdoc block should stay accurate for the same reason.🤖 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/src/randomforest/randomforest.cuh` around lines 265 - 266, Update the `input` parameter doc in `randomforest.cuh` so it matches the new `input_row_major` behavior instead of always saying column major format. Keep the note minimal but accurate in the `RandomForest`/`RandomForest`-related doc block, and reference the existing `input` parameter description so future maintainers understand that the device pointer may now be interpreted by row-major or column-major mode depending on the switch.Source: Learnings
cpp/src/randomforest/randomforest.cu (1)
334-350: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc for
inputparam is now stale after addinginput_row_major.The
@param[in] inputdoc still says "train data (n_rows samples, n_cols features) in column major format," excluding labels, but layout is now caller-selectable via the newinput_row_majorflag. Same wording exists in the regressorfitdoc block (around line 594).📝 Suggested doc fix
- * `@param`[in] input: train data (n_rows samples, n_cols features) in column major format, - * excluding labels. Device pointer. + * `@param`[in] input: train data (n_rows samples, n_cols features), column-major by default + * or row-major when input_row_major is true, excluding labels. Device pointer.🤖 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/src/randomforest/randomforest.cu` around lines 334 - 350, Update the fit API docs for RandomForestClassificationFit (and the matching regressor fit block) so the `@param`[in] input description no longer claims column-major only; describe the data layout as controlled by input_row_major and keep the existing labels/excluding labels wording. Make sure the parameter docs around input_row_major clearly state that input may be row-major or column-major depending on that flag, so the comments stay consistent with the new API behavior.
🤖 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/bench/sg/rf_regressor.cu`:
- Around line 48-61: The fit call in RandomForestRegressorF is passing device
containers instead of raw pointers, which will not match fit()’s float*/double*
signature. In the loopOnState lambda, update the fit invocation to use the
underlying raw buffers from this->data.X and this->data.y by calling their
data() accessors, while keeping the rest of the arguments in the same order. Use
the fit() call site in rf_regressor.cu as the target for this fix.
---
Outside diff comments:
In `@cpp/src/decisiontree/batched-levelalgo/quantiles.cuh`:
- Around line 123-152: The computeQuantiles documentation is inconsistent about
input layout: the function now supports both column-major and row-major, but the
existing `@param` data text still describes only column-major and does not
document row_major. Update the computeQuantiles comment to explicitly describe
the layout contract for data, add a `@param` row_major entry, and clarify the
expected shape/stride semantics when row_major is true versus false so the API
contract is unambiguous.
In `@cpp/src/randomforest/randomforest.cu`:
- Around line 334-350: Update the fit API docs for RandomForestClassificationFit
(and the matching regressor fit block) so the `@param`[in] input description no
longer claims column-major only; describe the data layout as controlled by
input_row_major and keep the existing labels/excluding labels wording. Make sure
the parameter docs around input_row_major clearly state that input may be
row-major or column-major depending on that flag, so the comments stay
consistent with the new API behavior.
In `@cpp/src/randomforest/randomforest.cuh`:
- Around line 265-266: Update the `input` parameter doc in `randomforest.cuh` so
it matches the new `input_row_major` behavior instead of always saying column
major format. Keep the note minimal but accurate in the
`RandomForest`/`RandomForest`-related doc block, and reference the existing
`input` parameter description so future maintainers understand that the device
pointer may now be interpreted by row-major or column-major mode depending on
the switch.
🪄 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: 7003f7c1-49d3-424c-a5d3-d3202bf92f3a
📒 Files selected for processing (11)
cpp/bench/sg/dataset.cuhcpp/bench/sg/rf_classifier.cucpp/bench/sg/rf_regressor.cucpp/include/cuml/ensemble/randomforest.hppcpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/dataset.hcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhcpp/src/decisiontree/batched-levelalgo/quantiles.cuhcpp/src/decisiontree/decisiontree.cuhcpp/src/randomforest/randomforest.cucpp/src/randomforest/randomforest.cuh
f421885 to
820764b
Compare
viclafargue
left a comment
viclafargue
left a comment
There was a problem hiding this comment.
Thanks for the prototype and benchmarks. Given the issue context, can you clarify the plan for the remaining user-facing pieces (Python/Treelite path)?
Additionally, a C/F input parity coverage would be necessary for this to be considered production-ready, since benchmarks alone won’t catch layout-specific correctness regressions.
|
@viclafargue thanks, I will strengthen tests. I don't think there are any changes to user facing parts. The prediction/treelite path is already row major. It will be unfortunately non-trivial to extend it to col major as well. For some reason training was col major only and prediction row major only so this is definitely an improvement. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/include/cuml/ensemble/randomforest.hpp (1)
170-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
input_row_majorparameter forfit_treelite.
fit_treelitehas no Doxygen@paramdocumentation, and the newinput_row_majorparameter isn't described anywhere (the correspondingfitoverloads do document it inrandomforest.cu, butfit_treelitedoesn't inherit that doc). Consider adding a short@param[in] input_row_majornote to keep the public header self-describing.As per path instructions, "For public C++ API headers, additionally check: Doxygen documentation for all public functions/classes."
Also applies to: 256-268
🤖 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/include/cuml/ensemble/randomforest.hpp` around lines 170 - 183, The public API declaration of fit_treelite is missing Doxygen for the new input_row_major parameter, so update the comment block for fit_treelite to add a concise `@param`[in] input_row_major description alongside the existing parameters. Make sure the documentation in randomforest.hpp is self-contained and matches the meaning used by the fit overloads in randomforest.cu, so the signature with user_handle, RF_params, and the optional sample_weight/input_row_major arguments is fully documented.Source: Path instructions
python/cuml/tests/test_random_forest.py (1)
300-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extending parity coverage to
bootstrap=True+sample_weight.Current tests only cover the deterministic, non-bootstrapped, unweighted path. Bootstrapping and weighted sampling exercise different kernels (row-sampling, weighted histograms) that could have layout-specific edge cases not caught here.
Also applies to: 338-374
🤖 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 `@python/cuml/tests/test_random_forest.py` around lines 300 - 336, Extend the random forest parity tests to cover the bootstrap and weighted paths, since the current fit-order parity only exercises the deterministic unweighted case. Add coverage in test_rf_classifier_fit_input_order_parity (and the matching sibling test) for bootstrap=True plus sample_weight so both C- and F-ordered inputs are validated through the row-sampling and weighted histogram code paths. Keep the same assertion pattern by fitting curfc with the new parameters and comparing predictions from c_model and f_model on the same X_pred.
🤖 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/include/cuml/ensemble/randomforest.hpp`:
- Around line 170-183: The public API declaration of fit_treelite is missing
Doxygen for the new input_row_major parameter, so update the comment block for
fit_treelite to add a concise `@param`[in] input_row_major description alongside
the existing parameters. Make sure the documentation in randomforest.hpp is
self-contained and matches the meaning used by the fit overloads in
randomforest.cu, so the signature with user_handle, RF_params, and the optional
sample_weight/input_row_major arguments is fully documented.
In `@python/cuml/tests/test_random_forest.py`:
- Around line 300-336: Extend the random forest parity tests to cover the
bootstrap and weighted paths, since the current fit-order parity only exercises
the deterministic unweighted case. Add coverage in
test_rf_classifier_fit_input_order_parity (and the matching sibling test) for
bootstrap=True plus sample_weight so both C- and F-ordered inputs are validated
through the row-sampling and weighted histogram code paths. Keep the same
assertion pattern by fitting curfc with the new parameters and comparing
predictions from c_model and f_model on the same X_pred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4142525f-c08e-4a7e-be82-21926fd8e6a4
📒 Files selected for processing (8)
cpp/bench/sg/rf_regressor.cucpp/include/cuml/ensemble/randomforest.hppcpp/src/randomforest/randomforest.cucpp/tests/sg/rf_test.cupython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/tests/test_random_forest.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/bench/sg/rf_regressor.cu
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Approving since implementation looks good to me!
I left a few comments on nits (most of them in test conciseness), feel free to do with them what you see fit.
|
/merge |
Summary
Addresses #8310.
Adds a runtime layout switch for RandomForest training so callers can pass row-major input directly instead of transposing to column-major first. Column-major remains the default.
For callers that already have row-major data, this avoids holding both the original row-major matrix and a transposed column-major copy during fit. Excluding RF/DT working space, the input data footprint is therefore reduced from two matrix copies to one, roughly halving input-data memory usage.
Changes
input_row_major = falseto the C++ RandomForestfitAPIs.Dataset::value(row, col)and storen_rows/n_colsasint64_t./coland/rowlayouts.Validation
pre-commit run --from-ref upstream/main --to-ref HEADninja -C /home/rorym/cuml-builds/codex-rf-layout-benchmark/cpp-rfbench cuml -j8