Unify reflection system & decorators - #8339
Merged
Merged
Conversation
jcrist
added
improvement
|
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. |
Member
Author
|
/ok to test bdd2c4b |
Small fixup from the PR adding this, noticed while refactoring this code.
jcrist
force-pushed
the
unify-reflection-system
branch
from
July 9, 2026 05:15
bdd2c4b to
c784349
Compare
jcrist
changed the title
jcrist
force-pushed
the
unify-reflection-system
branch
from
July 9, 2026 05:27
c784349 to
a3fc630
Compare
jcrist
marked this pull request as ready for review
July 9, 2026 05:31
jcrist
commented
Jul 9, 2026
jcrist
left a comment
jcrist
left a comment
Member
Author
There was a problem hiding this comment.
Annotating the diff for review. Most of the un-annotated files are purely mechanical changes. They're probably worth a skim, but I've flagged the noteworthy parts.
| from cuml.internals.output_utils import cudf_to_pandas | ||
|
|
||
|
|
||
| def decode_labels(y_encoded, classes, output_type="cupy", index=None): |
Member
Author
There was a problem hiding this comment.
This function is no longer needed, this implementation has been moved to cuml.internals.outputs.ClassLabels.to_output (with some modifications), and now has some direct tests too.
| "set_global_output_type", | ||
| "using_output_type", | ||
| "mlfunc", | ||
| "ReflectedAttr", |
Member
Author
There was a problem hiding this comment.
This file contains the implementation of mlfunc/ReflectedAttr. It's worth reviewing.
Member
Author
There was a problem hiding this comment.
I'm not 100% happy with the names here, but I'm also fine with them.
ReflectedAttrsays what it does - it's an attribute that participates in the output type reflection systemmlfuncis my attempt at a short memorable name for a decorator. Functions/methods decorated with this also participate in the type reflection system, but also get a few additional behaviors (and more intended).
I also considered cumlfunc, CumlAttr, MLAttr, ... Some parity between the descriptor and decorator names would be nice, but I don't think is necessary.
If anyone has a strong rename opinion, please let me know, but I'm ok with the current names.
| return inner | ||
|
|
||
|
|
||
| def run_in_internal_context(func): |
Member
Author
There was a problem hiding this comment.
run_in_internal_context and reflect are now implemented by mlfunc (with a _legacy flag). Once these are no longer used they can be deleted, and the few _legacy/CumlArray/SparseCumlArray paths above removed.
| cp.cuda.set_allocator(rmm_cupy_allocator) | ||
|
|
||
| # XXX: workaround for https://github.com/cupy/cupy/issues/10084 | ||
| copyreg.dispatch_table[cp.ndarray] = lambda x: ( |
Member
Author
There was a problem hiding this comment.
This was a cupy bug found while working on this. I have a PR up to cupy to resolve the issue (cupy/cupy#10086).
The old pickle paths masked the issue, since CumlArray-wrapped instances would implicitly fix the ordering themselves. Now that we're relying on cupy directly, this issue appeared.
FWIW, a fix like this is exactly what copyreg is for, and I don't view this as super hacky. We can gate it by cupy version if/once cupy has this fixed upstream.
There's a corresponding test added in test_pickle.py.
Contributor
There was a problem hiding this comment.
Should we make this a more explicit TODO? "Remove once cupy version (/with fix from xxx) has been released". Asssuming that the fix makes it into the next patch release.
Member
Author
There was a problem hiding this comment.
I'll add a comment in my next followup. I've created a followup to track the removal of the patch here: #8364
| from cuml.internals.base import Base | ||
| from cuml.internals.global_settings import GlobalSettings | ||
| from cuml.internals.outputs import infer_output_type | ||
| from cuml.internals.outputs import ( |
Member
Author
There was a problem hiding this comment.
The tests in this file have been updated and expanded.
We still have coverage for the old system, but I've updated most of the tests here to use the new system. In places where they diverge in behavior we have two copies (or a parametrized version) to ensure both remain valid while in transition.
This file is also worth reviewing.
| ) | ||
| @cuml.internals.reflect | ||
| def predict(self, X, *, convert_dtype="deprecated") -> CumlArray: | ||
| @mlfunc(preserve_index=True) |
Member
Author
There was a problem hiding this comment.
Most mechanical changes look like this:
- Remove all usage of
CumlArray - Update code to work with
cupydirectly as needed (often just removal ofto_outputcalls or update of.ptrto.data.ptr.) - Move
indexreflection handling to the decorator withpreserve_index=True - Return a
cupyarray directly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
python/cuml/tests/test_pickle.py (1)
171-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert data values survive the round-trip.
Test only checks contiguity flags; add value-equality assertions to confirm the workaround preserves array content, not just layout.
✅ Suggested addition
X_c2 = pickle.loads(pickle.dumps(X_c)) X_f2 = pickle.loads(pickle.dumps(X_f)) assert X_c2.flags.c_contiguous assert X_f2.flags.f_contiguous + cp.testing.assert_array_equal(X_c2, X_c) + cp.testing.assert_array_equal(X_f2, X_f)As per coding guidelines, "Missing validation of numerical correctness (only checking 'runs without error')" is a test-quality concern to avoid.
🤖 Prompt for 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. In `@python/cuml/tests/test_pickle.py` around lines 171 - 182, The test in test_cupy_pickle_roundtrip_order only verifies contiguity flags after pickle round-trip and does not confirm the array contents are preserved. Update the assertions in test_cupy_pickle_roundtrip_order to also compare X_c2 and X_f2 against the original X_c and X_f values, using the existing pickle.loads/pickle.dumps round-trip variables so the test validates both layout and numerical correctness.Source: Coding guidelines
🤖 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/internals/outputs.py`:
- Around line 440-456: The `convert_array`/output handling in
`cuml/internals/outputs.py` treats `output_type="dataframe"` as valid only when
`out` is already a `cudf.DataFrame`, so `cudf.Series` results from non-numeric
single-target labels fall through to `TypeError`. Update the `output_type ==
"dataframe"` branch to also accept `cudf.Series` and convert it into a
one-column DataFrame (matching the behavior used by `convert_arrays(...,
"dataframe")`), while preserving the existing passthrough cases for
`cudf.DataFrame`, `pandas`, and `numpy` outputs.
- Around line 351-353: The ClassLabels.dtype property currently assumes classes
is a single array and breaks for multi-target list inputs, so update it to
handle the same shapes supported by to_output() without raising AttributeError.
In outputs.py, adjust ClassLabels.dtype to detect a list/tuple of class arrays
and return a representative dtype (or the first target’s dtype) that keeps
callers like score() working with preds.dtype.kind before conversion. Keep the
behavior for single-target classes unchanged and use the ClassLabels and
to_output symbols as the reference points for the fix.
- Around line 860-873: The run_in_internal_context wrapper currently delegates
to mlfunc without disabling argument inference, which can break decoration of
variadic helpers. Update run_in_internal_context to call mlfunc with
convert_output=False and explicitly set model_arg=None and array_arg=None so it
only handles internal-context output conversion; use the run_in_internal_context
symbol to locate the change.
In `@python/cuml/cuml/linear_model/mbsgd_classifier.py`:
- Around line 213-222: The thresholding logic in mbsgd_classifier.py’s predict
method is too broad: it uses 0.5 for every non-hinge loss even though
decision_function() returns raw margins/logits, which breaks loss="log"
predictions. Update predict in MBSGDClassifier so the threshold is 0 for hinge,
0.5 only for squared_loss if intended, and use the correct margin-based
thresholding for log loss. Add a regression test covering
MBSGDClassifier.predict with loss="log" to verify scores in the (0, 0.5] range
map to the positive class.
In `@python/cuml/cuml/multiclass/multiclass.py`:
- Around line 75-89: Add a fitted-state guard in the multiclass inference
methods so they fail with a sklearn-compatible not-fitted error instead of
reaching self.multiclass_estimator directly. Update both predict and
decision_function in the Multiclass classifier to call check_is_fitted on the
estimator instance before check_inputs and the exit_internal_context block,
using the existing method names to place the check consistently for all
inference paths.
- Around line 75-89: The multiclass prediction path in
`cuml/multiclass/multiclass.py` is returning decoded class labels instead of
encoded class indices, which breaks `SVC.predict` when it later wraps
`_multiclass.predict(X)` with `ClassLabels(indices, self.classes_)`. Update
`MulticlassClassifier.predict` to return the estimator’s raw encoded prediction
indices (and keep the input validation/context handling intact) so downstream
code can correctly map indices through `self.classes_` without treating labels
as positional indices.
In `@python/cuml/cuml/neighbors/kneighbors_regressor.pyx`:
- Around line 284-290: The call to knn_regress is passing the shape arguments in
the wrong order, so update the invocation in kneighbors_regressor.pyx to match
the extern signature by supplying n_rows before n_samples_fit. Use the
knn_regress call site and its argument list (including inds_ptr, y_vec,
n_neighbors, and weights_ptr) to verify the query-row count and fitted-sample
count are not swapped, and keep the rest of the parameters unchanged.
- Around line 273-275: The raw pointer taken from compute_weights in knn_regress
assumes a flat contiguous float32 buffer, but the returned weights may be
strided or non-contiguous. Update the weights handling in
kneighbors_regressor.pyx to force the result into a contiguous float32 array
before assigning weights_ptr, using the compute_weights path and the
weights.data.ptr access as the main location to fix.
In `@python/cuml/cuml/neighbors/nearest_neighbors.pyx`:
- Around line 771-772: The sparse KNN path is passing cp.int32 indices into
self-edge removal, which can later be treated as long long int* by swap_kernel
and causes a device-memory type mismatch. In _kneighbors_sparse /
kneighbors(X=None), cast the indices to the expected 64-bit type before calling
_drop_self_edges, and keep the cast in the sparse-fitted self-query flow so
_drop_self_edges and the swap_kernel path always see a 64-bit buffer.
In `@python/cuml/cuml/solvers/qn.pyx`:
- Around line 492-493: The warm-start path in qn.pyx is still assuming `coef_`
and `intercept_` are `CumlArray` objects, but after switching them to
`ReflectedAttr` they may already be raw CuPy values inside `fit()`/`@mlfunc`, so
the existing `.to_output("cupy")` handling breaks on subsequent warm-start
training. Update the warm-start restore logic in `fit` (the code paths around
the current `coef_`/`intercept_` handling) to accept reflected/raw CuPy values
without calling `to_output`, and normalize them safely before reusing state so a
second `fit(..., warm_start=True)` works.
---
Nitpick comments:
In `@python/cuml/tests/test_pickle.py`:
- Around line 171-182: The test in test_cupy_pickle_roundtrip_order only
verifies contiguity flags after pickle round-trip and does not confirm the array
contents are preserved. Update the assertions in
test_cupy_pickle_roundtrip_order to also compare X_c2 and X_f2 against the
original X_c and X_f values, using the existing pickle.loads/pickle.dumps
round-trip variables so the test validates both layout and numerical
correctness.
🪄 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: ac11019a-e7c5-4720-88d9-a55a0c192052
📒 Files selected for processing (43)
python/cuml/cuml/__init__.pypython/cuml/cuml/common/classification.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml/internals/__init__.pypython/cuml/cuml/internals/mixins.pypython/cuml/cuml/internals/outputs.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/base_mg.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/linear_model/lars.pyxpython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/linear_regression_mg.pyxpython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/logistic_regression_mg.pyxpython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/linear_model/mbsgd_regressor.pypython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/linear_model/ridge_mg.pyxpython/cuml/cuml/multiclass/multiclass.pypython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/cuml/neighbors/kernel_density.pyxpython/cuml/cuml/neighbors/kneighbors_classifier.pyxpython/cuml/cuml/neighbors/kneighbors_classifier_mg.pyxpython/cuml/cuml/neighbors/kneighbors_regressor.pyxpython/cuml/cuml/neighbors/kneighbors_regressor_mg.pyxpython/cuml/cuml/neighbors/nearest_neighbors.pyxpython/cuml/cuml/neighbors/nearest_neighbors_mg.pyxpython/cuml/cuml/preprocessing/_label.pypython/cuml/cuml/preprocessing/label.pypython/cuml/cuml/solvers/cd.pyxpython/cuml/cuml/solvers/cd_mg.pyxpython/cuml/cuml/solvers/qn.pyxpython/cuml/cuml/solvers/sgd.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/tests/test_pickle.pypython/cuml/tests/test_random_forest.pypython/cuml/tests/test_reflection.py
💤 Files with no reviewable changes (1)
- python/cuml/tests/test_random_forest.py
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR replaces Changesmlfunc/ReflectedAttr Refactor
CuPy Allocator Setup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
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/preprocessing/_label.py (1)
204-226: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH: Drop the now-unused index from
inverse_transform.After Line 226 switched to
ClassLabels,indexis no longer read, so flake8 will flag F841.Proposed fix
- codes, index = check_y( - y, dtype=("i2", "i4", "i8", "u2", "u4", "u8"), return_index=True - ) + codes = check_y( + y, dtype=("i2", "i4", "i8", "u2", "u4", "u8") + )As per coding guidelines, “Lint Python code using flake8 to check for syntax errors and common code style issues.”
🤖 Prompt for 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. In `@python/cuml/cuml/preprocessing/_label.py` around lines 204 - 226, The inverse_transform logic in `_label.py` is still assigning `index` from `check_y(...)` even though it is no longer used after returning `ClassLabels`, which will trigger an F841 lint error. Update the `inverse_transform` implementation to stop binding the unused value from `check_y`, and keep the rest of the label handling flow unchanged in the `LabelEncoder`/`ClassLabels` path.Source: Coding guidelines
🤖 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/common/classification.py`:
- Around line 50-58: The inline class_weight validation in classification.py
currently accepts any Mapping and can silently ignore weights for labels not
present in the data. Update the validation around the class_weight handling to
explicitly compare the mapping keys against the known classes and raise a
ValueError when extra keys are supplied, so typos or unsupported labels fail
fast. Keep the existing checks in the classification helper that normalizes
class_weight, and add the extra-key validation alongside the current Mapping /
"balanced" / None logic.
In `@python/cuml/cuml/ensemble/randomforest_common.pyx`:
- Around line 277-279: The sklearn export path in `_attrs_to_cpu()` is leaving
OOB attributes as device arrays; update the `RandomForest` export logic so
`oob_decision_function_` and `oob_prediction_` are converted back to NumPy
before assigning them to the sklearn model. Use the existing `attrs` population
in `randomforest_common.pyx` as the source of truth and ensure the CPU-facing
estimator only receives host arrays for these fields.
In `@python/cuml/cuml/linear_model/linear_regression.pyx`:
- Around line 197-205: The LinearRegression sklearn interop path is converting a
scalar intercept into a 0-d array instead of preserving the documented float
shape. Update the _attrs_from_cpu and _attrs_to_cpu handling in LinearRegression
to mirror the scalar guard used by Ridge, so single-target intercept_ stays a
scalar while multi-target intercept_ remains array-like. Use the existing
LinearRegression methods as the fix point and keep as_sklearn()/from_sklearn()
round-trips sklearn-compatible.
In `@python/cuml/cuml/naive_bayes/naive_bayes.py`:
- Around line 209-216: The `predict_proba` method in `NaiveBayes` dropped the
existing `convert_dtype` API, breaking backward compatibility and causing
`TypeError` when callers pass it. Update `predict_proba` to accept
`convert_dtype` again, mirror the signature and dtype handling used by `predict`
and `predict_log_proba`, and make sure it forwards the validation/conversion
behavior consistently through `predict_log_proba` before applying `cp.exp`.
In `@python/cuml/cuml/neighbors/kernel_density.pyx`:
- Line 415: The `KernelDensity.score` implementation is calling the public
`score_samples`, which can trigger host conversion before `cp.sum` under
non-mirror output settings. Update `KernelDensity.score` to use the internal
CuPy-returning path used by the old explicit behavior, keeping the values on
CuPy until after the sum is computed, and avoid routing through the decorated
public `score_samples` method.
In `@python/cuml/cuml/neighbors/kneighbors_classifier.pyx`:
- Around line 267-270: The callable-weights handling in
kneighbors_classifier.pyx should normalize the result before converting it to a
raw pointer, because weights from compute_weights() may not be a contiguous
float32 buffer. Update the prediction paths that use weights.data.ptr so they
first ensure weights is a contiguous float32 array before assigning the float*
in both branches, using the existing compute_weights() flow and the two pointer
conversions near the prediction logic.
In `@python/cuml/cuml/solvers/sgd.pyx`:
- Around line 446-458: The predict path in SGD currently reads self.coef_.dtype
before verifying the estimator is fitted, which can surface a
reflected-attribute error instead of the standard fitted-state exception. Update
SGD.predict to call check_is_fitted(self) before any access to learned
attributes like coef_, then continue with check_inputs and the existing
dtype/order handling so unfitted calls fail cleanly.
---
Outside diff comments:
In `@python/cuml/cuml/preprocessing/_label.py`:
- Around line 204-226: The inverse_transform logic in `_label.py` is still
assigning `index` from `check_y(...)` even though it is no longer used after
returning `ClassLabels`, which will trigger an F841 lint error. Update the
`inverse_transform` implementation to stop binding the unused value from
`check_y`, and keep the rest of the label handling flow unchanged in the
`LabelEncoder`/`ClassLabels` path.
🪄 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: 6ecaac64-62e2-4ee7-b75c-87635a5721a0
📒 Files selected for processing (43)
python/cuml/cuml/__init__.pypython/cuml/cuml/common/classification.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml/internals/__init__.pypython/cuml/cuml/internals/mixins.pypython/cuml/cuml/internals/outputs.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/base_mg.pypython/cuml/cuml/linear_model/elastic_net.pypython/cuml/cuml/linear_model/lars.pyxpython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/linear_regression_mg.pyxpython/cuml/cuml/linear_model/logistic_regression.pypython/cuml/cuml/linear_model/logistic_regression_mg.pyxpython/cuml/cuml/linear_model/mbsgd_classifier.pypython/cuml/cuml/linear_model/mbsgd_regressor.pypython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/linear_model/ridge_mg.pyxpython/cuml/cuml/multiclass/multiclass.pypython/cuml/cuml/naive_bayes/naive_bayes.pypython/cuml/cuml/neighbors/kernel_density.pyxpython/cuml/cuml/neighbors/kneighbors_classifier.pyxpython/cuml/cuml/neighbors/kneighbors_classifier_mg.pyxpython/cuml/cuml/neighbors/kneighbors_regressor.pyxpython/cuml/cuml/neighbors/kneighbors_regressor_mg.pyxpython/cuml/cuml/neighbors/nearest_neighbors.pyxpython/cuml/cuml/neighbors/nearest_neighbors_mg.pyxpython/cuml/cuml/preprocessing/_label.pypython/cuml/cuml/preprocessing/label.pypython/cuml/cuml/solvers/cd.pyxpython/cuml/cuml/solvers/cd_mg.pyxpython/cuml/cuml/solvers/qn.pyxpython/cuml/cuml/solvers/sgd.pyxpython/cuml/cuml/svm/linear_svc.pypython/cuml/cuml/svm/linear_svr.pypython/cuml/cuml/svm/svc.pypython/cuml/cuml/svm/svm_base.pyxpython/cuml/cuml/svm/svr.pypython/cuml/tests/test_pickle.pypython/cuml/tests/test_random_forest.pypython/cuml/tests/test_reflection.py
💤 Files with no reviewable changes (1)
- python/cuml/tests/test_random_forest.py
This was referenced Jul 9, 2026
csadorf
reviewed
Jul 10, 2026
csadorf
left a comment
csadorf
left a comment
Contributor
There was a problem hiding this comment.
Overall LGTM. I have a few questions, but once those have been addressed, this should be good to ship.
| cp.cuda.set_allocator(rmm_cupy_allocator) | ||
|
|
||
| # XXX: workaround for https://github.com/cupy/cupy/issues/10084 | ||
| copyreg.dispatch_table[cp.ndarray] = lambda x: ( |
Contributor
There was a problem hiding this comment.
Should we make this a more explicit TODO? "Remove once cupy version (/with fix from xxx) has been released". Asssuming that the fix makes it into the next patch release.
This was referenced Jul 10, 2026
Member
Author
|
/merge |
This was referenced Jul 13, 2026
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jul 14, 2026
In #8339 we added `mlfunc` and `ReflectedAttr` as new output coercion mechanisms. As part of that, we only implemented coercion paths from device matrices (`cupy.ndarray`/`cupyx.scipy.sparse.spmatrix`) since cuml generally does all computations on device and any conversion to host will happen later. However, while porting `cuml.fil` I realized there are a few cases (`cuml.fil` included) where we natively output host memory but may still want to coerce it to device later as part of reflection. This PR: - Adds some code to handle `numpy.ndarray`/`scipy.sparse.spmatrix` as source arrays for coercion in `mlfunc`/`ReflectedAttr` - Adds tests for the same - Ports `cuml.fil` to use `mlfunc`/`ReflectedAttr` instead of the legacy equivalents - Also addresses a few suggestions for added comments leftover from #8339. Followup to #8339. Part of #8177. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Simon Adorf (https://github.com/csadorf) URL: #8373
This was referenced Jul 15, 2026
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
4 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 is a bit of a sprawling PR, but it has the following core motivations:
CumlArray/SparseCumlArrayfrom the final places they're used (the reflection system).reflectandrun_in_internal_contextdecorators into a single decorator to ease dev UX (Unify internal decorators #8178).To make this problem tractable, I've taken the following approach:
mlfunc. This decorator unifies the use cases forreflectandrun_in_internal_context, with an eye towards more functionality going forward. In the long run, any function that invokes a cuda kernel or needs to run in an internal context in cuml should use this decorator. The oldreflectandrun_in_internal_contextdecorators are updated to be based on this one (in those cases only, aCumlArray/SparseCumlArraycode path may still be hit).ReflectedAttrdescriptor to replace the oldCumlArrayDescriptor. As estimators are updated they'll end up using the new descriptor and drop the old descriptor (eventually letting us deleteCumlArrayDescriptor). Likemlfunc, the new descriptor doesn't invoke any of the legacy machinery.ClassLabelsobject for handling outputs of class labels (e.g. the output ofpredictfrom a classifier). These are a bit tricky, since labels in sklearn may include non-numeric dtypes, and cupy doesn't handle those. To support this case, we have a custom class that the reflection machinery understands. Previously we hacked around this in the implementation of every classifier'spredictmethod, this is now a builtin supported use case in the reflection machinery.indexonXfor dataframe-like inputs in the output (via settingpreserve_index=Trueinmlfunc). This eases reflecting the input index to the output for transform/predict calls.Following this, I then applied the new system to the following modules:
cuml.linear_modelscuml.ensemblecuml.solverscuml.neighborscuml.svmcuml.multiclasscuml.naive_bayescuml.preprocessing(LabelEncoder/LabelBinarizeronly)The goal with selecting these modules was to
decode_labelsclassifier code paths could be deleted in favor of the newClassLabelshandling.To ease review, I suspect we'll want a handful of follow-up PRs (I'd guess 1-3) to finish porting the remaining modules. They're mostly mechanical changes, but still may touch a number of lines.
Fixes #8178.
Part of #8177.