Skip to content
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

Fix TFLite RESHAPE assert #4320

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Conversation

apivovarov
Copy link
Contributor

@apivovarov apivovarov commented Nov 12, 2019

Recently I found that Reshape op input might have 1 or 2 input tensors.
in TVM code we assert that length is 2 but we only use input_tensor[0].
Looks like the second input tensor was reshape shape in the past. But current TFLite schema which we use r1.13 has ReshapeOptions which contains new_shape:

table ReshapeOptions {
  new_shape:[int];
}

We do not use the second input tensor in TFLite frontend, we use ReshapeOptions instead.

What is more is that I have custom tflite model which has only one input tensor for RESHAPE op.
Current assert for input tensors length makes the compilation fail.
I suggest what we allow input tensors length for RESHAPE operator to be 1 or 2.

We can not change it to just 1 because test ops/models in test_forward.py have 2 input tensors for RESHAPE

@apivovarov
Copy link
Contributor Author

@FrozenGene Can you have a look?

@FrozenGene
Copy link
Member

could we add one unittest?

@u99127
Copy link
Contributor

u99127 commented Nov 13, 2019

In addition I would request a comment in the code about the status in 1.13 and if possible the version of tflite when this changed.

@apivovarov
Copy link
Contributor Author

apivovarov commented Nov 14, 2019

@FrozenGene I tried to prepare tflite file with RESHAPE op having only one input tensor. But it always give me model with two input tensors for RESHAPE op.

Do you know how to force tflite converter to create RESHAPE op with only one tensor?

my models
https://www.dropbox.com/s/n8gg3gt2g3xlmkq/Screenshot%202019-11-13%2016.37.25.png?dl=0

https://www.dropbox.com/s/bhvuhrio277ka82/Screenshot%202019-11-13%2016.40.55.png?dl=0

hand_landmark.tflite model (has only one tensor in RESHAPE input)
https://www.dropbox.com/s/0wq2owq8p4hbw6l/Screenshot%202019-11-13%2016.37.49.png?dl=0

One of the solutions is to add hand_landmark.tflite to the end of tflite/test_forward.py. What you think? It will be good integration test for PRELU and RESHAPE.

@FrozenGene
Copy link
Member

I can not construct it too. A bit strange.

We could add hand_landmark.tflite, however, we should also have unittest of reshape in my opinion.

@apivovarov
Copy link
Contributor Author

apivovarov commented Nov 14, 2019

TVM code does not use the second input tensor in RESHAPE op. From the beginning the assert should just check that input tensors array length is at least 1.
The reason for assert line is to display human readable error message instead of array index out of boundary exception. Because we only use input tensor with index 0 in the code the assert should check that input tensors array length is at least 1.
I'll add hand_landmark.tflite to the tests after your PR to fix PRELU is merged

@u99127
Copy link
Contributor

u99127 commented Nov 14, 2019

I've been noticing differences in the tflite converter graph between 1.13 and 1.14 especially with split and unpack - have either of you guys played with multiple versions of the tfliteconverter ?

@apivovarov
Copy link
Contributor Author

@FrozenGene All checks have passed - Can you approve?

@yzhliu yzhliu merged commit 331f6fd into apache:master Nov 19, 2019
@yzhliu
Copy link
Member

yzhliu commented Nov 19, 2019

Thanks @apivovarov @FrozenGene @u99127

zxy844288792 pushed a commit to zxy844288792/tvm that referenced this pull request Nov 26, 2019
yongwww pushed a commit to neo-ai/tvm that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants