Skip to content
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

Adopt the numpy.array_api module as cupy.array_api #5698

Merged
merged 165 commits into from Sep 27, 2021

Conversation

leofang
Copy link
Member

@leofang leofang commented Sep 5, 2021

Very experimental attempt to see how far we can reach. DO NOT MERGE.

Close #4789.

This PR has two components:

  1. It brings in the PR ENH: Implementation of the NEP 47 (adopting the array API standard) numpy/numpy#18585 sent to NumPy by @asmeurer. It's mostly done as is -- check out the commit history. The only changes I made to fix the git am failures were those related to NumPy specifics, such as setup.py and _pytesttester.py; the relevant diffs were carefully removed from the patch to enable a simple git am 18585.patch call.
  2. It modifies the module to tailor for CuPy --- ongoing, see the latest progress: https://github.com/leofang/cupy/compare/fb93033..d3a6f0d

cc: @rgommers @asmeurer (feel free to tag more interested people)

This is based on the function stubs from the array API test suite, and is
currently based on the assumption that NumPy already follows the array API
standard. Now it needs to be modified to fix it in the places where NumPy
deviates (for example, different function names for inverse trigonometric
functions).
…dual files

The specific submodule organization is an implementation detail and should not
be used. Only the top-level numpy._array_api namespace should be used.
This avoids importing everything inside the individual functions, but still is
preferred over importing the functions used explicitly, as most of them clash
with the wrapper function names.
The docstrings just point back to the functions they wrap for now. More
thought may need to be put into this for the future. Most functions can
actually perhaps inherit the docstring of the function they wrap directly, but
there are some functions that have differences (e.g., different names,
different keyword arguments, fewer keyword arguments, etc.). There's also the
question of how to handle cross-references/see alsos that point to functions
not in the API spec and behavior shown in docstring examples that isn't
required in the spec.
This is mostly aimed at any potential reviewers of the module for now.
Some stubs still need to be modified to properly pass mypy type checking.
Also, 'device' is just left as a TypeVar() for now.
…eturn a scalar

This is needed to pass mypy type checks for the given type annotations.
…bmodule

That way they only work on actual ndarray inputs, not array-like, which is
more inline with the spec.
So far, it just is a wrapper with all the methods defined in the spec, which
all pass through. The next step is to make it so that the methods that behave
differently actually work as the spec describes. We also still need to modify
all the array_api functions to return this wrapper object instead of
np.ndarray.
…ndarray class

These methods aren't required by the spec, but without them, the array object
is harder to use interactively.
@kmaehashi
Copy link
Member

Adding a test axis for array api tests in CuPy is great, it should be as simple as build cupy; git clone https://github.com/data-apis/array-api-tests; ARRAY_API_TESTS_MODULE=cupy pytest. If the test failure is due to the test suite we can just create an issue.
Once array API support is merged to CuPy, it also makes sense to setup CI in array-api side so you can test pull-requests of array-api-tests suite against CuPy.

Free GPU CI is available, just not from GH, Azure, etc 😆 @aktech's cirun.io has been used by QuTiP folks to set up their CuPy tests (qutip/qutip-cupy#9) so it's definitely doable.

My understanding is that cirun.io itself is free, but to launch a self-hosted test runner AWS/GCP/Cloudocean account is needed which is pay-as-you-go?

@leofang
Copy link
Member Author

leofang commented Sep 8, 2021

but to launch a self-hosted test runner AWS/GCP/Cloudocean account is needed which is pay-as-you-go?

Right, I keep forgetting... 🤦🏻‍♂️

@leofang
Copy link
Member Author

leofang commented Sep 8, 2021

OK I've identified all the issues that led to the test failures (#5698 (comment)). Note: NumPy and CuPy suffer from the same linespace issue (numpy/numpy#18881), so unless NumPy has a fix out we should just skip the test as Aaron suggested.

Tomorrow, I'll add some simple docs explaining the current status, pointing users to the API spec, etc. I'll also try to inspect again the test suite and the NumPy code adopted in this PR.

@kmaehashi let me know if there is any test related change I should address; otherwise, feel free to take a look anytime 🙂

@asmeurer
Copy link
Contributor

asmeurer commented Sep 9, 2021

Great. Yes you can add me to the bot for the array-api label.

Adding a test axis for array api tests in CuPy is great, it should be as simple as build cupy; git clone https://github.com/data-apis/array-api-tests; ARRAY_API_TESTS_MODULE=cupy pytest

It should be ARRAY_API_TESTS_MODULE=cupy.array_api.

