Apply new validation to cuml.explainer - #8043
Merged
Merged
Conversation
github-actions
Bot
added
conda
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds 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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. 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 (1)
python/cuml/cuml/explainer/common.py (1)
35-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
model_func_calldocstring to match current return behavior.Line 39 still states this returns CuPy arrays, but the new path at Line 52 can return NumPy or CuPy depending on the model output/memory type.
As per coding guidelines "Missing docstrings for public methods ... must be addressed".Suggested docstring tweak
- Returns the results as CuPy arrays. + Returns validated model outputs as array-like values (NumPy/CuPy), + preserving host/device type where possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/explainer/common.py` around lines 35 - 40, The docstring for model_func_call is outdated (it claims the function always returns CuPy arrays) but the implementation may return NumPy or CuPy depending on the model output/memory type; update the docstring for model_func_call to clearly describe input expectations (X and gpu_model), the two code paths (converting NumPy->model input when gpu_model is False vs passing X directly when gpu_model is True) and the actual return behavior (may return NumPy arrays or CuPy arrays depending on the model's output/memory type), and include the return type and any side-effects so the public method documentation matches current behavior.
🤖 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/explainer/sampling.py`:
- Around line 54-75: The code currently coerces all inputs to GPU/CuPy and
rejects sparse inputs; update the input handling to preserve caller memory type
by using input_to_cuml_array(...) instead of check_array, record the original
input_type (numpy/pandas -> output_type="numpy", cudf/cupy -> "cupy", sparse ->
keep sparse and accept_sparse=True), pass that output_type into SimpleImputer
and KMeans, and set check/accept_sparse=True so the is_sparse branch can run;
ensure input_to_cuml_array also preserves row/column major order so subsequent
indexing (used in round_values and is_sparse checks) behaves the same.
---
Outside diff comments:
In `@python/cuml/cuml/explainer/common.py`:
- Around line 35-40: The docstring for model_func_call is outdated (it claims
the function always returns CuPy arrays) but the implementation may return NumPy
or CuPy depending on the model output/memory type; update the docstring for
model_func_call to clearly describe input expectations (X and gpu_model), the
two code paths (converting NumPy->model input when gpu_model is False vs passing
X directly when gpu_model is True) and the actual return behavior (may return
NumPy arrays or CuPy arrays depending on the model's output/memory type), and
include the return type and any side-effects so the public method documentation
matches current behavior.
🪄 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: 987b923e-9239-4b18-ac78-b499b64c926c
📒 Files selected for processing (13)
conda/environments/all_cuda-129_arch-aarch64.yamlconda/environments/all_cuda-129_arch-x86_64.yamlconda/environments/all_cuda-131_arch-aarch64.yamlconda/environments/all_cuda-131_arch-x86_64.yamldependencies.yamlpython/cuml/cuml/explainer/base.pyxpython/cuml/cuml/explainer/common.pypython/cuml/cuml/explainer/kernel_shap.pyxpython/cuml/cuml/explainer/sampling.pypython/cuml/cuml/explainer/tree_shap.pyxpython/cuml/pyproject.tomlpython/cuml/tests/explainer/test_gpu_treeshap.pypython/cuml/tests/explainer/test_shap_plotting.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/cuml/cuml/explainer/sampling.py (1)
54-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the caller’s memory type through validation and clustering.
check_arraydefaults tomem_type="device", and bothSimpleImputerandKMeansare then forced tooutput_type="cupy". For pandas/NumPy inputs this now moves the data to device, returns device-backed summaries/labels, and even converts the returned row index tocudf.Indexwhendetailed=True. That’s a behavior regression in exactly the input-preservation path this PR is touching.Possible localized fix
- X, index = check_array( - X, ensure_2d=False, ensure_all_finite=False, return_index=True - ) + X, index = check_array( + X, + mem_type=None, + ensure_2d=False, + ensure_all_finite=False, + return_index=True, + ) if X.ndim == 1: X = X.reshape(-1, 1) + output_type = "cupy" if isinstance(X, cp.ndarray) else "numpy" + # in case there are any missing values in data impute them imp = SimpleImputer( - missing_values=cp.nan, strategy="mean", output_type="cupy" + missing_values=cp.nan, strategy="mean", output_type=output_type ) X = imp.fit_transform(X) kmeans = KMeans( n_clusters=k, random_state=random_state, - output_type="cupy", + output_type=output_type, n_init="auto", ).fit(X)As per coding guidelines, "Correctly handle cuDF, pandas, and NumPy inputs using input_to_cuml_array() for consistent conversion; preserve input type in output where sensible; handle both row-major (C) and column-major (F) memory order".
Also applies to: 61-69, 82-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/explainer/sampling.py` around lines 54 - 56, The validation/clustering path currently forces device memory by calling check_array with the default mem_type and then using SimpleImputer/KMeans with output_type="cupy", which moves pandas/NumPy inputs to device and converts indexes to cudf; change this to use input_to_cuml_array() to detect and preserve the caller's input type and memory order (C/F), pass the detected mem_type into check_array and downstream SimpleImputer/KMeans instead of hardcoding output_type="cupy", and ensure any returned summaries/labels and the row index (when detailed=True) are converted back to the original input type (pandas/NumPy/cuDF) so the function preserves caller memory type and layout; update the logic around X, index = check_array(...) and the code paths using SimpleImputer and KMeans to follow this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/cuml/cuml/explainer/sampling.py`:
- Around line 54-56: The validation/clustering path currently forces device
memory by calling check_array with the default mem_type and then using
SimpleImputer/KMeans with output_type="cupy", which moves pandas/NumPy inputs to
device and converts indexes to cudf; change this to use input_to_cuml_array() to
detect and preserve the caller's input type and memory order (C/F), pass the
detected mem_type into check_array and downstream SimpleImputer/KMeans instead
of hardcoding output_type="cupy", and ensure any returned summaries/labels and
the row index (when detailed=True) are converted back to the original input type
(pandas/NumPy/cuDF) so the function preserves caller memory type and layout;
update the logic around X, index = check_array(...) and the code paths using
SimpleImputer and KMeans to follow this pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8ea67bf-244b-4659-9d1c-58f773ffb3c4
📒 Files selected for processing (1)
python/cuml/cuml/explainer/sampling.py
These tests were skipped in CI before. Unskipping them has turned up a few lingering tweaks needed to get cudf.pandas working.
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/explainer/sampling.py`:
- Around line 54-56: Update the check_array call that assigns X and index (the
line "X, index = check_array(...)" in sampling.py) to validate numeric dtypes up
front by passing dtype=np.number (or dtype="numeric") so
non-numeric/object/complex inputs raise a clear validation error before reaching
SimpleImputer or KMeans; ensure numpy is imported if not already and keep the
existing ensure_2d/ensure_all_finite/return_index flags.
🪄 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: e6aa9a93-0568-4fe7-bbb8-a2dd1baea471
📒 Files selected for processing (3)
python/cuml/cuml/explainer/sampling.pypython/cuml/cuml/explainer/tree_shap.pyxpython/cuml/tests/explainer/test_gpu_treeshap.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuml/tests/explainer/test_gpu_treeshap.py
csadorf
approved these changes
May 4, 2026
csadorf
left a comment
csadorf
left a comment
Contributor
There was a problem hiding this comment.
One question, but otherwise LGTM.
Member
Author
|
/merge |
msarahan
approved these changes
May 5, 2026
This was referenced May 6, 2026
rapids-bot Bot
pushed a commit
that referenced
this pull request
May 6, 2026
This test was skipped in CI for years (due to missing dependencies). When I readded them (#8043), I must've forgot to stress test the hypothesis tests, leading to failures in nightlies. I can confirm that the failures happened the same way before recent changes to explainer, so this isn't a new bug. Three things needed to be fixed: - A small tweak to the hypothesis generation to normalize `preds` to `numpy` - Fixup the indexing for 3d shap values in a test (the old method was incorrect, again, this is not a new bug) - Drop xgboost from the test. The xgboost test runs error due to missing categorical support, (same as other xfails we added). If you drop categorical generation for xgboost, you then get incorrect results. I'm just dropping the test for now. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #8054
Merged
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
5 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:
shapandlightgbmto our test dependencies. Without these, a few of the explainer tests don't run.cuml.explainerto use the new validation and ingest routines. I did the bare minimum here to get things working, functionally things should still run mostly the same as before (just take a different pipeline).TreeExplainer.shap_interaction_valuesandTreeExplainer.shap_values. These routines always return either acupy.ndarrayornumpy.ndarray, depending on the memory type of the input. However, previously there was a bug where pandas inputs were incorrectly treated as cuda inputs (and cupy returned). This is now fixed and matches the docstrings.Fixes #7993.