Support LabelEncoder in cuml.accel - #8082
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds accel support for sklearn.preprocessing.LabelEncoder: proxy generation updates, cuml.preprocessing.LabelEncoder CPU/GPU interop hooks, proxy export and discovery deduplication, tests for transform/round-trip and signature, and FAQ/limitations docs updates. ChangesLabelEncoder GPU Acceleration via Proxy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/source/cuml-accel/limitations.rst`:
- Line 464: Fix the spelling typo in the documentation sentence mentioning
LabelEncoder: change "neverf" to "never" so the line reads "LabelEncoder
supports all cases and will never fall back to CPU."; update the text around the
symbol "LabelEncoder" accordingly to preserve formatting (e.g., keep the inline
code markers).
In `@python/cuml/cuml/preprocessing/_label.py`:
- Around line 88-93: The error message in the _params_to_cpu method uses a
string with {self.handle_unknown} but lacks the f-string prefix, so the literal
text is shown; update the UnsupportedOnCPU raise in _params_to_cpu to use an
f-string that interpolates self.handle_unknown (i.e., make the message
f"`handle_unknown={self.handle_unknown}` is not supported") so the actual
attribute value appears in the logged exception.
🪄 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: 074d7091-5850-411e-b8a7-d160872cedd6
📒 Files selected for processing (7)
docs/source/cuml-accel/faq.rstdocs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/preprocessing/_label.pypython/cuml/cuml_accel_tests/integration/test_preprocessing.pypython/cuml/tests/test_sklearn_import_export.py
0194ee2 to
43005b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/cuml_accel_tests/integration/test_preprocessing.py`:
- Around line 113-120: Extend test_label_encoder to add edge cases and
multi-type input parity checks: add assertions for empty input and single-sample
input, run the same sequences (fit, transform, inverse_transform) on numpy
arrays, pandas.Series, and cuDF Series (when cuDF is available) and compare
outputs to scikit-learn's LabelEncoder to ensure parity; specifically use the
existing LabelEncoder instance (enc) and methods enc.fit_transform and
enc.inverse_transform and compare enc.classes_, transformed arrays, and
inverse-transformed results against sklearn.preprocessing.LabelEncoder for each
input type and edge case, plus ensure fit/transform consistency is validated for
each case.
🪄 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: be3b3c93-8313-4b26-b260-2245e0abf9af
📒 Files selected for processing (7)
docs/source/cuml-accel/faq.rstdocs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/preprocessing/_label.pypython/cuml/cuml_accel_tests/integration/test_preprocessing.pypython/cuml/tests/test_sklearn_import_export.py
✅ Files skipped from review due to trivial changes (3)
- docs/source/cuml-accel/faq.rst
- docs/source/cuml-accel/limitations.rst
- python/cuml/cuml/accel/_overrides/sklearn/preprocessing.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cuml/tests/test_sklearn_import_export.py
- python/cuml/cuml/accel/estimator_proxy.py
- python/cuml/cuml/preprocessing/_label.py
betatim
left a comment
There was a problem hiding this comment.
LGTM.
For my education: why not use the array API version from scikit-learn?
This adds support for `LabelEncoder` in `cuml.accel`.
Some tests import `all_estimators` from the full path instead of `sklearn.utils`. We need to patch both locations.
43005b2 to
32ad6db
Compare
We can't support non-numeric labels that way w/ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/cuml/accel/_patches/sklearn/utils/discovery.py`:
- Around line 42-44: Replace the hard assertion in the duplicate-name resolution
so all_estimators() doesn't crash: instead of assert len(proxied_cls) == 1,
implement a deterministic fallback that (1) if exactly one proxy exists use
proxied_cls[0], (2) if multiple proxies exist pick a consistent choice (e.g.,
the first element of proxied_cls), and (3) if no proxies exist pick a
deterministic class from cls_list (e.g., the first element); then append that
chosen class in the estimators.append((name, ...)) call and optionally emit a
non-fatal warning to aid debugging.
🪄 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: fe28587d-e480-4f64-ad5f-ad1770c41397
📒 Files selected for processing (11)
docs/source/cuml-accel/faq.rstdocs/source/cuml-accel/limitations.rstpython/cuml/cuml/accel/_overrides/sklearn/preprocessing.pypython/cuml/cuml/accel/_patches/sklearn/utils/__init__.pypython/cuml/cuml/accel/_patches/sklearn/utils/discovery.pypython/cuml/cuml/accel/core.pypython/cuml/cuml/accel/estimator_proxy.pypython/cuml/cuml/preprocessing/_label.pypython/cuml/cuml_accel_tests/integration/test_preprocessing.pypython/cuml/cuml_accel_tests/test_estimator_proxy.pypython/cuml/tests/test_sklearn_import_export.py
✅ 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 (5)
- python/cuml/cuml/accel/_overrides/sklearn/preprocessing.py
- docs/source/cuml-accel/faq.rst
- python/cuml/cuml_accel_tests/integration/test_preprocessing.py
- python/cuml/cuml/preprocessing/_label.py
- python/cuml/tests/test_sklearn_import_export.py
|
/merge |
This adds support for
LabelEncoderincuml.accel.Fixes #8015.