Support unlimited_depth for the random forest estimators - #7895
Merged
rapids-bot[bot] merged 10 commits intoApr 1, 2026
Conversation
csadorf
added
improvement
|
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 pull request implements support for unlimited Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Suggested labels
🚥 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 |
csadorf
force-pushed
the
rf-support-unlimited-depth
branch
from
March 13, 2026 22:07
8d096f4 to
7acd941
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh (1)
81-87: Trim the remaining dead node-split arguments.
nodeSplitKernelonly consumesmin_samples_leafandmin_impurity_decreasein Lines 89-97, somin_samples_splitandmax_leavesare still dead parameters after this cleanup. Removing them too would keep the internal CUDA interface aligned with the real host-side enforcement.♻️ Suggested cleanup
static __global__ void nodeSplitKernel(const IdxT min_samples_leaf, - const IdxT min_samples_split, - const IdxT max_leaves, const DataT min_impurity_decrease, const Dataset<DataT, LabelT, IdxT> dataset, const NodeWorkItem* work_items, const Split<DataT, IdxT>* splits) @@ void launchNodeSplitKernel(const IdxT min_samples_leaf, - const IdxT min_samples_split, - const IdxT max_leaves, const DataT min_impurity_decrease, const Dataset<DataT, LabelT, IdxT>& dataset, const NodeWorkItem* work_items, const size_t work_items_size, const Split<DataT, IdxT>* splits, cudaStream_t builder_stream) { auto constexpr smem_size = 2 * sizeof(IdxT) * TPB; nodeSplitKernel<DataT, LabelT, IdxT, TPB> - <<<work_items_size, TPB, smem_size, builder_stream>>>(min_samples_leaf, - min_samples_split, - max_leaves, - min_impurity_decrease, + <<<work_items_size, TPB, smem_size, builder_stream>>>(min_samples_leaf, + min_impurity_decrease, dataset, work_items, splits); }Update the matching declaration in
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuhand the call site incpp/src/decisiontree/batched-levelalgo/builder.cuhin the same sweep.Also applies to: 100-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh` around lines 81 - 87, The nodeSplitKernel signature still includes dead parameters min_samples_split and max_leaves; remove these two parameters from the device kernel declaration/definition for nodeSplitKernel (the signature shown) and update the corresponding declaration in builder_kernels.cuh and the invocation in builder.cuh so the CUDA kernel call and host-side declaration match the new signature that only takes min_samples_leaf and min_impurity_decrease (and the existing dataset, work_items, splits parameters).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/include/cuml/tree/decisiontree.hpp`:
- Around line 20-21: The header has inconsistent sentinel docs for "Maximum tree
depth" (doc at top mentions INT32_MAX while other docs/defaults reference -1);
update the comments and any default values so they state one clear contract:
either document that the public API accepts -1 as "unlimited" which is
internally normalized to INT32_MAX, or change the comments/defaults to use
INT32_MAX everywhere. Adjust the Javadoc for the max-depth parameter in
decisiontree.hpp (the top comment and the entries around the helper at lines ~64
and the default at ~80) to explicitly mention the chosen sentinel and, if you
keep -1 as the API sentinel, add a short note that code normalizes -1 ->
INT32_MAX before use.
In `@python/cuml/tests/dask/test_dask_random_forest.py`:
- Around line 360-388: The two tests test_unlimited_max_depth_classifier and
test_unlimited_max_depth_regressor only check prediction length; change them to
evaluate numerical correctness by splitting X,y into train/test via a
deterministic split (use train_test_split with a fixed random_state), use
_prep_training_data on the training portion, fit cuRFC_mg / cuRFR_mg on the
training Dask arrays, predict on the held-out test set (not the training data),
and compare predictions to a scikit-learn baseline (e.g.,
sklearn.ensemble.RandomForestClassifier/Regressor with same
n_estimators/max_depth/random_state) or assert a numeric metric (accuracy/MSE)
is within an acceptable threshold; ensure types remain float32/int32 as before
and use the same n_workers-derived sizing so tests remain distributed.
---
Nitpick comments:
In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh`:
- Around line 81-87: The nodeSplitKernel signature still includes dead
parameters min_samples_split and max_leaves; remove these two parameters from
the device kernel declaration/definition for nodeSplitKernel (the signature
shown) and update the corresponding declaration in builder_kernels.cuh and the
invocation in builder.cuh so the CUDA kernel call and host-side declaration
match the new signature that only takes min_samples_leaf and
min_impurity_decrease (and the existing dataset, work_items, splits parameters).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a7ffdd2-f4c2-4011-99c3-3205c526b032
📒 Files selected for processing (11)
cpp/include/cuml/tree/decisiontree.hppcpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhpython/cuml/cuml/dask/ensemble/randomforestclassifier.pypython/cuml/cuml/dask/ensemble/randomforestregressor.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/tests/dask/test_dask_random_forest.pypython/cuml/tests/test_random_forest.py
🚧 Files skipped from review as they are similar to previous changes (6)
- python/cuml/tests/test_random_forest.py
- python/cuml/cuml/ensemble/randomforestclassifier.py
- python/cuml/cuml/dask/ensemble/randomforestclassifier.py
- python/cuml/cuml/dask/ensemble/randomforestregressor.py
- python/cuml/cuml/ensemble/randomforestregressor.py
- python/cuml/cuml/ensemble/randomforest_common.pyx
Comment on lines
+360
to
+388
| def test_unlimited_max_depth_classifier(client): | ||
| n_workers = len(client.scheduler_info(n_workers=-1)["workers"]) | ||
| X, y = make_classification( | ||
| n_samples=n_workers * 200, n_features=10, random_state=42 | ||
| ) | ||
| X = X.astype(np.float32) | ||
| y = y.astype(np.int32) | ||
|
|
||
| X_dask, y_dask = _prep_training_data(client, X, y, partitions_per_worker=1) | ||
| clf = cuRFC_mg(n_estimators=n_workers * 5, max_depth=None) | ||
| clf.fit(X_dask, y_dask) | ||
| preds = cp.asnumpy(cp.array(clf.predict(X_dask).compute())) | ||
| assert len(preds) == len(y) | ||
|
|
||
|
|
||
| def test_unlimited_max_depth_regressor(client): | ||
| n_workers = len(client.scheduler_info(n_workers=-1)["workers"]) | ||
| X, y = make_regression( | ||
| n_samples=n_workers * 200, n_features=10, random_state=42 | ||
| ) | ||
| X = X.astype(np.float32) | ||
| y = y.astype(np.float32) | ||
|
|
||
| X_dask, y_dask = _prep_training_data(client, X, y, partitions_per_worker=1) | ||
| reg = cuRFR_mg(n_estimators=n_workers * 5, max_depth=None) | ||
| reg.fit(X_dask, y_dask) | ||
| preds = cp.asnumpy(cp.array(reg.predict(X_dask).compute())) | ||
| assert len(preds) == len(y) | ||
|
|
There was a problem hiding this comment.
Strengthen unlimited-depth tests to verify correctness, not just output length.
Both new tests currently pass even if predictions are wrong, because they only assert len(preds) == len(y) and predict on training data. Please compare against a scikit-learn baseline (or at minimum a quality threshold on held-out data) with fixed random_state.
Suggested test hardening
+from sklearn.ensemble import RandomForestRegressor as skrfr
@@
def test_unlimited_max_depth_classifier(client):
@@
- X_dask, y_dask = _prep_training_data(client, X, y, partitions_per_worker=1)
- clf = cuRFC_mg(n_estimators=n_workers * 5, max_depth=None)
- clf.fit(X_dask, y_dask)
- preds = cp.asnumpy(cp.array(clf.predict(X_dask).compute()))
- assert len(preds) == len(y)
+ X_train, X_test, y_train, y_test = train_test_split(
+ X, y, test_size=n_workers * 40, random_state=42
+ )
+ X_train_dask, y_train_dask = _prep_training_data(
+ client, X_train, y_train, partitions_per_worker=1
+ )
+ X_test_dask = from_array(X_test)
+
+ clf = cuRFC_mg(n_estimators=n_workers * 5, max_depth=None, random_state=42)
+ clf.fit(X_train_dask, y_train_dask)
+ preds = cp.asnumpy(cp.array(clf.predict(X_test_dask).compute()))
+ acc = accuracy_score(y_test, preds)
+
+ sk_clf = skrfc(
+ n_estimators=n_workers * 5, max_depth=None, random_state=42, n_jobs=-1
+ )
+ sk_clf.fit(X_train, y_train)
+ sk_acc = accuracy_score(y_test, sk_clf.predict(X_test))
+ assert acc >= (sk_acc - 0.07)
@@
def test_unlimited_max_depth_regressor(client):
@@
- X_dask, y_dask = _prep_training_data(client, X, y, partitions_per_worker=1)
- reg = cuRFR_mg(n_estimators=n_workers * 5, max_depth=None)
- reg.fit(X_dask, y_dask)
- preds = cp.asnumpy(cp.array(reg.predict(X_dask).compute()))
- assert len(preds) == len(y)
+ X_train, X_test, y_train, y_test = train_test_split(
+ X, y, test_size=n_workers * 40, random_state=42
+ )
+ X_train_dask, y_train_dask = _prep_training_data(
+ client, X_train, y_train, partitions_per_worker=1
+ )
+ X_test_dask = from_array(X_test)
+
+ reg = cuRFR_mg(n_estimators=n_workers * 5, max_depth=None, random_state=42)
+ reg.fit(X_train_dask, y_train_dask)
+ preds = cp.asnumpy(cp.array(reg.predict(X_test_dask).compute()))
+ r2 = r2_score(y_test, preds)
+
+ sk_reg = skrfr(
+ n_estimators=n_workers * 5, max_depth=None, random_state=42, n_jobs=-1
+ )
+ sk_reg.fit(X_train, y_train)
+ sk_r2 = r2_score(y_test, sk_reg.predict(X_test))
+ assert r2 >= (sk_r2 - 0.10)As per coding guidelines for python/**/test_*.py: “Test files must validate numerical correctness by comparing with scikit-learn…”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/tests/dask/test_dask_random_forest.py` around lines 360 - 388,
The two tests test_unlimited_max_depth_classifier and
test_unlimited_max_depth_regressor only check prediction length; change them to
evaluate numerical correctness by splitting X,y into train/test via a
deterministic split (use train_test_split with a fixed random_state), use
_prep_training_data on the training portion, fit cuRFC_mg / cuRFR_mg on the
training Dask arrays, predict on the held-out test set (not the training data),
and compare predictions to a scikit-learn baseline (e.g.,
sklearn.ensemble.RandomForestClassifier/Regressor with same
n_estimators/max_depth/random_state) or assert a numeric metric (accuracy/MSE)
is within an acceptable threshold; ensure types remain float32/int32 as before
and use the same n_workers-derived sizing so tests remain distributed.
Contributor
|
I will review soon |
jcrist
approved these changes
Mar 18, 2026
jcrist
left a comment
jcrist
left a comment
Member
There was a problem hiding this comment.
Overall approach LGTM! Do you have any thoughts on if/when/how we should change the default to max_depth=None?
Contributor
Author
I think we should deprecate the current default in this PR and then switch to unlimited by default in 26.06 and let users decide on the desired accuracy vs compute cost trade-off themselves. |
chyunsu3
approved these changes
Mar 18, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/cuml/tests/test_random_forest.py (1)
783-815:⚠️ Potential issue | 🟠 MajorThese unlimited-depth tests still don't prove
max_depth=Noneworks correctly.The classifier path only checks training accuracy against a shallow forest, which is not a stable invariant for bagged models, and the regressor path only checks output length. Both can pass without showing scikit-learn-compatible unlimited-depth behavior. Please use a fixed train/test split and compare classifier accuracy / regressor R² against sklearn with
max_depth=None.As per coding guidelines for
python/**/test_*.py: “Test files must validate numerical correctness by comparing with scikit-learn, include edge case coverage...”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/tests/test_random_forest.py` around lines 783 - 815, The tests test_unlimited_max_depth_classifier and test_unlimited_max_depth_regressor are insufficiently validating max_depth=None; replace the current checks with a deterministic train/test split (use make_classification/make_regression to create X,y then train_test_split), fit your estimator instances curfc and curfr on the train set, compute classifier accuracy (accuracy_score) and regressor R² on the test set, then fit sklearn.ensemble.RandomForestClassifier/RandomForestRegressor with identical parameters including max_depth=None and compare metrics (e.g., assert the custom classifier accuracy and regressor R² are close to sklearn's within a small tolerance) to prove scikit-learn-compatible unlimited-depth behavior.python/cuml/tests/dask/test_dask_random_forest.py (1)
360-387:⚠️ Potential issue | 🟠 MajorThese unlimited-depth Dask tests still don't verify correctness.
Both tests train and predict on the same partitions and only assert
len(preds) == len(y). They will still pass ifmax_depth=Nonesilently behaves like the legacy depth or if the predictions are numerically wrong but correctly sized. Please switch to a deterministic train/test split, setrandom_state, and compare accuracy/R² against a scikit-learnmax_depth=Nonebaseline on held-out data.As per coding guidelines for
python/**/test_*.py: “Test files must validate numerical correctness by comparing with scikit-learn, include edge case coverage...”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/tests/dask/test_dask_random_forest.py` around lines 360 - 387, Update both tests to validate numerical correctness by performing a deterministic train/test split and comparing model performance to scikit-learn's equivalent with max_depth=None: use train_test_split with a fixed random_state to split X, y into train and test (do not train and predict on the same partitions), prepare Dask partitions for the training set via _prep_training_data (keep partitions_per_worker=1), instantiate cuRFC_mg/cuRFR_mg with max_depth=None and fixed n_estimators, fit on the Dask training data, compute predictions on the held-out test set (use .compute() and convert to numpy), create a scikit-learn RandomForestClassifier/RandomForestRegressor with max_depth=None and the same random_state/n_estimators, fit on the same training arrays and compute baseline metrics (accuracy for test_unlimited_max_depth_classifier, r2_score for test_unlimited_max_depth_regressor), and assert the cuML metric is within a small tolerance of scikit-learn's metric.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/tests/dask/test_dask_random_forest.py`:
- Around line 524-541: The test currently only checks the default sentinel;
after fitting each estimator you must also assert the worker-side params and
compare predictions to ensure explicit branches are used: after
clf_explicit.fit(X_dask, y_dask) and clf_none.fit(X_dask, y_dask) fetch their
worker_params (via the same get_params/inspection used for clf_default) and
assert that p["max_depth"] == 16 for clf_explicit and p["max_depth"] is None (or
the concrete representation used on workers) for clf_none; additionally compute
predictions from clf_default.predict(X_dask) and clf_explicit.predict(X_dask)
(same random_state) and assert they are different (or use an appropriate
array-assertion) to prove the explicit max_depth=16 path was applied on workers.
---
Duplicate comments:
In `@python/cuml/tests/dask/test_dask_random_forest.py`:
- Around line 360-387: Update both tests to validate numerical correctness by
performing a deterministic train/test split and comparing model performance to
scikit-learn's equivalent with max_depth=None: use train_test_split with a fixed
random_state to split X, y into train and test (do not train and predict on the
same partitions), prepare Dask partitions for the training set via
_prep_training_data (keep partitions_per_worker=1), instantiate
cuRFC_mg/cuRFR_mg with max_depth=None and fixed n_estimators, fit on the Dask
training data, compute predictions on the held-out test set (use .compute() and
convert to numpy), create a scikit-learn
RandomForestClassifier/RandomForestRegressor with max_depth=None and the same
random_state/n_estimators, fit on the same training arrays and compute baseline
metrics (accuracy for test_unlimited_max_depth_classifier, r2_score for
test_unlimited_max_depth_regressor), and assert the cuML metric is within a
small tolerance of scikit-learn's metric.
In `@python/cuml/tests/test_random_forest.py`:
- Around line 783-815: The tests test_unlimited_max_depth_classifier and
test_unlimited_max_depth_regressor are insufficiently validating max_depth=None;
replace the current checks with a deterministic train/test split (use
make_classification/make_regression to create X,y then train_test_split), fit
your estimator instances curfc and curfr on the train set, compute classifier
accuracy (accuracy_score) and regressor R² on the test set, then fit
sklearn.ensemble.RandomForestClassifier/RandomForestRegressor with identical
parameters including max_depth=None and compare metrics (e.g., assert the custom
classifier accuracy and regressor R² are close to sklearn's within a small
tolerance) to prove scikit-learn-compatible unlimited-depth behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32eb8747-73f9-4060-b489-923a4169e4a4
📒 Files selected for processing (7)
python/cuml/cuml/dask/ensemble/randomforestclassifier.pypython/cuml/cuml/dask/ensemble/randomforestregressor.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/tests/dask/test_dask_random_forest.pypython/cuml/tests/test_random_forest.py
✅ Files skipped from review due to trivial changes (3)
- python/cuml/cuml/ensemble/randomforestregressor.py
- python/cuml/cuml/dask/ensemble/randomforestregressor.py
- python/cuml/cuml/dask/ensemble/randomforestclassifier.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/cuml/ensemble/randomforestclassifier.py
- python/cuml/cuml/ensemble/randomforest_common.pyx
None indicates unlimited.
csadorf
force-pushed
the
rf-support-unlimited-depth
branch
from
March 23, 2026 16:49
16c10c4 to
d1d6b7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh (1)
81-87: Remove the remaining unused kernel parameters in the node-split path.After dropping
max_depth,min_samples_splitandmax_leavesare still threaded through but never used innodeSplitKernel. Cleaning them out will simplify this API and reduce future call-site mistakes.♻️ Suggested cleanup
-static __global__ void nodeSplitKernel(const IdxT min_samples_leaf, - const IdxT min_samples_split, - const IdxT max_leaves, +static __global__ void nodeSplitKernel(const IdxT min_samples_leaf, const DataT min_impurity_decrease, const Dataset<DataT, LabelT, IdxT> dataset, const NodeWorkItem* work_items, const Split<DataT, IdxT>* splits) -void launchNodeSplitKernel(const IdxT min_samples_leaf, - const IdxT min_samples_split, - const IdxT max_leaves, +void launchNodeSplitKernel(const IdxT min_samples_leaf, const DataT min_impurity_decrease, const Dataset<DataT, LabelT, IdxT>& dataset, const NodeWorkItem* work_items, const size_t work_items_size, const Split<DataT, IdxT>* splits, cudaStream_t builder_stream) { auto constexpr smem_size = 2 * sizeof(IdxT) * TPB; nodeSplitKernel<DataT, LabelT, IdxT, TPB> - <<<work_items_size, TPB, smem_size, builder_stream>>>(min_samples_leaf, - min_samples_split, - max_leaves, - min_impurity_decrease, + <<<work_items_size, TPB, smem_size, builder_stream>>>(min_samples_leaf, + min_impurity_decrease, dataset, work_items, splits); }Also applies to: 100-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh` around lines 81 - 87, The nodeSplitKernel signature still accepts unused parameters (min_samples_split, max_leaves and min_depth was already removed) — remove these unused kernel parameters from the nodeSplitKernel declaration (and any overloads) and update all callers and kernel launches that pass min_samples_split or max_leaves to instead pass only the required arguments (e.g., min_samples_leaf, min_impurity_decrease, dataset, work_items, splits); also adjust any declarations or typedefs referencing nodeSplitKernel so the parameter list matches and rebuild to ensure all call-sites are fixed (search for nodeSplitKernel and its kernel launches to locate every usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/cuml/tests/test_random_forest.py`:
- Around line 799-813: The test test_unlimited_max_depth_regressor only checks
prediction length; update it to validate numerical correctness by computing
predictions = reg.predict(X), asserting all values are finite (use np.isfinite),
and computing R² with sklearn.metrics.r2_score; build a shallow baseline
regressor (e.g., baseline = curfr(n_estimators=10, max_depth=3,
random_state=42)), fit it on X,y, get baseline_preds and assert r2_score(y,
predictions) > r2_score(y, baseline_preds). Keep the existing param/params
checks and use the same random_state/makeup as curfr/reg/reg2 to ensure
reproducibility.
- Around line 779-797: Add a scikit-learn baseline check to the classifier and
regressor unlimited-depth tests: in test_unlimited_max_depth_classifier (and the
analogous regressor test) after the existing param/get_params assertions, create
an sklearn.ensemble.RandomForestClassifier/RandomForestRegressor with the same
n_estimators, random_state and max_depth=None and fit/predict on the same X,y,
then assert that curfc (and the regressor's cuRF) predictions have comparable
numerical performance to sklearn (use accuracy_score for classifier, and
mean_squared_error or r2_score for regressor) when max_depth is None; keep the
existing -1 case for backward-compat coverage and place these comparisons right
after the existing shallow-vs-full assertions so the tests validate numerical
equivalence to sklearn for None semantics.
---
Nitpick comments:
In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh`:
- Around line 81-87: The nodeSplitKernel signature still accepts unused
parameters (min_samples_split, max_leaves and min_depth was already removed) —
remove these unused kernel parameters from the nodeSplitKernel declaration (and
any overloads) and update all callers and kernel launches that pass
min_samples_split or max_leaves to instead pass only the required arguments
(e.g., min_samples_leaf, min_impurity_decrease, dataset, work_items, splits);
also adjust any declarations or typedefs referencing nodeSplitKernel so the
parameter list matches and rebuild to ensure all call-sites are fixed (search
for nodeSplitKernel and its kernel launches to locate every usage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65fdaadb-30c9-4c6a-85f1-71fee654e733
📒 Files selected for processing (11)
cpp/include/cuml/tree/decisiontree.hppcpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhpython/cuml/cuml/dask/ensemble/randomforestclassifier.pypython/cuml/cuml/dask/ensemble/randomforestregressor.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/tests/dask/test_dask_random_forest.pypython/cuml/tests/test_random_forest.py
✅ Files skipped from review due to trivial changes (6)
- cpp/include/cuml/tree/decisiontree.hpp
- python/cuml/cuml/dask/ensemble/randomforestregressor.py
- python/cuml/cuml/ensemble/randomforestclassifier.py
- python/cuml/tests/dask/test_dask_random_forest.py
- python/cuml/cuml/dask/ensemble/randomforestclassifier.py
- python/cuml/cuml/ensemble/randomforestregressor.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/cuml/ensemble/randomforest_common.pyx
- cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh
Member
|
/merge |
Contributor
Author
|
@coderabbitai Create two issues for the follow-up work to deprecate the current default for max_depth in 26.06 and then switch to the new default in 26.08. Assign both issues to me. |
This was referenced Apr 2, 2026
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create both follow-up issues now. 🏁 Script executed: Length of output: 93
Both issues reference this PR ( |
This was referenced May 19, 2026
This was referenced Jun 1, 2026
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
6 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 PR enables unlimited tree depth via
max_depth=None, matching the scikit-learn API convention (previously cuML required a positive integer and had no unlimited option).Implemented by mapping
NonetoINT32_MAXbefore passing to the C++ layer — the same approach scikit-learn uses. Trees then grow until leaves are pure or other stopping criteria are met. Also removesmax_depthfromnodeSplitKernelandcomputeSplitKernel, where it was passed but never used; depth enforcement was always host-side inNodeQueue::IsExpandable.Related to #6416; changing the default parameter will be done in a follow-up.