[WIP] doc: Tensor and DLPack #363
Conversation
Summary of ChangesHello @junrushao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces extensive documentation for TVM-FFI's tensor handling, centralizing around the DLPack standard. It aims to provide clear guidance on tensor types, ownership, memory layouts, and ABI conversions, along with practical API usage and integration strategies for various developer roles, ensuring a consistent and robust approach to tensor management within the TVM-FFI ecosystem. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
junrushao
force-pushed
the
2025-12-23/doc-tensor
branch
from
6ced0e7 to
c44af20
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation for Tensor and DLPack in TVM-FFI, which is a great addition for developers. The changes also include a minor documentation update in overload.h and adding the new documentation page to the main index. My review primarily focuses on improving the clarity and grammatical correctness of the new tensor.rst documentation file, with several suggestions to fix typos and rephrase sentences for better readability.
| View (non-owning) | ||
| A "header" that describes a tensor but does not own the tensor's memory. When the consumer | ||
| receives a view, it must respect that the producer owns the underlying storage and decides its | ||
| lifetime, and use the view can only be used when the producer guarantees it remains valid. |
There was a problem hiding this comment.
There's a grammatical error in this sentence. The phrase 'use the view can only be used' is redundant. It should be corrected for clarity.
| lifetime, and use the view can only be used when the producer guarantees it remains valid. | |
| lifetime, and the view can only be used when the producer guarantees it remains valid. |
|
|
||
| .. tip:: | ||
|
|
||
| Python side, only :py:class:`tvm_ffi.Tensor` exists. It is strictly follows DLPack semantics for interop, and can be converted to PyTorch via :py:func:`torch.from_dlpack`. |
There was a problem hiding this comment.
There's a minor grammatical error here. The verb 'follows' should not be preceded by 'is'.
| Python side, only :py:class:`tvm_ffi.Tensor` exists. It is strictly follows DLPack semantics for interop, and can be converted to PyTorch via :py:func:`torch.from_dlpack`. | |
| Python side, only :py:class:`tvm_ffi.Tensor` exists. It strictly follows DLPack semantics for interop, and can be converted to PyTorch via :py:func:`torch.from_dlpack`. |
| ~~~~~~~~~~~~~~ | ||
|
|
||
| :c:struct:`DLTensor` is a **view** and the fundamental tensor descriptor. It describes the device | ||
| the tensors lives, its shape, dtype, and data pointer. It does not own the underlying data. |
There was a problem hiding this comment.
There's a subject-verb agreement error. 'tensors' is plural, but 'lives' is singular. Given the context of describing a single tensor, 'tensor lives' is more appropriate.
| the tensors lives, its shape, dtype, and data pointer. It does not own the underlying data. | |
| the tensor lives, its shape, dtype, and data pointer. It does not own the underlying data. |
| the tensors lives, its shape, dtype, and data pointer. It does not own the underlying data. | ||
|
|
||
| The **managed tensor** is :c:struct:`DLManagedTensorVersioned`, or its legacy counterpart | ||
| :c:struct:`DLManagedTensor`. It wraps an non-owning :c:struct:`DLTensor` descriptor with some |
There was a problem hiding this comment.
There's a grammatical error here. 'an' should be used before words starting with a vowel sound. 'non-owning' starts with a consonant sound, so 'a' should be used instead.
| :c:struct:`DLManagedTensor`. It wraps an non-owning :c:struct:`DLTensor` descriptor with some | |
| :c:struct:`DLManagedTensor`. It wraps a non-owning :c:struct:`DLTensor` descriptor with some |
| extra fields, | ||
|
|
||
| * a ``deleter(self)`` callback, the cleanup callback consumer uses to release ownership when done with the tensor, and | ||
| * an opaque ``manager_ctx`` handle used by by the producer to store additional information. |
There was a problem hiding this comment.
There's a typo in this line. The word 'by' is repeated.
| * an opaque ``manager_ctx`` handle used by by the producer to store additional information. | |
| * an opaque ``manager_ctx`` handle used by the producer to store additional information. |
| :cpp:class:`TensorObj <tvm::ffi::TensorObj>` lives on the heap, contains a reference counter and a :c:struct:`DLTensor` descriptor embedded inside it. | ||
| Once the reference count drops to zero, the cleanup logic deallocates both the descriptor, and returns the ownership of the underlying data buffer. | ||
|
|
||
| :cpp:class:`Tensor <tvm::ffi::Tensor>` is the recommended interface for passing around managed tensors, and use owning tensors only when you need one or more of the following: |
There was a problem hiding this comment.
The phrasing of this sentence is a bit awkward. It can be improved for better readability by splitting it into two sentences.
| :cpp:class:`Tensor <tvm::ffi::Tensor>` is the recommended interface for passing around managed tensors, and use owning tensors only when you need one or more of the following: | |
| :cpp:class:`Tensor <tvm::ffi::Tensor>` is the recommended interface for passing around managed tensors. Use owning tensors only when you need one or more of the following: |
| What Tensor is not | ||
| ~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| TVM-FFI is not a tensor library. While presenting a unified standardized representation to tensors, |
There was a problem hiding this comment.
The preposition 'to' is not idiomatic here. 'for' would be more appropriate in this context.
| TVM-FFI is not a tensor library. While presenting a unified standardized representation to tensors, | |
| TVM-FFI is not a tensor library. While presenting a unified standardized representation for tensors, |
1 participant
No description provided.