[DLPack] Add inline C++ tests for torch DLPack Exchange API protocol#111
[DLPack] Add inline C++ tests for torch DLPack Exchange API protocol#111tqchen merged 6 commits intoapache:mainfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of inline C++ tests for the PyTorch DLPack Exchange API, which is a valuable addition for ensuring the correctness of this low-level protocol. The tests are well-structured and cover various aspects of the API, including versioning, function pointer validity, allocation, and conversions between PyTorch tensors and DLPack tensors. My review focuses on improving code maintainability and consistency within the new test file. I've suggested refactoring the tests to use a pytest fixture to reduce boilerplate code, and pointed out a minor inconsistency in an assertion message.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive inline C++ unit tests for the torch.Tensor.__c_dlpack_exchange_api__ protocol, which is a great addition for ensuring the robustness of the DLPack exchange mechanism. The tests are well-structured and cover various aspects of the API. I've identified a few potential resource leaks in the C++ test code due to exceptions and suggest using RAII patterns like std::unique_ptr to make the resource management more robust. Overall, this is a valuable contribution.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive inline C++ tests for the torch.Tensor.__c_dlpack_exchange_api__, which is a great addition for ensuring the correctness of the DLPack exchange protocol implementation. The tests are well-structured, covering various functions of the API. The update in tensor.pxi to use DLPack version constants instead of hardcoded values is also a good improvement for maintainability. I have one suggestion to enhance code consistency within the new C++ test code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
As a followup to PR #96, this PR adds comprehensive unit tests for
torch.Tensor.__c_dlpack_exchange_api__using inline C++. It validates PyTorch's implementation of theDLPackExchangeAPIstruct-based fast exchange protocol.Unlike the ctypes-based tests, these tests use
torch.utils.cpp_extension.load_inlineto avoid GIL release issueswhen calling
THPVariable_Wrap.