Skip to content

Preemptively fix incompatibilities with an upcoming array-api-strict release #31517

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 12, 2025
Merged

Preemptively fix incompatibilities with an upcoming array-api-strict release #31517

merged 2 commits into from
Jun 12, 2025

Conversation

ev-br
Copy link
Contributor

@ev-br ev-br commented Jun 10, 2025

What does this implement/fix? Explain your changes.

array-api-strict plans a release "soon" (tm). Testing it locally smokes out two small failures, which this PR aims to fix:

Any other comments?

One other effect of using the buffer protocol for np.asarray is that testing with array_api_strict needs to use python 3.12 or above. The array api discussion is https://hackmd.io/zn5bvdZTQIeJmb3RW1B-8g#Meeting-minutes-14-November-2024

ev-br added 2 commits June 10, 2025 12:21
@ev-br
MAINT: use tuples for shapes
51c819c 
This is required by the Array API and is harmless otherwise.
@ev-br
TST: account for np.asarray(X_np) raising a ValueError
ce3eb86 
This error originates from NumPy when `__array__` does not
produce a numpy array. This happens when array-api-strict
moves over to __buffer__.
@github-actions GitHub Actions
Copy link

✔️ Linting Passed

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

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

This was referenced Jun 10, 2025
Open
Open
lucyleeow
lucyleeow approved these changes Jun 11, 2025
View reviewed changes
Copy link
Member

@lucyleeow lucyleeow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code changes look fine.

One other effect of using the buffer protocol for np.asarray is that testing with array_api_strict needs to use python 3.12 or above.

Taking a look at our update_environments_and_lock_files.py, all array-api-strict envs are pylatest so I think this should be fine also.

@lucyleeow
Copy link
Member

@ev-br to clarify,

scikit-learn/sklearn/utils/_array_api.py

Lines 810 to 811 in 3962c28

elif _is_xp_namespace(xp, "array_api_strict"):
return numpy.asarray(xp.asarray(array, device=xp.Device("CPU_DEVICE")))

would still work right?

(I was confused by scipy/scipy#21828 though I see that last comment is quite old)

@ev-br
Copy link
Contributor Author

ev-br commented Jun 11, 2025

Yes, numpy.asarray(xp.asarray(array, device=xp.Device("CPU_DEVICE"))) will certainly keep working. scipy/scipy#21828 you mention reports the fallout from a previous attempt at removing __array__ from array-api-strict, and is the primary reason __array__ removal was reverted back in November 2024. This time, we intend to add __buffer__ instead, so that conversion to numpy and interaction with cython typed memoryviews keep working without any downstream changes.

@github-actions github-actions bot removed the CUDA CI label Jun 11, 2025
@betatim
Copy link
Member

betatim commented Jun 11, 2025

Thanks for this ahead of time fix :D

Running the CUDA tests just "to be sure to be sure".

I agree that the new minimum version of Python shouldn't be a problem. It only effects those running the tests as no real user should be using array-api-strict.

@lucyleeow
Copy link
Member

Merging, thanks @ev-br and @betatim !

@lucyleeow lucyleeow merged commit d171a3c into scikit-learn:main Jun 12, 2025
49 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betatim betatim betatim approved these changes

@lucyleeow lucyleeow lucyleeow approved these changes

Assignees
No one assigned
Labels
Array API module:metrics module:utils No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ev-br @lucyleeow @betatim @virchan