OK I've identified all the issues that led to the test failures (#5698 (comment)). Note: NumPy and CuPy suffer from the same linespace issue (numpy/numpy#18881), so unless NumPy has a fix out we should just skip the test as Aaron suggested.

I saw some of the cross-referenced issues. What is the test_eye error? I can't tell from the summary what the issue is (an unfortunate feature of the test suite, which I don't know how to improve), but from the exception it's possible I have the wrong strategy input to the hypothesis test.

@leofang
Copy link
Member Author

leofang commented Sep 9, 2021

I saw some of the cross-referenced issues. What is the test_eye error? I can't tell from the summary what the issue is (an unfortunate feature of the test suite, which I don't know how to improve), but from the exception it's possible I have the wrong strategy input to the hypothesis test.

@asmeurer all failures I discovered are CuPy's fault. I haven't found any misbehavior from the test suite, though I haven't dug hard either 🙂

@kmaehashi
Copy link
Member

The PR overall looks good to me. 👍

I put the code under cupyx/ just because it's the convention where we add the non-NumPy compliant stuff, but if you feel strongly in either way I can certainly change. We can also import all Array API functions in cupy/array_api/__init__.py but still keep the implementation in cupyx/array_api/. Just let me know whatever you decide 🙂

  • As numpy.array_api is implemented I think it can also be considered as a part of NumPy-compliant API.

  • I'm wondering how we catch-up with the latest NumPy fixes in numpy.array_api to cupy.array_api, as numpy.array_api is likely to be updated frequently. Maybe better to do import cupy as np and keep the remaining code using np to make diff comparison easier? (I agree this is not the best solution though...)

  • All the unittests will fail on Py37 for cupy.array_api? In that case module-level pytest.skip is needed.

@leofang
Copy link
Member Author

leofang commented Sep 20, 2021

@kmaehashi

  • As numpy.array_api is implemented I think it can also be considered as a part of NumPy-compliant API.

OK, I'll move cupyx.array_api to cupy.array_api. (Change 370c7db, maybe?)

  • I'm wondering how we catch-up with the latest NumPy fixes in numpy.array_api to cupy.array_api, as numpy.array_api is likely to be updated frequently. Maybe better to do import cupy as np and keep the remaining code using np to make diff comparison easier? (I agree this is not the best solution though...)

I was hoping to make diff easy too. Keeping np as is (revert e3104ec) would certainly help, with the understanding that the docstrings would contain numpy too. But we can improve them (by diverging from NumPy) once the implementation is considered stable in the future.

  • All the unittests will fail on Py37 for cupy.array_api? In that case module-level pytest.skip is needed.

Yes PY37 will fail, but not because of the tests, but because of the use of position-only arguments in the function signatures, which is a PY38+ thing. The module-level skip is already added in tests/cupyx_tests/array_api_tests/__init__.py and I also bumped the Python version in the pretest (.github/workflows/pretest.yml).

@asmeurer
Copy link
Contributor

I don't know how much cupy's testing framework diverges from NumPy's, but we never found a good way to make the tests for the numpy.array_api submodule work in Python 3.7. NumPy ended up just dropping 3.7 support before merging my PR, in part because of this issue. Just a heads up that you may run into the same issue.

@leofang leofang changed the title [WIP] Adopt the numpy.array_api module as cupyx.array_api Adopt the numpy.array_api module as cupyx.array_api Sep 24, 2021
@leofang leofang marked this pull request as ready for review September 24, 2021 04:52
@leofang
Copy link
Member Author

leofang commented Sep 24, 2021

Hi @kmaehashi this is ready. I've moved the module to under cupy/, and reverted most of cupy/cp occurrences.

@leofang leofang changed the title Adopt the numpy.array_api module as cupyx.array_api Adopt the numpy.array_api module as cupy.array_api Sep 24, 2021
@leofang
Copy link
Member Author

leofang commented Sep 24, 2021

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 7e8191c, target branch master) succeeded!

@kmaehashi kmaehashi added this to the v10.0.0b3 milestone Sep 27, 2021
@kmaehashi
Copy link
Member

Compared diff with numpy/numpy@098f874 and looks good to me!

@kmaehashi kmaehashi merged commit 9c90255 into cupy:master Sep 27, 2021
@leofang leofang deleted the array_api_from_numpy branch September 27, 2021 03:19
@leofang leofang mentioned this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array-api Topic: Array API Standard cat:feature New features/APIs prio:medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt Python Array API standard
5 participants