Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork11.9k
MAINT: Addarray-api-tests CI stage, addndarray.__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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ndarray.__array_namespace__array-api-tests CI stage, addndarray.__array_namespace__0d50cd4 toedd114cCompareUh oh!
There was an error while loading.Please reload this page.
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.
asmeurer commentedNov 16, 2023
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 bydata-apis/array-api-tests#212, so the!=6.88.4 can probably be dropped by updating to a newerarray-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 intest_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.
mtsokol commentedNov 24, 2023
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, sincenumpy.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 bydata-apis/array-api-tests#212, so the!=6.88.4 can probably be dropped by updating to a newerarray-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 intest_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 fromarray-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
| returnNULL; | ||
| } | ||
| 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.
Callingndarray.__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 say2023.12 in the future, asking for2022.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:
>>>importnumpyasnp>>>xp=np.arange(2).__array_namespace__()>>>assertxpisnp
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.
I addedtest__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\" oftheArray 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!
array-api-tests CI stage, addndarray.__array_namespace__array-api-tests CI stage, addndarray.__array_namespace__mtsokol commentedNov 24, 2023
Ok, I think it's ready on my end. |
ngoldbaum commentedNov 28, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
All looks good to me, merging to unblock the other array API PRs. Thanks@mtsokol! Just out of curiosity, how is |
rgommers commentedNov 28, 2023
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". |
asmeurer commentedNov 29, 2023
To add to that, at least the way array-api-compat is currently written, |
| returnNULL; | ||
| } | ||
| 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.
rgommers commentedNov 29, 2023
Hmm, we should get to ~100% compatibility. Do you have specific things in mind? If so, let's open an issue now in |
Uh oh!
There was an error while loading.Please reload this page.
Hi@rgommers@asmeurer,
This draft PR introduces
array-api-testsCI stage. Here are some important notes:array-api-testson PyPI, so I followed implementation inhttps://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, soAPI: Add and redefinenumpy.bool[Array API] #25080 needs to be merged before this PR[EDIT] Merged.