-
Notifications
You must be signed in to change notification settings - Fork 47
[DLPack] Add inline C++ tests for torch DLPack Exchange API protocol #111
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
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.