-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
Quick fix for new version of array_api_strict #29387
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
Conversation
No informed opinion on the fix, but it looks this is fixing some failures that I bumped into when looking at lock-file update PR issues and that are due to array-api-strict 2.0.1, see #29276 (comment) |
Yes it fixes this one error. array-api-strict seems to have included this function in their api but it is not supported in the current version. Therefore we need this specific kind of check. |
This looks reasonable? But I am not sure I understand why we need this. Maybe we can add a comment to explain it? In particular because it seems like |
I wrote my comment and then forgot to post it. This is why it looks a bit disconnected. Looking at the error in #29276 (comment) it makes more sense why we need to do something here. Instead of the proposed fix, I was wondering if we could request a newer version from |
Yes it seems like it didn't exist in older versions but now it seems like they might be planning to support it in the next version. This is the specific error The function searchsorted requires API version 2023.12 or later,
but the current API version for array-api-strict is 2022.12 This is what is mentioned on pypi array-api-strict currently supports the 2022.12 version of the standard.
2023.12 support is planned and is tracked by
[this issue](https://github.com/data-apis/array-api-strict/issues/25). |
I added a comment with a TODO to remove the additional check when array-api-strict supports the required version. |
I found https://data-apis.org/array-api-strict/api.html#array_api_strict.set_array_api_strict_flags so we could select a newer version already? |
Nice 👍 Let me try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment with a TODO saying that we should clean this up one day?
Just curious, is there a requirement that our tests work with a range of array-api-strict versions in this case 1.1.1
and 2.0.1
. Currently we only test this with whatever is in the GPU workflow and the array-api one:
❯ rg array-api-strict build_tools/**/*.lock
build_tools/azure/pylatest_conda_forge_mkl_linux-64_conda.lock
211:https://conda.anaconda.org/conda-forge/noarch/array-api-strict-1.1.1-pyhd8ed1ab_0.conda#941bbcd64d1a7b44aeb497f468fc85b4
build_tools/github/pylatest_conda_forge_cuda_array-api_linux-64_conda.lock
249:https://conda.anaconda.org/conda-forge/noarch/array-api-strict-1.1.1-pyhd8ed1ab_0.conda#941bbcd64d1a7b44aeb497f468fc85b4
I guess codecov being red is fine, that means that the code branch for new array-api-strict is currently not tested since array-api-strict version is 1.1.1. Note it will when we update the lock-file in #29388 and then array-api-strict 2.0.1 will be tested.
I think for now we are saying that "to use the experimental array API support, you need to use the latest versions of things". In particular for Overall this means that we should stick to the latest version of it. (A separate topic is which versions of the array API standard we want to be compatible with, but that is a new discussion.) |
I started the CUDA CI workflow for 4798651, results: https://github.com/scikit-learn/scikit-learn/actions/runs/9779581332 - tests pass |
Given the discussion and despite my lack of Array API "street cred" I am going to merge this one! What is the worst that could happen 😅? This will be tested in #29388 that will update to |
Reference Issues/PRs
None
What does this implement/fix? Explain your changes.
Any other comments?
CC: @ogrisel @betatim @EdAbati