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

[wasm] Build tfjs-backend-wasm entirely with Bazel #6458

Merged
merged 42 commits into from Jun 14, 2022

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented May 26, 2022

Build tfjs-backend-wasm with Bazel. tfjs-backend-wasm already uses Bazel for Emscripten. This PR makes it use Bazel for TypeScript as well and replaces the build scripts with a single pkg_npm target.

Fixes #5282

  • Make sure the package is added to BAZEL_PACKAGES in e2e/scripts/publish-tfjs-ci.sh
  • Make sure the package is added to BAZEL_PACKAGES in scripts/bazel_packages.ts
  • Make sure the package generated by pkg_npm has all the files it needs, e.g. the README.
  • Make sure the package is added to the link-package's package.json and that downstream pakcages are updated to point to the link package's copy instead of the package's directory.
  • For browser tests, it may be worth checking that all desired browser configurations will run in nightly CI.
  • Make sure browser tests include all required tests. The enumerate_tests rule is usually necessary to make the browser actually run tests.
  • Make sure as many cloudbuild steps as possible are converted to Bazel, and that those steps are removed from the cloudbuild file.
  • If the build and tests are fully handled by Bazel and don't need any other cloudbuild steps, make sure the package's cloudbuild.yml file is removed. Do not remove the package from scripts/package_dependencies.json.
  • Make sure tests are tagged with nightly or ci (tfjs_web_test automatically tags tests with nightly and ci).
  • Make sure the main pkg_npm rule is tagged with ci or nightly so all parts of the build are tested.
  • Make sure the package.json scripts are updated and that the package.json includes @bazel/bazelisk as a dev dependency.
  • Make sure the package has a build-npm script and a publish-npm script. These are used by the release script.
  • Check the generated bundle sizes and make sure they don't include any unexpected files. Check the _stats files for info on this.
  • Make sure the package is added to the repo-wide tslint tsconfig and that its original lint scripts are removed.

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


This change is Reviewable

@mattsoulanille mattsoulanille added this to In progress in Adopt Bazel via automation May 26, 2022
@mattsoulanille mattsoulanille force-pushed the wasm_bazel branch 2 times, most recently from 98f5cae to 87bd487 Compare June 2, 2022 23:13
@mattsoulanille mattsoulanille force-pushed the wasm_bazel branch 2 times, most recently from c97c499 to 126dccc Compare June 6, 2022 23:25
@mattsoulanille mattsoulanille marked this pull request as ready for review June 6, 2022 23:25
tsconfig.json Outdated Show resolved Hide resolved
Copy link
Member Author

@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.

Reviewable status: 0 of 1 approvals obtained (waiting on @jinjingforever and @pyu10055)


link-package/build_deps.ts line 23 at r2 (raw file):

import * as path from 'path';
import * as rimraf from 'rimraf';
import {BAZEL_PACKAGES} from '../scripts/bazel_packages';

This change is moved to #6510

Code quote:

import {BAZEL_PACKAGES} from '../scripts/bazel_packages';

scripts/bundle-size-util.js line 24 at r2 (raw file):

      +(exec(`ls -l ${filename} | awk '{print $5}'`, {silent: true}));
  const gzipFileSizeBytes =
      +(exec(`ls -l ${gzipFilename} | awk '{print $5}'`, {silent: true}));

Moved to #6509

Code quote:

function getFileSizeBytes(filename) {
  const gzipFilename = `${filename}.gzip`;
  exec(`gzip -c ${filename} > ${gzipFilename}`, {silent: true});
  const fileSizeBytes =
      +(exec(`ls -l ${filename} | awk '{print $5}'`, {silent: true}));
  const gzipFileSizeBytes =
      +(exec(`ls -l ${gzipFilename} | awk '{print $5}'`, {silent: true}));
  return {fileSizeBytes, gzipFileSizeBytes};

Copy link
Member Author

@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.

Reviewable status: 0 of 1 approvals obtained (waiting on @jinjingforever and @pyu10055)


tfjs-backend-wasm/wasm-out/BUILD.bazel line 57 at r3 (raw file):

create_worker_module(
    name = "create_worker_module",
    cjs = True,

Note that this changes the WASM worker module's format to commonjs.

export const wasmWorkerContents = becomes module.exports.wasmWorkerContents =

This is to make node tests work.

Code quote:

    cjs = True,

Copy link
Collaborator

@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.

This is awesome! Thank you for making this work. And it is nice to see the checklist in the PR!

BTW, do we already have a way to test the output package of this new build method in some end-to-end tests?

@mattsoulanille
Copy link
Member Author

Thanks for the review, Jing! I added tfjs-backend-wasm to our e2e tests. It loads correctly, but there seems to be a memory leak when calculating gradients.

1) can compute grad of prediction
     #SMOKE tf.grad for layers models layers_model wasm {}
     Expected function not to throw, but it threw Error: Backend 'wasm' has an internal memory leak (4 data ids) after running 'fusedMatMul__op'.
    at <Jasmine>
    at Object.<anonymous> (integration_tests/grad_layers_test.ts:49:14 <- integration_tests/grad_layers_test.js:60:24)
    at step (integration_tests/grad_layers_test.js:32:23)
    at Object.next (integration_tests/grad_layers_test.js:13:53)
    at integration_tests/grad_layers_test.js:7:71
    at <Jasmine>
    at __awaiter (integration_tests/grad_layers_test.js:3:12)
    at UserContext.<anonymous> (integration_tests/grad_layers_test.ts:38:42 <- integration_tests/grad_layers_test.js:49:67)
    at <Jasmine>

I added the original wasm build to e2e to see if this is a regression, and it doesn't seem to be. The same error appears in the original build. Maybe I should skip that test for this PR and fix it in a different one.

@jinjingforever
Copy link
Collaborator

SGTM. Thanks!

@mattsoulanille mattsoulanille force-pushed the wasm_bazel branch 2 times, most recently from 19888d6 to c6491d3 Compare June 9, 2022 18:27
Copy link
Collaborator

@pyu10055 pyu10055 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 for pushing this forward!

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @mattsoulanille)


e2e/integration_tests/setup_test.ts line 39 at r5 (raw file):

});

registerTestEnv({

if we are not testing wasm yet in e2e, should this registration be added later?

Copy link
Member Author

@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.

Thanks for the review!

Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @pyu10055)


e2e/integration_tests/setup_test.ts line 39 at r5 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

if we are not testing wasm yet in e2e, should this registration be added later?

We're still testing wasm in e2e. I've just disabled the one failing test that appeared when I added it to e2e.

@mattsoulanille mattsoulanille merged commit 2745ea9 into tensorflow:master Jun 14, 2022
Adopt Bazel automation moved this from In progress to Done Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Build tfjs-backend-wasm entirely with Bazel
3 participants