-
Notifications
You must be signed in to change notification settings - Fork 156
Update exchange API to be capsule following new convention #180
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
base: main
Are you sure you want to change the base?
Conversation
|
cc @Kathryn-cat |
|
RFC thread #179 |
This PR updates the exchange API to be capsule following the new convention. Note that the exchange API python recommendation is not yet updated on the array-api side and so far there hasn't yet been downstream implementation, so this change should be safe in generally.
|
Array api spec PR data-apis/array-api#984 |
|
Sorry for delay, will try to finish my review in a day or two! |
leofang
left a comment
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.
Thanks, @tqchen! LGTM overall, left some questions that we should address before merging.
|
|
||
| .. code-block:: C | ||
| PyObject *api_obj = type(tensor_obj).__dlpack_c_exchange_api__; // as C code. |
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.
I am confused. Is this valid C code? Shouldn't we use PyObject_HasAttrString or alike to query if the Python attribute exists with this type?
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.
This is not valid C code, but as a pesudo cython like code to query the types, happy to change to C code here
| A reference implementations of the C Exchange API in frameworks: | ||
|
|
||
|
|
||
| * PyTorch: `C++ <https://github.com/pytorch/pytorch/blob/main/torch/csrc/Module.cpp#L692>`__ |
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.
Let's link to a particular commit (copy permalink) in case the upstream drifts away.
|
|
||
|
|
||
| * PyTorch: `C++ <https://github.com/pytorch/pytorch/blob/main/torch/csrc/Module.cpp#L692>`__ | ||
| * Paddle: `C++ <https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/fluid/pybind/pybind.cc#L856>`__ |
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.
Ditto
| direct interaction between the array framework PyObject* and DLPack, | ||
| as a result it is harder to implement the C Exchange API through ctypes (because | ||
| ctypes releases GIL by default and sometimes in non-free-threading environment, | ||
| GIL is needed to interact with the Python C API). |
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.
| * | ||
| * PyObject *api_obj = type(tensor_obj).__c_dlpack_exchange_api__; // as C-code | ||
| * MyDLPackExchangeAPI *api = PyLong_AsVoidPtr(api_obj); | ||
| * PyObject *api_obj = type(tensor_obj).__dlpack_c_exchange_api__; // as C-code |
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.
Ditto, now that I take a closer look, this seems odd to me
Co-authored-by: Leo Fang <[email protected]>
Co-authored-by: Leo Fang <[email protected]>
This PR updates the exchange API to be capsule following the new convention. Note that the exchange API python recommendation is not yet updated on the array-api side and so far there hasn't yet been downstream implementation, so this change should be safe in generally.