Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop support for the redundant and deprecated cupy.array_api in favor of array_api_compat. #29639

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Aug 8, 2024

While reviewing #29615 I realized that we don't necessarily need to maintain our own sklearn.utils._array_api._clip fallback since we can instead rely on array-api-compat in recent versions.

It's still a bit verbose to use to correctly handle non-cpu devices with PyTorch but I think this should be fixed upstream:

EDIT: the scope of this PR has widened as explained in #29639 (comment).

Copy link

github-actions bot commented Aug 8, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: d8a04d3. Link to the linter CI: here

@ogrisel
Copy link
Member Author

ogrisel commented Aug 8, 2024

v1.6.rst already mentions cosine_distances for #29265. I don't think it's worth cluttering the changelog with 2 PRs for this function.

@ogrisel ogrisel added the CUDA CI label Aug 8, 2024
@github-actions github-actions bot removed the CUDA CI label Aug 8, 2024
@ogrisel
Copy link
Member Author

ogrisel commented Aug 8, 2024

Actually, array_api_compat does not yet provide a xp.clip for the CuPy wrapper. So we better stick to using our own _clip fallback for the time being.

Here is the error reported by our CUDA CI on CuPy inputs:

AttributeError: module 'cupy.array_api' has no attribute 'clip'. Did you mean: 'flip'?

EDIT: this is unexpected because the changelog of array-api-compat 1.8 explicitly mentions the clip function.

EDIT 2: I also tried the following snippet after install array-api-compat 1.8 on google colab and it works as expected:

import array_api_compat.cupy as xp

data = xp.linspace(0, 1, num=5)
xp.clip(data, 0.1, 0.9)

Google Colab is running with CuPy 12.2.0 installed by default at the time of writing.

EDIT 3: I understand what's going on: the array_api_compat.cupy (for the cupy module) is up to date but the cupy.array_api module (that is not wrapped by array_api_compat is not yet updated to implement clip.

We could either add a clip method to our internal sklearn.utils._array_api._ArrayAPIWrapper class that is currently only used to handle missing stuff in cupy.array_api.

Or alternatively we could decide to no longer run the tests for cupy.array_api and instead recommend cupy users to use array_api_compat.cupy instead of cupy.array_api.

@ogrisel ogrisel added the CUDA CI label Aug 8, 2024
@github-actions github-actions bot removed the CUDA CI label Aug 8, 2024
@ogrisel
Copy link
Member Author

ogrisel commented Aug 9, 2024

Or alternatively we could decide to no longer run the tests for cupy.array_api and instead recommend cupy users to use array_api_compat.cupy instead of cupy.array_api.

Since numpy.array_api was removed in numpy 2, I expect a similar fate for cupy.array_api. I opened cupy/cupy#8470 to check whether is this is planned or not.

@ogrisel
Copy link
Member Author

ogrisel commented Aug 13, 2024

Since we have the confirmation from upstream that the experimental cupy.array_api will be sunset in a near future release and since array_api_compat.cupy works well for inputs directly passed as cupy arrays, let's remove the code dedicated to testing and bringing compat wrapping for this module (see 360b943). Since array API support is experimental in scikit-learn, we don't need to follow the usual deprecation cycle for this removal.

This removes quite a bit of complexity / technical debt and should also speed up our test suite by not testing for cupy twice.

I will push an update to the changelog in a forthcoming commit.

@ogrisel ogrisel changed the title Use xp.clip from array_api_compat in cosine_distances Drop support for the redundant and deprecated cupy.array_api in favor of array_api_compat. Aug 13, 2024
@github-actions github-actions bot removed the CUDA CI label Aug 13, 2024
doc/whats_new/v1.6.rst Outdated Show resolved Hide resolved
# TODO: add cupy and cupy.array_api to the list of libraries once the
# the following upstream issue has been fixed:
# TODO: add cupy to the list of libraries once the the following upstream issue
# has been fixed:
# https://github.com/cupy/cupy/issues/8180
Copy link
Member Author

@ogrisel ogrisel Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: cupy/cupy#8180 was fixed and will be included in the next release (v14) but we still have to wait for the time being.

@github-actions github-actions bot removed the CUDA CI label Aug 13, 2024
@ogrisel ogrisel requested a review from OmarManzoor August 13, 2024 13:46
@ogrisel ogrisel requested review from thomasjpfan, betatim and OmarManzoor and removed request for OmarManzoor and thomasjpfan August 13, 2024 13:46
@ogrisel
Copy link
Member Author

ogrisel commented Aug 13, 2024

I believe this is ready for review.

@ogrisel ogrisel requested a review from thomasjpfan August 13, 2024 14:03
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. I'm happy that we can finally remove _ArrayAPIWrapper.

Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you @ogrisel

@OmarManzoor OmarManzoor merged commit a3a0e51 into scikit-learn:main Aug 14, 2024
33 checks passed
@ogrisel ogrisel deleted the array-api-xp.clip branch August 14, 2024 07:57
MarcBresson pushed a commit to MarcBresson/scikit-learn that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants