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

[Desktop] MacOS support #87

Merged
merged 48 commits into from
Sep 27, 2023
Merged

Conversation

CaptainDario
Copy link
Contributor

@CaptainDario CaptainDario commented Jun 10, 2023

This PR should add desktop MacOS support.

Environment:

  • M1 Pro
  • MacOS Venturs 13.4

Changes:

  • Load TF Lite binary
  • update examples to support MacOS

Todo:

  • Agree on how to bundle the TF Lite library
    • Currently, the binaries are being added in Xcode as a bundle resource to the examples
      • This is not desirable as every user would need to add the binaries to their app manually

Notes:

  • Camera is disabled for the examples on desktop as this is not really supported

Progress:

  • load TF Lite on macOS
  • test examples
    • audio classification
      • uses some platform channels that do not work on macOS
    • image classification mobilenet
      • only from file works
    • live object detection ssd mobile net
      • uses camera package that only works on iOS / mobile
    • object detection ssd mobile net
      • only from file works
    • style transfer
      • works
    • super resolution esrgan
      • works
    • text classification
      • works

@CaptainDario
Copy link
Contributor Author

@PaulTR all examples are verified to run on M1 Pro. Currently, I am using a TF Lite binary locally build via Make.

It would be nice if the libraries for desktop could be hosted somewhere (or is there already a place for this?). Given the binaries I will check / add support for Linux / Windows. Maybe support via WASM via web would also be possible.

@PaulTR
Copy link
Collaborator

PaulTR commented Jun 12, 2023

Can you share the files with [email protected]? I can find a place to host them and can link to them in the docs. Thanks!

@PaulTR
Copy link
Collaborator

PaulTR commented Jun 12, 2023

Also can you run dart format . on your machine? You might need to check flutter doctor beforehand to make it work :) Another contributor and I ran into similar problems with our dart path when trying to run it.

@PaulTR
Copy link
Collaborator

PaulTR commented Jun 12, 2023

Basically ran into this issue:

[!] Flutter (Channel stable, 3.10.3, on macOS 13.4 22F66 darwin-arm64, locale en)
! Warning: dart on your path resolves to /opt/homebrew/Cellar/dart/2.19.5/libexec/bin/dart, which is not inside your current Flutter SDK checkout at /Users/ptruiz/development/flutter. Consider adding /Users/ptruiz/development/flutter/bin
to the front of your path.

fixed with brew uninstall dart

@CaptainDario
Copy link
Contributor Author

Thanks for the feedback. I will do so in the coming days.

@CaptainDario
Copy link
Contributor Author

@PaulTR I ran dart format .

@CaptainDario
Copy link
Contributor Author

I shared a google drive folder with the email you gave me.
The binaries are build on TF 2.12 using CMake (advantage: OpenCL gpu acceleration, disadvantage: no way to include select tf ops). There are 3 binaries one for arm, one for x86 and a universal one merged using lipo.
I added the delegate options for OpenCL and XNNPack.

Notes:

  • cross compiling in CMake can be achieved using:
    -DCMAKE_OSX_ARCHITECTURES=x86_64|arm64

  • bundling the two architectures using lipo:
    lipo -create arm64/libtensorflowlite_c.dylib x86/libtensorflowlite_c.dylib -output libtensorflowlite_c.dylib

@PaulTR
Copy link
Collaborator

PaulTR commented Jun 27, 2023

Looks like there's a few things breaking when CI runs :) I also received your files, but am asking internally if we have official 'source of truth' files that we host somewhere first just so we're not duplicating efforts and potentially building with the wrong thing. Will keep you posted.

@PaulTR
Copy link
Collaborator

PaulTR commented Jun 28, 2023

Alright so you'll have to forgive my ignorance a bit here - how are the binaries used for this? Is it something every dev will need when using the plugin for MacOS, or just for building the plugin? And can you put in documentation for both use-cases in your main readme? :)

I'm assuming it's something every dev on MacOS would need based on your initial comment, which makes this a bit more difficult, but also I really hate the idea of us having to say just go out and read this massive document to make it work.

Basically the eng team for TFLite said they expect users to build their own binaries any time they need them, so I want to make sure I understand the whole situation before advocating for a solution.

Thanks!

@CaptainDario
Copy link
Contributor Author

Looks like there's a few things breaking when CI runs :) I also received your files, but am asking internally if we have official 'source of truth' files that we host somewhere first just so we're not duplicating efforts and potentially building with the wrong thing. Will keep you posted.

With the CI I can take a look but it seems really weird that it says all those things are not defined. But I am not sure when I would get around to this.

@CaptainDario
Copy link
Contributor Author

Alright so you'll have to forgive my ignorance a bit here - how are the binaries used for this? Is it something every dev will need when using the plugin for MacOS, or just for building the plugin? And can you put in documentation for both use-cases in your main readme? :)

