Deprecate legacy output types - #8389
Conversation
|
Opening this now, but it should go in after #8385. I also still need to update all the docstrings to remove mention of the deprecated types. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds deprecation warnings for legacy output types in global and estimator configuration, warns for numba device-array inputs to ChangesOutput type deprecation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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/internals/outputs.py`:
- Around line 63-72: Update warn_if_output_type_deprecated so the migration
guidance for “series” and “dataframe” explicitly tells users to coerce the
result to cudf.Series or cudf.DataFrame after switching to output_type='cudf',
while preserving the existing cupy guidance for “numba” and “array”. Update the
warning tests to assert these explicit Series/DataFrame coercion instructions.
🪄 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: 9980aa9a-6446-4512-a268-b57b6f16c87a
📒 Files selected for processing (6)
python/cuml/cuml/internals/base.pypython/cuml/cuml/internals/outputs.pypython/cuml/tests/test_dataset_generator_types.pypython/cuml/tests/test_make_arima.pypython/cuml/tests/test_reflection.pypython/cuml/tests/test_svm.py
1b84769 to
689c37f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cuml/tests/test_make_arima.py (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the deprecation warning instead of suppressing it.
This filter lets the
numbacase pass even if the warning regresses or is emitted from an unexpected path. Keep the warning behavior covered by asserting the expectedFutureWarningaround thenumbaexecution, while leaving other output types unchanged.Suggested direction
-@pytest.mark.filterwarnings("ignore:`output_type='numba'`:FutureWarning") def test_make_arima(...): ...Use
pytest.warns(FutureWarning, match="output_type='numba'")conditionally for thenumbaparameter 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/tests/test_make_arima.py` at line 43, Replace the filterwarnings decorator on the test with a conditional pytest.warns assertion around the numba execution, matching FutureWarning text for output_type='numba'. Leave execution and warning behavior unchanged for all other output_type parameter cases.
🤖 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 72-77: Update the deprecation warning in
python/cuml/cuml/internals/outputs.py at lines 72-77 to pass an appropriate
stacklevel that points to the user call site, and apply the same correction to
the warning in python/cuml/cuml/internals/base.py at lines 258-265. Adjust only
the warning calls, preserving their existing messages and behavior.
---
Nitpick comments:
In `@python/cuml/tests/test_make_arima.py`:
- Line 43: Replace the filterwarnings decorator on the test with a conditional
pytest.warns assertion around the numba execution, matching FutureWarning text
for output_type='numba'. Leave execution and warning behavior unchanged for all
other output_type parameter cases.
🪄 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: ff2fb1ef-757f-4595-a25e-a207062d0457
📒 Files selected for processing (6)
python/cuml/cuml/internals/base.pypython/cuml/cuml/internals/outputs.pypython/cuml/tests/test_dataset_generator_types.pypython/cuml/tests/test_make_arima.pypython/cuml/tests/test_reflection.pypython/cuml/tests/test_svm.py
🚧 Files skipped from review as they are similar to previous changes (3)
- python/cuml/tests/test_svm.py
- python/cuml/tests/test_dataset_generator_types.py
- python/cuml/tests/test_reflection.py
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 `@python/cuml/cuml/model_selection/_split.py`:
- Around line 36-38: Update the input description in the relevant splitter
docstring to explicitly identify numba device arrays as deprecated inputs
accepted through CuPy conversion and slated for removal, or narrow the generic
array-like wording so it does not imply they are fully supported.
- Around line 117-123: Update the deprecation warning in the train_test_split
input-type handling block to pass stacklevel=2 to warnings.warn, so the warning
points to the caller’s call site instead of the internal helper.
🪄 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: 431105cd-895c-457a-b83c-945fc2b5357e
📒 Files selected for processing (2)
python/cuml/cuml/model_selection/_split.pypython/cuml/tests/test_train_test_split.py
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/tests/test_compose.py`:
- Around line 34-36: Remove the module-wide pytestmark warning filters from
python/cuml/tests/test_compose.py (lines 34-36) and
python/cuml/tests/test_preprocessing.py (lines 75-77), then apply the same
FutureWarning suppression only to the specific tests that exercise
output_type='numba'.
🪄 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: 1f35467e-d72f-4479-baf6-c48d13d6e6d0
📒 Files selected for processing (3)
python/cuml/tests/dask/test_dask_global_settings.pypython/cuml/tests/test_compose.pypython/cuml/tests/test_preprocessing.py
This deprecates the 'numba', 'array', 'df_obj', 'dataframe', and 'series' output types. - 'numba': `numba.cuda` itself is being deprecated, and `DeviceNDArray` is going away. The advice from `numba.cuda` devs is to stop directly using it. - 'array': this is an alias for 'cupy', the user should just use 'cupy'. - 'df_obj': this is an alias for 'cudf', the user should just use 'cudf'. - 'dataframe' and 'series': these are 'cudf', but with coercion to a specific ndim value (erroring if not possible). This doesn't fit well within the standard sklearn api. If a user wants an output to be specifically a Series or DataFrame, they should handle the coercion themselves later on.
dcfc59a to
d1d38a8
Compare
Doesn't actually make sense to compare distances if indices are shuffled, since distances would likewise be shuffled. Just compare indices.
|
/merge |
This deprecates the 'numba', 'array', 'df_obj', 'dataframe', and 'series' output types.
numba.cudaitself is being deprecated, andDeviceNDArrayis going away. The advice fromnumba.cudadevs is to stop directly using it.Deprecation warnings are raised:
output_type(either globally, contextually, or on an estimator)numbaandoutput_type="input". This warns the user that in the future this method will return acupyarray instead, and that they can silence the warning by explicitly settingoutput_type="cupy"for now.Additionally, special case handling of
numbaarray inputs intrain_test_splitis deprecated and will be removed in version 26.10. Users relying on this are warned to coerce their inputs tocupyarrays prior to callingtrain_test_split.Fixes #8192.