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 tfjs-react-native to use tfjs 4.2.0 #7415

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Feb 24, 2023

Add missing function for isTypedArray to platform_react_native.ts. Update tfjs, react native, and expo dependencies.

Update the integration tests app to use expo and the latest version of react native.

Fixes #7323

TODO: Update the app CI before merging this.
Edit: I will split this PR into two PRs. One that updates the dependencies that do not require new native modules and one that updates the app's native modules.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@vikaiello
Copy link

Hi @mattsoulanille

Just wanted to let you know that me and my team (from DWS Brazil) are very excited with this update. I'm not sure how you guys work of if there's any other way to show the engagement for this merge (if there is, let me know!)

And also if there's anything that us as a community can help you with this update.

Thanks!

@Guilherme-Inkotte
Copy link

Hi, @mattsoulanille. Sorry to bother you, but are there any updates on this PR? We are really looking forward to this merge so we can upgrade our app and release it.
Thanks!

@paradite
Copy link
Contributor

Hi @mattsoulanille. I noticed that this PR has been in draft for some time and you plan to split it up into two PRs.

I have been using tfjs in production with various Expo versions for the past few months using my forked platform_react_native.ts.

I believe indeed to get tfjs working with latest Expo it should be a simple change in platform_react_native.ts. Would you like me to open a minimal PR to get it working with Expo to address the issues that people have been opening up:

@mattsoulanille
Copy link
Member Author

I believe indeed to get tfjs working with latest Expo it should be a simple change in platform_react_native.ts. Would you like me to open a minimal PR to get it working with Expo to address the issues that people have been opening up:

@paradite, If you're willing to do that, I would happily review and merge that PR! To be perfectly honest, our team lacks the expertise to properly support React Native, particularly around native dependencies, and we've been hoping someone in the community might be able to help us there. Thank you!

@paradite
Copy link
Contributor

paradite commented Oct 27, 2023

Hi @mattsoulanille, thanks for the reply. Upon a closer look, your PR #7451 has already fixed platform_react_native.ts.

Based on my testing, there are a few more outstanding issues to get tfjs working on React Native:

  1. Argument X passed to Y must be a Tensor or TensorLike, but got 'Tensor' errors
  2. outdated dependencies in tfjs-react-native (for example No way to create new react-native app with tfjs-react-native because of dependency issues??? #7454 tfjs-react-native package not updated and contains obsolete dependencies #7323) and documentation on how to setup (some dependencies are not needed for certain use cases)
  3. updated sample React Native project for testing and reference

For 1, my previous PR #7947 should fix them, just waiting for the next release 4.13.0 to (hopefully) include it.

For 2 and 3, I am plan to open two separate PRs to fix them (since 3 is a big change):

First PR:

  • change all peer dependencies to "*" to avoid install errors when using npm and avoid having to update this constantly every time upstream updates the versions.
  • remove some peer dependencies that are not necessary (expo-camera for example, if the user is not using camera)
  • update documentation to reflect reduced number of dependencies required and how to install optional dependencies if needed.

Second PR:

  • update the sample project to using latest Expo SDK 49 (I see you already switched to using Expo in your PR as well) while still retaining the old functionalities and demos.
  • This would remove the need for having any native module code in this repo. However to ensure the project is maintainable, some docs will need to be added on how to build the app locally without relying on Expo hosted services (EAS).
  • Not sure about the app CI part because likely I don't have necessary permissions to tinker with it, so maybe I can setup a separate online CI service as reference to run the integration tests.

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.

tfjs-react-native package not updated and contains obsolete dependencies
4 participants