-
Notifications
You must be signed in to change notification settings - Fork 950
Add rn integration test app #1874
base: master
Are you sure you want to change the base?
Conversation
Can we avoid checking in the generated files? It would be good to keep the clone time small on slow connections (already a big issue). |
Clarified in person, that by autogenerated I meant boilerplate that we don't need to change, but not code that can be generated on the fly. However I will look into:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 72 of 72 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tafsiri)
tfjs-react-native/README.md, line 11 at r1 (raw file):
## Setting up a React Native app with tfjs-react-native These instructions assume that you are generally familiar with [react native](https://facebook.github.io/react-native/) developement. This library has only been tested with React Native 0.58.X & 0.59.X. React Native 0.60 is not supported.
wait, isnt our dep on 0.60?
tfjs-react-native/README.md, line 68 at r1 (raw file):
### Optional Steps If you want use the AsyncStorageHandler to save and load models. Add [async-storage](https://github.com/react-native-community/async-storage) to your project
nit: period
tfjs-react-native/integration_rn59/android/app/build.gradle, line 1 at r1 (raw file):
apply plugin: "com.android.application"
I feel like we should put licenses at the top of these files
tfjs-react-native/integration_rn59/assets/model/model.json, line 1 at r1 (raw file):
{
as we spoke about offline, can we either make this smaller or put it on GCP?
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 27 at r1 (raw file):
StatusBar, Image, Text,
remove the comma here and this will format properly
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 72 at r1 (raw file):
// Compute a checksum for the image. Useful for debugging. const imageTensorSum = imageTensor.sum(); const imageChecksum = await imageTensorSum.dataSync()[0];
use .data()
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 84 at r1 (raw file):
predictionTime: end - start, imageChecksum, })
semicolon
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 88 at r1 (raw file):
} imageToTensor(rawImageData: ArrayBuffer) {
return type
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 89 at r1 (raw file):
imageToTensor(rawImageData: ArrayBuffer) { const { width, height, data } = jpeg.decode(rawImageData, true);
pull true to a named const
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 92 at r1 (raw file):
// Drop the alpha channel info for mobilenet const buffer = new Uint8Array(width * height * 3); let offset = 0; //offset into original data
nit: space after //
tfjs-react-native/integration_rn59/components/ml.ts, line 21 at r1 (raw file):
import * as tf from '@tensorflow/tfjs'; import {asyncStorageIO, bundleResourceIO} from '@tensorflow/tfjs-react-native';
remove blank line
tfjs-react-native/integration_rn59/components/ml.ts, line 28 at r1 (raw file):
export async function simpleOpRunner() { return async () => { const res = tf.tidy(() => tf.square(3));
tidy() isnt doing anything here
tfjs-react-native/integration_rn59/components/ml.ts, line 29 at r1 (raw file):
return async () => { const res = tf.tidy(() => tf.square(3)); const data = res.dataSync()[0];
use data()
tfjs-react-native/integration_rn59/components/ml.ts, line 37 at r1 (raw file):
* Run a simple precision test runner. */ export async function precisionTestRunner() {
why do you need to return functions here? just call it directly. if for some reason you need the function, the parent doesnt need to be async
tfjs-react-native/integration_rn59/components/ml.ts, line 42 at r1 (raw file):
const data = res.dataSync()[0]; return JSON.stringify(data); }
semicolon
tfjs-react-native/integration_rn59/components/ml.ts, line 55 at r1 (raw file):
await model.classify(input); // const pic = tf.tensor(coffeePicData, [1, 24, 24, 3]);
remove this comment
tfjs-react-native/integration_rn59/components/ml.ts, line 66 at r1 (raw file):
* Runner for a model bundled into the app itself. */ const modelJson = require('../assets/model/model.json');
maybe this model should be named?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)
tfjs-react-native/README.md, line 11 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
wait, isnt our dep on 0.60?
Nope our peer dep is on < 59.. There is a types dependency on 0.60 types (which seems to be what the 0.59 cli template uses). So we should be type compatible with the next version, but from an ecosystem perspective a bunch of native libraries need to be updated to support RN 60.
I've updated the peer dep to specify the range of things i've actually tried it on.
tfjs-react-native/README.md, line 68 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
nit: period
Done
tfjs-react-native/integration_rn59/android/app/build.gradle, line 1 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
I feel like we should put licenses at the top of these files
Added here and elsewhere (every code file that didn't already have a copyright notice and some config files).
tfjs-react-native/integration_rn59/assets/model/model.json, line 1 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
as we spoke about offline, can we either make this smaller or put it on GCP?
replaced with a 44 byte model.
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 27 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
remove the comma here and this will format properly
I removed the comma, and that didn't auto fix it. but i manually put it on one line.
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 72 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use .data()
Done
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 84 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
semicolon
Fixed. Looks like clang-format doesn't work great with .tsx files
It looks like https://github.com/google/gts has switched to using prettier (and wrap it in the gts tool). We should compare with internal standard and see if there is anything to update here wrt our tooling.
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 88 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
return type
Done
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 89 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
pull true to a named const
Done
tfjs-react-native/integration_rn59/components/MobilenetDemo.tsx, line 92 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
nit: space after //
Done
tfjs-react-native/integration_rn59/components/ml.ts, line 21 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
remove blank line
Done
tfjs-react-native/integration_rn59/components/ml.ts, line 28 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
tidy() isnt doing anything here
Removed
tfjs-react-native/integration_rn59/components/ml.ts, line 29 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
use data()
Done
tfjs-react-native/integration_rn59/components/ml.ts, line 37 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
why do you need to return functions here? just call it directly. if for some reason you need the function, the parent doesnt need to be async
This is to conform with the spec of the Run component used to run these. Some of these have an async initialization that the runner allows to complete before running the returned function (which can be run many times in a loop). See the mobileNetRunner for an example, the outer function (which loads mobilenet) will be run once while the prediction can be run a bunch of times.
tfjs-react-native/integration_rn59/components/ml.ts, line 42 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
semicolon
Done
tfjs-react-native/integration_rn59/components/ml.ts, line 55 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
remove this comment
Done
tfjs-react-native/integration_rn59/components/ml.ts, line 66 at r1 (raw file):
Previously, nsthorat (Nikhil Thorat) wrote…
maybe this model should be named?
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 42 of 72 files at r1, 13 of 14 files at r2.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tafsiri)
tfjs-react-native/README.md, line 11 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
Nope our peer dep is on < 59.. There is a types dependency on 0.60 types (which seems to be what the 0.59 cli template uses). So we should be type compatible with the next version, but from an ecosystem perspective a bunch of native libraries need to be updated to support RN 60.
I've updated the peer dep to specify the range of things i've actually tried it on.
Do you foresee this dependency versioning becoming more relaxed as tfjs-react-native matures? Or are we always going to be strict? Do react-native developers expect such strict versioning requirement?
tfjs-react-native/README.md, line 15 at r2 (raw file):
### Step 1. Create your react native app. You can use the [React Native CLI or Expo](https://facebook.github.io/react-native/docs/getting-started). This library relies on a couple of dependencies from the Expo project so it may be convenient to use expo but is not mandatory. You will also need to use Cocoapods to install these dependencies.
is cocoapods only for macs? if yes, let's qualify the statement: If you are on Mac OS, you need to use cocoapods...
tfjs-react-native/README.md, line 19 at r2 (raw file):
### Step 2: Install expo related libraries Note if in a _managed_ expo application these libraries should be present and you should be able to skip this step.
to make it a bit clearer, how about breaking step 1 into two options with bullet points: a) react-native-cli and b) expo. Then here, you can say: if you chose b) in step 1, ....
tfjs-react-native/README.md, line 24 at r2 (raw file):
Install and configure [expo-gl-cpp](https://github.com/expo/expo/tree/master/packages/expo-gl-cpp) and [expo-gl](https://github.com/expo/expo/tree/master/packages/expo-gl) > Note that after this point, if you are using XCode to build for ios, you should use a ‘.workspace’ file instead of the ‘.xcodeproj’
nit: you can remove the word "Note" since you are already prefixing it with >
tfjs-react-native/README.md, line 26 at r2 (raw file):
> Note that after this point, if you are using XCode to build for ios, you should use a ‘.workspace’ file instead of the ‘.xcodeproj’ ### Step 3: Configure Metro
Would be good to say what metro is in 1 sentence. Also mention if there are alternatives? Or do we require metro?
tfjs-react-native/README.md, line 68 at r2 (raw file):
### Optional Steps If you want use the AsyncStorageHandler to save and load models. Add [async-storage](https://github.com/react-native-community/async-storage) to your project.
wrap asyncstoragehandler in quotes and potentially link to some docs. Also s/models. Add/models, add/
tfjs-react-native/integration_rn59/.watchmanconfig, line 1 at r2 (raw file):
{}
curious what is this config file for?
tfjs-react-native/integration_rn59/README.md, line 3 at r2 (raw file):
This is an app that serves and an integration testbed for tfjs-react-native. For more info please see the README in the folder above this one.
link the README using markdown syntax
tfjs-react-native/integration_rn59/tsconfig.json, line 2 at r2 (raw file):
{ "compilerOptions": {
extend from the top-level tsconfig and only override the options you need.
tfjs-react-native/integration_rn59/tslint.json, line 2 at r2 (raw file):
{ "extends": ["tslint-no-circular-imports"],
extend from the top-level tslint and only override the options you need (hopefully no overrides)
tfjs-react-native/integration_rn59/tests/App-test.tsx, line 17 at r2 (raw file):
* ============================================================================= */
do tests have to live in __tests__
folder? Is that a react native convention? If not, let's call the folder simply "tests"
tfjs-react-native/integration_rn59/android/app/build.gradle, line 1 at r1 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
Added here and elsewhere (every code file that didn't already have a copyright notice and some config files).
Not why all config files (e.g. this file is still missing a license)?
tfjs-react-native/integration_rn59/android/app/src/main/res/mipmap-hdpi/ic_launcher.png, line 0 at r2 (raw file):
Curios, where these images generated by a project builder? Do we need them?
tfjs-react-native/integration_rn59/assets/model/bundleModelTest.json, line 1 at r2 (raw file):
{"modelTopology":{"class_name":"Sequential","config":[{"class_name":"Dense","config":{"units":1,"activation":"linear","use_bias":true,"kernel_initializer":{"class_name":"VarianceScaling","config":{"scale":1,"mode":"fan_avg","distribution":"normal","seed":null}},"bias_initializer":{"class_name":"Zeros","config":{}},"kernel_regularizer":null,"bias_regularizer":null,"activity_regularizer":null,"kernel_constraint":null,"bias_constraint":null,"name":"dense_Dense2","trainable":true,"batch_input_shape":[null,10],"dtype":"float32"}}],"keras_version":"tfjs-layers 1.2.7","backend":"tensor_flow.js"},"format":"layers-model","generatedBy":"TensorFlow.js tfjs-layers v1.2.7","convertedBy":null,"weightsManifest":[{"paths":["./bundleModelTest.weights.bin"],"weights":[{"name":"dense_Dense2/kernel","shape":[10,1],"dtype":"float32"},{"name":"dense_Dense2/bias","shape":[1],"dtype":"float32"}]}]}
Let's do camel casing for the file names for consistency (e.g. bundle_model.json
) unless required by the dev environment. Here and elsewhere.
tfjs-react-native/integration_rn59/components/ml.ts, line 34 at r2 (raw file):
/** * Run a simple precision test runner.
Better to say it returns a function, which when called, runs a simple precision test. Here and elsewhere.
tfjs-react-native/integration_rn59/ios/integration_rn59Tests/integration_rn59Tests.m, line 1 at r2 (raw file):
/**
is this a test that you wrote? Can you add a comment what this test does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool to see this progress!! Left a few comments, mostly for my own understanding
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tafsiri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tafsiri)
tfjs-react-native/integration_rn59/android/app/build.gradle, line 1 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Not why all config files (e.g. this file is still missing a license)?
TYPO: Why not* all config files
The monorepo is done, let's move this PR there: https://github.com/tensorflow/tfjs |
@tafsiri before you move this PR, can you reply to my comments here. Once we wrap up the discussions, I can LGTM here, and then you can simply open a PR in the union repo. |
@dsmilkov yep will finish it here and then remake against new repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)
tfjs-react-native/README.md, line 11 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Do you foresee this dependency versioning becoming more relaxed as tfjs-react-native matures? Or are we always going to be strict? Do react-native developers expect such strict versioning requirement?
I expect it to relax over time. Especially backwards. RN 0.60 was a very large jump so requires a lot of changes in the ecosystem (for example it introduces a new JS runtime on Android devices) and from what I can tell changes a number of things about how native libraries work. In our particular case expo-gl does not yet work with RN 0.60 so as soon as it does i imagine this warning will go away.
tfjs-react-native/README.md, line 15 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
is cocoapods only for macs? if yes, let's qualify the statement: If you are on Mac OS, you need to use cocoapods...
Done
tfjs-react-native/README.md, line 19 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
to make it a bit clearer, how about breaking step 1 into two options with bullet points: a) react-native-cli and b) expo. Then here, you can say: if you chose b) in step 1, ....
Done. Added bullets in step 2. I think it is better to do this here since there are multiple expo options and we don't want to leave most of that decision to the user in step 1 as the read about the options.
tfjs-react-native/README.md, line 24 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
nit: you can remove the word "Note" since you are already prefixing it with
>
Done
tfjs-react-native/README.md, line 26 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Would be good to say what metro is in 1 sentence. Also mention if there are alternatives? Or do we require metro?
Yes its a fundamental part of react native (it's the bundler used by react native apps.). I added a link to it here since I think its better to have those explanations live outside of this repo since i think we are targeting people familiar with RN.
tfjs-react-native/README.md, line 68 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
wrap asyncstoragehandler in quotes and potentially link to some docs. Also s/models. Add/models, add/
Done.
Don't have docs to link to right now, but will work on that in a future PR.
tfjs-react-native/integration_rn59/.watchmanconfig, line 1 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
curious what is this config file for?
Watchman is one of the dev tools RN suggests you install (its a filesystem watcher.
I can delete it since I'm not using it.
tfjs-react-native/integration_rn59/README.md, line 3 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
link the README using markdown syntax
Done
tfjs-react-native/integration_rn59/tsconfig.json, line 2 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
extend from the top-level tsconfig and only override the options you need.
Done
tfjs-react-native/integration_rn59/tslint.json, line 2 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
extend from the top-level tslint and only override the options you need (hopefully no overrides)
Done. Just one override. require
is fairly idiomatic RN and can do things that import can't do.
tfjs-react-native/integration_rn59/tests/App-test.tsx, line 17 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
do tests have to live in
__tests__
folder? Is that a react native convention? If not, let's call the folder simply "tests"
I think this is just the default. I don't really use this test. I was leaving it there for future potential investigation of running our unit tests via jest. However since yesterday I've made some progress in another branch of running out unit tests to can probably remove this.
Deleted.
tfjs-react-native/integration_rn59/android/app/build.gradle, line 1 at r1 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
TYPO: Why not* all config files
Ah, i think i just missed this one. (there are two build.gradle's) Fixed.
For others I don't know their syntaxes well enough to know if they support comments (similar to how json doesn't support comments), and vscode was not offering any suggestions (comment shortcut did nothing).
I've dug through a few more and added a bunch more (a few don't seem to support comments from what I can tell.
tfjs-react-native/integration_rn59/android/app/src/main/res/mipmap-hdpi/ic_launcher.png, line at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Curios, where these images generated by a project builder? Do we need them?
These are the app icons that get rendered by os when you install the app.
tfjs-react-native/integration_rn59/assets/model/bundleModelTest.json, line 1 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Let's do camel casing for the file names for consistency (e.g.
bundle_model.json
) unless required by the dev environment. Here and elsewhere.
Done
tfjs-react-native/integration_rn59/components/ml.ts, line 34 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
Better to say it returns a function, which when called, runs a simple precision test. Here and elsewhere.
Since every function in this file does this, I added a comment at the top explaining what a 'runner' is and made all the comments more consistent with this description. PTAL
tfjs-react-native/integration_rn59/ios/integration_rn59Tests/integration_rn59Tests.m, line 1 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
is this a test that you wrote? Can you add a comment what this test does?
Did not write this one. I do think that if we ever wanted to tap into native test writing this would be an example to follow. Probably good to keep around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just one small followup comment. LGTM (feel free to move this PR to the union repo after that, and we can skip re-review there)
Reviewed 1 of 72 files at r1, 1 of 14 files at r2, 20 of 20 files at r3.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @nsthorat)
tfjs-react-native/integration_rn59/ios/integration_rn59Tests/integration_rn59Tests.m, line 1 at r2 (raw file):
Previously, tafsiri (Yannick Assogba) wrote…
Did not write this one. I do think that if we ever wanted to tap into native test writing this would be an example to follow. Probably good to keep around.
Let's add this later when/if we decide to add a test. That way, the PR that adds this file will give more context about it.
To see the logs from the Cloud Build CI, please join either
our discussion
or announcement mailing list.
This adds an app that can serve as an integration testbed for tfjs-react-native. The PR looks somewhat big because there is a fair amount of generated iOS and Android boilerplate. The files that needs review all end in
.ts
.js
and.tsx
(and the regular js and ts configuration files we are used to). A few assets that the app uses are also included. The android and ios subfolders are pretty much all autogenerated.This change is