Remove non-array api dpnp support - #3322
Conversation
|
/intelci: run |
1 similar comment
|
/intelci: run |
|
/intelci: run |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/intelci: run |
|
/intelci: run |
|
Please update also these doc pages: |
| return wrapper_impl | ||
|
|
||
|
|
||
| def support_sycl_format(func): |
There was a problem hiding this comment.
Necessary change, but I wanted to highlight IntelPython/dpctl#2131 where after this change things are going to require the use of experimental scikit-learn features for use of dpnp/dpctl with Scikit-learn-intelex (also already the case in scikit-learn support).
Derived from this line of code (which was originally also in dpctl tensors)
https://github.com/IntelPython/dpnp/blob/75453b62b0d8add89ad32aafbc3ef409bb13b7bc/dpnp/dpnp_array.py#L174
| _check_set_output_transform(est, method, X, estimator) | ||
|
|
||
| else: | ||
| # For dpnp, the method is only exercised under array_api_dispatch in the |
There was a problem hiding this comment.
I think standard estimator patching and special estimator patching should now become split into two separate tests, one with array api enabled (using a pytest.fixture available in the conftest.py for dpnp, torch, array_api_strict, etc.), and one without (for use of pandas, numpy).
I hope this isn't annoying, but its time.
Its been a while since I've looked at these tests that I refactored a long time ago, and its mentioning features that I didn't think was possible or intentional. I can give it a deeper look, but I recommend the split instead (where fit is under array api dispatch).
There was a problem hiding this comment.
Check if current state aligns with what you had in mind
icfaust
left a comment
There was a problem hiding this comment.
I know its still draft, but I wanted to help steer things a bit. Hope its useful.
Much appreciated! |
|
/intelci: run |
1 similar comment
|
/intelci: run |
david-cortes-intel
left a comment
There was a problem hiding this comment.
@ethanglaser I believe you also need to remove this:
It might also require modifying some code paths in the other functions from that file.
| @pytest.mark.parametrize("dataframe, queue", get_dataframes_and_queues("dpnp,array_api")) | ||
| @pytest.mark.parametrize("estimator, method", gen_models_info(PATCHED_MODELS)) | ||
| def test_standard_estimator_patching_array_api( | ||
| with_array_api, caplog, dataframe, queue, dtype, estimator, method |
There was a problem hiding this comment.
Since you're modifying these: array API tests should be skipped on numpy<2.2.
There was a problem hiding this comment.
Updated. But I believe it's numpy 2.1.
There was a problem hiding this comment.
The error messages typically show a branch with <=2.1. Maybe @yuejiaointel could comment here.
There was a problem hiding this comment.
Numpy 2.1 has array api-related updates
https://numpy.org/devdocs/release/2.1.0-notes.html
I don't think numpy 2.2 has any
https://numpy.org/devdocs/release/2.2.0-notes.html
There was a problem hiding this comment.
But the code branches in pytorch that show up in error messages have numpy<=2.1.
There was a problem hiding this comment.
If you're able to share a link to CI fail / reproducer for something that fails with numpy 2.1 that would be great
There was a problem hiding this comment.
You won't see those in CI logs, because those don't run torch. But you would see them if running things locally on torch + GPU and remove some of the current pytest skips.
There was a problem hiding this comment.
The test currently doesn't run torch. Lets cross that bridge when we come to it (i.e. when we make torch more common in central sklearnex testing). I'd imagine that sklearn conformance testing would likely complain first.
It was already removed |
|
/intelci: run |
What about the other functions? Are they still needed for target offload? |
dispatch/_get_backend/wrap_output_data are still needed for target offload - that path was never the dpnp-copy path. support_input_format is still needed for its output namespace conversion, but its input-side device/queue path (the sycl_usm_array_interface block at L273-275 and the kwargs.get("queue") device branch) is dead now that it only wraps host-side sklearn/daal4py fallbacks - none of which take a queue, so I removed those - please check |
There was a problem hiding this comment.
Pull request overview
This PR removes the project’s custom/non-standard dpnp “device array” compatibility layer in favor of relying on scikit-learn’s Array API dispatch (sklearn.utils._array_api.get_namespace + array_api_dispatch=True). It updates runtime code paths, tests, examples, and docs so dpnp is only supported as an Array API framework (and is otherwise treated as an unsupported device input that is host-transferred).
Changes:
- Replace
sklearnex.utils._array_api.get_namespaceand oneDAL SYCL-namespace helpers with scikit-learn’ssklearn.utils._array_api.get_namespace. - Update tests/examples/docs to require
config_context(array_api_dispatch=True)for dpnp/array-api inputs. - Improve SYCL USM table conversion correctness by applying
__sycl_usm_array_interface__["offset"]during pointer conversion.
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sklearnex/utils/validation.py | Switch get_namespace import to scikit-learn Array API utility. |
| sklearnex/utils/class_weight.py | Switch get_namespace import to scikit-learn Array API utility. |
| sklearnex/utils/_array_api.py | Remove custom namespace selection wrapper; rely on scikit-learn get_namespace. |
| sklearnex/tests/test_patching.py | Split array-api vs non-array-api patching tests; add tolerated sklearn array-api gaps list. |
| sklearnex/svm/_classes.py | Switch to scikit-learn get_namespace; keep sklearnex enable_array_api. |
| sklearnex/svm/_base.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/spmd/neighbors/tests/test_neighbors_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/neighbors/neighbors.py | Switch to scikit-learn get_namespace. |
| sklearnex/spmd/linear_model/tests/test_logistic_regression_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/linear_model/tests/test_linear_regression_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/linear_model/tests/test_incremental_linear_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/ensemble/tests/test_forest_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/decomposition/tests/test_pca_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/decomposition/tests/test_incremental_pca_spmd.py | Always enable array_api_dispatch=True; run deferred-finalize attribute access under dispatch. |
| sklearnex/spmd/covariance/tests/test_incremental_covariance_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/covariance/tests/test_covariance_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/cluster/tests/test_kmeans_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/cluster/tests/test_dbscan_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/basic_statistics/tests/test_incremental_basic_statistics_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/spmd/basic_statistics/tests/test_basic_statistics_spmd.py | Always enable array_api_dispatch=True (drop False branch). |
| sklearnex/preview/preprocessing/tests/test_data.py | Gate dpnp/array-api inputs behind array_api_dispatch=True; normalize fitted-attr comparisons via _as_numpy. |
| sklearnex/preview/preprocessing/_data.py | Remove support_sycl_format fallback wrapper; switch to scikit-learn get_namespace. |
| sklearnex/preview/decomposition/incremental_pca.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/preview/covariance/covariance.py | Remove support_sycl_format wrappers; switch to scikit-learn get_namespace; simplify dpnp-without-dispatch handling. |
| sklearnex/neighbors/knn_unsupervised.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/neighbors/knn_regression.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/neighbors/knn_classification.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/neighbors/common.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/neighbors/_lof.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/linear_model/ridge.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/linear_model/logistic_regression.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/linear_model/linear.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/linear_model/incremental_ridge.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/linear_model/incremental_linear.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/linear_model/_base_linear_model.py | Switch to scikit-learn get_namespace. |
| sklearnex/ensemble/_forest.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/dummy/_dummy.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/decomposition/pca.py | Remove support_sycl_format usage; switch to scikit-learn get_namespace. |
| sklearnex/covariance/tests/test_incremental_covariance.py | Update test logic to use array_api_dispatch=True for dpnp/array-api inputs. |
| sklearnex/covariance/incremental_covariance.py | Remove support_sycl_format usage; switch to scikit-learn get_namespace. |
| sklearnex/cluster/k_means.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/cluster/dbscan.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/basic_statistics/incremental_basic_statistics.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/basic_statistics/basic_statistics.py | Switch to scikit-learn get_namespace; remove local import. |
| sklearnex/_device_offload.py | Remove dpnp-copy backpath and support_sycl_format; align offload/fallback behavior with Array API dispatch. |
| onedal/utils/_array_api.py | Remove SYCL namespace detection helpers (dpnp special casing). |
| onedal/decomposition/pca.py | Remove unused SYCL-namespace import. |
| onedal/datatypes/tests/test_data.py | Use __array_namespace__() directly instead of removed _get_sycl_namespace. |
| onedal/datatypes/sycl_usm/sycl_usm_utils.hpp | Add declaration for SUA offset accessor. |
| onedal/datatypes/sycl_usm/sycl_usm_utils.cpp | Implement SUA offset accessor (for sliced views). |
| onedal/datatypes/sycl_usm/data_conversion.cpp | Apply SUA offset when computing the data pointer for table conversion. |
| onedal/datatypes/_sycl_usm.py | Remove dpnp copy helper module. |
| onedal/datatypes/init.py | Remove copy_to_dpnp from public API exports. |
| onedal/covariance/covariance.py | Remove unused SYCL-namespace import. |
| examples/sklearnex/random_forest_regressor_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/random_forest_classifier_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/pca_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/logistic_regression_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/linear_regression_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/knn_bf_regression_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/knn_bf_classification_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/kmeans_spmd.py | Wrap dpnp GPU runs in config_context(array_api_dispatch=True). |
| examples/sklearnex/incremental_pca_dpnp.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| examples/sklearnex/incremental_linear_regression_dpnp.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| examples/sklearnex/incremental_covariance_spmd.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| examples/sklearnex/incremental_basic_statistics_dpnp.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| examples/sklearnex/dbscan_spmd.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| examples/sklearnex/covariance_spmd.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| examples/sklearnex/basic_statistics_spmd.py | Wrap dpnp GPU usage in config_context(array_api_dispatch=True). |
| doc/sources/oneapi-gpu.rst | Document dpnp as Array API-only; clarify host fallback when dispatch is disabled. |
| doc/sources/input-types.rst | Document dpnp support only when array API mode is enabled; adjust fallback description. |
| @@ -303,27 +286,3 @@ def wrapper_impl(*args, **kwargs): | |||
| return result | |||
| ) | ||
| _check_output_type(result2, y2, method, estimator, caplog, X=X2, est=est) | ||
| _check_fitted_attributes(est, X2, estimator, caplog, queue=queue) | ||
| except Exception as e: |
There was a problem hiding this comment.
Not your fault, but this test is starting to get hard to follow. I refactored this code long ago, and now I don't even fully know what's going on... :( . Ideally it should run the code once as a fixture, and then each of these individual sub function checks should be separate tests. Not sure its worth the time though...
| if len(args) == 0 and len(kwargs) == 0: | ||
| return invoke_func(self, *args, **kwargs) | ||
|
|
||
| if kwargs.get("queue") is not None: |
There was a problem hiding this comment.
This is removed because we have removed all direct onedal estimator interfaces in sklearnex (spmd?)?
There was a problem hiding this comment.
Yeah even in current main branch, support_input_format only wraps host-side sklearn fallback functions, with none accepting a queue, so it's dead code at this point
icfaust
left a comment
There was a problem hiding this comment.
The changes in the active code weren't as extensive as I thought, however the test suite is brutal. We need to have an internal discussion of test hygiene and what we are going to do to make this easier to maintain. I accept a bit of responsibility for having caused this as I sorta pushed get_dataframes_and_queues through a bunch of tests, and now with array API support required, we need to go back and simplify things further. Would like weigh in from others.
good work.
|
/intelci: run |
|
Last call for reviews on this, planning to merge by friday |
|
/intelci: run |
Description
Removes direct support for non-array API dpnp support. Migrates all examples and spmd tests to set
array_api_dispatch=Truefor fully on-device handling without conversion to host. Using dpnp inputs without settingarray_api_dispatchnow returns numpy arrays.Checklist:
Completeness and readability
Testing
Performance