gemini-code-assist[bot] commented on code in PR #315:
URL: https://github.com/apache/tvm-ffi/pull/315#discussion_r2592718904
##########
python/tvm_ffi/cython/function.pxi:
##########
@@ -740,12 +740,12 @@ cdef int TVMFFIPyArgSetterFactory_(PyObject* value,
TVMFFIPyArgSetter* out) exce
# as a member variable
out.func = TVMFFIPyArgSetterFFIObjectProtocol_
return 0
- if os.environ.get("TVM_FFI_SKIP_C_DLPACK_EXCHANGE_API", "0") != "1":
+ if os.environ.get("TVM_FFI_SKIP_DLPACK_C_EXCHANGE_API", "0") != "1":
# Check for DLPackExchangeAPI struct (new approach)
# This is checked on the CLASS, not the instance
- if hasattr(arg_class, "__c_dlpack_exchange_api__"):
+ if hasattr(arg_class, "__dlpack_c_exchange_api__"):
out.func = TVMFFIPyArgSetterDLPackExchangeAPI_
- _get_dlpack_exchange_api(arg_class.__c_dlpack_exchange_api__,
&(out.c_dlpack_exchange_api))
+ _get_dlpack_exchange_api(arg_class.__dlpack_c_exchange_api__,
&(out.dlpack_c_exchange_api))
return 0
Review Comment:
This change removes the check for the legacy `__c_dlpack_exchange_api__`
attribute, which breaks backward compatibility for tensor-like objects from
other libraries that have not yet adopted the new `__dlpack_c_exchange_api__`
attribute. To maintain compatibility as stated in the PR description, both
attributes should be checked.
```
if hasattr(arg_class, "__dlpack_c_exchange_api__"):
out.func = TVMFFIPyArgSetterDLPackExchangeAPI_
_get_dlpack_exchange_api(arg_class.__dlpack_c_exchange_api__,
&(out.dlpack_c_exchange_api))
return 0
elif hasattr(arg_class, "__c_dlpack_exchange_api__"):
out.func = TVMFFIPyArgSetterDLPackExchangeAPI_
_get_dlpack_exchange_api(arg_class.__c_dlpack_exchange_api__,
&(out.dlpack_c_exchange_api))
return 0
```
##########
python/tvm_ffi/_optional_torch_c_dlpack.py:
##########
@@ -152,8 +166,7 @@ def load_torch_c_dlpack_extension() -> Any: # noqa:
PLR0912, PLR0915
# Create a PyCapsule from the pointer
capsule = _create_dlpack_exchange_api_capsule(func())
# Set the DLPackExchangeAPI pointer on the class
- setattr(torch.Tensor, "__c_dlpack_exchange_api__", capsule)
-
+ setattr(torch.Tensor, "__dlpack_c_exchange_api__", capsule)
Review Comment:
The JIT compilation path for the torch c-dlpack extension only sets the new
`__dlpack_c_exchange_api__` attribute. For backward compatibility, it should
also set the legacy `__c_dlpack_exchange_api__` attribute, similar to how it's
done in `addons/torch_c_dlpack_ext/torch_c_dlpack_ext/core.py`. This ensures
that consumers relying on the old attribute name continue to work when the
extension is JIT-compiled. Also, calling `func()` once and storing its result
is more efficient.
```suggestion
ptr_as_int = func()
capsule = _create_dlpack_exchange_api_capsule(ptr_as_int)
# Set the DLPackExchangeAPI pointer on the class
setattr(torch.Tensor, "__dlpack_c_exchange_api__", capsule)
setattr(torch.Tensor, "__c_dlpack_exchange_api__", ptr_as_int)
```
--
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]
