-
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
[CMSIS-NN] Moved TFLite model making to common area #10939
Conversation
c13908b
to
ef3c85a
Compare
cc: @manupa-arm @Mousius @grant-arm @lhutton1 for code review. |
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 @ashutosh-arm, overall I think moving these utilities to a common place is useful :) However, it would be nice to make the utilities generic so that any TFLite function could be used rather than just convolution. In the microNPU tests we use a similar set of utilities such as generate_ref_data_tflite
and get_tflite_graph
(in test_ethosu/infra.py
). I wonder if we could unify these approaches?
Thanks @lhutton1. Since there are different types of test graphs in use by different backends, I got a lot of conflicts/failures the first time getting it right. So, I have added the infrastructure for now and we can keep moving individual tests one after the other gradually if its okay with you. For now, I would just like to add this infrastructure, but the idea is to use this common implementation for all tests eventually. |
Change-Id: Ic4dbc1919ff0b481c05daf7e57cf9b055c714c9c
Change-Id: I7a520beec9c244e9c790d3e82733c2fb476f7e5e
Change-Id: Iefe58dd321efae6eae26cd54a31c5923d0f1e32b
ef3c85a
to
f2a3484
Compare
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 @ashutosh-arm apologies just catching up on this now, I left a couple of comments below
So, I have added the infrastructure for now and we can keep moving individual tests one after the other gradually if its okay with you. For now, I would just like to add this infrastructure, but the idea is to use this common implementation for all tests eventually.
Yeah looking at the subtle differences, it might be a bit too much to tackle right now, it would be cool to see this happen in the future though to help reduce the duplication!
python/tvm/relay/testing/tflite.py
Outdated
self.dtype_dict = {} | ||
|
||
@tf.function | ||
def conv2d_single_function(self, ifm_tensor, args): |
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 feel like creation of this function should be part of the test itself so its clear what's being tested, rather than hidden away in common testing utilities
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.
Yeah, I was thinking about it too. But wanted to wait until I had seen more cases that might be potential users of it. The problem with exposing this function is that the end user could chain this with other functions and would expect create_model() to work, but it might not because of the tf.function pragmas.
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.
As discussed offline, we will keep single layer creation as part of the infra to avoid duplication of this bit across multiple tests. If necessary, to include complex cases, user can write their own @tf.function and still create a model using this infra.
Change-Id: I7fbf6a5a2163c1fada49477f86d84f1bc09bd57c
bce3d56
to
fe4ab87
Compare
Change-Id: If1fb8bb09c538c04e333ccab65a20cff247a504d
fe4ab87
to
13eb13f
Compare
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 for the improvements @ashutosh-arm, LGTM!
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.
LGTM!
* [CMSIS-NN] Moved TFLite model making to common area Change-Id: Ic4dbc1919ff0b481c05daf7e57cf9b055c714c9c * Fixed lint issues with tensorflow import Change-Id: I7a520beec9c244e9c790d3e82733c2fb476f7e5e * Resolved merge conflict with main Change-Id: Iefe58dd321efae6eae26cd54a31c5923d0f1e32b * Made TFLite layer creation explicit Change-Id: I7fbf6a5a2163c1fada49477f86d84f1bc09bd57c * Lint fix: added a missing docstring Change-Id: If1fb8bb09c538c04e333ccab65a20cff247a504d
Moving out TFLite model creation from CMSIS-NN area to common area of python/tvm/testing.