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

ENH: support CuPy to_device "cpu" #40

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

tylerjereddy
Copy link
Contributor

  • allow to_device(x, "cpu") for CuPy--this makes it easier to write portable unit tests where the
    expected value is on the host (a NumPy array)

  • otherwise, you can end up writing different shims in downstream libraries to move the array-like back to the host (.get() for CuPy...); if CuPy conforms to the standard, we shouldn't need this shim long-term

* allow `to_device(x, "cpu")` for CuPy--this makes
it easier to write portable unit tests where the
expected value is on the host (a NumPy array)

* otherwise, you can end up writing different shims
in downstream libraries to move the array-like back
to the host (`.get()` for CuPy...); if CuPy conforms
to the standard, we shouldn't need this shim long-term
@asmeurer
Copy link
Member

Looks fine to me, although this might also be something to request upstream from cupy.

@asmeurer
Copy link
Member

if CuPy conforms to the standard, we shouldn't need this shim long-term

The standard doesn't say anything about how devices are specified https://data-apis.org/array-api/latest/design_topics/device_support.html#syntax-for-device-assignment. The "cpu" device for NumPy was just a convention that was chosen, probably based off pytorch. Maybe the standard should specify that "cpu" == host? It could be worth opening an issue about. I'm not sure what has already been discussed here.

@asmeurer
Copy link
Member

CuPy won't be tested until I test it manually. I'll be sure to do that before making another release.

@asmeurer asmeurer merged commit 2ec609d into data-apis:main Apr 28, 2023
@tylerjereddy
Copy link
Contributor Author

For the record I did test on CuPy (and Torch) locally with a 1080 Ti and passed for me.

@asmeurer
Copy link
Member

asmeurer commented May 1, 2023

I'm actually a little unsure about this thinking about it. If you transfer a CuPy array to NumPy, you change what namespace it uses. That's different from PyTorch where CPU and GPU tensors both use the torch namespace.

@tylerjereddy
Copy link
Contributor Author

Good point, I shared that in the cross-linked array API standard issue. That said, at least for my use case, I didn't care so much about staying in the same array namespace as being able to have the memory usable directly in assert_allclose(). The fact that different array libraries have such drastically different semantics in moving data between device and host probably further underlines the need for a portable shim of some sort? Unless you really want each lib to write its own shims for this.

@asmeurer
Copy link
Member

asmeurer commented May 1, 2023

Well you would need to make sure to recall array_namespace after calling to_device.

My feeling is that this goes against some of the design decisions of the standard (see https://data-apis.org/array-api/latest/purpose_and_scope.html and https://data-apis.org/array-api/latest/design_topics/data_interchange.html). But device stuff isn't really my wheelhouse so hopefully other folks will weigh in on the issue or at the consortium meeting.

@asmeurer asmeurer mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants