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

[TFLite] Support TFLite FP32 Relay frontend. #2365

Merged
merged 9 commits into from Jan 23, 2019

Conversation

@FrozenGene
Copy link
Member

FrozenGene commented Jan 3, 2019

This is the first PR of #2351 to support importing exist quantized int8 TFLite model. The base version of Tensorflow / TFLite is 1.12.

This PR support TFLite FP32 Relay frontend. The input layout is NCHW, which is not the same as TFLite's NHWC layout, because TVM's default supporting layout is NCHW and support NCHW very well like AutoTVM on ARM CPU. Don't worry, I have completed the translation layout work in the TFLite FE. Currently, it could run Mobilenet V1 successfully.

Prerequisite of Python packages:

  • flatbuffers
  • tflite

You could install flatbuffers using pip3 install flatbuffers

For the tflite package, as far as I know, we haven't pip package until now. You could choose two ways to install:

  1. git clone my generated tflite package: https://github.com/FrozenGene/tflite
  2. generate it by yourself. You could find the instructions how to do it in previous repo's README.

@tqchen To run the TFLite frontend test, we should have flatbuffers and tflite packages in our CI machine. Please let me know if you have any questions.

After this PR is merged, I will write the tutorial of how to use TFLite frontend like previous frontends do.

@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 3, 2019

Seems that our CI Tensorflow has low version. (i.e. smaller than 1.12). It report error:
AttributeError: module 'tensorflow.contrib.lite.python.lite' has no attribute 'TFLiteConverter'
@tqchen Could we upgrade our Tensorflow version to 1.12? I think this shouldn't have side-effect on our Tensorflow frontend. If not correct, please correct me @srkreddy1238 .

However, flatbuffers / tflite package should be installed in our CI machine as said before.

@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 4, 2019

@FrozenGene no issues to upgrade with TF 1.12.
With these dependencies you could propose another PR for CI recipe to bring in TF upgrade and other dependencies while this PR under review.

@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 4, 2019

@FrozenGene no issues to upgrade with TF 1.12.
With these dependencies you could propose another PR for CI recipe to bring in TF upgrade and other dependencies while this PR under review.

@srkreddy1238 Do you mean I could make a PR to install / upgrade our CI's machine python packages? Even I can git clone and set PYTHONPATH environment? Could you help to give me one example how to do it?

@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 4, 2019

I mean the PR for docker changes to accommodate the dependencies for TFLite.
Ref. https://github.com/dmlc/tvm/blob/master/docker/Dockerfile.ci_cpu for CPU docker changes.
Ref. https://github.com/dmlc/tvm/blob/master/docker/install/ubuntu_install_keras.sh and for the script which installs tensorflow package.
You need to build docker image locally with changes, verify and then propose it for merging and deploy.

@FrozenGene I can help with docker changes if you have difficulty.

@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 4, 2019

I mean the PR for docker changes to accommodate the dependencies for TFLite.
Ref. https://github.com/dmlc/tvm/blob/master/docker/Dockerfile.ci_cpu for CPU docker changes.
Ref. https://github.com/dmlc/tvm/blob/master/docker/install/ubuntu_install_keras.sh and for the script which installs tensorflow package.
You need to build docker image locally with changes, verify and then propose it for merging and deploy.

@FrozenGene I can help with docker changes if you have difficulty.

Yes. I am not familiar with docker. If you could help to install TFLite dependency, that's very nice! Thanks in advance.

@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 4, 2019

@FrozenGene I see tensorflow version is already 1.12.
Or
@tqchen Do we have to rebuild the CI docker image?

 ---> Using cache
 ---> f448a5f23a1e
Step 53/54 : ENV LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:${VULKAN_SDK}/lib
 ---> Using cache
 ---> 2fdae98744fc
Step 54/54 : ENV VK_LAYER_PATH=${VULKAN_SDK}/etc/explicit_layer.d
 ---> Using cache
 ---> 8c35d2ddd51e
Successfully built 8c35d2ddd51e
Successfully tagged tvm.ci_gpu:latest
Running 'pip3 search tensorflow' inside tvm.ci_gpu...
nvidia-docker
mesg: ttyname failed: Inappropriate ioctl for device
Adding group `srk' (GID 1002) ...
Done.
tensorflow (1.12.0)                                      - TensorFlow is an open source machine learning framework for everyone.
  INSTALLED: 1.12.0 (latest)
tensorflow-qndex (0.0.22)                                - tensorflow-qnd x tensorflow-extenteten
tensorflow-plot (0.2.0)                                  - TensorFlow Plot
tensorflow-estimator (1.10.12)                           - TensorFlow Estimator.
@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 4, 2019

@srkreddy1238 I think we should rebuild the docker image. Because I find this latest commit: e8d6d9a, the tensorflow version is 1.10. I have tested tensorflow 1.10, which will raise the same error: AttributeError: module 'tensorflow.contrib.lite.python.lite' has no attribute 'TFLiteConverter'. But tensorflow 1.12 will not.

@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 4, 2019

@FrozenGene
Ref. #2371 & dmlc/web-data#148
Building new docker image with these changes should resolve tflite dependencies.

I simplified TVM CI recipe by making a python package out of your tflite repository and uploaded it under web-data.
I will send a PR to your branch with changes I added in there.

@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 4, 2019

Thanks @srkreddy1238 I have seen your prs, which helps a lot to resolve dependencies, thanks again!

@FrozenGene FrozenGene force-pushed the FrozenGene:tflite_fe branch from f42e326 to 51f4519 Jan 11, 2019
@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 11, 2019

@srkreddy1238 I saw your PR #2371 is merged and rebase to the latest code. However, it raise the same error:

  • module 'tensorflow.contrib.lite.python.lite' has no attribute 'TFLiteConverter'

  • ImportError: The tflite package must be installed

Have we upgraded successfully? or we need some time waiting docker image is updated?

@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 11, 2019

I think docker images should be rebuilt with new changes. cc @tqchen

@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 15, 2019

@tqchen could you help to rebuild docker image? Then I could update related code. Thanks in advance.

@tqchen

This comment has been minimized.

Copy link
Member

tqchen commented Jan 15, 2019

I have updated the ci docker image, please check again

@FrozenGene FrozenGene force-pushed the FrozenGene:tflite_fe branch from 769c1d0 to 945f9c7 Jan 16, 2019
@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 16, 2019

@tqchen Thanks. it works now.

@srkreddy1238 could you help to review this PR?

@tqchen

This comment has been minimized.

Copy link
Member

tqchen commented Jan 18, 2019

@srkreddy1238 @ajtulloch @junrushao1994 @zhreshold @xqdan please help to review this PR when you have time

-------
tensor name in UTF-8 encoding
"""
return subgraph.Tensors(tensor_idx).Name().decode("utf-8")

This comment has been minimized.

Copy link
@junrushao1994

junrushao1994 Jan 18, 2019

Member

@FrozenGene I am a little bit surprised that this could pass python3 CI, because str.decode shouldn't exist in Python3...Any ideas? Is the resulted type of Name() str or bytearray in Python3?

This comment has been minimized.

Copy link
@FrozenGene

FrozenGene Jan 18, 2019

Author Member

The resulted type of Name() is not str, it is bytes() in TFLite. See: https://github.com/FrozenGene/tflite/blob/master/tflite/Tensor.py#L58

This comment has been minimized.

Copy link
@junrushao1994

junrushao1994 Jan 18, 2019

Member

Gotcha. So it is reasonable to me. Thanks!

# 3: N H W C, reshape to N H*W C, transpose to N C H*W
# 4: N H W C, transpose to N C H W
# add more if we need target shapes in future
if output_shape_length == 1 or output_shape_length == 2:

This comment has been minimized.

Copy link
@junrushao1994

junrushao1994 Jan 18, 2019

Member

Ok, I understand the logic here, but I do think it introduces some underlying assumption.

The assumption is: the input of squeeze operator comes out of conv2d with some specific layout, otherwise it will transpose layers crazily.

It doesn't look safe to me, but for now for specific models like MobileNet, it is fine. @tqchen please advise.

This comment has been minimized.

Copy link
@FrozenGene

FrozenGene Jan 18, 2019

Author Member

Yes. When we do handle Reshape or squeeze ops we should be very careful if we handle from input layout NHWC to NCHW. So, this is to obey channel first rule, i.e. C is before H, W but after N. Currently we only handle 1-4D, not N-D. This issue is not special for us, other converters will meet the same issue. Please see Tensorflow to CoreML converter, which has similar logic: https://github.com/tf-coreml/tf-coreml/blob/master/tfcoreml/_shape_sensitive_layers.py#L275

This comment has been minimized.

Copy link
@junrushao1994

junrushao1994 Jan 18, 2019

Member

I feel like it is ok for me for now, but probably we could document it somewhere? DL models in other fields like NLP heavily use squeeze, which may violate our rules and crashes every thing.

This comment has been minimized.

Copy link
@FrozenGene

FrozenGene Jan 18, 2019

Author Member

This is the reason why I comment the rule in code. I think another place we could comment is our TFLite frontend tutorial. Any other suggestions?

This comment has been minimized.

Copy link
@junrushao1994

junrushao1994 Jan 18, 2019

Member

Okay, it looks good to me. @tqchen if you have any suggestions.

This comment has been minimized.

Copy link
@srkreddy1238

srkreddy1238 Jan 19, 2019

Contributor

Layout is specific to vision operators, bringing in layout specific customization into basic transforms will not work for all models. Here it's only mobilenet hence it works, but I doubt it may get more complex when more models are added. A simple NLP model with one LSTM layer will fail these assumptions.

Copy link
Member

junrushao1994 left a comment

Looks good. Future improvement may include

  1. multi subgraph support
  2. documenting how squeeze is handled (see our discussion for details)

In the scope of this PR, I think everything looks good.

@tqchen tqchen assigned srkreddy1238 and unassigned ZihengJiang Jan 18, 2019
@tqchen

This comment has been minimized.

Copy link
Member

tqchen commented Jan 18, 2019

Thanks for the reviews, I will leave this PR's management to @srkreddy1238 :)

python/tvm/relay/frontend/tflite.py Show resolved Hide resolved
python/tvm/relay/frontend/tflite.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/tflite.py Show resolved Hide resolved
python/tvm/relay/frontend/tflite.py Show resolved Hide resolved
# 3: N H W C, reshape to N H*W C, transpose to N C H*W
# 4: N H W C, transpose to N C H W
# add more if we need target shapes in future
if output_shape_length == 1 or output_shape_length == 2:

This comment has been minimized.

Copy link
@srkreddy1238

srkreddy1238 Jan 19, 2019

Contributor

Layout is specific to vision operators, bringing in layout specific customization into basic transforms will not work for all models. Here it's only mobilenet hence it works, but I doubt it may get more complex when more models are added. A simple NLP model with one LSTM layer will fail these assumptions.

return tflite_output


def compare_tflite_with_tvm(tflite_in_data, tvm_in_data, in_name, input_tensors,

This comment has been minimized.

Copy link
@srkreddy1238

srkreddy1238 Jan 19, 2019

Contributor

Different input data for TVM and tflite ??
I understand this conversion is to felicitate NCHW across the model, this impose preprocessing.
Again non vision models may have challenges.

cc @tqchen pls advice.

This comment has been minimized.

Copy link
@FrozenGene

FrozenGene Jan 20, 2019

Author Member

This is discussed with @junrushao1994 before. As discussed, the issue is not our specific. Like TF-CoreML (https://github.com/tf-coreml/tf-coreml/blob/master/tfcoreml/_shape_sensitive_layers.py#L275), Intel OpenVINO, TensorRT and so on do the same things like us, we will meet the same problem. Yes, I agree there will be challenges, but currently let us start from little things, such as here limited 1-4D transformation and vision models (under internal development, we have tested many other models, doesn't have problems until now). When we want to support more generally ND reshape and NLP, let us see how to handle it better in future.

This comment has been minimized.

Copy link
@srkreddy1238

srkreddy1238 Jan 21, 2019

Contributor

(under internal development, we have tested many other models, doesn't have problems until now).

Have you tested InceptionV1 & V3 together ?

This comment has been minimized.

Copy link
@FrozenGene

FrozenGene Jan 21, 2019

Author Member

Yes. I tested Inception V3 just now(https://www.tensorflow.org/lite/models) and no problem, don't find official Inception V1 TFLite model. Maybe I can convert Tensorflow Inception V1 model into TFLite. Inception V3 model has one op named concatenation not included in this PR, I will supply more ops later. If someone need Inception V3 support, I will provide concatenation op implementation without any problem. Current PR's main purpose is to provide infrastructure of TFLite frontend.

tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 21, 2019

@FrozenGene
We need add a tutorial under frontend for tflite too.

@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 21, 2019

@FrozenGene
We need add a tutorial under frontend for tflite too.

Yes. I will add the tutorial in the following PR after this is merged like our frontends do.

Copy link
Contributor

srkreddy1238 left a comment

LGTM with the assumption of tflite frontend will be applicable to only vision models and NCHW layout as default for input.

@srkreddy1238

This comment has been minimized.

Copy link
Contributor

srkreddy1238 commented Jan 23, 2019

@FrozenGene request to bring in Inception V1/3/4 support soon to have an evidence that this layout conversion generalize for all types.

@srkreddy1238 srkreddy1238 merged commit 10df78a into apache:master Jan 23, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@FrozenGene

This comment has been minimized.

Copy link
Member Author

FrozenGene commented Jan 23, 2019

@FrozenGene request to bring in Inception V1/3/4 support soon to have an evidence that this layout conversion generalize for all types.

Thanks @srkreddy1238 I will add related ops and Inception Models testing soon.

zhiics added a commit to zhiics/tvm that referenced this pull request Jan 23, 2019
* Support TFLite FP32 Relay frontend.

* Fix lint issue

* Remove unnecessary variables and packages

* Add method doc string

* Fix capital letter of method doc string

* Add TFLite FP32 Relay frontend based on latest code

* Merge the latest code

* Solve cython issue with relay type inference

* Modify code based on suggestions
This was referenced Jan 24, 2019
Anthony-Mai added a commit to Anthony-Mai/tvm that referenced this pull request Jan 25, 2019
* Support TFLite FP32 Relay frontend.

* Fix lint issue

* Remove unnecessary variables and packages

* Add method doc string

* Fix capital letter of method doc string

* Add TFLite FP32 Relay frontend based on latest code

* Merge the latest code

* Solve cython issue with relay type inference

* Modify code based on suggestions
merrymercy added a commit to merrymercy/tvm that referenced this pull request Feb 18, 2019
* Support TFLite FP32 Relay frontend.

* Fix lint issue

* Remove unnecessary variables and packages

* Add method doc string

* Fix capital letter of method doc string

* Add TFLite FP32 Relay frontend based on latest code

* Merge the latest code

* Solve cython issue with relay type inference

* Modify code based on suggestions
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* Support TFLite FP32 Relay frontend.

* Fix lint issue

* Remove unnecessary variables and packages

* Add method doc string

* Fix capital letter of method doc string

* Add TFLite FP32 Relay frontend based on latest code

* Merge the latest code

* Solve cython issue with relay type inference

* Modify code based on suggestions
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
* Support TFLite FP32 Relay frontend.

* Fix lint issue

* Remove unnecessary variables and packages

* Add method doc string

* Fix capital letter of method doc string

* Add TFLite FP32 Relay frontend based on latest code

* Merge the latest code

* Solve cython issue with relay type inference

* Modify code based on suggestions
ricann pushed a commit to ricann/tvm that referenced this pull request Mar 4, 2019
* Support TFLite FP32 Relay frontend.

* Fix lint issue

* Remove unnecessary variables and packages

* Add method doc string

* Fix capital letter of method doc string

* Add TFLite FP32 Relay frontend based on latest code

* Merge the latest code

* Solve cython issue with relay type inference

* Modify code based on suggestions
@FrozenGene FrozenGene deleted the FrozenGene:tflite_fe branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.