You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/12/02 14:57:53 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Lunderberg opened a new pull request, #13539:
URL: https://github.com/apache/tvm/pull/13539
Previously, the FFI would automatically convert all instances of `tvm::runtime::DataType` to a string for FFI usage. This was an unnecessary step of formatting/parsing to every FFI call with `tvm::runtime::DataType` arguments, and resulted in duplicate parsing/formatting implementations in C++ and Python.
This commit updates the FFI to pass `tvm::runtime::DataType` directly, using the existing `TVMArgTypeCode::kTVMDataType` type code. The `tvm.DataType` wrapper class is updated with additional methods for backwards compatibility (e.g. `"float" in dtype`, `bits = int(dtype[-2:])`), which can be phased out over time.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
tqchen commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1365569792
I put a bit more thoughts into this, here is one possible proposal that might be interesting.
We can make runtime::DataType an ObjectRef. This way most of the dtype can naturally be exposed as TVMObject which have similar kind of semantics defined. Then it would be natural to have `Optional[DataType]` and place data type along with other object containers such as Map and Array.
We can still preserve the overloading behavior like @Lunderberg mentioned. To maintain compatibility, we can preserve DLDataType convention, and allow auto convert to runtime::DataType. RPC will pass DataType as DLDataType.
When we return DataType into the FFI, it now directly becomes an Object just like String. One possible concern is the additional memory alloca overhead of DataType object. That is unlikely would become an issue, in case it is, we can have static table of common ones(like i32, i64, f32) and return the same tabled object.
The benefit on the other hand is we can mix data type freely with containers, optional and other things.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] wrongtest-intellif commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
wrongtest-intellif commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1338793250
Another concern is that very often we use comparation with string like `if xxxx.dtype == "int32"`, would be great if we do not need to modify these applications.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tvm-bot commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1335372002
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1335715433
@tqchen Absolutely agreed, user-friendliness and consistency with other APIs should not be broken. The primary goal was to see whether the developer experience for internal APIs could be improved without sacrificing the end-user experience. Being able to pass strings as arguments that expect a `DataType` is useful functionality that I wouldn't want to remove. However, passing a `DataType` as an argument and receiving a `str` result is less useful, especially when it results in many duplicated instances of ad-hoc parsing of the string.
Regarding numpy specifically, the updated `tvm.DataType` would be usable in any numpy API. Numpy supports custom objects as dtypes by checking for a `arg.dtype` attribute. By implementing this attribute [here](https://github.com/apache/tvm/pull/13539/files#diff-1a640415b179ec94fe877eef5ebb47935b21ec3780a91ac5184200e6104e7180R240), the compatibility is maintained.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
tqchen commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1335419855
The main reason why DataType remained as string in the python side also have some UX side of consideration.
Returning `X.dtype` being string makes it compatible with the numpy APIs and most of the data type passing through the python interface. We would need to put a bit more thoughts if we want to rolling it out explicitly
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1335375110
After going down a rabbit hole trying to figure out why a function that accepted an optional `DataType` was being passed an empty string instead of `None`, I was curious whether it was possible to avoid the `DataType -> str -> DataType` conversions that occur in each FFI call.
This is a very experimental PR, barely at a "proof of concept", and I expect this to fail all cython usages, Java, and Rust tests.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1370071611
@tqchen If possible, I think that would be the ideal. When I was experimenting with it, the DataType seems very intertwined with the FFI, and so I wasn't sure whether that would break things further. I like the idea of being able to use all the normal TVM containers with `DataType`.
As I'm thinking on it, the `Optional[DataType]` would be very useful for separating out two use cases of `DataType::Void()`. Currently, it is sometimes used as "this expression has `void` type", and sometimes used as "this expression has unknown type". If `Optional[DataType]` is supported, then the former could be written as `DataType::Void()`, while the latter is written as `NullOpt`.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #13539: [Draft][Experimental][FFI] Remove string conversion of tvm::runtime::DataType
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #13539:
URL: https://github.com/apache/tvm/pull/13539#issuecomment-1345263498
@wrongtest-intellif Absolutely agreed. The `__eq__` comparison was the first one I added, specifically so that it could convert arguments from string to `DataType` whenever a `my_array.dtype == 'float32'` comparison is done.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org