[tfjs-layers] Bazel migration #5672

pyu10055 · 2021-09-27T23:19:13Z

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

This change is

mattsoulanille

Reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

tfjs-layers/BUILD.bazel, line 33 at r1 (raw file):

nodejs_test(
    name = "test_snippets_test",

We could perhaps define a rule or macro that tests snippets if we have multiple packages that need it. Not in this PR though.

tfjs-layers/BUILD.bazel, line 60 at r1 (raw file):

    random = "false",

Did this end up working?

tfjs-layers/BUILD.bazel, line 64 at r1 (raw file):

Quoted 6 lines of code…

        "//tfjs-backend-cpu:tf-backend-cpu.min.js",
        "//tfjs-backend-cpu:tf-backend-cpu.min.js.map",
        "//tfjs-backend-webgl:tf-backend-webgl.min.js",
        "//tfjs-backend-webgl:tf-backend-webgl.min.js.map",
        "//tfjs-core:tf-core.min.js",
        "//tfjs-core:tf-core.min.js.map",

Are these bundles needed? The tfjs-layers test bundle should include everything needed for the test, including tfjs-core and the backends.

tfjs-layers/cloudbuild.yml, line 18 at r1 (raw file):

Quoted 7 lines of code…

# Lint.
- name: 'gcr.io/learnjs-174218/release'
  dir: 'tfjs-layers'
  entrypoint: 'yarn'
  id: 'lint'
  args: ['lint']
  waitFor: ['yarn']

We should have a global lint script. I can write that in a different PR.

tfjs-layers/package.json, line 16 at r1 (raw file):

    "@bazel/ibazel": "^0.15.10",

You should also add @bazel/bazelisk for when this is build for release.

tfjs-layers/package.json, line 48 at r1 (raw file):

Quoted 22 lines of code…

    "prep": "yarn install && yarn build-ci",
    "build": "tsc && yarn bundle",
    "build-ci": "tsc && yarn bundle-ci",
    "bundle": "rollup -c",
    "bundle-ci": "rollup -c --ci",
    "build-backend-webgl": "cd ../tfjs-backend-webgl && yarn && yarn build",
    "build-backend-webgl-ci": "cd ../tfjs-backend-webgl && yarn && yarn build-ci",
    "build-link-package": "cd ../link-package && yarn build",
    "build-deps": "yarn build-link-package && yarn build-backend-webgl",
    "build-deps-ci": "yarn build-link-package && yarn build-backend-webgl-ci",
    "build-npm": "./scripts/build-npm.sh",
    "format": "./tools/clang_format_ts.sh",
    "link-local": "yalc link",
    "publish-local": "yarn build-npm && yalc push",
    "publish-npm": "bazel run :tfjs-layers_pkg.publish",
    "coverage": "KARMA_COVERAGE=1 karma start --browsers='Chrome' --singleRun",
    "test": "yarn && yarn build-deps && karma start",
    "test-dev": "karma start",
    "test-ci": "./scripts/test-ci.sh",
    "test-snippets": "yarn && yarn build-deps && yarn build && ts-node --skip-ignore -s ./scripts/test_snippets.ts",
    "test-snippets-ci": "ts-node --skip-ignore -s ./scripts/test_snippets.ts",
    "run-browserstack": "karma start --browsers='bs_chrome_mac' --singleRun --reporters='dots,karma-typescript'",

I think some of these scripts should be updated / removed for the bazel build. build-npm will need to be updated since the release script uses it.

You can probably also remove a few of the files in the scripts directory, like build-npm, test-ci, and tsconfig.

tfjs-layers/src/model_save_test.ts, line 300 at r1 (raw file):

Quoted 4 lines of code…

    // Assert that orthogonal initializer hasn't been obtained during
    // the model loading.
    expect(getInitSpy).toHaveBeenCalledWith('zeros');
    expect(gramSchmidtSpy).not.toHaveBeenCalled();

Nit: The diff shows this file having only deletions. Just checking if that's intentional.

tfjs-layers/src/engine/training_test.ts, line 2454 at r1 (raw file):

  // resolveScalarsInLogs, it will be better to test result instead of internal
  // code structure.
  // tslint:disable-next-line: ban

Sounds good to me

tools/karma_template.conf.js, line 101 at r1 (raw file):

jasmine: {random: TEMPLATE_random}

Did this end up working?

tools/tfjs_web_test.bzl, line 47 at r1 (raw file):

Quoted 4 lines of code…

        "random": attr.string(
            default = "true",
            doc = "Jasmine tests run in random",
        ),

Same question here. Does this work, or does Bazel override the argument?

mattsoulanille · 2021-09-29T23:29:42Z

One more thing, which esbuild found:

 > bazel-out/k8-fastbuild/bin/tfjs-layers/src/layers/normalization.mjs:343:30: warning: Comparison using the "!==" operator here is always true
    343 │                     this.axis !== [nDims - 1]) {

LayerNormalization might have a bug. We can fix this in a different PR, though.

pyu10055

thanks for the review, fixed this error.

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)

