Closed tirthasheshpatel closed 1 year ago
An ABI break may also be an opportunity to reexamine the signed/unsignedness of all DLTensor
members (PR #102).
Thanks for the help so far everyone! I think the discussion on #34 has settled on adding the __dlpack_info__
method which returns the max version supported by the producer. I have updated the PR with the new spec (that includes __dlpack_info__
). I haven't changed capsule renaming yet. @leofang Let us know if remaining the capsule makes sense to you when the new ABI is exported (as mentioned in #34, it provides some extra protection without breaking backward compatibility).
I originally was in favor of capsule renaming, but I think it is not needed given the consensus around the protocol. While it does provide another layer of safety, I think it is redundant.
There are still a number of things I find confusing about the Python specification. Some of them are related to this specific PR. Some of them are things that are unclear in general.
stream
argument of __dlpack__()
can presumably be used to pass a CUDA stream object. Is the interface specific to CUDA? If so, the documentation should probably say so.CUDA streams are not a natural representation in CUDA. Even in C++, they are simply a pointer to an opaque NVIDIA data structure. Are they passed as capsules, uintptr_t cast to a python int, etc.? The documented interface lacks type annotations to make this clear.
The document mentions a new __dlpack_info__
function but does not provide a clear type-checkable signature of this new function. This is a recipe for inconsistencies. I recommend providing an example raw python version with MyPy-style type signature as an implementation guide.
What are the requirements on the version
attribute of __dlpack__
? Is it a Python int
? If so, this could be stated in the type signature.
The document mentions a new
__dlpack_info__
function but does not provide a clear type-checkable signature of this new function. This is a recipe for inconsistencies. I recommend providing an example raw python version with MyPy-style type signature as an implementation guide.What are the requirements on the
version
attribute of__dlpack__
? Is it a Pythonint
? If so, this could be stated in the type signature.
Yeah, making the signatures explicit should make it easier to understand the spec, thanks.
- The
stream
argument of__dlpack__()
can presumably be used to pass a CUDA stream object. Is the interface specific to CUDA? If so, the documentation should probably say so.
stream
is an integer; the ID of the GPU device where the data is present. It is specific to CUDA and ROCm (and the devices that use a stream mechanism). This is explicitly stated in the semantics section of the python specification. Is it not clear enough?
I originally was in favor of capsule renaming, but I think it is not needed given the consensus around the protocol. While it does provide another layer of safety, I think it is redundant.
Okay, I have removed capsule renaming now.
Also, I have added release notes and updated the DLPack diagram with the new structs. This PR is ready for review again from my side.
@tirthasheshpatel just realized that we checkedin the png image to the repo in a previous path. Is it possible to checkin the image to a different location/repo? On one hand we would like to keep repo self-contained, on the other hand, it can be a bit annoying for a repo to contain a rich history of binaries through multiple revisions.
One approach is to send a PR to a separate repo instead (we used https://github.com/dmlc/web-data) for some of that purposes then link to via https://gitcontent
One approach is to send a PR to a separate repo instead (we used https://github.com/dmlc/web-data) for some of that purposes then link to via https://gitcontent
Sounds good! I will remove the image here and propose a PR on dmlc/web-data.
stream is an integer; the ID of the GPU device where the data is present. It is specific to CUDA and ROCm (and the devices that use a stream mechanism). This is explicitly stated in the semantics section of the python specification. Is it not clear enough?
I actually find this part of the documentation quite confusing (That said, I am only familiar with the CUDA part and can't speak about ROCm).
There are three concepts in CUDA that play a role here:
Of these three, only the CUDA device ID is representable as an integer. Both CUDA context and CUDA stream are represented by opaque handles to a proprietary data structures, in both CUDA runtime and driver APIs. Essentially a void*
pointer.
I think that there is I think a fundamental confusion between the word "stream" and "CUDA context" or "CUDA device ID" in the DLPack documentation. I suspect that will you want to call this "device ID" and not "stream" and also establish the convention that CUDA memory allocations are assumed to be located in the primary context associated with the associated CUDA device ID.
There is one situation in which the notion of a stream would make sense to me: if the tensor is not yet computed and the __dlpack__
function needs to perform a kernel launch to actually compute the data that will then be wrapped. In that case, we might want this computation to be ordered with respect to other computation performed by the caller (which might be issued to the same context+stream pair). However, in this case, providing the stream as an int
would not be a suitable interface.
closed in favor of #113
We would like to thanks @tirthasheshpatel for your effort on bringing in this change onward
Closes #34 Supersedes #72
This PR adds version info to the
DLTensor
struct. This is under discussion in #34 (see this comment). Since this is an ABI break, the ABI version has been incremented to 2 and the DLPack Version has been bumped to v0.7.0. This also updates the Python spec with a detailed upgrade policy and future ABI compatibility. The upgrade policy has been designed such that the libraries that want to keep supporting (exporting and importing) the old ABI or support multiple DLPack and ABI versions can do so without breaking compatibility with the old Python API.