gemini-code-assist[bot] commented on code in PR #301:
URL: https://github.com/apache/tvm-ffi/pull/301#discussion_r2581854063
##########
python/tvm_ffi/cython/tensor.pxi:
##########
@@ -131,12 +131,14 @@ cdef inline int _from_dlpack_universal(
# as of most frameworks do not yet support v1.1
# move to false as most frameworks get upgraded.
cdef int favor_legacy_dlpack = True
+ cdef const DLPackExchangeAPI* exchange_api = NULL
Review Comment:
Initializing `exchange_api` to `NULL` is a good defensive programming
practice. It ensures the pointer has a known, safe state before it is
potentially assigned a value, which can help prevent dereferencing
uninitialized pointers.
##########
python/tvm_ffi/cython/tensor.pxi:
##########
@@ -131,12 +131,14 @@ cdef inline int _from_dlpack_universal(
# as of most frameworks do not yet support v1.1
# move to false as most frameworks get upgraded.
cdef int favor_legacy_dlpack = True
+ cdef const DLPackExchangeAPI* exchange_api = NULL
if hasattr(ext_tensor, "__c_dlpack_exchange_api__"):
try:
+ _get_dlpack_exchange_api(ext_tensor.__c_dlpack_exchange_api__,
&exchange_api)
return _from_dlpack_exchange_api(
Review Comment:
Passing the `exchange_api` pointer directly after it has been properly
retrieved and typed is a cleaner and safer way to invoke
`_from_dlpack_exchange_api`. This aligns with the function's updated signature
and improves overall code correctness.
##########
python/tvm_ffi/cython/tensor.pxi:
##########
@@ -131,12 +131,14 @@ cdef inline int _from_dlpack_universal(
# as of most frameworks do not yet support v1.1
# move to false as most frameworks get upgraded.
cdef int favor_legacy_dlpack = True
+ cdef const DLPackExchangeAPI* exchange_api = NULL
if hasattr(ext_tensor, "__c_dlpack_exchange_api__"):
try:
+ _get_dlpack_exchange_api(ext_tensor.__c_dlpack_exchange_api__,
&exchange_api)
Review Comment:
This change to use `_get_dlpack_exchange_api` for retrieving the
`DLPackExchangeAPI` pointer is a significant improvement. It provides a more
robust and type-safe mechanism compared to direct casting from `long long`,
directly addressing the issue described in the pull request and making the code
more maintainable.
##########
python/tvm_ffi/cython/tensor.pxi:
##########
@@ -105,7 +105,7 @@ cdef inline int _from_dlpack_versioned(
cdef inline int _from_dlpack_exchange_api(
- object ext_tensor, DLPackExchangeAPI* exchange_api, int require_alignment,
+ object ext_tensor, const DLPackExchangeAPI* exchange_api, int
require_alignment,
Review Comment:
Adding the `const` qualifier to `DLPackExchangeAPI* exchange_api` is a good
practice. It clearly indicates that the `_from_dlpack_exchange_api` function
will not modify the `DLPackExchangeAPI` structure it points to, improving code
clarity and preventing accidental side effects.
##########
tests/python/test_dlpack_exchange_api.py:
##########
@@ -28,7 +28,7 @@
# Import tvm_ffi to load the DLPack exchange API extension
# This sets torch.Tensor.__c_dlpack_exchange_api__
- import tvm_ffi # noqa: F401
+ import tvm_ffi
Review Comment:
Removing the `# noqa: F401` comment is appropriate here, as `tvm_ffi` is now
actively used within the file due to the addition of the new test case. This
improves code cleanliness by removing unnecessary linting directives.
##########
tests/python/test_dlpack_exchange_api.py:
##########
@@ -222,5 +222,14 @@ def test_dlpack_exchange_api() -> None:
mod.test_dlpack_api(tensor, api_ptr, torch.cuda.is_available())
[email protected](not _has_dlpack_api, reason="PyTorch DLPack Exchange API
not available")
+def test_from_dlpack_torch() -> None:
+ # Covers from_dlpack to use fallback fastpath
+ tensor = torch.arange(24, dtype=torch.float32).reshape(2, 3, 4)
+ tensor_from_dlpack = tvm_ffi.from_dlpack(tensor)
+ assert tensor_from_dlpack.shape == tensor.shape
+ assert tensor_from_dlpack.dtype == tvm_ffi.float32
Review Comment:
The addition of `test_from_dlpack_torch` is a valuable enhancement to the
test suite. It specifically targets the `from_dlpack` functionality with
PyTorch tensors, ensuring that the fallback fastpath works correctly and
providing better coverage for the DLPack exchange API integration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
