ENH: Simplify CuPy asarray and to_device
#314
Conversation
| elif not isinstance(device, _Device): | ||
| raise ValueError(f"Unsupported device {device!r}") | ||
| else: | ||
| # see cupy/cupy#5985 for the reason how we handle device/stream here |
There was a problem hiding this comment.
Issue fixed in 2021
There was a problem hiding this comment.
Pull Request Overview
This PR simplifies CuPy’s asarray and to_device functions by streamlining the handling of the copy parameter and device context, and by updating test exclusions accordingly.
- Removed outdated test references regarding copy=False from cupy-xfails.txt
- Updated the asarray function in cupy/_aliases.py to accept copy as None and handle it via cp.asarray
- Revised device and stream handling in _helpers.py to use direct type checks against cp.cuda.Device and cp.cuda.Stream
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cupy-xfails.txt | Removed xfail tests related to copy=False implementation |
| array_api_compat/cupy/_aliases.py | Simplified asarray implementation and removed legacy copy handling and device context management |
| array_api_compat/common/_helpers.py | Updated device and stream handling in to_device |
Comments suppressed due to low confidence (1)
array_api_compat/common/_helpers.py:791
- The updated type check for device using cp.cuda.Device is concise; however, verify that this check fully replaces the earlier logic and correctly handles all valid device cases in production.
if not isinstance(device, cp.cuda.Device):
array_api_compat/cupy/_aliases.py
Outdated
| if not copy and res is not obj: | ||
| raise ValueError("Unable to avoid copy while creating an array as requested") |
There was a problem hiding this comment.
This is less than ideal as it causes a brief spike in memory usage, but IMHO it's better than outright failing
crusaderky
force-pushed
the
cupy_asarray
branch
from
d922875 to
8c31248
Compare
| if device == x.device: | ||
| return x | ||
| elif device == "cpu": | ||
| if device == "cpu": | ||
| # allowing us to use `to_device(x, "cpu")` | ||
| # is useful for portable test swapping between | ||
| # host and device backends | ||
| return x.get() |
There was a problem hiding this comment.
Whoa. I'm confused: this returns a numpy array if x was a cupy array. This might be intended, but then the return annotation is misleading? Same for the device argument annotation. So maybe add a comment if a numpydoc docstring with Parameters and Returns sections is an overkill.
There was a problem hiding this comment.
None of this has changed. See also #87.
There was a problem hiding this comment.
My point is that gh-87 is still open, and there are no tests, so if we touch this, great, let's take the opportunity to also improve testing.
| raise TypeError(f"Unsupported stream type {stream!r}") | ||
|
|
||
| with device, stream: | ||
| return cp.asarray(x) |
There was a problem hiding this comment.
Do we have test coverage for this part? TBH I'm ready to believe it's totally correct but would prefer to have some tests to ensure that it really is.
Also I wouldn't be totally surprised if this is cupy version dependent, in which case a decent test coverage would be really helpful for diagnosing/debugging.
There was a problem hiding this comment.
None of this is covered by array-api-tests data-apis/array-api-tests#302.
I've tested this PR manually against the latest CuPy.
Also I wouldn't be totally surprised if this is cupy version dependent
The previous design cites an upstream issue resolved in 2021.
I have not tested against old versions. How old is too old to care? At the moment I understand that the policy is "don't willfully break backwards compatibility", but there are no tests whatsoever for old versions of any backend other than numpy.
in which case a decent test coverage would be really helpful for diagnosing/debugging.
Fully agree, and the place for it is data-apis/array-api-tests#302
There was a problem hiding this comment.
Yes, testing it in array-api-tests would be great. Two things to consider though:
- Given that there are no tests at all, either here (cupy-specific) or generic, I think this PR is a great opportunity to add some testing
- Given how underspecified the device story is in the standard (for better or worse), my impression is that array-api-tests would be somewhat limited (as we cannot test implementation-dependent behaviors in there). Thus, cupy-specific tests here would be great.
Re: how old is a version we care? Might want some input from CuPy devs, too. Off the cuff, I'd say we definitely care about 13.4; previous 13.x versions with x < 4, not necessary;
Whether we care about 13.x when 14.x stops being alpha... Maybe not unless there are strong reasons?
| res = cp.array(obj, dtype=dtype, copy=copy, **kwargs) | ||
| if not copy and res is not obj: | ||
| raise ValueError("Unable to avoid copy while creating an array as requested") | ||
| return res |
There was a problem hiding this comment.
This part seems to streamline and remove workarounds for presumably older cupy versions.
- Do we have test coverage?
- Which cupy version made the change which renders the removed workaround obsolete?
- Do we then need to formulate some sort of policy for a minimum supported CuPy version? I personally would be OK with any sort of policy ("the currently most recent released version" is fine by me), as long as it's spelled out and lower capped in
pyproject.toml.
There was a problem hiding this comment.
Nope, the latest cupy still has the same exact issues as the old ones.
The API I'm using hasn't changed since CuPy 1.0.
Do we then need to formulate some sort of policy for a minimum supported CuPy version?
We need to formulate policy for a minimum supported version of all backends.
So far it has not been done deliberately in order to reduce attrition (much appreciated).
This is a matter of discussion for the Committee.
There was a problem hiding this comment.
The latest cupy you mean 13.4 or 14.0.a1?
One thing to bear in mind with CuPy specifically: IIUC they are still working through the numpy 2.0 transition. 13.4 is sort-of compatible (one goal was to make it import with both numpy 1 and numpy 2), 14.x series is supposed to complete the transition.
Which is partly a motivation for my questions/comments: if it's a moving target now, we should have test coverage.
|
@ev-br added unit tests; all green locally on CuPy 13.4.1. There should not be any outstanding complaints. |
Co-authored-by: Evgeni Burovski <[email protected]>
|
Merged, thanks @crusaderky |
Salvaged from #293
Simplify to_device and asarray for cupy and implement copy=False