-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[tfjs-react-native] Fix bundleResourceIO in release builds #2345
Conversation
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.
Few comments, all small. LGTM!!
Reviewed 16 of 18 files at r1, 3 of 3 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @tafsiri)
tfjs-react-native/package.json, line 24 at r2 (raw file):
"test": "karma start", "test-ci": "./scripts/test-ci.sh", "copydist": "\\cp -rf dist ./integration_rn59/node_modules/@tensorflow/tfjs-react-native"
do you need those 2 back slashes at the start? Also can you move this command to the package.json of the integration app? ideally the package.json of the react-package itself shouldn't have anything to do with end-user apps.
tfjs-react-native/README.md, line 75 at r2 (raw file):
- Install @tensorflow/tfjs-react-native - `npm install @tensorflow/tfjs-react-native@alpha` ### Step 5: Install and configure other peerDependencies
nit: add space between peer and dependencies, unless you really want to be precise with the keyword in package.json
tfjs-react-native/integration_rn59/components/ml.ts, line 84 at r2 (raw file):
* through it. */ const modelJson2 = require('../assets/graph_model/model.json');
for my own understanding, does this mean that the integration test will now use a bundled model, instead of fetching over http?
tfjs-react-native/src/bundle_resource_io.ts, line 91 at r2 (raw file):
modelArtifacts.weightSpecs = modelJson.weightsManifest[0].weights; //@ts-ignore delete modelArtifacts.weightManifest;
is this explicit delete here so that the GC can collect memory sooner?
tfjs-react-native/src/test_utils/expo_asset_mock.ts, line 22 at r2 (raw file):
export const Asset = { fromModule: () => { // The actual response will be mocked in the test so we only need t
typo at the very end
tfjs-react-native/src/test_utils/react_native_fs_mock.ts, line 20 at r2 (raw file):
// Mock react_native_fs module in browsers. // tslint:disable-next-line export default {};
no need to provide mocks for readFile and readFileRes here?
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 18 of 18 files at r1.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)
tfjs-react-native/package.json, line 24 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
do you need those 2 back slashes at the start? Also can you move this command to the package.json of the integration app? ideally the package.json of the react-package itself shouldn't have anything to do with end-user apps.
Done. (Moved to integration test app.
The first slash escapes the second one which allows the cp command to run uninterrupted by prompts for file overwriting in more shells (including mine).
tfjs-react-native/README.md, line 75 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
nit: add space between peer and dependencies, unless you really want to be precise with the keyword in package.json
Yeah here i really want to reference the package.json peerDependencies concept.
tfjs-react-native/integration_rn59/components/ml.ts, line 84 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
for my own understanding, does this mean that the integration test will now use a bundled model, instead of fetching over http?
This particular addition was just to add an option to use a graphModel in addition to the layersModel used above. Syntax wise it implies no http server.
However this overall change was triggered by learning that in dev mode even bundled resources are actually served by a dev server (even though they are in the bundle). Testing release builds is currently manual.
tfjs-react-native/src/bundle_resource_io.ts, line 91 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
is this explicit delete here so that the GC can collect memory sooner?
The deletion is to align to a the io.ModelArtifacts interface which doesn't have a weightManifest property, it has a weightSpec instead which is constructed from the weightManifest on the previous line. In these methods I opted not to clone + filter the original object.
tfjs-react-native/src/test_utils/expo_asset_mock.ts, line 22 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
typo at the very end
Fixed
tfjs-react-native/src/test_utils/react_native_fs_mock.ts, line 20 at r2 (raw file):
Previously, dsmilkov (Daniel Smilkov) wrote…
no need to provide mocks for readFile and readFileRes here?
Yep. Those paths cannot be triggered in the browser tests.
Updates how files are resolved by the bundle resource loader in prod builds. Addresses #2177
Also update the message from the unit tests when browserstack is unable to talk to cloud.
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is