No worries, happy if I can contribute back to make TF a bit greater!
Yes, every dev will need the binaries. It's basically the same as the files that are being downloaded from maven / cocoa pods, just for Mac (later also Windows / Linux and potentially web). So they are necessary to lookup the symbols in dart using DynamicLibrary..
Sure thing, I can add this to the Readme when a decision was made how to distribute them.

I'm assuming it's something every dev on MacOS would need based on your initial comment, which makes this a bit more difficult, but also I really hate the idea of us having to say just go out and read this massive document to make it work.

To be honest, I think it would be a bad idea to make every user build their own version using CMake or bazel. I opened an issue how to build TF Lite using CMake on windows and their is still no answer, thus I cannot / do not know how to build on Windows (tensorflow/tensorflow#55970).

Basically the eng team for TFLite said they expect users to build their own binaries any time they need them, so I want to make sure I understand the whole situation before advocating for a solution.

I would like if either
this repo provides pre-built binaries and only make the user build custom ones if they want special features. This has the advantage that the user can decide if they want to use CMake or Bazel for building (as mentioned they have different feature sets).
or the TF Lite's build process gets incorporated in the build process of the plugin. Some time ago a member of the Ubuntu team did an effort to do this on Linux, I think MacOS would be quite similar (see this PR am15h/tflite_flutter_plugin#172).

@CaptainDario
Copy link
Contributor Author

fixed the dart anlyze problems, actually don't understand how it could pass those tests before.

@CaptainDario
Copy link
Contributor Author

@PaulTR It would be nice if we could agree on a way to distribute and include the TF Lite binaries on desktop.
Because TF Lite's version is independent of this plugin I think it would be nice if the user can choose which one to use.

How about using the same approach as the flutter docs recommend? It is slightly more effort on the first setup, but the libraries can be chosen by the user.

@PaulTR
Copy link
Collaborator

PaulTR commented Aug 4, 2023

Yeah I think that test went in during this PR being written.

I'll read over that documentation on Monday and get back to you.

Thanks!

@PaulTR
Copy link
Collaborator

PaulTR commented Aug 7, 2023

So I brought up hosting the binaries and got some pushback. I do think our best bet right now is giving people directions for building them if they want desktop support, unfortunately. I'm guessing it shouldn't be too difficult from the documentation I've seen, but it's definitely not ideal. I'm kind of under the impression that desktop won't be a very commonly used feature, but super valuable to those that need it, so the extra step shouldn't be a blocker.

@CaptainDario
Copy link
Contributor Author

Does that mean I should remove RL completely?

@PaulTR
Copy link
Collaborator

PaulTR commented Sep 14, 2023

Yeah it doesn't need to be in this PR since it's going in under another (RL shouldn't be in yours is my guess, it exists in that folder because I pulled a separate PR to test, then it leaves the empty directories)

@CaptainDario
Copy link
Contributor Author

CaptainDario commented Sep 18, 2023

Will check the new examples today, and update the check list at the beginning.

I will add the tf lite binary in Xcode as a relative path to a folder called blobs in the main plugin, like this

tf_lite_plugin

  • blobs
  • ...
  • examples
    • audio classification
    • ...

This way the examples should work by just adding the binary in the main folder. If this is not desired let me know.

@PaulTR
Copy link
Collaborator

PaulTR commented Sep 23, 2023

Having devs add the binary to the folder sounds like a good plan to me. Thanks!

@CaptainDario
Copy link
Contributor Author

The text classification example already has the desktop binaries included. Is there a reason for that?

@CaptainDario
Copy link
Contributor Author

Will check the new examples today, and update the check list at the beginning.

I will add the tf lite binary in Xcode as a relative path to a folder called blobs in the main plugin, like this

tf_lite_plugin

  • blobs

  • ...

  • examples

    • audio classification
    • ...

This way the examples should work by just adding the binary in the main folder. If this is not desired let me know.

Need to revise this, that does not work on win / lin therefore gonna add a blobs folder to each example (for some reason text classification already has it).

@PaulTR
Copy link
Collaborator

PaulTR commented Sep 25, 2023

The text classification example already has the desktop binaries included. Is there a reason for that?

Oof, so it is. No reason - that's an oversight that I'll fix after this merge.

@CaptainDario
Copy link
Contributor Author

I guess the problem is again about me not using the newest flutter version. But I cannot currently change the version.

@PaulTR
Copy link
Collaborator

PaulTR commented Sep 27, 2023

No worries. I'm going to merge this one and try to fix it on my side since I have a little downtime right now before leaving Europe. Thanks for sticking through on this and getting it made, it's a huge addition!

@PaulTR PaulTR merged commit 22cdbdd into tensorflow:main Sep 27, 2023
1 of 2 checks passed
@CaptainDario
Copy link
Contributor Author

Thank you for your effort. I am currently on vacation for the next 3 weeks. Therefore, I cannot do much. However, when this landed and I am back, I will work on windows and Linux support.

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