-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[RUNTIME][NDArray] Allowing External Libraries to Subclass NDArrays #2613
Conversation
(Outdated, don't read) Added a type trait A subclass of NDArray should do the following things in their own codebase, without modifying TVM.
|
(Outdated, don't read) Two issues with this PR
|
We do not need a virtual destructor because deleter is called. refer to external type on how to enable automatic explicit conversion https://github.com/dmlc/tvm/blob/master/include/tvm/runtime/packed_func.h#L456 |
(Outdated, don't read) @tqchen On the C++ side, automatic implicit conversion has already been enabled via overriding TVMValue::operator T() in external developers' codebase, here, so why we need explicit conversion on the C++ side? |
(Outdated, don't read) Or you mean moving the |
I am not sure if operator T() can be overridden in the external codebase |
(Outdated, don't read) @tqchen Updated accordingly. A potential issue of current |
@junrushao1994 that could indeed be an issue. In that case, let us just use AsNDArray()(rename AsSubNDArray->AsNDArray). Sorry for not noticing this before. |
(Outdated, don't read) Weird CI fails. I can at least pass "BUILD: CPU" on my machine. UPDATE: Addressed via rebasing to master. |
The error seems to be related to cython, try to directly use NDArrayBase as opposed to NDArray |
@tqchen @were @reminisce Could you kindly do another round of review? Thanks! |
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.
Implementation looks good, some final comments
This PR allows an external library to subclass TVM's NDArray infrastructure without too much work.
It introduces a type trait
array_type_index<T>::code
. For TVM's NDArray itself, the trait is 0; For irrelevant classes, the trait is -1; Any external subclass of TVM NDArray should override it to be a positive int32 number.To use this extension, an external library should
array_type_index
for the NDArray.tvm.nd.NDArrayBase
, define the class attribute_array_type_index
consistent to the C++ type trait, and register the subclass usingtvm.register_extension
.CC: @tqchen, @were, @reminisce