Skip to content

MNT co-vendor array-api-{compat, extra} #30340

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

Merged
merged 5 commits into from
Mar 25, 2025
Merged

Conversation

lucascolley
Copy link
Contributor

@lucascolley lucascolley commented Nov 23, 2024

closes gh-30367

  • Vendors array-api-compat and array-api-extra. array-api-compat no longer needs to be installed by users. Some behaviour that previously was enabled/disabled based on array-api-compat being installed is now enabled/disabled based on SCIPY_ARRAY_API being set
  • uses array_api_compat.numpy instead of _NumPyAPIWrapper
  • replaces the private helper here with xpx.setdiff1d

TODO:

  • regenerate lock files

Copy link

github-actions bot commented Nov 23, 2024

✔️ Linting Passed

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

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

@lucascolley lucascolley force-pushed the xpx branch 10 times, most recently from b53a032 to 0ebef66 Compare November 23, 2024 23:43
@lucascolley

This comment was marked as outdated.

@lucascolley
Copy link
Contributor Author

thanks for the script example @thomasjpfan, that seems to have worked! @betatim could I get a responsible adult to add the GPU CI label please 😁

Copy link
Contributor Author

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

oops, converting to draft

@lucascolley lucascolley marked this pull request as draft November 30, 2024 13:47
@OmarManzoor
Copy link
Contributor

@lucascolley I have a questions about integrating with array-api-extra. If we are implementing the array API for a particular function or class and in the process there seems to be some function that is not implemented in the array API specification and we need to override it, if we implement it in array-api-extra instead of how we are doing it currently in the utils, won't this be a blocker until the next version of array-api-extra is released?

@lucascolley
Copy link
Contributor Author

lucascolley commented Nov 30, 2024

if we implement it in array-api-extra instead of how we are doing it currently in the utils, won't this be a blocker until the next version of array-api-extra is released?

It doesn't have to be instead of utils. I would suggest you keep your current workflow the exact same - you can still put private helpers in utils to start off with. But in addition, every now and then, you can offload them to array-api-extra so that others can use them. That's what we're doing in SciPy.

@OmarManzoor
Copy link
Contributor

It doesn't have to be instead of utils. I would suggest you keep your current workflow the exact same - you can still put private helpers in utils to start off with. But in addition, every now and then, you can offload them to array-api-extra so that others can use them. That's what we're doing in SciPy.

Okay I understand. Thank you.

@lucascolley

This comment was marked as outdated.

@lucascolley lucascolley marked this pull request as ready for review November 30, 2024 17:25
@lucascolley

This comment was marked as outdated.

Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

A few comments but generally 👍 from me.

I think this makes the install story nicer and solves a problem I had been pondering but didn't know a good solution to: which versions of array-api-compat should we support? Thanks for mentioning that Lucas!

@lucascolley
Copy link
Contributor Author

lucascolley commented Mar 22, 2025

Thanks all, I count +4! Is anyone else wanting to check this over before it acquires a merge conflict?

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 @lucascolley

Just a few points:

  • Is diff1d the only function that we can utilize from array-api-extra right now as we have a bunch of them in the array api utils?
  • We might need to take an additional responsibility to either open an issue on array-api-extra or add the functionality ourselves whenever we add a new function in the scikit-learn array api utils?

@lucascolley
Copy link
Contributor Author

lucascolley commented Mar 22, 2025

Is diff1d the only function that we can utilize from array-api-extra right now as we have a bunch of them in the array api utils?

It is worth checking http://data-apis.org/array-api-extra/api-reference.html. We should be able to transfer a lot over from sklearn, isin should be pretty simple since we have in1d private already.

We might need to take an additional responsibility to either open an issue on array-api-extra or add the functionality ourselves whenever we add a new function in the scikit-learn array api utils?

That would be appreciated!

@OmarManzoor
Copy link
Contributor

OmarManzoor commented Mar 22, 2025

@lucascolley Here is a list of functions I observed that could be moved. However I think if we want to integrate them it should be done in a follow up PR after this is merged. Let me know what you think about these:

  • _expit
  • _fill_or_add_to_diagonal
  • _nanmin
  • _nanmax
  • _nanmean
  • _ravel
  • _isin

@lucascolley
Copy link
Contributor Author

Yep, feel free to open issues (or PRs) over at array-api-extra and we can discuss!

@ogrisel
Copy link
Member

ogrisel commented Mar 22, 2025

Maybe @thomasjpfan has an opinion as the original author of many of the array API helper tooling of scikit-learn.

@betatim
Copy link
Member

betatim commented Mar 24, 2025

@lucascolley can you re-run the lock file generation script? The weekly update happened today so we now have conflicts :-/

Should we wait a few more days and then merge? Technically we have enough 👍 already, but maybe worth waiting a moment longer? But not too long so we don't make Lucas sort out conflicts repeatedly

@lucascolley
Copy link
Contributor Author

🫡

DimitriPapadopoulos added a commit to DimitriPapadopoulos/scikit-learn that referenced this pull request Mar 24, 2025
DimitriPapadopoulos added a commit to DimitriPapadopoulos/scikit-learn that referenced this pull request Mar 24, 2025
DimitriPapadopoulos added a commit to DimitriPapadopoulos/scikit-learn that referenced this pull request Mar 24, 2025
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.

We did not initially vendor array-api-compat because they were releasing bug fixes quickly and scikit-learn has a longer release cycle.

I am +1 on vendoring both array-api-{compat, extra}.

@adrinjalali adrinjalali merged commit e17a12a into scikit-learn:main Mar 25, 2025
33 checks passed
@lucascolley
Copy link
Contributor Author

thanks all!

lucyleeow pushed a commit to lucyleeow/scikit-learn that referenced this pull request Apr 2, 2025
Co-authored-by: Guido Imperiale <crusaderky@gmail.com>
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.

Array API: integrating array-api-extra
8 participants