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

Set tensorflow submodule to v2.0.0 tag #55

Merged
merged 5 commits into from Oct 9, 2019
Merged

Conversation

Tombana
Copy link
Collaborator

@Tombana Tombana commented Oct 7, 2019

No description provided.

.gitmodules Outdated Show resolved Hide resolved
@Tombana Tombana changed the title Set tensorflow submodule to r2.0 branch Set tensorflow submodule to v2.0.0 tag Oct 7, 2019
@Tombana
Copy link
Collaborator Author

Tombana commented Oct 7, 2019

I'm going crazy... I've spent hours trying to figure out why the pip package is not using our version of register.cc. Then I deleted both our version of register.cc and the original one in the tensorflow repo, expecting build errors. The cpp examples then indeed give the build errors you expect, saying it can not find a reference to BuiltinOpResolver. The Python package however, builds just fine and the only error it gives is that it can not find the Bsign op. So how do the cpp files of this python package even compile in the first place??? It's missing source files and it still works. And there is no other definition of BuiltinOpResolver to be found. What?!

@Tombana
Copy link
Collaborator Author

Tombana commented Oct 9, 2019

After only a couple of hours of pulling my hairs out I think I figured out what went wrong. The tflite_runtime pip package is completely fine, and it contains our custom op. So why does it fail in the v2.0.0 tag, or the r2.0 branch, and not in master ? Because in the v2.0.0 version, the tflite python script first tries to import tensorflow and uses the version of tflite that is embedded in tensorflow instead! Only when tensorflow is not available it actually uses the tflite in the package! In master, tflite will properly use its own package and not switch to tensorflow.
The "fix" in master is line 26 of this file.
It was applied by this commit.

Possible solutions:

  • In azure (or GH actions), uninstall tensorflow for the final tflite package test. However, we want to use both tensorflow and tflite in the last unit test to compare the model outputs. Its still possibly by first running TF and saving it to a file, then uninstalling TF etc, but it seems hacky.
  • Use master. Disadvantage: we wanted to speedup builds by having a pre-built tflite library in the repo (see Issue Add prebuilt TF lite static library to repo to speedup the build #51), so we have to rebuild and re-push that everytime tf master updates.
  • Keep using master for now, and once the fix gets merged into r2.0 and then tagged, like v2.0-rc3 or whatever, then switch to that tag.
  • Set our submodule to a fixed commit somewhere in master.

@lgeiger @arashb What do you guys think?

@lgeiger
Copy link
Member

lgeiger commented Oct 9, 2019

Thanks for investigating.

I'd opt for one of the following:

  • Keep using master for now, and once the fix gets merged into r2.0 and then tagged, like v2.0-rc3 or whatever, then switch to that tag.
  • Set our submodule to a fixed commit somewhere in master.

TF master changes quite rapidly and can be unstable from time to time, which can be really annoying. It think we should switch to a commit that works and leave it there until a new version with the fix is released.

@Tombana
Copy link
Collaborator Author

Tombana commented Oct 9, 2019

Allright. I'll just fix it at the current latest master commit, and I'll add an issue that we have to periodically check for tags that include the fix.

Ofcourse a commit in master from 2 days ago breaks tflite...

@Tombana Tombana merged commit 4aa0a94 into master Oct 9, 2019
@Tombana Tombana deleted the tf_submodule_branch branch October 9, 2019 12:52
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.

None yet

2 participants