Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

OmarManzoor
Copy link
Contributor

Reference Issues/PRs

None

What does this implement/fix? Explain your changes.

  • Fixes a small inconsistency with the latest array-api-strict version 2.0.1

Any other comments?

CC: @ogrisel @betatim @EdAbati

Copy link

github-actions bot commented Jul 2, 2024

✔️ Linting Passed

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

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

@lesteve
Copy link
Member

lesteve commented Jul 2, 2024

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)

@OmarManzoor
Copy link
Contributor Author

No informed opinion on the fix, but it looks this is fixing some failures that I bumped into when looking at the lock-file update PR issues and that are due to updating 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.

@betatim
Copy link
Member

betatim commented Jul 2, 2024

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 searchsorted exists in the namespace, but for some reason we can't use it?

@betatim
Copy link
Member

betatim commented Jul 2, 2024

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 array-api-strict? It also seems odd that searchsorted exists in the namespace but you aren't allowed to use it, somehow I'd expect it to not exist at all in older versions?

@OmarManzoor
Copy link
Contributor Author

OmarManzoor commented Jul 2, 2024

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 array-api-strict? It also seems odd that searchsorted exists in the namespace but you aren't allowed to use it, somehow I'd expect it to not exist at all in older versions?

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).

@OmarManzoor
Copy link
Contributor Author

I added a comment with a TODO to remove the additional check when array-api-strict supports the required version.

@betatim
Copy link
Member

betatim commented Jul 2, 2024

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?

@OmarManzoor
Copy link
Contributor Author

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

Copy link
Member

@lesteve lesteve left a 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.

@betatim
Copy link
Member

betatim commented Jul 3, 2024

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 array-api-strict I think we should focus on having the newest version working. The package is only meant to be used by projects like scikit-learn to test if their array API code is compatible/correct. Normal users shouldn't use it to get a "array API compatible array", they should use PyTorch, Cupy, Numpy, etc for that.

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.)

@betatim
Copy link
Member

betatim commented Jul 3, 2024

I started the CUDA CI workflow for 4798651, results: https://github.com/scikit-learn/scikit-learn/actions/runs/9779581332 - tests pass

@lesteve
Copy link
Member

lesteve commented Jul 3, 2024

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 array-api-strict 2.0.1 and we can fix issues if any arise.

@lesteve lesteve merged commit 012de1e into scikit-learn:main Jul 3, 2024
29 of 30 checks passed
@OmarManzoor OmarManzoor deleted the array_api_strict_fix branch July 3, 2024 15:05
snath-xoc pushed a commit to snath-xoc/scikit-learn that referenced this pull request Jul 5, 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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.