Skip to content

Conversation

AdamHillier
Copy link
Contributor

@AdamHillier AdamHillier commented Oct 22, 2020

What do these changes do?

This PR uses a Bazel genrule to generate the Python TFLite schema file and copy it into the correct place during compilation, which means we can remove the checked-in file.

I'm not 100% sure it works because the end2end test takes ages to compile on my local machine, but running bazel build -c opt //larq_compute_engine/mlir:tflite_schema_py locally does generate the correct file as bazel-bin/larq_compute_engine/mlir/python/tflite_schema.py, so hopefully it works! Let's see what CI thinks.

This is a draft, and is based on #545.

How Has This Been Tested?

CI.

Benchmark Results

N/A.

Related issue number

N/A.

@AdamHillier AdamHillier changed the title Auto-generate the tflite Python schema. Auto-generate the TFLite Python schema. Oct 22, 2020
@AdamHillier AdamHillier added the internal-improvement Internal Improvements and Maintenance label Oct 22, 2020
@AdamHillier AdamHillier force-pushed the autogen-tflite-schema branch 2 times, most recently from d5ca3ab to e3264f0 Compare October 22, 2020 13:02
@AdamHillier AdamHillier marked this pull request as draft October 22, 2020 13:43
Base automatically changed from tf-r2.4 to master October 22, 2020 14:55
@AdamHillier AdamHillier force-pushed the autogen-tflite-schema branch from d590d57 to 15248ac Compare October 22, 2020 14:56
@AdamHillier AdamHillier requested a review from a team October 22, 2020 14:56
@AdamHillier AdamHillier marked this pull request as ready for review October 22, 2020 14:56
Copy link
Collaborator

@Tombana Tombana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, having to checkin this file has annoyed me for quite a while!
The @org_tensorflow//tensorflow/lite/python:schema_py used to generate broken imports, glad that this has been fixed in the latest version now.

Do we need to add python/tflite_schema.py to .gitignore or is it only included in the bazel exec_dir?

Does this work correctly when building the pip packages? I think it should, but not 100% sure.

@AdamHillier
Copy link
Contributor Author

Do we need to add python/tflite_schema.py to .gitignore or is it only included in the bazel exec_dir?

I don't think this should be necessary, it's only in the bazel outputs.

Does this work correctly when building the pip packages? I think it should, but not 100% sure.

Yes I agree we need to check that. Once CI passes here I'm going to trigger a nightly release build and see what happens.

@AdamHillier
Copy link
Contributor Author

Good and bad news:

  • The test-build I did worked for Mac; I downloaded and installed the built Pip package and tested that it works correctly to convert a model.
  • The Linux test-build failed, with the error
    020-10-22T19:08:34.4885188Z bazel-out/k8-opt/bin/external/local_config_python/python_include/pyconfig.h:3:12: fatal error: x86_64-linux-gnu/python3.6m/pyconfig.h: No such file or directory
    2020-10-22T19:08:34.4886028Z  #  include <x86_64-linux-gnu/python3.6m/pyconfig.h>
    2020-10-22T19:08:34.4886356Z             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    2020-10-22T19:08:34.4886606Z compilation terminated.
    2020-10-22T19:08:34.5977016Z Target //:build_pip_pkg failed to build
    2020-10-22T19:08:34.5985991Z Use --verbose_failures to see the command lines of failed build steps.
    2020-10-22T19:08:35.1872804Z INFO: Elapsed time: 13429.448s, Critical Path: 142.10s
    
    which doesn't sound like a problem that could be caused by this PR - a more likely culprit is the recent TF2.4 upgrade. This comment suggests that it means the Docker container doesn't include all the necessary dependencies.

@lgeiger
Copy link
Member

lgeiger commented Oct 22, 2020

Sounds good, thanks for testing!

Maybe ac3afd2#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34R158 broke the build or we need an newer Docker image. In any case let's merge, I can take a look at the failure next week. TF or TFA might run into a similar issue so we can take a lot at their build config.

@lgeiger lgeiger merged commit ea8471b into master Oct 22, 2020
@lgeiger lgeiger deleted the autogen-tflite-schema branch October 22, 2020 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-improvement Internal Improvements and Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants