Several ensemble scikit-learn compatibility improvements - #8023
Conversation
jcrist
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:
📝 WalkthroughWalkthroughCentralizes input validation using check_inputs/check_array across RandomForest and FIL, adds a shared RandomForestMixin for GPU input checks and predict* overrides, preserves GPU presence across pickle via load_on_gpu, updates Cython pointer access, adds predict_log_proba, and documents NaN-triggered CPU fallbacks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuml/cuml/fil/fil.pyx (2)
311-332:⚠️ Potential issue | 🔴 CriticalFix the device-argument order in the double-precision branch.
The
float64path passesin_devandout_devin the opposite order from thefloat32branch, so FIL will misinterpret the source/target memory locations for double-precision inference.🐛 Proposed fix
else: self.model.predict[double]( self.raft_proto_handle, <double*> out_ptr, <double*> in_ptr, n_rows, - in_dev, out_dev, + in_dev, infer_type_enum, chunk_specification )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/fil/fil.pyx` around lines 311 - 332, The double-precision branch passes device arguments in the wrong order causing FIL to misinterpret source/target memory; in the else branch where model_dtype != np.float32 (the double path), swap the positions of in_dev and out_dev when calling self.model.predict[double] so the argument order matches the float branch (use out_dev then in_dev), keeping all other parameters (self.raft_proto_handle, out_ptr/in_ptr casts, n_rows, infer_type_enum, chunk_specification) unchanged.
257-300:⚠️ Potential issue | 🟠 MajorAdd type validation or update
predsparameter documentation.The code accesses
preds.index(line 297) andpreds.ptr(line 303) without validating thatpredsis aCumlArray. However, the docstrings documentpredsas accepting any "C-major array" (NumPy, CuPy), which do not have.indexor.ptrattributes. This will raiseAttributeErrorif users pass raw NumPy/CuPy arrays despite what the documentation suggests.Either:
- Add explicit type checking and conversion to
CumlArrayif needed, or- Clarify in docstrings that
predsmust be aCumlArrayif provided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/fil/fil.pyx` around lines 257 - 300, The code assumes preds has CumlArray attributes (preds.index, preds.ptr) but docs claim preds can be any C-major array (NumPy/CuPy), which will raise AttributeError; update the predict path in predict/predict_type handling to validate and convert preds to a CumlArray when supplied (e.g., detect types that are not CumlArray and call CumlArray.from_array/constructor or use existing CumlArray.empty semantics), or alternatively enforce and document that preds must already be a CumlArray by raising a clear TypeError; specifically modify the block that checks preds (symbol: preds) after output_shape is computed and before using preds.index/ptr to perform type checking/conversion, and ensure dtype/device/layout checks mirror the existing TODO handling so downstream code can safely access preds.index and preds.ptr.
🧹 Nitpick comments (1)
python/cuml/cuml/accel/_overrides/sklearn/ensemble.py (1)
15-29: Short-circuitsample_weightbefore validatingX.
sample_weightalways triggers CPU fallback here, so thecheck_array(X, ...)work on Lines 17-26 is wasted and will be repeated by the CPU estimator anyway. Moving that guard to the top avoids an extra full validation pass on large inputs.♻️ Proposed change
class _RandomForestMixin: def _check_inputs(self, X, y=None, sample_weight=None): + if sample_weight is not None: + raise UnsupportedOnGPU("`sample_weight` is not supported") + # Fallback to CPU if NaN in X try: check_array( X, mem_type=None, order=None, ensure_2d=False, input_name="X" ) except ValueError as exc: if "NaN" in str(exc): raise UnsupportedOnGPU( "Missing values are not supported" ) from None raise - - if sample_weight is not None: - raise UnsupportedOnGPU("`sample_weight` is not supported")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/accel/_overrides/sklearn/ensemble.py` around lines 15 - 29, In _check_inputs, sample_weight is checked after running expensive check_array on X causing redundant validation and CPU fallback; move the sample_weight guard to the top of _check_inputs (before calling check_array) so UnsupportedOnGPU("`sample_weight` is not supported") returns immediately when sample_weight is not None, avoiding the expensive check_array call and duplicate validation.
🤖 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/ensemble/randomforestclassifier.py`:
- Around line 225-235: The fit method currently forces dtype conversion by
calling check_inputs with convert_dtype=True; modify the check_inputs call in
randomforestclassifier.fit to pass through the method's convert_dtype parameter
(i.e., convert_dtype=convert_dtype) so the public convert_dtype argument is
honored (or if fit uses an instance attribute, use
convert_dtype=self.convert_dtype) and ensure any related callers/tests reflect
the intended behavior; keep the rest of the check_inputs args unchanged and
update any docstring or signature usage if needed.
---
Outside diff comments:
In `@python/cuml/cuml/fil/fil.pyx`:
- Around line 311-332: The double-precision branch passes device arguments in
the wrong order causing FIL to misinterpret source/target memory; in the else
branch where model_dtype != np.float32 (the double path), swap the positions of
in_dev and out_dev when calling self.model.predict[double] so the argument order
matches the float branch (use out_dev then in_dev), keeping all other parameters
(self.raft_proto_handle, out_ptr/in_ptr casts, n_rows, infer_type_enum,
chunk_specification) unchanged.
- Around line 257-300: The code assumes preds has CumlArray attributes
(preds.index, preds.ptr) but docs claim preds can be any C-major array
(NumPy/CuPy), which will raise AttributeError; update the predict path in
predict/predict_type handling to validate and convert preds to a CumlArray when
supplied (e.g., detect types that are not CumlArray and call
CumlArray.from_array/constructor or use existing CumlArray.empty semantics), or
alternatively enforce and document that preds must already be a CumlArray by
raising a clear TypeError; specifically modify the block that checks preds
(symbol: preds) after output_shape is computed and before using preds.index/ptr
to perform type checking/conversion, and ensure dtype/device/layout checks
mirror the existing TODO handling so downstream code can safely access
preds.index and preds.ptr.
---
Nitpick comments:
In `@python/cuml/cuml/accel/_overrides/sklearn/ensemble.py`:
- Around line 15-29: In _check_inputs, sample_weight is checked after running
expensive check_array on X causing redundant validation and CPU fallback; move
the sample_weight guard to the top of _check_inputs (before calling check_array)
so UnsupportedOnGPU("`sample_weight` is not supported") returns immediately when
sample_weight is not None, avoiding the expensive check_array call and duplicate
validation.
🪄 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: 3a01dd07-15e9-4ef5-bc7a-34c4c153e5a6
📒 Files selected for processing (11)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/ensemble.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml/fil/fil.pyxpython/cuml/cuml_accel_tests/test_estimator_proxy.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_random_forest.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
35bd461 to
c813365
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/accel/_overrides/sklearn/ensemble.py`:
- Around line 17-26: The _check_inputs function currently calls
sklearn.utils._array_api.check_array which by default rejects sparse inputs with
a TypeError that isn't caught, causing hard failures and preventing CPU
fallback; update _check_inputs to detect scipy sparse inputs (e.g., via
scipy.sparse.issparse or checking for sparse input types) before calling
check_array and raise UnsupportedOnGPU("Sparse input not supported on GPU") so
that sparse X triggers the CPU fallback used by _gpu_predict, _gpu_score,
_gpu_predict_proba, and _gpu_predict_log_proba; keep the existing NaN ValueError
handling intact and only add the explicit sparse check and raise.
🪄 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: f1133444-c89b-4802-97e5-cade66c50aa8
📒 Files selected for processing (10)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/ensemble.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml_accel_tests/test_estimator_proxy.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_random_forest.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
✅ Files skipped from review due to trivial changes (4)
- python/cuml/cuml/ensemble/randomforest_common.pyx
- python/cuml/tests/test_random_forest.py
- docs/source/cuml-accel/limitations.rst
- python/cuml/cuml/ensemble/randomforestclassifier.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/cuml/cuml/accel/estimator_proxy.py
- python/cuml/cuml/ensemble/randomforestregressor.py
|
Hmmm, maybe FIL does support missing inputs for inference? AFAICT from the statistical tests our random forests don't fit properly with missing data inputs, so I think they should be excluded from fit. But FIL itself has a test with missing inputs, so they should be allowed there? Before I fixup the change, I want to confirm this understanding of whether non-finite data should be allowed/forbidden is correct. cc @hcho3 - do you know what the behavior around |
ef4f459 to
fe52d0f
Compare
|
I've updated this PR to only disallow NaN inputs in I still would appreciate feedback on whether my understanding of missing value support is correct here (I think it is), but if it is I think the PR as is is the cleanest path forward. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuml/cuml/fil/fil.pyx (1)
612-635:⚠️ Potential issue | 🟠 Major
allow_nanis added in__init__but not exposed via estimator param introspection.Line 624 introduces a new constructor parameter, but
_get_param_names()doesn’t include it. That meansget_params()/set_params()/clone()can dropallow_nan, causing inconsistent behavior after cloning or parameter roundtrips.🔧 Proposed fix
`@classmethod` def _get_param_names(cls): return [ *super()._get_param_names(), "treelite_model", "is_classifier", "layout", "default_chunk_size", "align_bytes", "precision", "device_id", + "allow_nan", ]As per coding guidelines: "API breaking changes to Python estimator interfaces, removing or renaming public methods/attributes without deprecation, or breaking backward compatibility require at least one release cycle for deprecations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/fil/fil.pyx` around lines 612 - 635, The constructor adds a new parameter allow_nan but it isn't included in the estimator introspection, so get_params/set_params/clone can lose it; update the estimator parameter list returned by _get_param_names() to include "allow_nan" (ensuring the string matches the __init__ arg) so that sklearn-style introspection preserves this option across get_params()/set_params()/clone() operations and maintain the default handling in __init__ and _load_to_fil calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/cuml/cuml/fil/fil.pyx`:
- Around line 612-635: The constructor adds a new parameter allow_nan but it
isn't included in the estimator introspection, so get_params/set_params/clone
can lose it; update the estimator parameter list returned by _get_param_names()
to include "allow_nan" (ensuring the string matches the __init__ arg) so that
sklearn-style introspection preserves this option across
get_params()/set_params()/clone() operations and maintain the default handling
in __init__ and _load_to_fil calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 268b4897-5e52-44ff-9024-43bcb1de72cf
📒 Files selected for processing (11)
docs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/ensemble.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml/fil/fil.pyxpython/cuml/cuml_accel_tests/test_estimator_proxy.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_random_forest.pypython/cuml/tests/test_sklearn_compatibility.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
✅ Files skipped from review due to trivial changes (1)
- docs/source/cuml-accel/limitations.rst
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cuml/cuml/ensemble/randomforest_common.pyx
- python/cuml/tests/test_random_forest.py
- python/cuml/cuml/ensemble/randomforestregressor.py
fe52d0f to
4ae6f38
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuml/cuml/fil/fil.pyx (2)
624-635:⚠️ Potential issue | 🔴 CriticalAssign
allow_nanbefore any reload-triggering setters.
self.treelite_model = treelite_modelcan call_reload_model()immediately, and that path now readsself.allow_nan. With the current order, construction can hitAttributeErrorbeforeallow_nanis initialized.Proposed fix
- self.treelite_model = treelite_model - self.allow_nan = allow_nan + self.allow_nan = allow_nan + self.treelite_model = treelite_model🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/fil/fil.pyx` around lines 624 - 635, The constructor assigns self.treelite_model (which may call _reload_model()) before setting self.allow_nan, causing a possible AttributeError; fix by assigning self.allow_nan = allow_nan (and any other fields that _reload_model/_load_to_fil read) before assigning self.treelite_model and before calling self._load_to_fil(device_id=...), i.e., move the allow_nan assignment earlier in the __init__ so it's initialized prior to any reload-triggering setters like treelite_model or calls to _load_to_fil.
327-336:⚠️ Potential issue | 🔴 CriticalFix the double-precision device-argument order.
The
predict[double]call at lines 327-336 passesin_devandout_devin the opposite order from the float branch (lines 316-326). This reversal routes reads/writes to the wrong memory space when input and output live on different devices.Proposed fix
self.model.predict[double]( self.raft_proto_handle, <double*> out_ptr, <double*> in_ptr, n_rows, - in_dev, - out_dev, + out_dev, + in_dev, infer_type_enum, chunk_specification )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/fil/fil.pyx` around lines 327 - 336, The double-precision predict call passes device arguments swapped; in the call to self.model.predict[double] swap the in_dev and out_dev arguments so they match the float branch (i.e., pass out_ptr, in_ptr, n_rows, out_dev, in_dev, infer_type_enum, chunk_specification) — ensure the order of device args for predict[double] mirrors the order used by predict[float] to avoid routing reads/writes to the wrong memory space.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/cuml/cuml/fil/fil.pyx`:
- Around line 624-635: The constructor assigns self.treelite_model (which may
call _reload_model()) before setting self.allow_nan, causing a possible
AttributeError; fix by assigning self.allow_nan = allow_nan (and any other
fields that _reload_model/_load_to_fil read) before assigning
self.treelite_model and before calling self._load_to_fil(device_id=...), i.e.,
move the allow_nan assignment earlier in the __init__ so it's initialized prior
to any reload-triggering setters like treelite_model or calls to _load_to_fil.
- Around line 327-336: The double-precision predict call passes device arguments
swapped; in the call to self.model.predict[double] swap the in_dev and out_dev
arguments so they match the float branch (i.e., pass out_ptr, in_ptr, n_rows,
out_dev, in_dev, infer_type_enum, chunk_specification) — ensure the order of
device args for predict[double] mirrors the order used by predict[float] to
avoid routing reads/writes to the wrong memory space.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1b3cef00-5e73-4500-8154-2498a134f6b2
📒 Files selected for processing (2)
python/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/fil/fil.pyx
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/cuml/ensemble/randomforest_common.pyx
|
@jcrist Yes, your understanding is correct. cuML random forest does not allow |
chyunsu3
left a comment
chyunsu3
left a comment
There was a problem hiding this comment.
Commenting on the changes on the FIL side.
178e04b to
2bd2395
Compare
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
PR looks good to me, had a couple of questions but not blocking
| X, mem_type=None, order=None, ensure_2d=False, input_name="X" | ||
| ) | ||
| except ValueError as exc: | ||
| if "NaN" in str(exc): |
There was a problem hiding this comment.
we control this error so this is fine, just gave me a bit of paus thinking about an obscure bug if we change say to "nan" instead of "NaN" or another change. Probably not worth mulling much about, but still gave me a bit of pause.
There was a problem hiding this comment.
I didn't love it, but it was quick to do and there's tests that will start failing if behavior ever changes. Seems better than nothing.
| n_rows, | ||
| in_dev, | ||
| out_dev, | ||
| in_dev, |
| layout=layout, | ||
| default_chunk_size=default_chunk_size, | ||
| align_bytes=align_bytes, | ||
| ensure_all_finite=True, |
There was a problem hiding this comment.
Quick question for the accel path here, with ensure_all_finite=True getting passed to ForestInference from _predict_model_on_gpu, and the proxy's _check_inputs already running check_array(..., ensure_all_finite=True) on X to detect NaN, aren't we now traversing X twice during accel predict? Once to translate NaN -> UnsupportedOnGPU, once inside FIL. Another small thing probably, but just wanted to ask about it more than block on it
There was a problem hiding this comment.
Indeed we are in cuml.accel alone. These checks run at ~600GiBs (on my machine) though, so I don't anticipate the double traversal being a measurable perf issue. Users using cuml proper also won't run into that issue.
|
/merge |
5 participants
This:
cuml.ensembleto the new validation system. Fixes Updatecuml.ensembleto new input validation #7991.cuml.filto the new validation system. Fixes Updatecuml.filto new input validation #7995.RandomForestClassifier.predict_log_proba. This method was missing and leading to an xfail incuml.accel.cuml.accelimplementations forRandomForestClassifierandRandomForestRegressorto fallback to CPU whenNaNis in the input. The CPU version natively supports missing values, our version does not. This fixes several xfailed tests.ProxyBaseto not try to reload models that were fit on CPU on GPU upon unpickling. In most cases this would already lead to a fallback (due to unsupported hyperparameters), but in some cases if the model was fit on unsupported data (like NaN inputs here) this could lead to issues. A test is also added.I recognize that this seems like a bunch of unrelated fixes, but all of these were needed to avoid no new regressions in the tests while moving to the new validation system. Felt simplest to handle these all in one PR.