-
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
Migrate tfjs-tflite package to bazel #5358
Conversation
I can see that tflite package has been successfully built and tested 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.
LGTM with a nit about the package.json jsnext:main
field. Thank you!
Reviewed 21 of 22 files at r1, 1 of 1 files at r2.
Reviewable status:complete! 1 of 1 approvals obtained (waiting on @jinjingforever)
scripts/publish-npm.ts, line 30 at r2 (raw file):
const BAZEL_PACKAGES = new Set(['tfjs-core', 'tfjs-backend-cpu']);
Nit: We can add tflite to this set even though we don't publish it yet since we only check if the set contains the package, we don't iterate the set to publish packages. Alternatively, we can do it once we start publishing tflite. Either is fine.
Same with the e2e test.
tfjs-backend-cpu/package.json, line 52 at r2 (raw file):
"bundle": "bazel build :tfjs-backend-cpu_pkg",
Thanks for the fix!
tfjs-tflite/package.json, line 7 at r2 (raw file):
"jsnext:main": "dist/index.js"
Nit: I think this should be dist/index.mjs
, (and we may want to replace jsnext:main
with module
) :
jsforum/jsforum#5
https://github.com/rollup/rollup/wiki/pkg.module
2f3afcd
to
56c5f55
Compare
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.
Thank you!
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @mattsoulanille)
scripts/publish-npm.ts, line 30 at r2 (raw file):
Previously, mattsoulanille (Matthew Soulanille) wrote…
const BAZEL_PACKAGES = new Set(['tfjs-core', 'tfjs-backend-cpu']);
Nit: We can add tflite to this set even though we don't publish it yet since we only check if the set contains the package, we don't iterate the set to publish packages. Alternatively, we can do it once we start publishing tflite. Either is fine.
Same with the e2e test.
Done
tfjs-tflite/package.json, line 7 at r2 (raw file):
Previously, mattsoulanille (Matthew Soulanille) wrote…
"jsnext:main": "dist/index.js"
Nit: I think this should be
dist/index.mjs
, (and we may want to replacejsnext:main
withmodule
) :jsforum/jsforum#5
https://github.com/rollup/rollup/wiki/pkg.module
Good to know! Thanks for the links.
* save * save * save * save * save * save * save * save * fix * fix
I mostly followed the migration instructions (very well written! Thanks!). For downloading stuff from google storage, I used genrule that runs the download script. To make it work, I had to change the download script to use "wget" (instead of gsutil) to download the zipped file from google storage and unzip it. (the bucket is public-accessible). The reason is that our "node:10" docker image doesn't have gsutil installed, and the image from google cloud (gcr.io/google.com/cloudsdktool/cloud-sdk) does have gsutil installed, but not nodejs. "node:10" does have "wget" and "unzip" installed.
I also updated the demo of tfjs-tflite to use
link-package
.Fixes #5284
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is