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

Update libcoral for Python 3.11 and modern versions of Tensorflow #36

Open
wants to merge 43 commits into
base: master
Choose a base branch
from

Conversation

feranick
Copy link

libcoral (and pycoral that depends on it, making both the google-recommended ways to interact with EdgeTPU with C++ and Python respectively) are outdated due to several deprecations and changes within TF. This PR aims at updating libcoral by fixing such deprecations, modernizing the bazel building infrastructure, and docker files.

One change is needed in TF to allow compilation. A separate issue is on file with separate PR Change visibility status for lite/schema/schema_conversion_utils to allow compilation of external libraries.

There may be more issue to fix, but this PR provides a significant attempt at modernize the codebase.

Reference issue: google-coral/pycoral#137

libedgetpu has been updated to support TF 2.15.0.
1. Reworked after deprecation fo Tensorflow/toolchains.
2. Added definition of TF 2.15.0
3. Updated google benchmark to fix missing header call.
Replace deprecated Eigen::all with Eigen::indexing::all
This is for tflite::Interpreter
The former call is deprecated, replaced by the latter
To include modern OS /Python versions
Replace deprecated Eigen::last with Eigen::placeholders::all
@feranick
Copy link
Author

cc: @Namburger @pkgoogle

Several compilation errors are fixed
@feranick
Copy link
Author

feranick commented Mar 1, 2024

@Namburger @pkgoogle, I can confirm that with this PR (and the manual application of tensorflow/tensorflow#63074, compilation of libcoral is successful in both MacOS and Linux. As far as I can see this is ready to be merged.

@Namburger
Copy link

Thanks @feranick! I don't think it's a good idea to merge this until we have tensorflow/tensorflow#63074 figured out. I prefer not having users checking out project and hacking cached buildtool configs to get things to build. Although I can see the significant impact of having this repo updates and getting up to date with tensorflow. I'll try to see if we can gain more visibility for tensorflow/tensorflow#63075

@Namburger Namburger self-assigned this Mar 1, 2024
@feranick
Copy link
Author

feranick commented Mar 1, 2024

Absolutely agree. In fact, even if tensorflow/tensorflow#63075 and tensorflow/tensorflow#63074 are out, they will certainly be on TF master, and hopefully in time for TF 2.16.0 but certainly not for 2.15.0 (unless it's backported on future maintenance release). So I would think this could be merged after TF 2.16.0 is out possibly with the fix above...

With rules-python v.0.27.0 intruduced a better way to handle pypi requirements, removing the need of
 several calls within WORKSPACE.
Stable libedgetpu only supports TF2.15.0. It will be reverted back to the official repo once that has been updated to TF2.16.0
While the original commit simplified WORKSPACE, it is not alligned with the current version of TF. REverting to the previous version of rules_python v.0.26.0 which is the same shipped with TF 2.16.0.
Additional calls to gstreamer-1.-1
A visibility issue in TF SCHEMA was preventing compilation of libcoral.
Ref: tensorflow/tensorflow#63074

This is now fixed in TF 2.17.0-dev with commit 79ecb3f
@feranick
Copy link
Author

feranick commented Mar 5, 2024

Compilation of libcoral works fine with MacOS and Linux by building against TF 2.17.0-dev commit 79ecb3f, where the visibility issue is now fixed.

@feranick
Copy link
Author

feranick commented Mar 5, 2024

@Namburger I already found a possible breakage in compilation for arm due to 2.17 (See log here) when crosscompiling for arm (there are no issues in MacOS and Linux for x86). I forked 2.16.0 and added the visibility patch to test whether the issue is in 2.17 or not.

When compiling against a local fork of TF r2.16 with the visibility patch, the above issue is not present, but another one is specific to libcoral and arm. Log attached.
log_arm_TF2.16.0.txt

@Namburger
Copy link

@Namburger I already found a possible breakage in compilation for arm due to 2.17 (See log here) when crosscompiling for arm (there are no issues in MacOS and Linux for x86). I forked 2.16.0 and added the visibility patch to test whether the issue is in 2.17 or not.

When compiling against a local fork of TF r2.16 with the visibility patch, the above issue is not present, but another one is specific to libcoral and arm. Log attached. log_arm_TF2.16.0.txt

@Namburger I already found a possible breakage in compilation for arm due to 2.17 (See log here) when crosscompiling for arm (there are no issues in MacOS and Linux for x86). I forked 2.16.0 and added the visibility patch to test whether the issue is in 2.17 or not.

When compiling against a local fork of TF r2.16 with the visibility patch, the above issue is not present, but another one is specific to libcoral and arm. Log attached. log_arm_TF2.16.0.txt

@feranick
Copy link
Author

feranick commented Mar 5, 2024

I fixed the compilation with TF 2.16.0 and visibility patch by adding --cxxopt=-mfp16-format=ieee to arm specific flags.

Compilation is successful now across Linux architectures. Will go back and see what is going on with 2.17.0.

@feranick
Copy link
Author

feranick commented Mar 6, 2024

@Namburger Since this PR seems to work for the most part (i.e. MacOS, Linux_X86-64, Linux_aarch64), I was wondering if would make sense to merge now that it builds to a workable version of TF 2.17.0. It will be in a better state than it currently is (where it doesn't build anywhere). A couple of observations:

  1. If you think we can go ahead, we will need to do it in stages, i.e. first merging an updated version of libedgetpu, then libcoral, then pycoral all built against the same TF 2.17.0 with visibility patch. Once each has been merged, the next needs to be quickly adapted to the right repo/commit, which I can do.
  2. libedgetpu builds and works fine with TF 2.16.0, but it will need to be built against TF2.17.0 to be compatible with libcoral/pycoral. Thoughts?

Of course the other option is to wait until much later when 2.17.0 is ready, but that would mean not having pycoral at all for quite a while. Please let me know.

@Namburger
Copy link

@Namburger Since this PR seems to work for the most part (i.e. MacOS, Linux_X86-64, Linux_aarch64), I was wondering if would make sense to merge now that it builds to a workable version of TF 2.17.0. It will be in a better state than it currently is (where it doesn't build anywhere). A couple of observations:

  1. If you think we can go ahead, we will need to do it in stages, i.e. first merging an updated version of libedgetpu, then libcoral, then pycoral all built against the same TF 2.17.0 with visibility patch. Once each has been merged, the next needs to be quickly adapted to the right repo/commit, which I can do.
  2. libedgetpu builds and works fine with TF 2.16.0, but it will need to be built against TF2.17.0 to be compatible with libcoral/pycoral. Thoughts?

Of course the other option is to wait until much later when 2.17.0 is ready, but that would mean not having pycoral at all for quite a while. Please let me know.

@feranick appreciate all your work so far, I'm trying to work it out, it's not promising that we keep running into one issue after another.. I don't know if it's a good idea to break things for armv7 forks at the moment. I'm talking to some forks who pushed the change that broke the build will try to get thing working..

Full disclaimer I'm currently having other priories and coral work is pure volunteering, so I can only do so much. Other forks I'm talking to are also working on other things so I can't push too hard, but I understand the importance of keeping this project updated and will try my best to keep things moving asap. Please see google-coral/libedgetpu#60 (comment)

@feranick
Copy link
Author

feranick commented Mar 6, 2024

@Namburger, I absolutely understand, your interest even voluntary is great and much appreciated. Let's see what happens, and if we can find ways to push things a bit at the time, that is great anyway. I'll be here for any help I can provide.

Thanks!

@feranick
Copy link
Author

Before this can be merged, a new PR for libedgetpu needs to be pushed first (as it adds support for TF 2.17.0).

google-coral/libedgetpu#70

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.

2 participants