tfjs-layers/BUILD.bazel, line 60 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

    random = "false",
Did this end up working?

removed, I don't think this change has an effects.

tfjs-layers/BUILD.bazel, line 64 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

        "//tfjs-backend-cpu:tf-backend-cpu.min.js",
        "//tfjs-backend-cpu:tf-backend-cpu.min.js.map",
        "//tfjs-backend-webgl:tf-backend-webgl.min.js",
        "//tfjs-backend-webgl:tf-backend-webgl.min.js.map",
        "//tfjs-core:tf-core.min.js",
        "//tfjs-core:tf-core.min.js.map",

Are these bundles needed? The tfjs-layers test bundle should include everything needed for the test, including tfjs-core and the backends.

tfjs-layers run tests on cpu and webgl backends.

tfjs-layers/package.json, line 16 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

    "@bazel/ibazel": "^0.15.10",
You should also add @bazel/bazelisk for when this is build for release.

Done.

tfjs-layers/package.json, line 48 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

    "prep": "yarn install && yarn build-ci",
    "build": "tsc && yarn bundle",
    "build-ci": "tsc && yarn bundle-ci",
    "bundle": "rollup -c",
    "bundle-ci": "rollup -c --ci",
    "build-backend-webgl": "cd ../tfjs-backend-webgl && yarn && yarn build",
    "build-backend-webgl-ci": "cd ../tfjs-backend-webgl && yarn && yarn build-ci",
    "build-link-package": "cd ../link-package && yarn build",
    "build-deps": "yarn build-link-package && yarn build-backend-webgl",
    "build-deps-ci": "yarn build-link-package && yarn build-backend-webgl-ci",
    "build-npm": "./scripts/build-npm.sh",
    "format": "./tools/clang_format_ts.sh",
    "link-local": "yalc link",
    "publish-local": "yarn build-npm && yalc push",
    "publish-npm": "bazel run :tfjs-layers_pkg.publish",
    "coverage": "KARMA_COVERAGE=1 karma start --browsers='Chrome' --singleRun",
    "test": "yarn && yarn build-deps && karma start",
    "test-dev": "karma start",
    "test-ci": "./scripts/test-ci.sh",
    "test-snippets": "yarn && yarn build-deps && yarn build && ts-node --skip-ignore -s ./scripts/test_snippets.ts",
    "test-snippets-ci": "ts-node --skip-ignore -s ./scripts/test_snippets.ts",
    "run-browserstack": "karma start --browsers='bs_chrome_mac' --singleRun --reporters='dots,karma-typescript'",

I think some of these scripts should be updated / removed for the bazel build. build-npm will need to be updated since the release script uses it.

You can probably also remove a few of the files in the scripts directory, like build-npm, test-ci, and tsconfig.

Done.

tfjs-layers/src/model_save_test.ts, line 300 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

    // Assert that orthogonal initializer hasn't been obtained during
    // the model loading.
    expect(getInitSpy).toHaveBeenCalledWith('zeros');
    expect(gramSchmidtSpy).not.toHaveBeenCalled();

Nit: The diff shows this file having only deletions. Just checking if that's intentional.

This is the work around for the spyOn calls, assuming the data is correct instead of verifying internal method has been called.

tools/karma_template.conf.js, line 101 at r1 (raw file):

removed, I don't think this change has an effects.

tools/tfjs_web_test.bzl, line 47 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

        "random": attr.string(
            default = "true",
            doc = "Jasmine tests run in random",
        ),
Same question here. Does this work, or does Bazel override the argument?

removed, I don't think this change has an effects.

mattsoulanille

Thanks for the fixes! I have a few more comments.

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

tfjs-layers/BUILD.bazel, line 64 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

tfjs-layers run tests on cpu and webgl backends.

I tested without them, and the tests passed. I think Bazel bundles them in the test bundle, and we can skip adding them here and avoid the extra dependency.

