Remove legacy array and reflection machinery - #8385
Merged
Merged
Conversation
jcrist
added
improvement
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR removes CumlArray and SparseCumlArray integration, deletes legacy reflection and output utilities, updates conversion behavior and mlfunc, relocates cuda_ptr, and migrates testing utilities and coverage to NumPy/CuPy-based paths. ChangesArray and output migration
Estimated code review effort: 4 (Complex) | ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuml/cuml/testing/strategies.py (1)
144-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the misspelled default strategy name.
cuml_array_input_tyes()should becuml_array_input_types().As per coding guidelines, check all files for spelling mistakes using codespell and fix identified errors.
🤖 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/testing/strategies.py` around lines 144 - 146, Rename the misspelled default strategy call in the input-type configuration from cuml_array_input_tyes() to cuml_array_input_types(), and run codespell across the repository to correct any additional reported spelling errors.Source: Coding guidelines
python/cuml/cuml/testing/datasets.py (1)
76-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the float32 representability constraint.
Removing the legacy helper also widens this test predicate, allowing values previously rejected as incompatible. Replace it with an equivalent NumPy-based bounds check so the CumlArray removal preserves dataset-generation behavior.
🤖 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/testing/datasets.py` around lines 76 - 80, Update the dataset validity predicate in the surrounding validation function to retain the float32 representability constraint removed with the legacy helper. Add an equivalent NumPy-based bounds check for X_train values alongside the existing shape and finiteness checks, preserving rejection of values outside float32 limits.
🤖 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/base.py`:
- Around line 65-66: Update the subclass guidance near the removed reflect
documentation to reference cuml.internals.mlfunc(set_input_type=True) instead of
cuml.internals.reflect(reset=True), matching the existing example below.
In `@python/cuml/cuml/testing/test_preproc_utils.py`:
- Around line 56-60: Update the convert helper to disable one-column Series
coercion when calling convert_arrays for dataframe or cudf output, ensuring
converted_dataset is a dataframe before rename(columns=renaming). Add a
regression test covering a one-column input with cudf output and verify the
renamed column is preserved.
---
Outside diff comments:
In `@python/cuml/cuml/testing/datasets.py`:
- Around line 76-80: Update the dataset validity predicate in the surrounding
validation function to retain the float32 representability constraint removed
with the legacy helper. Add an equivalent NumPy-based bounds check for X_train
values alongside the existing shape and finiteness checks, preserving rejection
of values outside float32 limits.
In `@python/cuml/cuml/testing/strategies.py`:
- Around line 144-146: Rename the misspelled default strategy call in the
input-type configuration from cuml_array_input_tyes() to
cuml_array_input_types(), and run codespell across the repository to correct any
additional reported spelling errors.
🪄 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: 5bd7279e-f404-499a-9dff-34ba9024380b
📒 Files selected for processing (27)
python/cuml/cuml/comm/serialize.pypython/cuml/cuml/common/__init__.pypython/cuml/cuml/common/array_descriptor.pypython/cuml/cuml/internals/__init__.pypython/cuml/cuml/internals/array.pypython/cuml/cuml/internals/array_sparse.pypython/cuml/cuml/internals/base.pypython/cuml/cuml/internals/interop.pypython/cuml/cuml/internals/output_utils.pypython/cuml/cuml/internals/outputs.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/lars.pyxpython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/preprocessing/encoders.pypython/cuml/cuml/testing/datasets.pypython/cuml/cuml/testing/strategies.pypython/cuml/cuml/testing/test_preproc_utils.pypython/cuml/cuml/testing/utils.pypython/cuml/tests/dask/test_dask_serialization.pypython/cuml/tests/test_array.pypython/cuml/tests/test_array_sparse.pypython/cuml/tests/test_input_utils.pypython/cuml/tests/test_kernel_density.pypython/cuml/tests/test_kernel_ridge.pypython/cuml/tests/test_reflection.pypython/cuml/tests/test_strategies.py
💤 Files with no reviewable changes (8)
- python/cuml/cuml/common/array_descriptor.py
- python/cuml/tests/test_array_sparse.py
- python/cuml/cuml/internals/output_utils.py
- python/cuml/cuml/internals/interop.py
- python/cuml/tests/test_input_utils.py
- python/cuml/cuml/internals/array_sparse.py
- python/cuml/tests/test_array.py
- python/cuml/cuml/internals/array.py
There was a problem hiding this comment.
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/testing/strategies.py (1)
144-145: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented default strategy name.
Line 145 says
cuml_array_input_tyes(), but the callable iscuml_array_input_types().Proposed fix
- default=cuml_array_input_tyes() + default=cuml_array_input_types()🤖 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/testing/strategies.py` around lines 144 - 145, In the strategy definition, replace the misspelled default callable `cuml_array_input_tyes()` with the existing `cuml_array_input_types()` function, leaving the `input_types` strategy and other configuration unchanged.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.
Outside diff comments:
In `@python/cuml/cuml/testing/strategies.py`:
- Around line 144-145: In the strategy definition, replace the misspelled
default callable `cuml_array_input_tyes()` with the existing
`cuml_array_input_types()` function, leaving the `input_types` strategy and
other configuration unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9bd2c415-21d2-4fff-a652-be5b0722c618
📒 Files selected for processing (27)
python/cuml/cuml/comm/serialize.pypython/cuml/cuml/common/__init__.pypython/cuml/cuml/common/array_descriptor.pypython/cuml/cuml/internals/__init__.pypython/cuml/cuml/internals/array.pypython/cuml/cuml/internals/array_sparse.pypython/cuml/cuml/internals/base.pypython/cuml/cuml/internals/interop.pypython/cuml/cuml/internals/output_utils.pypython/cuml/cuml/internals/outputs.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/lars.pyxpython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/preprocessing/encoders.pypython/cuml/cuml/testing/datasets.pypython/cuml/cuml/testing/strategies.pypython/cuml/cuml/testing/test_preproc_utils.pypython/cuml/cuml/testing/utils.pypython/cuml/tests/dask/test_dask_serialization.pypython/cuml/tests/test_array.pypython/cuml/tests/test_array_sparse.pypython/cuml/tests/test_input_utils.pypython/cuml/tests/test_kernel_density.pypython/cuml/tests/test_kernel_ridge.pypython/cuml/tests/test_reflection.pypython/cuml/tests/test_strategies.py
💤 Files with no reviewable changes (8)
- python/cuml/cuml/internals/interop.py
- python/cuml/cuml/common/array_descriptor.py
- python/cuml/cuml/internals/array_sparse.py
- python/cuml/cuml/internals/output_utils.py
- python/cuml/cuml/internals/array.py
- python/cuml/tests/test_input_utils.py
- python/cuml/tests/test_array_sparse.py
- python/cuml/tests/test_array.py
🚧 Files skipped from review as they are similar to previous changes (15)
- python/cuml/cuml/internals/init.py
- python/cuml/cuml/common/init.py
- python/cuml/cuml/linear_model/base.py
- python/cuml/cuml/linear_model/ridge.pyx
- python/cuml/tests/test_kernel_density.py
- python/cuml/tests/test_strategies.py
- python/cuml/cuml/preprocessing/encoders.py
- python/cuml/tests/dask/test_dask_serialization.py
- python/cuml/cuml/testing/datasets.py
- python/cuml/cuml/linear_model/linear_regression.pyx
- python/cuml/tests/test_kernel_ridge.py
- python/cuml/cuml/testing/utils.py
- python/cuml/cuml/testing/test_preproc_utils.py
- python/cuml/tests/test_reflection.py
- python/cuml/cuml/internals/outputs.py
This was referenced Jul 16, 2026
csadorf
requested changes
Jul 17, 2026
csadorf
left a comment
csadorf
left a comment
Contributor
There was a problem hiding this comment.
Fantastic clean up!
As part of this PR, we should remove all mentions of the legacy machinery within the ESTIMATOR_GUIDE.md.
This utility is only used in this module.
This is the only place `cudf_to_pandas` was called.
Also removes some no longer used strategies.
There was a problem hiding this comment.
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/testing/datasets.py (1)
77-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate all supplied dataset components.
Line 79 only checks
X_train, soX_testory_trainmay contain NaN/inf while the dataset is reported as cuML-compatible. Preserve the finite-value validation across all non-Noneinputs; removing only the representability-range restriction should not remove this safety check.Proposed fix
return ( X_train.shape[0] >= 2 and X_train.shape[1] >= 1 - and np.isfinite(X_train).all() + and all( + np.isfinite(x).all() + for x in (X_train, X_test, y_train) + if x is not None + ) )🤖 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/testing/datasets.py` around lines 77 - 80, Update the dataset compatibility validation around the existing shape and finiteness checks to apply np.isfinite(...).all() to every supplied non-None component, including X_train, X_test, and y_train. Keep the validation for required dimensions and retain the finite-value safety check while removing only the representability-range restriction.
🧹 Nitpick comments (1)
python/cuml/cuml/linear_model/base.py (1)
176-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for
cuda_ptr. Cover CUDA-interface objects, non-CUDA inputs returningNone, and thedata[0] == 0edge case.🤖 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/linear_model/base.py` around lines 176 - 180, Add direct tests for the cuda_ptr function covering an object with __cuda_array_interface__ and verifying its data[0] pointer, ordinary non-CUDA inputs returning None, and CUDA-interface objects whose data[0] is zero still returning 0.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.
Outside diff comments:
In `@python/cuml/cuml/testing/datasets.py`:
- Around line 77-80: Update the dataset compatibility validation around the
existing shape and finiteness checks to apply np.isfinite(...).all() to every
supplied non-None component, including X_train, X_test, and y_train. Keep the
validation for required dimensions and retain the finite-value safety check
while removing only the representability-range restriction.
---
Nitpick comments:
In `@python/cuml/cuml/linear_model/base.py`:
- Around line 176-180: Add direct tests for the cuda_ptr function covering an
object with __cuda_array_interface__ and verifying its data[0] pointer, ordinary
non-CUDA inputs returning None, and CUDA-interface objects whose data[0] is zero
still returning 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 005b7723-e23c-4e2c-bb86-75329e7ff13e
📒 Files selected for processing (27)
python/cuml/cuml/comm/serialize.pypython/cuml/cuml/common/__init__.pypython/cuml/cuml/common/array_descriptor.pypython/cuml/cuml/internals/__init__.pypython/cuml/cuml/internals/array.pypython/cuml/cuml/internals/array_sparse.pypython/cuml/cuml/internals/base.pypython/cuml/cuml/internals/interop.pypython/cuml/cuml/internals/output_utils.pypython/cuml/cuml/internals/outputs.pypython/cuml/cuml/linear_model/base.pypython/cuml/cuml/linear_model/lars.pyxpython/cuml/cuml/linear_model/linear_regression.pyxpython/cuml/cuml/linear_model/ridge.pyxpython/cuml/cuml/preprocessing/encoders.pypython/cuml/cuml/testing/datasets.pypython/cuml/cuml/testing/strategies.pypython/cuml/cuml/testing/test_preproc_utils.pypython/cuml/cuml/testing/utils.pypython/cuml/tests/dask/test_dask_serialization.pypython/cuml/tests/test_array.pypython/cuml/tests/test_array_sparse.pypython/cuml/tests/test_input_utils.pypython/cuml/tests/test_kernel_density.pypython/cuml/tests/test_kernel_ridge.pypython/cuml/tests/test_reflection.pypython/cuml/tests/test_strategies.py
💤 Files with no reviewable changes (8)
- python/cuml/cuml/internals/array.py
- python/cuml/cuml/internals/interop.py
- python/cuml/cuml/internals/output_utils.py
- python/cuml/tests/test_input_utils.py
- python/cuml/cuml/internals/array_sparse.py
- python/cuml/cuml/common/array_descriptor.py
- python/cuml/tests/test_array_sparse.py
- python/cuml/tests/test_array.py
🚧 Files skipped from review as they are similar to previous changes (14)
- python/cuml/cuml/linear_model/lars.pyx
- python/cuml/cuml/linear_model/ridge.pyx
- python/cuml/cuml/common/init.py
- python/cuml/cuml/internals/init.py
- python/cuml/cuml/linear_model/linear_regression.pyx
- python/cuml/tests/dask/test_dask_serialization.py
- python/cuml/cuml/comm/serialize.py
- python/cuml/cuml/testing/test_preproc_utils.py
- python/cuml/cuml/testing/utils.py
- python/cuml/tests/test_kernel_ridge.py
- python/cuml/tests/test_kernel_density.py
- python/cuml/cuml/testing/strategies.py
- python/cuml/tests/test_reflection.py
- python/cuml/cuml/internals/outputs.py
Member
Author
|
/merge |
4 tasks
This was referenced Jul 20, 2026
This was referenced Aug 3, 2026
rapids-bot Bot
pushed a commit
that referenced
this pull request
Aug 4, 2026
- Removes serializer override for `MultinomialNB`, this is unnecessary - Simplifies the registration of serializers for other types, and improves the comments to better state why overrides are needed. This is a small followup to #8385. I noticed in that PR that some of the code here could be simplified/was no longer necessary. This should have no runtime effects (the same effective code should run before and after), it's just a small code cleanup. Authors: - Jim Crist-Harif (https://github.com/jcrist) Approvers: - Victor Lafargue (https://github.com/viclafargue) URL: #8443
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 PR removes:
CumlArrayincuml.testing. This required a bit of code reorg, but the tests should run functionally the same as before. This is handled in the first few commits, the rest should be pure deletion.CumlArray-specific hypothesis strategiescuml.internals.arraycuml.internals.array_sparsecuml.internals.output_utilscuml.common.array_descriptorcuml.internals.interop.to_gpu/cuml.internals.interop.to_cpucuml.internals.outputs.reflectandcuml.internals.outputs.run_in_internal_contextFixes #8177.