@okuta How do you think?

After thinking about this PR overnight, I realized there's a few improvements that could be made to make this change safer and better. Please take a look at the latest commits as well.

It may be helpful also to see what we envision a use case for such a change is. Please refer to dask/dask-glm@e39aafd#diff-6fa4900cefa9ee15a64539f9fcbf1639R182.

Assume step is a NumPy array and X is a CuPy array. We then create a CuPy array step_like with np.empty_like(X, shape=step.shape). Finally, we copy the contents via the empty slice assignment step_like[:] = step. In this situation, we guarantee an agnostic implementation that will convert step to a CuPy array from a NumPy array resulting from an algorithm that has no CuPy implementation.

Note that for this particular case, we need np.empty_like() to support passing an arbitrary shape. This is waiting numpy/numpy#13046 to be merged, and after it gets merged, I have already a PR prepared to add the shape argument to CuPy as well.

After a conversation with @anaruse, he said one of the concerns here was due to broadcasting. From our understanding, the issue arises when the NumPy array has a different stride, when it's a view, for example. The latest commits address that matter.

@niboshi could you confirm we understood your concerns correctly and if there are any others that we should address?

Currently we intentionally disallow implicit host-to-device synchronization like this.
@okuta @asi1024 Do you have any comments?

Just extending a bit on the original reason why we want to have this special case: with the __array_function__ protocol, it's important to have a way to permit such copies implicitly for when there different types of arrays (e.g., a NumPy and a CuPy array) coming from different sources, allowing us to target the correct implementation. Also note that this change only does not allow implicit device-to-host copies, to prevent an accidental fallback to CPU, but only host-to-device.

I understand the implications this change has, so I'd be fine if we could have this in 6.0.0 as opt-in-only, through a mechanism such as an environment variable, just as NumPy does current for __array_function__ itself.

Disucssed with @asi1024 .

In this particular case (dask/dask-glm@e39aafd#diff-6fa4900cefa9ee15a64539f9fcbf1639R182), it seems that after #2165 is merged, the code should work because cupy.linalg.lstsq will return cupy.ndarray as step. Even without #2165, we're considering adding "NumPy-fallback" feature that should make the code work in this case. In this feature, even non-implemented functions (cupy.linalg.lstsq) should work by accepting and returning cupy.ndarrays and falling back to numpy's corresponding functions.

As for mixed-array-type case you mentioned above, could you elaborate why you need such cases supported?

Thanks @niboshi and @asi1024 for checking on this, and for letting me know about #2165. This would certainly solve that particular case from dask-glm.

For the mixed arrays, consider a slightly different example in the same dask-glm PR, dask/dask-glm@e39aafd#diff-6fa4900cefa9ee15a64539f9fcbf1639R240. This function receives a beta which is the output of some fortran code in scipy, which isn't easy to be ported either to pure NumPy nor CuPy, so we're bound to get this output as a NumPy array. However, X could be, depending on the user input, either a NumPy array, or a CuPy array. It's this last case that we're interested in, we want to have beta copied to a CuPy array, so the remaining computation can be done in the GPU.

I hope this clarifies a bit the sort of use case we have for this.

As for the "NumPy-fallback" feature you mentioned, would it work to wrap a third-party functions (like SciPy) as well? For example, could we pass CuPy arrays to a pure-CPU implementation in SciPy and ensure we get back a CuPy array? I can't think of a way to do that, but if we could, that would be great and probably solve this as well.

For the mixed arrays, consider a slightly different example in the same dask-glm PR, dask/dask-glm@e39aafd#diff-6fa4900cefa9ee15a64539f9fcbf1639R240. This function receives a beta which is the output of some fortran code in scipy, which isn't easy to be ported either to pure NumPy nor CuPy, so we're bound to get this output as a NumPy array. However, X could be, depending on the user input, either a NumPy array, or a CuPy array. It's this last case that we're interested in, we want to have beta copied to a CuPy array, so the remaining computation can be done in the GPU.

In this case, as you know which argument may be of differerent types, I think you can write np.asarray(beta) before passing to func (I assume np may be numpy or cupy). Do you think that works?

As for the "NumPy-fallback" feature you mentioned, would it work to wrap a third-party functions (like SciPy) as well? For example, could we pass CuPy arrays to a pure-CPU implementation in SciPy and ensure we get back a CuPy array? I can't think of a way to do that, but if we could, that would be great and probably solve this as well.

I don't think such cases will be supported either (@asi1024 How do you think?).
Even so, I think np.asarray method above can be used in any case.

In this case, as you know which argument may be of differerent types, I think you can write np.asarray(beta) before passing to func (I assume np may be numpy or cupy). Do you think that works?

I don't think that works, we would have to assume that we know beta needs to be a CuPy array, but that isn't always true, as it depends on the type of X array.

Maybe my last comment wasn't very clear, when I say beta needs to be a CuPy array, I mean that beta needs to be converted to a CuPy array, depending on whether X is also a CuPy array.

My assumption was np may be either numpy or cupy that matches the type of X.

If this assumption is incorrect and np is always numpy, you can use xp = cupy.get_array_module(X) and xp.asarray.

My assumption was np may be either numpy or cupy that matches the type of X.

If this assumption is incorrect and np is always numpy, you can use xp = cupy.get_array_module(X) and xp.asarray.

Sure, this would work. But the point of having __array_function__ in place is to avoid the need to handle explicitly other libraries (and even to let internals know about their existence), and rely solely on NumPy to do the whole work, no matter what the array type.

We understand the potential harm for allowing implicit copies, so having this special copy cupy_array[:] = numpy_array would allow libraries such as dask-glm to be agnostic of CuPy and at the same time prevent unnecessary copies elsewhere, other than a few places where this is absolutely necessary, since during implementation we know of this restriction when using SciPy, or any other CPU-only library.

Ah, OK, I missed the existence of other libraries.

No worries, there are indeed many details and different use cases involved, it's not too difficult to get confused. :)

Please let me know what you guys discuss further, once again, we're open to different solutions as well, provided that we can keep the main benefits of __array_function__.

I'm sorry, maybe it's not clear yet.

My assumption was np may be either numpy or cupy that matches the type of X.

If this assumption is incorrect and np is always numpy, you can use xp = >cupy.get_array_module(X) and xp.asarray.

Sure, this would work. But the point of having __array_function__ in place is to avoid the need to handle explicitly other libraries (and even to let internals know about their existence), and rely solely on NumPy to do the whole work, no matter what the array type.

So, np is actually numpy or cupy (or similar thing in other libraries). Is that correct? Otherwise np.empty_like wouldn't create a CuPy ndarray.
Then, what's the problem about writing np.asarray(beta)? Because you already have np, I think writing that does not require additional knowledge about external libraries like CuPy or others.

Thank you for the fix.
Jenkins, test this please.

Successfully created a job for commit 0e329c7:

• Dashboard for commit 0e329c7

Jenkins CI test (for commit 0e329c7, target branch master) failed with status FAILURE.

Looks like the failure is in chainer-doc, probably not related to this PR.

Looks so. Retrying.
Jenkins, test this please

Successfully created a job for commit 0e329c7:

• Dashboard for commit 0e329c7

Jenkins CI test (for commit 0e329c7, target branch master) succeeded!

Thanks. LGTM!

Awesome! Thanks @niboshi for the review and merging!

By the way, since this now requires explicit opt-in, do you think it could be backported to the next CuPy 6 release?

Yes, sure. Thank you for pointing out!