tfjs-layers/package.json, line 48 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

Done.

Thanks! Looks good.

tfjs-layers/src/model_save_test.ts, line 300 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

This is the work around for the spyOn calls, assuming the data is correct instead of verifying internal method has been called.

Sounds good. Thanks!

tools/karma_template.conf.js, line 101 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

removed, I don't think this change has an effects.

I think this file was missed in your latest push.

tools/tfjs_web_test.bzl, line 47 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

removed, I don't think this change has an effects.

I think this was missed in the latest push.

pyu10055

Reviewable status: 0 of 1 approvals obtained (waiting on @mattsoulanille)

tfjs-layers/BUILD.bazel, line 64 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

I tested without them, and the tests passed. I think Bazel bundles them in the test bundle, and we can skip adding them here and avoid the extra dependency.

Done.

tools/karma_template.conf.js, line 101 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

I think this file was missed in your latest push.

sorry, just added.

tools/tfjs_web_test.bzl, line 47 at r1 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

I think this was missed in the latest push.

Done.

mattsoulanille

Thanks for the fixes. No more comments on them, but I found something else. The new bundle contains the long package while the old one does not. Is that intentional? You can see this if you check the visualization at dist/bin/tfjs-layers/tf-layers.min_stats.html against the original build's visualization file, which can be generated with yarn build-npm and appears in tfjs-layers/dist/tf-layers.min.js.html. The new bundle also seems to be ~40Kb (~13%) larger, but this might be acceptable.

Reviewed 3 of 3 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained

pyu10055 added 12 commits September 22, 2021 10:25

add more bazel build files

1111f91

remove the integration test directory

3068e33

Merge branch 'master' into bazel_layers

c0d5cab

enumerate tests

c9440b1

require tests files

6ef95d2

fix tests

198afc0

fixed spyOn test errors

49cf0c7

fix more tests

4c33721

keep function name for esbuild

20173ff

fix more tests

7662938

allow random flag for jasmine test

0bfff65

updated deps

1c0f987

google-cla bot added the cla: yes label Sep 27, 2021

pyu10055 added 8 commits September 27, 2021 16:20

Merge branch 'master' into bazel_layers

f467af4

merged

9cc3d71

fixed bazel lint

c42f2f7

fixed layers snippet tests

10a2d3c

fixed tests

83dc37e

Merge branch 'master' into bazel_layers

aff9817

fix test failure

0311586

fix bazel tests

0bd9afa

pyu10055 requested a review from mattsoulanille September 29, 2021 20:55

mattsoulanille requested changes Sep 29, 2021

View reviewed changes

pyu10055 commented Sep 30, 2021

View reviewed changes

addressed comments

435e163

mattsoulanille requested changes Sep 30, 2021

View reviewed changes

addressed comments

58d3cd0

pyu10055 commented Sep 30, 2021

View reviewed changes

Merge branch 'master' into bazel_layers

efa9490

mattsoulanille approved these changes Sep 30, 2021

View reviewed changes

pyu10055 added 3 commits September 30, 2021 15:23

fix test

3ca131b

fix bazel failure

43d733b

Merge branch 'master' into bazel_layers

ad77125

pyu10055 merged commit ed94a27 into master Oct 1, 2021

pyu10055 deleted the bazel_layers branch October 1, 2021 05:50

mattsoulanille mentioned this pull request Oct 22, 2021

Build tfjs-layers with Bazel #5278

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tfjs-layers] Bazel migration #5672

[tfjs-layers] Bazel migration #5672

pyu10055 commented Sep 27, 2021 •

edited by nsthorat

Loading

mattsoulanille left a comment

mattsoulanille commented Sep 29, 2021

pyu10055 left a comment

mattsoulanille left a comment

pyu10055 left a comment

mattsoulanille left a comment

[tfjs-layers] Bazel migration #5672

[tfjs-layers] Bazel migration #5672

Conversation

pyu10055 commented Sep 27, 2021 • edited by nsthorat Loading

mattsoulanille left a comment

Choose a reason for hiding this comment

mattsoulanille commented Sep 29, 2021

pyu10055 left a comment

Choose a reason for hiding this comment

mattsoulanille left a comment

Choose a reason for hiding this comment

pyu10055 left a comment

Choose a reason for hiding this comment

mattsoulanille left a comment

Choose a reason for hiding this comment

pyu10055 commented Sep 27, 2021 •

edited by nsthorat

Loading