Fix wrapping metaestimators in Pipeline in cuml.accel - #8115
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds import-time patching for ChangesColumnTransformer and FeatureUnion NumPy output patching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
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.
🧹 Nitpick comments (1)
python/cuml/cuml_accel_tests/test_pipeline.py (1)
362-385: ⚡ Quick winAdd explicit NumPy-boundary assertions and cover predict path.
Both new tests currently stop at
fit, so they don’t directly verify the patchedtransformpath or that downstream estimator inputs remain NumPy. Please assert input/output array types during bothfitandpredict(as done in earlier pipeline tests).Proposed test hardening
`@requires_sklearn_18` -def test_column_transfomer_in_pipeline_works(): +def test_column_transfomer_in_pipeline_works(patch_methods): """Ensure outputs of steps in `ColumnTransformer` return as numpy""" + patch_methods(Ridge, "fit", "predict") rng = np.random.default_rng(0) X = rng.standard_normal((200, 20)).astype(np.float32) y = rng.standard_normal(200).astype(np.float32) @@ pipe = Pipeline( [ ("ct", ct), # Shouldn't be accelerated ("scaler", RobustScaler()), # Not accelerated ("ridge", Ridge()), # Accelerated ] ) pipe.fit(X, y) + assert isinstance(Ridge.fit.args[0], np.ndarray) + out = pipe.predict(X[:10]) + assert isinstance(Ridge.predict.args[0], np.ndarray) + assert isinstance(out, np.ndarray) @@ `@requires_sklearn_18` -def test_feature_union_in_pipeline_works(): +def test_feature_union_in_pipeline_works(patch_methods): """Ensure outputs of steps in `FeatureUnion` return as numpy""" + patch_methods(Ridge, "fit", "predict") rng = np.random.default_rng(0) X = rng.standard_normal((200, 20)).astype(np.float32) y = rng.standard_normal(200).astype(np.float32) @@ pipe = Pipeline( [ ("features", union), # Shouldn't be accelerated ("scaler", RobustScaler()), # Not accelerated ("ridge", Ridge()), # Accelerated ] ) pipe.fit(X, y) + assert isinstance(Ridge.fit.args[0], np.ndarray) + out = pipe.predict(X[:10]) + assert isinstance(Ridge.predict.args[0], np.ndarray) + assert isinstance(out, np.ndarray)As per coding guidelines:
python/**/test_*.py: “Test files must ... test fit/predict/transform consistency”.Also applies to: 387-409
🤖 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_accel_tests/test_pipeline.py` around lines 362 - 385, The test test_column_transfomer_in_pipeline_works stops at pipe.fit and must assert NumPy-boundaries and cover predict: during fit, assert that data passed into ColumnTransformer ct and into downstream steps (e.g., "scaler" and "ridge") are NumPy arrays (use isinstance(..., np.ndarray)) and that ct.transform output is NumPy; then call pipe.predict(X) and assert the inputs received by downstream steps and the final prediction output are also NumPy arrays; mirror these same fit/predict assertions in the sibling test around lines 387-409 to ensure both transform and predict paths are covered.
🤖 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.
Nitpick comments:
In `@python/cuml/cuml_accel_tests/test_pipeline.py`:
- Around line 362-385: The test test_column_transfomer_in_pipeline_works stops
at pipe.fit and must assert NumPy-boundaries and cover predict: during fit,
assert that data passed into ColumnTransformer ct and into downstream steps
(e.g., "scaler" and "ridge") are NumPy arrays (use isinstance(..., np.ndarray))
and that ct.transform output is NumPy; then call pipe.predict(X) and assert the
inputs received by downstream steps and the final prediction output are also
NumPy arrays; mirror these same fit/predict assertions in the sibling test
around lines 387-409 to ensure both transform and predict paths are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3b5ee11a-f17e-41cb-87fc-74625b64c69c
📒 Files selected for processing (5)
python/cuml/cuml/accel/_patches/sklearn/compose.pypython/cuml/cuml/accel/_patches/sklearn/pipeline.pypython/cuml/cuml/accel/core.pypython/cuml/cuml_accel_tests/test_pipeline.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-examples.yaml
|
Thanks for the quick turn-around!! |
|
/merge |
da8e0cd
into
NVIDIA:release/26.06
|
Whoop whoop |
Our pipeline data transfer optimization didn't work if any of the steps were other compositional metaestimators that wrapped accelerated estimators (since these could then accidentally use the accelerated versions, resulting in a mix of
cupyandnumpyresults).This PR patches the other two compositional estimators (
FeatureUnionandColumnTransformer) so they always run within anumpyoutput-type context.Fixes #8112.
Fixes a few sklearn examples as well (yay!)