Deprecate max_depth=16 in RandomForest and add None support - #7958
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:
📝 WalkthroughWalkthroughAdds a sentinel default for RandomForest Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![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)
python/cuml/cuml/ensemble/randomforest_common.pyx (1)
438-439: Training-time validation appears redundant after constructor validation.After
__init__,self.max_depthis guaranteed to be either a positive integer or-1(sinceNoneis mapped to-1at line 342-343, and the validation at line 330-331 rejects non-positive values exceptNone).This check
self.max_depth <= 0 and self.max_depth != -1can only be true ifmax_depthis 0 or a negative number other than -1, but such values are already rejected in__init__. Consider removing this redundant check or clarifying the intent.♻️ Consider simplifying or removing
If this is defensive coding for cases where
max_depthmight be modified after construction:- if self.max_depth <= 0 and self.max_depth != -1: - raise ValueError("Must specify max_depth > 0 or None") + if self.max_depth <= 0 and self.max_depth != -1: + raise ValueError("max_depth must be > 0 or -1 (unlimited)")Or remove entirely if truly unreachable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/ensemble/randomforest_common.pyx` around lines 438 - 439, The runtime check "if self.max_depth <= 0 and self.max_depth != -1: raise ValueError(...)" in randomforest_common.pyx is redundant given the constructor (__init__) already maps None->-1 and validates max_depth; remove this check from the training path (e.g., in the fit/training method where it appears) or replace it with a short defensive assertion/comment indicating it should be unreachable. Update or remove the validation in the training code that references self.max_depth so only the constructor (__init__) enforces the invariant (or keep a one-line assert like "assert self.max_depth > 0 or self.max_depth == -1" with an explanatory comment).
🤖 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/cuml/dask/ensemble/base.py`:
- Around line 49-55: The warning currently fires even when a caller explicitly
passes max_depth=16 because the code checks kwargs.get('max_depth', 16) == 16;
change this to only warn when max_depth was not provided (e.g. if 'max_depth'
not in kwargs or kwargs.get('max_depth', None) is None) so explicit max_depth=16
does not trigger it, and add stacklevel=2 to the warnings.warn call to point the
warning at the caller; update the block around the existing condition in base.py
where the default max_depth warning is emitted to use the new presence check and
include stacklevel=2 in the warnings.warn invocation.
In `@python/cuml/cuml/ensemble/randomforest_common.pyx`:
- Around line 333-339: The warning is emitted whenever max_depth == 16,
including when a user explicitly passes max_depth=16; change the logic so the
warning only fires when the caller relied on the default (i.e., max_depth was
not provided). Implement this by using a sentinel default (e.g.,
max_depth=_DEFAULT_MAX_DEPTH or a private sentinel like _MAX_DEPTH_NOT_SET) or
by adding an explicit flag parameter (e.g., max_depth_provided) set in the
constructor call site, then check for the sentinel/not-provided state before
issuing the warning; also add stacklevel=2 to the warnings.warn call so the
warning points at the user's call site. Ensure you update the function/class
signature where max_depth is declared and adjust all internal callers to pass
the sentinel or the provided flag accordingly, and keep the warning message and
FutureWarning type unchanged.
---
Nitpick comments:
In `@python/cuml/cuml/ensemble/randomforest_common.pyx`:
- Around line 438-439: The runtime check "if self.max_depth <= 0 and
self.max_depth != -1: raise ValueError(...)" in randomforest_common.pyx is
redundant given the constructor (__init__) already maps None->-1 and validates
max_depth; remove this check from the training path (e.g., in the fit/training
method where it appears) or replace it with a short defensive assertion/comment
indicating it should be unreachable. Update or remove the validation in the
training code that references self.max_depth so only the constructor (__init__)
enforces the invariant (or keep a one-line assert like "assert self.max_depth >
0 or self.max_depth == -1" with an explanatory comment).
🪄 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: Pro
Run ID: 19eb66ae-31b7-4bca-b076-f22489182ad9
📒 Files selected for processing (7)
CHANGELOG.mdpython/cuml/cuml/dask/ensemble/base.pypython/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.py
There was a problem hiding this comment.
Pull request overview
This PR introduces a deprecation path for cuML RandomForest’s current default max_depth=16 and adds support for max_depth=None (mapped internally to -1) across single-GPU and Dask RandomForest estimators, along with doc/release-note updates.
Changes:
- Emit a
FutureWarningrelated to the upcoming default change frommax_depth=16toNone(unlimited depth). - Add
max_depth=Nonesupport by mappingNone -> -1for the backend and tightenmax_depthvalidation. - Update docstrings for RandomForest classes and add a changelog entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| python/cuml/cuml/ensemble/randomforest_common.pyx | Adds max_depth validation, emits FutureWarning, and maps None to -1. |
| python/cuml/cuml/ensemble/randomforestclassifier.py | Updates max_depth parameter docs for single-GPU classifier. |
| python/cuml/cuml/ensemble/randomforestregressor.py | Updates max_depth parameter docs for single-GPU regressor. |
| python/cuml/cuml/dask/ensemble/base.py | Adds a deprecation FutureWarning in Dask model creation path. |
| python/cuml/cuml/dask/ensemble/randomforestclassifier.py | Updates max_depth parameter docs for Dask classifier. |
| python/cuml/cuml/dask/ensemble/randomforestregressor.py | Updates max_depth parameter docs for Dask regressor. |
| CHANGELOG.md | Adds a breaking-change note about the max_depth default deprecation. |
Comments suppressed due to low confidence (1)
python/cuml/cuml/ensemble/randomforest_common.pyx:351
- Mapping
max_depth=Noneto-1is fine for the backend, butBaseRandomForestModel._params_to_cpucurrently returns{"max_depth": self.max_depth}. With this change, exporting a model configured withNonewill pass-1to scikit-learn, which is not a validmax_depththere. Ensure CPU export maps-1back toNone(and similarly treat-1consistently as 'unlimited' in any public-facing params).
# Map None to -1 for the backend
if max_depth is None:
max_depth = -1
self.split_criterion = split_criterion
self.n_estimators = n_estimators
self.bootstrap = bootstrap
self.max_samples = max_samples
self.max_depth = max_depth
self.max_leaves = max_leaves
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Only allow positive numbers or None | ||
| if max_depth is not None and max_depth <= 0: | ||
| raise ValueError("max_depth must be > 0 or None") |
There was a problem hiding this comment.
max_depth validation currently rejects -1 (since -1 <= 0), but the docs now state that -1 is supported for unlimited depth and _fit_forest explicitly allows self.max_depth == -1. Align these by allowing -1 here (and update the error message/comment accordingly).
| # Only allow positive numbers or None | |
| if max_depth is not None and max_depth <= 0: | |
| raise ValueError("max_depth must be > 0 or None") | |
| # Only allow positive numbers, None, or -1 for unlimited depth | |
| if max_depth is not None and max_depth != -1 and max_depth <= 0: | |
| raise ValueError("max_depth must be > 0, -1, or None") |
| if max_depth == 16: | ||
| warnings.warn( | ||
| "The default value of 'max_depth' will change from 16 to " | ||
| "None (unlimited depth) in release 26.08. To suppress this " | ||
| "warning, set 'max_depth' explicitly.", | ||
| FutureWarning | ||
| ) |
There was a problem hiding this comment.
The FutureWarning triggers whenever max_depth == 16, including when a caller explicitly sets max_depth=16. That makes the warning text misleading (it claims setting explicitly will suppress it) and will also break cuML's pytest config where FutureWarning is treated as an error. Consider moving this warning to the Python wrapper __init__ where you can warn only when max_depth was not provided, or use a sentinel to distinguish default vs explicit 16; also consider adding an appropriate stacklevel so the warning points at user code.
| * Deprecate `max_depth=16` default in `RandomForestClassifier` and | ||
| `RandomForestRegressor` (both single-GPU and Dask variants). The default will change to `None` (unlimited depth) in release 26.08 |
There was a problem hiding this comment.
Markdown list formatting: the second line of this bullet (RandomForestRegressor...) is not indented, so it won’t be treated as part of the bullet item. Keep the bullet on one line or indent continuation lines so the list renders correctly.
| * Deprecate `max_depth=16` default in `RandomForestClassifier` and | |
| `RandomForestRegressor` (both single-GPU and Dask variants). The default will change to `None` (unlimited depth) in release 26.08 | |
| * Deprecate `max_depth=16` default in `RandomForestClassifier` and `RandomForestRegressor` (both single-GPU and Dask variants). The default will change to `None` (unlimited depth) in release 26.08 |
| Maximum tree depth. Must be greater than 0 or -1 (None covers -1). | ||
| Unlimited depth (i.e., until leaves are pure) is supported by | ||
| providing None or -1.\n |
There was a problem hiding this comment.
Docstring says max_depth supports -1, but the new validation in BaseRandomForestModel.__init__ currently raises for max_depth=-1. Either adjust the implementation to accept -1 (for backward compatibility) or update the docs to advertise only None for unlimited depth.
| Maximum tree depth. Must be greater than 0 or -1 (None covers -1). | |
| Unlimited depth (i.e., until leaves are pure) is supported by | |
| providing None or -1.\n | |
| Maximum tree depth. Must be greater than 0, or `None`. | |
| Unlimited depth (i.e., until leaves are pure) is supported by | |
| providing `None`.\n |
| Maximum tree depth. Must be greater than 0 or -1 (None covers -1). | ||
| Unlimited depth (i.e., until leaves are pure) is supported by | ||
| providing None or -1.\n |
There was a problem hiding this comment.
Docstring says max_depth supports -1, but the new validation in BaseRandomForestModel.__init__ currently raises for max_depth=-1. Either adjust the implementation to accept -1 (for backward compatibility) or update the docs to advertise only None for unlimited depth.
| Maximum tree depth. Must be greater than 0 or -1 (None covers -1). | |
| Unlimited depth (i.e., until leaves are pure) is supported by | |
| providing None or -1.\n | |
| Maximum tree depth. Must be greater than 0, or ``None`` for | |
| unlimited depth (i.e., until leaves are pure).\n |
| Maximum tree depth. Must be greater than 0 or -1 (None covers -1). | ||
| Unlimited depth (i.e., until leaves are pure) is supported by | ||
| providing None or -1.\n |
There was a problem hiding this comment.
Docstring says max_depth supports -1, but the new validation in BaseRandomForestModel.__init__ currently raises for max_depth=-1. Either adjust the implementation to accept -1 (for backward compatibility) or update the docs to advertise only None for unlimited depth.
| Maximum tree depth. Must be greater than 0 or -1 (None covers -1). | |
| Unlimited depth (i.e., until leaves are pure) is supported by | |
| providing None or -1.\n | |
| Maximum tree depth. Must be greater than 0, or ``None`` for | |
| unlimited depth. | |
| Unlimited depth (i.e., until leaves are pure) is supported by | |
| providing ``None``.\n |
| # Only allow positive numbers or None | ||
| if max_depth is not None and max_depth <= 0: | ||
| raise ValueError("max_depth must be > 0 or None") | ||
|
|
There was a problem hiding this comment.
The new check only validates the sign of max_depth. Since the public API documents max_depth as int or None, passing a float/NumPy float (e.g. 3.0) will currently make it through __init__ and then fail later when Cython tries to pass it to the C++ backend as an int. Consider validating max_depth is an integer-like type (and raising a clear TypeError/ValueError) close to this check.
| if kwargs.get('max_depth',16) == 16: | ||
| warnings.warn( | ||
| "The default value of 'max_depth' will change from 16 to " | ||
| "None (unlimited depth) in release 26.08. To suppress this " | ||
| "warning, set 'max_depth' explicitly.", | ||
| FutureWarning | ||
| ) |
There was a problem hiding this comment.
With the current implementation, the Dask path can emit the deprecation warning twice: once here on the client, and again on each worker when cuRFC/cuRFR is constructed (since the single-GPU constructors also warn on max_depth==16). This can create very noisy logs and increases the chance of failures in environments treating FutureWarning as errors. Consider emitting this warning in only one place (preferably at the user-facing entrypoint where you can detect default usage).
|
We haven't really decided when to introduce deprecation. Closing for now. cc @csadorf |
|
Thanks for the feedback, @hcho3. I noticed the docstrings for |
|
@Nzouh After internal discussion, we decided to introduce the deprecation of TODOs
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 3: Change the "### 🚨 Breaking Changes" heading in CHANGELOG.md to a
level-2 heading ("## 🚨 Breaking Changes") so the file goes H1 → H2 and
satisfies markdownlint MD001; update the single heading token to "##" to remove
the H1→H3 jump.
In `@python/cuml/cuml/dask/ensemble/base.py`:
- Around line 49-55: The _create_model() path warns when 'max_depth' is missing
but does not add the legacy value to kwargs, causing worker-side model_func(...)
to re-trigger the same FutureWarning; after emitting the warnings.warn call in
_create_model(), set kwargs["max_depth"] = 16 so the legacy value is forwarded
explicitly to worker/model_func and the warning is only shown once.
In `@python/cuml/cuml/ensemble/randomforest_common.pyx`:
- Around line 337-339: Tighten validation for max_depth in the constructor by
rejecting any value that is not None and not a positive integer: change the
current check to verify isinstance(max_depth, numbers.Integral) (or int) and
max_depth > 0, and raise ValueError("max_depth must be > 0 or None") for
anything else; add/import numbers if needed and ensure np.nan, floats like 1.5,
and string values are rejected at __init__ time (reference max_depth in
randomforest_common.pyx constructor).
🪄 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: Pro
Run ID: babb23ae-89ad-42ef-abcc-df653a783eda
📒 Files selected for processing (7)
CHANGELOG.mdpython/cuml/cuml/dask/ensemble/base.pypython/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.py
✅ Files skipped from review due to trivial changes (3)
- python/cuml/cuml/dask/ensemble/randomforestclassifier.py
- python/cuml/cuml/ensemble/randomforestclassifier.py
- python/cuml/cuml/ensemble/randomforestregressor.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/dask/ensemble/randomforestregressor.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
CHANGELOG.md (1)
3-3:⚠️ Potential issue | 🟡 MinorUse an H2 heading for “Breaking Changes” under the new release header.
Line 3 should be
##(not###) to keep heading levels sequential (H1 → H2) and avoid markdownlint MD001 failures.Suggested diff
-### 🚨 Breaking Changes +## 🚨 Breaking Changes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 3, Replace the third-line H3 heading "### 🚨 Breaking Changes" with an H2 heading "## 🚨 Breaking Changes" in CHANGELOG.md so the heading level follows the H1 → H2 sequence and avoids markdownlint MD001 failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CHANGELOG.md`:
- Line 3: Replace the third-line H3 heading "### 🚨 Breaking Changes" with an H2
heading "## 🚨 Breaking Changes" in CHANGELOG.md so the heading level follows
the H1 → H2 sequence and avoids markdownlint MD001 failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af5ca71d-ca2b-4102-b24a-1fed309906f5
📒 Files selected for processing (1)
CHANGELOG.md
…_depth type validation
…h/cuml into feature/max-depth-deprecate
|
Hi @hcho3, I've updated the PR to address the requested TODOs:
The PR is now ready for a final review. Thanks! |
|
/ok to test 59f1b73 |
|
@hcho3 fixed the stylistic issues |
|
/ok to test 55fca14 |
|
/ok to test 39342b5 |
|
@hcho3 fixed FutureWarning matching on the test_random_forest.py file |
|
/ok to test 3dfc806 |
|
@hcho3 Fixed the Future warning issue |
|
@csadorf Are there any other issues you would like me to take a look at? |
|
/ok to test 8e8be69 |
|
/ok to test 064e496 |
|
/merge |
|
@csadorf I'll take a look and try to address it — thanks again for all the help and guiding me through this PR! |
xref NVIDIA/cuml#7958 (IIUC, 16 was the previous default) Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Matthew Murray (https://github.com/Matt711) URL: #22324
…2324) xref NVIDIA/cuml#7958 (IIUC, 16 was the previous default) Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Matthew Murray (https://github.com/Matt711) URL: NVIDIA#22324
…2324) xref NVIDIA/cuml#7958 (IIUC, 16 was the previous default) Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Matthew Murray (https://github.com/Matt711) URL: NVIDIA#22324
Closes #7982 Mirrors sklearn PR #32050 on cuml.SVC and cuml.LinearSVC. Sentinel + _effective_X property pattern, same as PR #7958. FutureWarning fires from fit when the user passes an explicit value. Also fixes a latent bug in the accel proxy where _gpu_fit was reading self.probability truthily. Default SVC() on small data would have routed through the probability code path since the sentinel string is truthy. Authors: - Steve Collins (https://github.com/switch527) - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Jim Crist-Harif (https://github.com/jcrist) URL: #8089
Summary
This PR implements the deprecation of the
max_depth=16default in cuML Random Forest estimators (Single-GPU and Dask) to align with scikit-learn's unlimited depth policy, fulfilling issue #7945.Changes
max_depth=16; default will change toNonein 26.08.max_depth=None(unlimited depth), mapped toINT_MAXfor the C++ backend.max_depthis a positive integer orNone.pytestmarkfilters withTODO(26.08)to suppress warnings in existing tests for CI stability.CHANGELOG.md.Verification
Logic verified via custom tests for default behavior,
Nonemapping, and value validation. Test suite updated to handle new warnings.Closes #7945