Skip to content

Do we need from_dlpack when DLPack is supported by asarray? #155

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

Closed
shoyer opened this issue Mar 27, 2021 · 8 comments · Fixed by #301
Closed

Do we need from_dlpack when DLPack is supported by asarray? #155

shoyer opened this issue Mar 27, 2021 · 8 comments · Fixed by #301
Labels
API change Changes to existing functions or objects in the API. topic: DLPack DLPack.
Milestone

Comments

@shoyer
Copy link
Contributor

shoyer commented Mar 27, 2021

I guess one possible advantage of from_dlpack() is that it's more explicit? If that's desired, then perhaps we should add other explicit constructors like from_buffer(). Otherwise, it seems redundant with the generic asarray(), and I would lean towards removing it.

@leofang
Copy link
Contributor

leofang commented Mar 29, 2021

Hmmm the only argument I can think of is from_dlpack() does not need to try multiple protocols, but asarray() will have to (#142). But it doesn't seem to hurt if people are happy to remove from_dlpack() from the API, though it's probably something already implemented by everyone.

@rgommers
Copy link
Member

I think there's a couple of problems with asarray:

  • it's slow because it needs to check many input types and protocols, and I've seen it be a performance bottleneck in end user code because it's called a lot.
  • it's too forgiving for input one did not intend to let pass
  • it's a catch-all for random input types, and I expect it will have implementation-dependent behavior for nested containers.

from_dlpack is the one function that one should be using when converting one kind of array into another where you know both support the array API standard. It's more explicit (signalling intent to the reader of code matters), reliable and fast.

then perhaps we should add other explicit constructors like from_buffer().

I think this doesn't follow logically, it's what I would call "design drift". I'd much rather remove buffer protocol support from asarray. We did not have it there in the first place, and only added it because @leofang argued it was convenient for libraries that already implement it like mpi4py. It's a convenience only. As an exchange protocol, it's strictly inferior to DLPack, so there's no reason to create a separate function for it.

@rgommers rgommers added the API change Changes to existing functions or objects in the API. label Mar 29, 2021
@leofang
Copy link
Contributor

leofang commented Mar 30, 2021

from_dlpack is the one function that one should be using when converting one kind of array into another where you know both support the array API standard. It's more explicit (signalling intent to the reader of code matters), reliable and fast.

I'd much rather remove buffer protocol support from asarray.

Would it be better, then, to remove both DLPack and buffer protocol supports from asarray(), so it is more focused and has a separate responsibility from from_dlpack()?

@leofang
Copy link
Contributor

leofang commented Apr 2, 2021

Forgot to reply:

It's a convenience only. As an exchange protocol, it's strictly inferior to DLPack, so ...

I'd say whether or not it's inferior to DLPack is arguable. DLPack is good now because we put a lot of thoughts on the stream sync that leads to the __dlpack__ etc methods living in Python, but from the perspective of CUDA/ROCm whatever can be done using DLPack (without the Python methods) can also be done using the buffer protocol, mpi4py proves it is working fine and we are pushing all major MPI vendors to get mpi4py + GPU awareness tested in their CIs. I'd say it's safer with buffers than with DLTensors in terms of the deleter behavior. Take a look at CuPy and PyTorch, none of them handled the destructor correctly in the first shot; the need to mark used_dltensor is just so counterintuitive to C/C++ programmers. (By no means here I am picking up a fight or overturning a decision I was on board with 🙂 I bring this up only because a comparison is being made...)

@rgommers
Copy link
Member

rgommers commented May 8, 2021

Would it be better, then, to remove both DLPack and buffer protocol supports from asarray(), so it is more focused and has a separate responsibility from from_dlpack()?

I'm not sure, maybe. I'm fine removing just DLPack as well, or leaving as is, or removing both. I think idiomatic code would use:

  • from_dlpack for standard-compliant objects
  • asarray for converting scalars or sequences
  • preferably nothing else (because nothing else will have universal support)

It's not great in a standard to have to deal with optional other protocols that are hard to test for. Saying the buffer protocol must/may be supported is not ideal in both cases. And __array_interface__ or __cuda_array_interface__ the same. I'd lean towards an explicit "may" if we want to do anything, not a "must".

from the perspective of CUDA/ROCm whatever can be done using DLPack (without the Python methods) can also be done using the buffer protocol,

I'm not sure what this means, since the buffer protocol does not support GPUs. Do you mean __cuda_array_interface__ maybe?

@leofang
Copy link
Contributor

leofang commented May 19, 2021

from the perspective of CUDA/ROCm whatever can be done using DLPack (without the Python methods) can also be done using the buffer protocol,

I'm not sure what this means, since the buffer protocol does not support GPUs. Do you mean __cuda_array_interface__ maybe?

No, I meant the buffer protocol. You can create a Py_buffer struct and fill in all the information available from DLPack or __cuda_array_interface__. It'd be a valid buffer from GPUs' perspective, because all we need is the device pointer + metadata (shape, strides, dtype, etc) to correctly interpret the memory. For example see here.

The only trouble with the buffer protocol that one might run into is it's hard to control who'd be the recipient. If the GPU buffer is intercepted by a CPU library, we obviously would get a segfault. But at least for mpi4py this is a desired behavior (so that we know the MPI library is not built correctly to support CUDA, or that the MPI launch configuration is set to disable CUDA).

@rgommers rgommers added the topic: DLPack DLPack. label Jun 30, 2021
@kgryte kgryte added this to the v2021 milestone Oct 4, 2021
@rgommers
Copy link
Member

rgommers commented Oct 7, 2021

It'd be great to come to a conclusion here.

If the GPU buffer is intercepted by a CPU library, we obviously would get a segfault. But at least for mpi4py this is a desired behavior

I dunno, I can't imagine any situation where you'd prefer a segfault over an exception with a clear message? Unless I'm misunderstanding, you could make a mistake as a user here and pass a CUDA buffer like that to a CPU-only library, and you get a segfault. So I'd say that's off-label buffer protocol usage that is used at ones' own risk but should certainly not be recommended in a standard.

The buffer protocol could be made properly device-aware, but it takes ~4 years before it's landed in Python versions that can be used as the minimum supported Python version for a library. So I'm personally not that interested in investing time there.

We do have to make a decision on the original "remove from_dlpack" proposal in this issue. My order of preference would be:

  1. Leave things as they are now.
  2. Keep from_dlpack and remove support of DLPack from asarray
  3. Remove buffer protocol support completely, in addition to (2)
  4. Only have asarray which supports both DLPack and the buffer protocol (@shoyer's initial proposal).
  5. Add a separate from_buffer

@kgryte
Copy link
Contributor

kgryte commented Nov 3, 2021

Per the consortium meeting held on 21 October 2021, the preferred option was 2, keeping from_dlpack and removing support of DLPack from asarray.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API. topic: DLPack DLPack.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants