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

use tensorflow 2.8.0 and bazel 5.0.0 #268

Merged
merged 7 commits into from
Jun 6, 2022
Merged

Conversation

freedomtan
Copy link
Contributor

Update tensorflow to 2.8.0 and specify bazel version in
.bazelverion so that if wrong bazel version is used and there
is no right version in the system, bazel will show instructions.

We should make sure all the vendor backends get matching performance numbers (#231) before we upgrade TensorFlow

bazelbuild/bazel#145 bazelbuild/bazel#241

@github-actions
Copy link

github-actions bot commented Mar 15, 2022

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

@freedomtan
Copy link
Contributor Author

the error messages of failed to build iOS Flutter app provide a good example how how the .bazelversion works

...
�[31mERROR: The project you're trying to build requires Bazel 5.0.0 (specified in /Users/jenkins/jenkins-agent/workspace/mobile_app_open/PullRequest-ios-build/.bazelversion), but it wasn't found in /Users/jenkins/.bazel/bin.�[0m

Bazel binaries for all official releases can be downloaded from here:
  https://github.com/bazelbuild/bazel/releases

You can download the required version directly using this command:
  (cd "/Users/jenkins/.bazel/bin" && curl -fLO https://releases.bazel.build/5.0.0/release/bazel-5.0.0-darwin-x86_64 && chmod +x bazel-5.0.0-darwin-x86_64)
make: *** [flutter/ios/native] Error 1
Build step 'Execute shell' marked build as failure
....

@d-uzlov
Copy link
Contributor

d-uzlov commented Mar 18, 2022

Are we going to completely drop support for older bazel versions?

@freedomtan
Copy link
Contributor Author

TensorFlow is somewhat bazel version sensitive. When a major bazel version is used by TensorFlow, it may break building with older bazel.

@freedomtan
Copy link
Contributor Author

freedomtan commented Mar 29, 2022

There are two issues,

  1. bazel 5.0 check Android NDK (because we have ndk entry in WORKSPACE), 5.0rc1: android_ndk_repository is fetched eagerly bazelbuild/rules_android_ndk#92
  2. the output of ios_static_framework is in bazel-out/ (and with variable name) instead of bazel-bin/
    on a macbook pro x86, it's bazel-out/applebin_ios-ios_arm64-opt-ST-82c2c37ad712/bin/flutter/cpp/flutter/ios_backend_fw_static.zip

@freedomtan
Copy link
Contributor Author

There are two issues,

  1. bazel 5.0 check Android NDK (because we have ndk entry in WORKSPACE), 5.0rc1: android_ndk_repository is fetched eagerly bazelbuild/bazel#14260
  2. the output of ios_static_framework is in bazel-out/ (and with variable name) instead of bazel-bin/
    on a macbook pro x86, it's bazel-out/applebin_ios-ios_arm64-opt-ST-82c2c37ad712/bin/flutter/cpp/flutter/ios_backend_fw_static.zip

I fixed the two issues.

@AhmedTElthakeb
Copy link
Contributor

For samsung backend, matching performance numbers are seen using tensorflow 2.8.0 and bazel 5.0.0

.bazelrc Show resolved Hide resolved
@anhappdev
Copy link
Collaborator

This PR also closes #145

@freedomtan
Copy link
Contributor Author

@freedomtan to rebase this to resolve conflicts.

Update tensorflow to 2.8.0 and specify bazel version in
`.bazelverion` so that if wrong bazel version is used and there
is no right version in the system, bazel will show instructions
it doesn't work with bazel 5.0.0 on windows
use the script to cp the framework to a fixed place
@freedomtan freedomtan force-pushed the use_tensorflow_280 branch from d12ff5f to 00cace8 Compare June 1, 2022 07:10
@freedomtan
Copy link
Contributor Author

I rebased the patch and added the memory leak patch for Core ML delegate.

Comment on lines +26 to +33
if sdk_home:
sdk_rule = """
native.android_sdk_repository(
name="androidsdk",
path="{}",
build_tools_version="30.0.3",
)
""".format(_escape_for_windows(sdk_home))
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently Android SDK is not needed for bazel builds in our repo.
Bazel builds only the .so files which only require NDK.
SDK is required for APK build but that can be done on a separate machine.

I personally build Android libraries for the app in a Docker container or in WSL and I don't install Android SDK there.
But with this patch I get an error because SDK is missing.

I think it would be better to comment these lines or just disable the condition using something like if sdk_home and False

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds right. will do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d-uzlov are you sure that on windows this doesn't work? I added this to get around the Android SDK problem in bazel you mentioned on macOS. I met the Android SDK missing problem when I tried to build this on macOS. It should work on Windows also. I mean if $ANDROID_HOME is not, sdk_home should not be set.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are no issues building supported targets on Windows.
I don't have android-related environment variables defined on Windows, and bazel doesn't try to load them.

The issue is in the WSL virtual machine.
ANDROID_HOME is defined there because some of the Android tools are installed, also NDK is installed there, but the there is no actual SDK because it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh, got it, thanks. let me check how to handle this on Linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d-uzlov Could elaborate more on why we need ANDROID_HOME. I was able to build and run flutter app for Android devices on Ubuntu without ANDROID_HOME set.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need it to build native libraries. And Android SDK is not required.
But if ANDROID_HOME is defined for whatever reason then Android SDK becomes required, even though it's not used by bazel.

I don't think requiring users not to have ANDROID_HOME is reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's merge it and I remove unused dependencies in a separate PR.

@freedomtan
Copy link
Contributor Author

To build with TensorFlow 2.9.x, we need

  1. -std=c++17,
  2. extra dependency for //flutter/cpp:util, and
  3. couple lines of changes to metal delegate source code (otherwise, it doesn't work properly on iPhone 13.)

I'll create another branch for 2.9.x.

@freedomtan
Copy link
Contributor Author

freedomtan commented Jun 2, 2022

A TensorFlow 2.9.1 branch based on this 2.8.0 one, https://github.com/mlcommons/mobile_app_open/tree/use_tensorflow_291.

And comparing with 2.8.0, #334

@anhappdev
Copy link
Collaborator

App built with XCode 13.4 run all benchmarks successfully on iPhone XS. Great work @freedomtan 👍.

@freedomtan freedomtan merged commit 4499a5d into master Jun 6, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2022
@anhappdev anhappdev deleted the use_tensorflow_280 branch November 11, 2022 02:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

possible memory leak in iOS app The app is impossible to build on macOS Monterey
4 participants