Forward-merge release/26.04 into main - #7894
Conversation
`sklearn` requires that X is 2-dimensional, and errors nicely otherwise. `cuml` doesn't _uniformly_ require that X is 2-dimensional. Some estimators error on non-2D-X, but they usually do so accidentally, not as part of the input validation code. This leads to a bunch of xfailed tests in our sklearn compatibility test suite (both in the `cuml.accel` upstream tests, as well as in `test_sklearn_compatibility.py`). This PR: - Adds a deprecation warning to all estimators when `X` is non-2-dimensional. In 26.06 we'll remove the deprecation warning and error instead. - If `cuml.accel` is enabled, this warning is an error matching the sklearn error message instead. This lets us un-xfail a bunch of tests right now. - Updates our test suite to not trigger the warning, ensuring we're always passing in 2-dimensional X in tests. This was most common for `TargetEncoder`, only a few other locations needed it. Since it was so common for `TargetEncoder`, I added a deprecation test there as well to check that everything still worked on 1D inputs. - Updates `reflect` to support `reset="type"`, for setting the type on fit-like functions alone (and not `n_features_in_`/`feature_names_in_`). This was needed for 2 "transformers" sklearn (and cuml) supports that are meant to operate on `y` instead of `X` (`LabelEncoder` and `LabelBinarizer`). These estimators operate on `y` alone and shouldn't support `n_features_in_`/`feature_names_in_`. They also shouldn't validate that the array input is 2 dimensional, since `y` can be 1D. This is a stop-gap solution as we refactor our validation functions - in the long run we might remove `reset` entirely from `reflect` and instead move setting the input type to the validation functions (with the `reflect` decorator only remaining for coercing outputs). As per our deprecation policy, I've marked this PR as "breaking" since it adds a new deprecation warning around non-2-dimensional X. All prior working code should continue to work, users providing 1D X should just see a warning. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: NVIDIA#7889
| else: | ||
| warnings.warn( | ||
| "Support for passing non-2-dimensional X was deprecated in 26.04 " | ||
| "and will be removed in 26.06. In cuml 26.06 this will error " |
There was a problem hiding this comment.
Gah, looks like another version string snuck in. I think this will fix it:
| "and will be removed in 26.06. In cuml 26.06 this will error " | |
| "and will be removed in 26.06. In cuml version 26.06 this will error " |
reiterating that I find this linter a bit more annoying than it's worth.
There was a problem hiding this comment.
I think it's fine to fix this right here in a forward-merger PR, let's do it.
There was a problem hiding this comment.
Gah, that didn't appease the regex gods.
There was a problem hiding this comment.
I just pulled this branch down to work on this. Pushed 92757ff with a fix that worked for me (pre-commit run --all-files) locally.
Force-pushed over the inline one here so we only add 1 more commit to main for this.
jameslamb
added
improvement
📝 WalkthroughSummary by CodeRabbit
Walkthroughreflect(reset) now accepts bool or the string "type"; _get_n_features treats non-2D inputs specially (3D -> error; 1D -> FutureWarning + fallback), many tests/docs updated to prefer 2D inputs, and numerous upstream xfail entries removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 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 `@python/cuml/cuml/preprocessing/TargetEncoder.py`:
- Around line 177-178: Update the TargetEncoder class docstrings for the public
methods fit, fit_transform, and transform to reflect that X must be 2D (e.g., a
single-column DataFrame or 2D array) rather than a 1D Series; locate the
parameter docs for X in TargetEncoder.fit, TargetEncoder.fit_transform, and
TargetEncoder.transform and change the type/description to mention DataFrame/2D
array inputs (and mark Series/1D X as deprecated if the project uses a
deprecation policy), add an explicit scikit-learn compatibility note stating 2D
inputs are required, and ensure the example in the class-level doc and these
method docstrings matches the 1-column DataFrame usage shown in the example
block.
In `@python/cuml/tests/test_target_encoder.py`:
- Line 36: Update the typo in the test comment: change the comment string "#
Warns in tarnsform" to "# Warns in transform" in
python/cuml/tests/test_target_encoder.py (look for that exact comment near the
top of the test file) to satisfy the codespell guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa06e6b5-da1b-4a5a-8b90-5aa7764386a6
📒 Files selected for processing (12)
python/cuml/cuml/internals/outputs.pypython/cuml/cuml/internals/validation.pypython/cuml/cuml/preprocessing/TargetEncoder.pypython/cuml/cuml/preprocessing/label.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/test_coordinate_descent.pypython/cuml/tests/test_dbscan.pypython/cuml/tests/test_label_binarizer.pypython/cuml/tests/test_label_encoder.pypython/cuml/tests/test_target_encoder.pypython/cuml/tests/test_tsne.pypython/cuml/tests/test_validation.py
| ndim = len(shape) | ||
|
|
||
| if ndim != 2: | ||
| import cuml.accel | ||
|
|
||
| if isinstance(X, (cudf.Series, pd.Series)): | ||
| msg = ( | ||
| f"Expected a 2-dimensional container but got {type(X).__name__} " | ||
| "instead. Pass a DataFrame containing a single row (i.e. " | ||
| "single sample) or a single column (i.e. single feature) " | ||
| "instead." | ||
| ) | ||
| else: | ||
| kind = "scalar" if ndim == 0 else f"{ndim}D" | ||
| msg = ( | ||
| f"Expected 2D array, got {kind} array instead. Reshape your data " | ||
| "using array.reshape(-1, 1) if your data has a single feature, " | ||
| "or array.reshape(1, -1) if it contains a single sample." | ||
| ) | ||
|
|
||
| if cuml.accel.enabled() or ndim > 2: | ||
| raise ValueError(msg) | ||
| else: | ||
| warnings.warn( | ||
| "Support for passing non-2-dimensional X was deprecated in 26.04 " | ||
| "and will be removed in 26.06. In cuml 26.06 this will error " | ||
| f"with the following message:\n\n{msg}", | ||
| FutureWarning, | ||
| ) | ||
| # Fallback to 1 feature until the deprecation is completed | ||
| return 1 | ||
| return shape[1] |
There was a problem hiding this comment.
3D Python sequences still bypass this new ndim check.
This branch only runs after shape inference, but _get_n_features() still returns early for nested list/tuple inputs above. A value like [[[1], [2]]] currently reports 2 features instead of raising for non-2D input, so the stricter validation is still incomplete for one of the supported raw input forms.
As per coding guidelines, python/**/**/cuml/**/*.py: Missing input dimension checks (n_samples, n_features) and not handling edge cases (empty datasets, single sample) must be addressed in Python estimators.
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 107-107: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
| >>> train_encoded = encoder.fit_transform(train[["category"]], train.label) | ||
| >>> test_encoded = encoder.transform(test[["category"]]) |
There was a problem hiding this comment.
Finish the doc cleanup for 2D X inputs.
The example now correctly uses a 1-column DataFrame, but the fit, fit_transform, and transform parameter docs below still advertise Series/1D X inputs. That now contradicts the new deprecation path and will steer users into warnings.
As per coding guidelines, python/**/**/cuml/**/*.py: Missing docstrings for public methods, undocumented hyperparameters, or missing scikit-learn compatibility notes in documentation must be addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/cuml/preprocessing/TargetEncoder.py` around lines 177 - 178,
Update the TargetEncoder class docstrings for the public methods fit,
fit_transform, and transform to reflect that X must be 2D (e.g., a single-column
DataFrame or 2D array) rather than a 1D Series; locate the parameter docs for X
in TargetEncoder.fit, TargetEncoder.fit_transform, and TargetEncoder.transform
and change the type/description to mention DataFrame/2D array inputs (and mark
Series/1D X as deprecated if the project uses a deprecation policy), add an
explicit scikit-learn compatibility note stating 2D inputs are required, and
ensure the example in the class-level doc and these method docstrings matches
the 1-column DataFrame usage shown in the example block.
| with pytest.warns(FutureWarning, match="non-2-dimensional X"): | ||
| encoder.fit(df.category, df.label) | ||
|
|
||
| # Warns in tarnsform |
There was a problem hiding this comment.
Fix typo in test comment (tarnsform → transform).
✏️ Suggested patch
- # Warns in tarnsform
+ # Warns in transformAs per coding guidelines, "Check for spelling mistakes using codespell".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Warns in tarnsform | |
| # Warns in transform |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/tests/test_target_encoder.py` at line 36, Update the typo in the
test comment: change the comment string "# Warns in tarnsform" to "# Warns in
transform" in python/cuml/tests/test_target_encoder.py (look for that exact
comment near the top of the test file) to satisfy the codespell guideline.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
python/cuml/cuml/internals/validation.py (1)
84-105:⚠️ Potential issue | 🟠 MajorNested 3D Python sequences still bypass this new ndim guard
The new
ndim != 2path won’t run for nestedlist/tupleinputs because the fast-path returns at Line 71 first. Example:[[[1], [2]]]still reports2features instead of raising on>2D.💡 Proposed fix
@@ if isinstance(X, (list, tuple)): if len(X) == 0: return 0 row = X[0] @@ if not isinstance(row, (str, bytes, dict)): try: - return len(row) + row_len = len(row) except Exception: pass + else: + # If row elements are themselves sequence-like, defer to shape-based + # ndim handling below so >2D inputs can be rejected. + if row_len > 0: + first = row[0] + if not isinstance(first, (str, bytes, dict)): + try: + len(first) + except Exception: + return row_len + else: + return row_len + else: + return row_lenAs per coding guidelines,
python/**/**/cuml/**/*.py: Missing input dimension checks (n_samples, n_features) and not handling edge cases (empty datasets, single sample) must be addressed in Python estimators.
🤖 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/internals/validation.py`:
- Around line 107-112: The FutureWarning emitted in validation.py (the
warnings.warn(...) call that warns about non-2-dimensional X deprecation) should
include a stacklevel so the warning points at the user's callsite; update that
warnings.warn invocation to pass stacklevel=2 (or an appropriate value to reach
the external caller) while keeping the message and category unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11d89197-ffe4-4346-af15-98ce2727c801
📒 Files selected for processing (1)
python/cuml/cuml/internals/validation.py
| warnings.warn( | ||
| "Support for passing non-2-dimensional X was deprecated in 26.04 " | ||
| "and will be removed in 26.06. In cuml version 26.06 this will error " | ||
| f"with the following message:\n\n{msg}", | ||
| FutureWarning, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify warnings.warn callsites and whether stacklevel is set in this file.
rg -n "warnings\.warn\(" python/cuml/cuml/internals/validation.py -A5 -B1Repository: rapidsai/cuml
Length of output: 1277
Set warning stacklevel so deprecation points to caller
Line 107 emits warnings.warn(...) without stacklevel, so users get an internal location instead of their callsite (also matches Ruff B028).
💡 Proposed fix
else:
warnings.warn(
"Support for passing non-2-dimensional X was deprecated in 26.04 "
"and will be removed in 26.06. In cuml version 26.06 this will error "
f"with the following message:\n\n{msg}",
FutureWarning,
+ stacklevel=2,
)🧰 Tools
🪛 Ruff (0.15.5)
[warning] 107-107: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/cuml/cuml/internals/validation.py` around lines 107 - 112, The
FutureWarning emitted in validation.py (the warnings.warn(...) call that warns
about non-2-dimensional X deprecation) should include a stacklevel so the
warning points at the user's callsite; update that warnings.warn invocation to
pass stacklevel=2 (or an appropriate value to reach the external caller) while
keeping the message and category unchanged.
9a13181 to
92757ff
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
python/cuml/cuml/internals/validation.py (2)
107-112:⚠️ Potential issue | 🟡 MinorAdd
stacklevelto thisFutureWarningLine 107 emits
warnings.warn(...)withoutstacklevel, so warning attribution points to internals instead of caller code.#!/bin/bash # Verify warnings.warn callsites and whether stacklevel is present rg -n "warnings\.warn\(" python/cuml/cuml/internals/validation.py -A6 -B1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/internals/validation.py` around lines 107 - 112, The FutureWarning emitted via warnings.warn in validation.py is missing a stacklevel, causing the warning to point to internal code; update the warnings.warn call (the one that emits the deprecation about non-2-dimensional X) to include an appropriate stacklevel (e.g., stacklevel=2) so the warning attributes to the caller site instead of internals; keep the existing message and warning class (FutureWarning) and only add the stacklevel argument to that warnings.warn invocation.
60-73:⚠️ Potential issue | 🟠 MajorNested Python 3D inputs still bypass the new non-2D guard
Because of the early return at Line 71, inputs like
[[[1], [2]]]can return a feature count instead of reaching thendim > 2error path. This leaves non-2D validation incomplete for list/tuple inputs.Suggested fix
def _get_n_features(X): if isinstance(X, (list, tuple)): if len(X) == 0: return 0 row = X[0] @@ if not isinstance(row, (str, bytes, dict)): try: - return len(row) + row_len = len(row) except Exception: pass + else: + # Don't bypass ndim validation for nested sequences (>2D) + if row_len > 0 and not isinstance(row[0], (str, bytes, dict)): + try: + len(row[0]) + except Exception: + return row_len + else: + return row_lenAs per coding guidelines,
python/**/**/cuml/**/*.py: “Missing input dimension checks (n_samples, n_features) and not handling edge cases (empty datasets, single sample) must be addressed in Python estimators”.Also applies to: 84-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/internals/validation.py` around lines 60 - 73, The early return inside the list/tuple branch returns len(row) for inputs like X = [[[1],[2]]] and thus bypasses the ndim>2 guard; update the branch handling X in validation.py (the block that inspects X, row and returns len(row)) to detect nested sequence elements (e.g., row elements that are themselves list/tuple/ndarray-like) and, if those nested sequences exist, raise or route to the existing non-2D error path instead of returning a feature count; ensure you still handle empty X and non-sequence row types (str/bytes/dict) the same way so other callers of this logic (the X/row check) remain correct.
🤖 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/internals/validation.py`:
- Around line 107-112: The FutureWarning emitted via warnings.warn in
validation.py is missing a stacklevel, causing the warning to point to internal
code; update the warnings.warn call (the one that emits the deprecation about
non-2-dimensional X) to include an appropriate stacklevel (e.g., stacklevel=2)
so the warning attributes to the caller site instead of internals; keep the
existing message and warning class (FutureWarning) and only add the stacklevel
argument to that warnings.warn invocation.
- Around line 60-73: The early return inside the list/tuple branch returns
len(row) for inputs like X = [[[1],[2]]] and thus bypasses the ndim>2 guard;
update the branch handling X in validation.py (the block that inspects X, row
and returns len(row)) to detect nested sequence elements (e.g., row elements
that are themselves list/tuple/ndarray-like) and, if those nested sequences
exist, raise or route to the existing non-2D error path instead of returning a
feature count; ensure you still handle empty X and non-sequence row types
(str/bytes/dict) the same way so other callers of this logic (the X/row check)
remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd770466-4c05-42f9-b391-d3ed3deb9408
📒 Files selected for processing (1)
python/cuml/cuml/internals/validation.py
|
Admin-merging this to save CI resources and so the automatic forward-merger PR doesn't pick up any more commits. |
5 participants
No description provided.