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

Migrate tfjs-tflite package to bazel #5358

Merged
merged 10 commits into from
Aug 2, 2021
Merged

Migrate tfjs-tflite package to bazel #5358

merged 10 commits into from
Aug 2, 2021

Conversation

jinjingforever
Copy link
Collaborator

@jinjingforever jinjingforever commented Jul 20, 2021

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 Reviewable

@google-cla google-cla bot added the cla: yes label Jul 20, 2021
@jinjingforever
Copy link
Collaborator Author

I can see that tflite package has been successfully built and tested here

Copy link
Member

@mattsoulanille mattsoulanille left a 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: :shipit: 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

@lina128 lina128 added the bazel Issues or PRs related to the Bazel build system label Aug 2, 2021
Copy link
Collaborator Author

@jinjingforever jinjingforever left a 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: :shipit: 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 replace jsnext:main with module) :

jsforum/jsforum#5
https://github.com/rollup/rollup/wiki/pkg.module

Good to know! Thanks for the links.

@jinjingforever jinjingforever merged commit 6bfe297 into master Aug 2, 2021
@jinjingforever jinjingforever deleted the tflitebazel branch August 2, 2021 23:02
mattsoulanille pushed a commit to mattsoulanille/tfjs that referenced this pull request Aug 6, 2021
* save

* save

* save

* save

* save

* save

* save

* save

* fix

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel Issues or PRs related to the Bazel build system cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build tfjs-tflite with Bazel
3 participants