New input validation utilities - #7973
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test b1f6a4a |
b1f6a4a to
4c78294
Compare
|
/ok to test 4c78294 |
|
I've pushed a PR up applying these utilities to |
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoved legacy label-preprocessing helpers from cuml.common.classification, added a comprehensive validation module cuml.internals.validation (including check_y and related helpers), updated estimators to use new validation APIs and consistent length checks, and expanded tests and xfail mappings to reflect the new validation behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
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/internals/validation.py`:
- Around line 843-875: The code indexes y.flat[0] for object-dtype labels which
crashes on empty arrays and allows nullable/missing object/string labels to
pass; update the branches that handle object dtype (the block that checks
y.dtype == "object" and the similar block around lines 914-928) to first check
if y.size == 0 and raise a stable validation error for empty targets, and then
check for any missing/null entries (use pandas/cudf isnull/isna as appropriate
for numpy/cudf inputs) and raise a ValueError rejecting nullable object/string
labels; also replace direct y.flat[0] access with a safe access only after
size>0 (e.g., inspect first element via ravel()[0] after the size check) so both
the empty-array and nullable-label cases are handled consistently.
- Around line 172-177: The helper that computes sample count currently treats
any sized object as array-like and thus accepts strings/bytes and mappings;
update the logic in _get_n_samples (the try/except block shown) to first
explicitly reject instances of (str, bytes) and collections.abc.Mapping by
raising a TypeError with a message like "Expected array-like, got {type(X)}"
before calling len(), so check isinstance(X, (str, bytes)) or isinstance(X,
Mapping) and raise accordingly, then fall back to the existing len() try/except
for other objects.
In `@python/cuml/cuml/neighbors/kneighbors_classifier.pyx`:
- Around line 178-187: The length check for X and y must run before expensive
index construction and before calling super().fit; move
check_consistent_length(X, y) (and any other input validation like check_y or
_validate_data) to precede super().fit(X, ...) in the fit method so malformed
inputs raise early and no partial fitted state is built; ensure any learned
attributes (e.g., self.classes_) are only set after validation and successful
super().fit completes.
🪄 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 Plus
Run ID: 0e56a0c6-73fa-479e-be1c-c4db906fd7d0
📒 Files selected for processing (12)
python/cuml/cuml/common/classification.pypython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/internals/validation.pypython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/neighbors/kneighbors_classifier.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/conftest.pypython/cuml/tests/test_sklearn_compatibility.pypython/cuml/tests/test_validation.py
💤 Files with no reviewable changes (2)
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
dantegd
left a comment
There was a problem hiding this comment.
This is great! Way easier to reason than input_to_cuml_array and also love the better standardization on pydata data types.
Just had some comments about small issues and small questions in general.
| if mem_type is None: | ||
| mem_type = "host" if isinstance(y, np.ndarray) else "device" | ||
| if np.isdtype(y.dtype, ("numeric", "bool")): | ||
| y = cp.asarray(y) |
There was a problem hiding this comment.
Thinking about this:
For a numpy y with mem_type="host", the flow is:
- check_array(mem_type=None) keeps it as numpy
- cp.asarray(y) copies N elements to device
- _encode runs cp.unique(..., return_inverse=True)
- classes.get() copies classes back
- At the end, y.get(order=order) copies N codes back to host
That's two full N-element transfers for an encode that np.unique(y, return_inverse=True) can do entirely on host. On machines with fast CPU-GPU connection or unified memory this definitely is worth it, but I wonder if this might be slower for the case when PCI express or other things slow down CPU-GPU communications.
I wonder if there would be merit to looking into these kind of optimizations, or the fact that we always move in the direction of faster interconnects and unifed memory systems means this is mostly fine.
Mainly thinking out loud, but I know you've thought about these type of problems so wanted to get your thoughts on this.
There was a problem hiding this comment.
I agree that this could be done on host just fine. All the code for handling classes was merged from the previously existing cuml.common.classification.process_labels function I wrote months ago. I opted to not port it to run on host for simplicity. In every call path we have currently we want mem_type="device" as an output here, I only plumbed through support for mem_type="host" for completeness. I'm personally happy to leave this as a follow-up TODO, but don't think optimizing this for host-side computation should be a blocker here.
In contrast, we do have several places where check_array(..., mem_type="host") will be used and we want the ensure_all_finite checks to run. Those checks did need a host-side version since a host -> device -> host transfer would be inefficient.
There was a problem hiding this comment.
I think it would be a good idea to audit our input-validation path for unnecessary transfers and the optimize that in a follow-up. Maybe after we have done 3-4 conversions of other modules?
There was a problem hiding this comment.
I'm not clear on what transfers you're speaking about, so just in case: to be clearer about the above conversation:
Yes, check_y(..., mem_type="host") will do unnecessary transfers currently. However, no estimator we currently have will ever take that path (all current classifiers would use mem_type="device"). I agree we could optimize that code path to avoid transfers, but don't think it's worth it without a use case.
Regarding other code paths, I was pretty careful when writing check_array to only do one conversion ever if needed. I may have missed something, but I did spend a bunch of time trying to get that correct. If anyone finds an unnecessary transfer in check_array then I agree we should fix that.
There was a problem hiding this comment.
I'm not clear on what transfers you're speaking about
The one where check_y(..., mem_type="host").
If that's the only one, then we could just add a comment and fix it in a follow-up once it's actually taken.
This leads to more straightforward tracebacks, and also lets `--pdb` work with pytest. Without this some errors are not debuggable. This will _not_ degrade the quality of the testing, cases that would error before will still error and the tests will still have the same coverage. It just changes how errors are raised within the test itself.
|
I believe all comments have been addressed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/cuml/cuml/internals/validation.py (1)
97-97: Consider adding parentheses for clarity.The expression
ndim == 0 or ndim == 1 and ensure_2drelies on operator precedence (andbinds tighter thanor). While correct, explicit parentheses would improve readability:if ndim == 0 or (ndim == 1 and ensure_2d):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/internals/validation.py` at line 97, The conditional `ndim == 0 or ndim == 1 and ensure_2d` is relying on operator precedence and should be made explicit for readability; update the `if` condition to use parentheses so it reads `if ndim == 0 or (ndim == 1 and ensure_2d):`, keeping the same semantics but improving clarity around `ndim` and `ensure_2d`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/cuml/internals/validation.py`:
- Line 97: The conditional `ndim == 0 or ndim == 1 and ensure_2d` is relying on
operator precedence and should be made explicit for readability; update the `if`
condition to use parentheses so it reads `if ndim == 0 or (ndim == 1 and
ensure_2d):`, keeping the same semantics but improving clarity around `ndim` and
`ensure_2d`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f42c2df-7871-404a-8f48-be09d623421e
📒 Files selected for processing (12)
python/cuml/cuml/common/classification.pypython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/internals/validation.pypython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/neighbors/kneighbors_classifier.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/conftest.pypython/cuml/tests/test_sklearn_compatibility.pypython/cuml/tests/test_validation.py
💤 Files with no reviewable changes (2)
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
✅ Files skipped from review due to trivial changes (1)
- python/cuml/cuml/svm/svc.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cuml/tests/conftest.py
- python/cuml/cuml/svm/linear_svc.py
- python/cuml/cuml/neighbors/kneighbors_classifier.pyx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/cuml/cuml/internals/validation.py (2)
903-910: Addstacklevel=2to point warning at caller.The warning currently points to this validation code rather than the user's code that passed the column-vector. Adding
stacklevel=2helps users identify where in their code the issue originates.🔧 Suggested fix
warnings.warn( "A column-vector y was passed when a 1d array was expected. " "Please change the shape of y to (n_samples,), for example " "using ravel().", DataConversionWarning, + stacklevel=2, )🤖 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 903 - 910, The DataConversionWarning raised when y is a column-vector should include stacklevel=2 so the warning points at the caller; update the warnings.warn call that checks y.ndim == 2 and y.shape[1] == 1 (the one that mentions "A column-vector y was passed...") to pass stacklevel=2 as an argument to warnings.warn so the stack trace references the user's code rather than validation.py.
97-97: Add parentheses to clarify operator precedence.The expression
ndim == 0 or ndim == 1 and ensure_2drelies on implicit precedence (andbinds tighter thanor). While the behavior is correct, explicit parentheses improve readability.🔧 Suggested fix
- if ndim == 0 or ndim == 1 and ensure_2d: + if ndim == 0 or (ndim == 1 and ensure_2d):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/cuml/cuml/internals/validation.py` at line 97, The conditional expression using ndim and ensure_2d relies on implicit operator precedence; update the boolean test to use explicit parentheses for clarity by changing the line `if ndim == 0 or ndim == 1 and ensure_2d:` to `if (ndim == 0) or (ndim == 1 and ensure_2d):` (locate this check in the validation routine where ndim and ensure_2d are used) so the intent is unambiguous while preserving the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@python/cuml/cuml/internals/validation.py`:
- Around line 903-910: The DataConversionWarning raised when y is a
column-vector should include stacklevel=2 so the warning points at the caller;
update the warnings.warn call that checks y.ndim == 2 and y.shape[1] == 1 (the
one that mentions "A column-vector y was passed...") to pass stacklevel=2 as an
argument to warnings.warn so the stack trace references the user's code rather
than validation.py.
- Line 97: The conditional expression using ndim and ensure_2d relies on
implicit operator precedence; update the boolean test to use explicit
parentheses for clarity by changing the line `if ndim == 0 or ndim == 1 and
ensure_2d:` to `if (ndim == 0) or (ndim == 1 and ensure_2d):` (locate this check
in the validation routine where ndim and ensure_2d are used) so the intent is
unambiguous while preserving the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 83d9a3fd-63ec-42ca-abee-88dbcaae1eb2
📒 Files selected for processing (2)
python/cuml/cuml/internals/validation.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
|
/merge |
csadorf
left a comment
There was a problem hiding this comment.
LGTM! Just a few minor post-merge questions.
| if mem_type is None: | ||
| mem_type = "host" if isinstance(y, np.ndarray) else "device" | ||
| if np.isdtype(y.dtype, ("numeric", "bool")): | ||
| y = cp.asarray(y) |
There was a problem hiding this comment.
I think it would be a good idea to audit our input-validation path for unnecessary transfers and the optimize that in a follow-up. Maybe after we have done 3-4 conversions of other modules?
This applies the new input validation utilities added in #7973 to `cuml.linear_models` and `cuml.solvers`. Doing this fixed ~70 failing sklearn compatibility tests for cuml proper, and at least 60 upstream tests for `cuml.accel`. Fixes #7986 Fixes #7987 Part of #7428 Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #7978
This applies the new input validation utilities added in #7973 to `cuml.decomposition`. Doing this fixed ~9 failing sklearn compatibility tests for cuml proper, and at least 36 upstream tests for `cuml.accel`. Fixes #7990 Part of #7428 Authors: - Simon Adorf (https://github.com/csadorf) Approvers: - Jim Crist-Harif (https://github.com/jcrist) URL: #8006
Summary
The goals for this PR were:
input_to_cuml_array(and friends) functions are both unwieldly and complex. They recurse on themselves in some cases, they have many many branches, the code paths are split between two files, they return non-standard types likeCumlArray/SparseCumlArray... We desire something simpler to understand and maintain.X,y,sample_weight, ...). All this led to differences in validation workflows for each estimator, sometimes resulting in brittle code. A single set of well tested and flexible utilities that handle common patterns will result in more robust and easier to maintain code.cumltries to mimic sklearn's APIs, and as such often has to look at sklearn's implementation. Having validation utilities that more closely resemble sklearn counterparts eases porting code and ensuring sklearn compatibility for things likecuml.accel.To accomplish this, this PR adds a new set of input data validation utilities in the existing
cuml.internals.validationnamespace:check_array: Generic array ingest utility for validating and coercing user inputs into a supported type. Supports both dense and sparse data, and has many configuration knobs. Similar tosklearn.utils.validation.check_array, but customized for cuml. Should be used to validate any array-like inputs (either directly or indirectly).check_y: Validation ofyinputs to estimators. Also handles label encoding for classifiers (merged with pre-existing functionality fromcuml.common.classification). Should be called on everyyinput (either directly or indirectly).check_sample_weight: Validation ofsample_weightinputs to estimators. Should be called on everysample_weightinput (either directly or indirectly).check_consistent_length: Validates that input arrays all have a consistent length (number of samples). Corresponds tosklearn.utils.validation.check_consistent_length.check_inputs: Wires together all of the above (along withcheck_features) to perform common validation processing for most estimators. Should be preferred for all estimator methods when possible. If an estimator requires a special case, you can always manually call the composing checks as needed. Similar tosklearn.utils.validation.validate_data, though tailored to cuml's use cases and my own API design opinions.check_all_finiteandcheck_non_negative: data validation utilities. While documented, these are typically called throughcheck_arrayand friends rather than called directly. Only mentioning them here for completeness.Details
All of these APIs standardize on the following types:
numpy.ndarray: host dense arraycupy.ndarray: device dense arrayscipy.sparse.spmatrix: host sparse arraycupyx.scipy.sparse.spmatrix: device sparse arrayNote that the legacy internal array types (
CumlArray,SparseCumlArray) are absent from the list, and don't take any part in the ingest validation pipeline. Dropping these types lets us simplify our internals - we no longer have to mentally track whetherxis acupy.ndarrayor aCumlArray(or something else), of whether it's host or device backed. The output of the validation functions above are always the standard pydata types listed above, leading to a simpler mental model and fewer things to track. Acupy.ndarraycan do anything aCumlArraycan do, with the added benefit that we don't have to maintain that code.In the short term,
CumlArrayandSparseCumlArraywill still be used in the codebase:CumlArrayDescriptorlogic to better work with cupy types natively)For handling host/device code paths,
check_array/check_y/check_sample_weight/check_inputstake amem_typekwarg. This defaults to'"device"for device arrays, but can also accept"host"for host arrays orNoneto match the memory type of the input. This should help avoid the explosing of ingest utilities currently found incuml.internals.input_utils-check_arraycan do it all.Differences in the new ingest pipelines
check_arrayis mostly a new pipeline accomplishing the same things the oldinput_to_cuml_arraypipeline did (but in a different way, with more checks). That said, there are a few notable differences:Full support for array-like inputs is always on
Previously support for
list/tupleinputs was hacked in and only enabled ifcuml.accelwas enabled. This complicated our code (we needed more code to limit a feature to only when a switch was turned). It also wasn't applied uniformly (some code paths always accepted lists as input), and also didn't support other array-likes that sklearn accepts and makes use of in their test suite. Accepting these inputs doesn't slow down the fast-paths, is easy to support, and also easy to add performance logging/warnings to later on if we want to enable a way for users to track performance issues in their code. I view this as a harmless addition that lets us pass many more sklearn validation checks.Addition of some data validation checks by default
Notably
check_all_finiteis called by default on all input arrays. This does a full array scan looking for non-finite values. We've seen many cases where non-finite values have led to memory issues within our cuda kernels - guarding against these seems worth it to me (and helps us better match sklearn conventions).This check may be disabled the same ways it can be in sklearn:
SKLEARN_ASSUME_FINITEenvironment variablesklearn.set_config(assume_finite=True)sklearn.config_context(assume_finite=True)With the view that cuml estimators should be equal members of the sklearn ecosystem, I believe that relying on the sklearn configuration (been around for years, unlikely to change, familiar to users) is the right path over a cuml-specific configuration knob.
I did a brief benchmark of this utility. On my machine (RTX A6000), the new
check_all_finitecheck runs with a bandwidth of ~600 GiB/s. For most user inputs the added validation time will be negligible. Further, our oldinput_to_*pipelines would also sometimes do a full data scan when casting inputs (and some estimators have also manually written a slower version ofcheck_all_finiteto guard against non-finite inputs). I view standardizing on an optimized version to have limited downsides and many benefits.Plans
This PR (mostly) doesn't add these utilities to any code paths. The one exception is classifiers. I merged the old
cuml.common.classification.preprocess_labelsinto the newcuml.internals.validation.check_y(the code is almost the same). The old call sites have been updated in this PR to use the newcheck_yfunction, and thus a few more xfailed tests are passing.Once this PR is in, follow-up PRs will apply the utilities to modules individually. When doing so, we might adjust the APIs here. As such, I'd prefer to not block too much on minute details of these APIs and instead focus on the overall big picture plan. I've already worked through
cuml.linear_modelsandcuml.solversas a test case, so I'm pretty confident that these APIs can be applied throughout the codebase, but there may be unforseen issues.We don't need to apply these all within a single release. This is an easy change to incrementally role out. Updated estimators get more validation and drop the use of the legacy types in their internals. They'll pass more of the sklearn validation test suite.
Updating a module is roughly:
check_inputs(with appropriate kwargs) for all priorinput_to_*functions.cupy.ndarrayinstead ofCumlArray@reflect(reset=True)with@reflect(reset="type"). Once all versions usingreset=Trueare gone, we can rip out the call tocheck_featureswithinreflectand swap back toreset=Truemeaning whatreset="type"means now.CumlArraywrappers around fitted attributes or values returned from reflected functions. These will eventually be dropped/refactored, but keeping the legacy types at the borders simplifies things.A simple estimator should roughly look like:
Fixes #7977, First part of #7428.