-
-
Notifications
You must be signed in to change notification settings - Fork 12k
MAINT: Add array-api-tests CI stage, add ndarray.__array_namespace__
#25167
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
ndarray.__array_namespace__array-api-tests CI stage, add ndarray.__array_namespace__
0d50cd4 to
edd114c
Compare
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.
Don't know if there's a cleaner place to put this file than the project root.
|
This could also be used to run tests for numpy.array_api, but I guess that's lower priority. |
.github/workflows/linux.yml
Outdated
| run: | | ||
| python -m pip install -r build_requirements.txt | ||
| python -m pip install -r test_requirements.txt | ||
| python -m pip install hypothesis!=6.88.4 # 6.88.4 leads to a strange error |
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.
This needs to be "hypothesis<6.88.4". The problem is warnings from hypothesis, so it's only an issue is if your pytest configuration errors on warnings. (Reported here: data-apis/array-api-tests#211)
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.
Oh, I see -W below, so you definitely will need this version bound until that is addressed upstream.
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.
This should be fixed by data-apis/array-api-tests#212, so the !=6.88.4 can probably be dropped by updating to a newer array-api-tests commit?
And if not, should it be < rather than !=, because there are newer hypothesis versions out already and the problem is unlikely to only exist in that exact 6.88.4 version?
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.
Actually, hypothesis is already present in test_requirements.txt, so this line does nothing - I think it can be removed.
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.
That's right - removed it.
edd114c to
f0f337f
Compare
f0f337f to
4373469
Compare
|
Hi @rgommers, I rebased the branch and now |
rgommers
left a comment
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.
This is looking pretty good to me, and test is green with a clean test log. All my suggestions are pretty small.
array-api-skips.txt
Outdated
| array_api_tests/test_set_functions.py | ||
|
|
||
| # newaxis not included in numpy namespace as of v1.26.2 | ||
| array_api_tests/test_constants.py::test_newaxis |
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.
This one can be left out I think, since numpy.newaxis exists.
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.
Sure, removed!
.github/workflows/linux.yml
Outdated
| run: | | ||
| python -m pip install -r build_requirements.txt | ||
| python -m pip install -r test_requirements.txt | ||
| python -m pip install hypothesis!=6.88.4 # 6.88.4 leads to a strange error |
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.
This should be fixed by data-apis/array-api-tests#212, so the !=6.88.4 can probably be dropped by updating to a newer array-api-tests commit?
And if not, should it be < rather than !=, because there are newer hypothesis versions out already and the problem is unlikely to only exist in that exact 6.88.4 version?
array-api-skips.txt
Outdated
| # newaxis not included in numpy namespace as of v1.26.2 | ||
| array_api_tests/test_constants.py::test_newaxis | ||
|
|
||
| # linalg.solve issue in numpy.array_api as of v1.26.2 (see numpy#25146) |
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.
This issue has been fixed already, and didn't affect the main namespace. Can this be dropped?
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.
Yes, removed.
array-api-skips.txt
Outdated
| # linalg.solve issue in numpy.array_api as of v1.26.2 (see numpy#25146) | ||
| array_api_tests/test_linalg.py::test_solve | ||
|
|
||
| # https://github.com/numpy/numpy/issues/21373 |
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.
also fixed already
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.
Right, removed.
array-api-skips.txt
Outdated
| # missing copy arg | ||
| array_api_tests/test_signatures.py::test_func_signature[reshape] | ||
|
|
||
| # https://github.com/numpy/numpy/issues/21211 |
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.
Closed, ultimately fixed by #21218 I believe.
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.
Removed.
.github/workflows/linux.yml
Outdated
| run: | | ||
| python -m pip install -r build_requirements.txt | ||
| python -m pip install -r test_requirements.txt | ||
| python -m pip install hypothesis!=6.88.4 # 6.88.4 leads to a strange error |
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.
Actually, hypothesis is already present in test_requirements.txt, so this line does nothing - I think it can be removed.
.github/workflows/linux.yml
Outdated
| submodules: 'true' | ||
| path: 'array-api-tests' | ||
| - name: Fix array-apis bug | ||
| # Temporary workaround for https://github.com/data-apis/array-api/issues/631 |
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.
This should have been fixed yesterday, can you check if things pass now without this fixup with the latest commit from array-api-tests?
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.
Yes, it's fixed now.
| with: | ||
| submodules: recursive | ||
| fetch-depth: 0 | ||
| - name: Checkout array-api-tests |
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.
This is fine for now, but once a bunch of the updates are made we should instead use a regular git submodule for these tests, so that they get run locally. Right now if this job fails, re-running locally will be a bit of a hassle.
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.
Right - at some point I will make it a submodule.
numpy/_core/src/multiarray/methods.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| if (strcmp(array_api_version, "2022.12") != 0) { |
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.
Calling ndarray.__array_namespace__(api_version='2021.12') should work too. The newer version is a superset of the older one, so we'd want to support both. And when we upgrade to say 2023.12 in the future, asking for 2022.12 should not break. So adding a test to check this will be useful.
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.
Second suggested regression test:
>>> import numpy as np
>>> xp = np.arange(2).__array_namespace__()
>>> assert xp is npThere 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.
I added test__array_namespace__ for that purpose.
numpy/_core/src/multiarray/methods.c
Outdated
|
|
||
| if (strcmp(array_api_version, "2022.12") != 0) { | ||
| PyErr_Format(PyExc_ValueError, | ||
| "Version \"%s\" of Array API is not supported.", |
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.
minor:
| "Version \"%s\" of Array API is not supported.", | |
| "Version \"%s\" of the Array API Standard is not supported.", |
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.
Updated!
58aaff9 to
724d86e
Compare
array-api-tests CI stage, add ndarray.__array_namespace__array-api-tests CI stage, add ndarray.__array_namespace__
|
Ok, I think it's ready on my end. |
|
All looks good to me, merging to unblock the other array API PRs. Thanks @mtsokol! Just out of curiosity, how is |
It isn't - adding the method is kinda saying "I fully support this". In practice one has to be pragmatic and some gaps that aren't too important can simply be ignored. But if you're missing lots of functionality, don't add this method. Think of it like a C standard - there's defines to check whether say C99 is used, but not "C99 minus x, y and z". |
|
To add to that, at least the way array-api-compat is currently written, |
| return NULL; | ||
| } | ||
|
|
||
| if (strcmp(array_api_version, "2021.12") != 0 && |
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.
Is the point of array_api_version to be forwards compatible, or should this error too?
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.
2022.12 is a superset of 2021.12, so if a user asks for the latter, it's fine to return the former.
Hmm, we should get to ~100% compatibility. Do you have specific things in mind? If so, let's open an issue now in |
Hi @rgommers @asmeurer,
This draft PR introduces
array-api-testsCI stage. Here are some important notes:array-api-testson PyPI, so I followed implementation in https://github.com/google/jax/pull/16099/files, wherearray-api-testsis cloned, and some issues are manually fixed (for now).array-api-skips.txtcontains and disables quite a lot of tests - the rationale is to merge it first, and then, as Array API PRs arrive, enable tests gradually.__array_namespace__tondarrayclass and__array_api_version__to the main namespace.xp.boolis required to run the tests and it can't be XFAILed, so API: Add and redefinenumpy.bool[Array API] #25080 needs to be merged before this PR [EDIT] Merged.