Improve docs for from_dlpack and to_dlpack
#70437
Conversation
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit ec8a29e (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
Hmm, the docs preview job has been removed and https://hud.pytorch.org/pr/70437 wants so many permissions that I can't access it. This should be good to go, but I can't verify (local KaTeX has a problem for me). |
|
It mentions "old-style DLPack capsule", but is there a "new-style DLPack capsule"? I think it'd be a frequent question |
the "new style DLPack" is to just pass a tensor directly. It's still the same capsule, it's now just under the hood, so better UX and no change of consuming it twice. Could you propose a rephrase? |
|
Well, I still don't understand the meaning of old/new style :( We often need a DLPack tensor to develop an interoperable C interface (not only for exchanging tensors between NumPy / Deep Learning frameworks), like I did here: https://github.com/vadimkantorov/dotnetdlpack From phrasing |
|
To explain:
It should be, because it's pretty much useless. The name is derived from |
|
I think I understand now, but it should be explicitly explained what is the new and old style, or remove the refernece to these concepts at all and just explain that two kinds of arguments are accepted. In my case, I needed DLPack not for interop between frameworks but to create a .NET wrapper consuming DLPack API using libtorch. So I needed to understand exactly what is the capsule and what is the field name. All this is better be explained. There was an issue in DLPack complaining that the name of the field inside the capsule is not documented: dmlc/dlpack#51. It would be better if PyTorch documented it as well or referenced some ground-truth documentation. It may also be nice to include a snippet of manual working with these capsules. This example is very useful if you are not a framework developer. E.g. I had to figure this out in https://github.com/vadimkantorov/pydlpack and figuring out the proper semantics of DLTensor deallocator and its interaction with capsule is quite non-trivial. |
That belongs in the DLPack docs, it doesn't make sense to explain the kind of details you are mentioning here in the PyTorch Python API docs - they are irrelevant to Python users. You are only running into it because you are using C/C++ APIs. The minor catch is that DLPack currently has no docs (outside of inline comments). I plan to address that in H1 2022. For now this is probably the best user-facing DLPack documentation: https://data-apis.org/array-api/latest/design_topics/data_interchange.html#dlpack-support. Most of that should move to DLPack itself in the future.
Then I'd prefer to just tweak the text a bit to remove "old style", and only state that using |
Why not! Just please specify that the tensor object should have a
Yep! And especially no docs on PyCapsule format ("dltensor" field name and deleter semantics). So having at least some notes on what PyTorch assumes of those
Agreed!
A bit disagreeing. I'm running into it very much because DLPack is for doing interop with PyTorch's Python API, and interop with custom C code is a big part of it. |
Okay, interesting. Not quite agreeing that we should include content that is irrelevant for Python users and only needed if you write custom C code, but I understand better what you're doing at least after I had a look at your repo. Side note: NumPy 1.22.0 will have support for DLPack, so your custom code for that will become obsolete - it'll just be |
|
Yeah, I also needed this when I was doing a "framework-agnostic" pure C module for reading audio with ffmpeg: https://github.com/vadimkantorov/readaudio. Basically you get a native DLPack structures from C land and then somehow you need to wrap it into a PyCapsule to have it ingested by NumPy / PyTorch. It was somewhat untrivial to understand how to do it correctly. These "framework-agnostic" C extensions (that can be consumed with NumPy or other framework in absence of PyTorch python/native libraries) can be an interesting usecase in general and would be nice to have a tutorial on something like this. |
Leave `to_dlpack` in the `torch.utils.dlpack` namespace here. It is present in the main namespace as well, but undocumented there and it's not clear if we want to keep it there - see pytorch#70543 (comment)
|
|
||
| .. warning:: | ||
| Only call ``from_dlpack`` once per capsule produced with ``to_dlpack``. | ||
| Behavior when a capsule is consumed multiple times is undefined. |
There was a problem hiding this comment.
Nitpick: technically it's well defined (an error would be raised if the capsule has been consumed with its name changed) long before we settled on __dlpack__, but I'm fine leaving it as is.
|
@mruberry has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples. There may be more to do to make `from_dlpack` more discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or `__array_interface__` support) that `from_dlpack` already provides. Pull Request resolved: #70437 Reviewed By: albanD Differential Revision: D33760552 Pulled By: mruberry fbshipit-source-id: e8a61fa99d42331cc4bf3adfe494cab13ca6d499
Summary: This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples. There may be more to do to make `from_dlpack` more discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or `__array_interface__` support) that `from_dlpack` already provides. Pull Request resolved: #70437 Reviewed By: albanD Differential Revision: D33760552 Pulled By: mruberry fbshipit-source-id: e8a61fa99d42331cc4bf3adfe494cab13ca6d499 (cherry picked from commit 880ad96)
Summary: This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples. There may be more to do to make `from_dlpack` more discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or `__array_interface__` support) that `from_dlpack` already provides. Pull Request resolved: pytorch/pytorch#70437 Reviewed By: albanD Differential Revision: D33760552 Pulled By: mruberry fbshipit-source-id: e8a61fa99d42331cc4bf3adfe494cab13ca6d499 (cherry picked from commit 880ad96)
Summary: This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples. There may be more to do to make `from_dlpack` more discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or `__array_interface__` support) that `from_dlpack` already provides. Pull Request resolved: pytorch/pytorch#70437 Reviewed By: albanD Differential Revision: D33760552 Pulled By: mruberry fbshipit-source-id: e8a61fa99d42331cc4bf3adfe494cab13ca6d499 (cherry picked from commit 880ad96)
Summary: This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples. There may be more to do to make `from_dlpack` more discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or `__array_interface__` support) that `from_dlpack` already provides. Pull Request resolved: pytorch/pytorch#70437 Reviewed By: albanD Differential Revision: D33760552 Pulled By: mruberry fbshipit-source-id: e8a61fa99d42331cc4bf3adfe494cab13ca6d499 (cherry picked from commit 880ad96)
Summary: This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples. There may be more to do to make `from_dlpack` more discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or `__array_interface__` support) that `from_dlpack` already provides. Pull Request resolved: pytorch/pytorch#70437 Reviewed By: albanD Differential Revision: D33760552 Pulled By: mruberry fbshipit-source-id: e8a61fa99d42331cc4bf3adfe494cab13ca6d499 (cherry picked from commit 880ad96)
This moves the warning to the legacy function where it belongs, improves the phrasing, and adds examples.
There may be more to do to make
from_dlpackmore discoverable as a follow-up, because in multiple issues/PR we discovered people wanted new things (e.g., a memoryview-like object, or__array_interface__support) thatfrom_dlpackalready provides.