[tfjs-data] Bazel migration for tfjs-data #5748

pyu10055 · 2021-10-19T23:54:27Z

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

This change is

mattsoulanille

Thanks! I have a few comments. I'll test the build on Windows, and I'll test the output of the build (non-Windows) on tfjs-models (speech commands).

Reviewed 15 of 18 files at r1, 3 of 5 files at r2, 3 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @pyu10055)

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

  ":package_json",

This might not be necessary. I removed it and the tests passed.

tfjs-data/BUILD.bazel, line 38 at r1 (raw file):

   link_workspace_root = True,

This also might not be necessary. Tests passed without it.

link_workspace_root allows absolute requires from the workspace root, like import * as foo from tfjs/tfjs-core/src/index.

tfjs-data/BUILD.bazel, line 48 at r1 (raw file):

        ":tsconfig.json",

Might not need this. It still runs 15 snippets without it.

tfjs-data/BUILD.bazel, line 55 at r1 (raw file):

    link_workspace_root = True,

Might not need this either. Still runs 15 snippets without it.

tfjs-data/BUILD.bazel, line 141 at r4 (raw file):

Quoted 7 lines of code…

js_library(
    name = "package_json",
    package_name = "tfjs-data",
    srcs = [
        ":package.json",
    ],
)

This might not be necessary if there's no version test. Tests passed without it (after removing it from the rules that depended on it).

tfjs-data/cloudbuild.yml, line 5 at r4 (raw file):

Quoted 5 lines of code…

# Install common dependencies.
- name: 'gcr.io/learnjs-174218/release'
  entrypoint: 'yarn'
  id: 'yarn-common'
  args: ['install']

This can be removed since it's only necessary when this cloudbuild file is run on its own, and we don't do that anymore.

tfjs-data/package.json, line 60 at r1 (raw file):

:tfjs-data_webgl2_test

:tfjs-data_browser_test

tfjs-data/package.json, line 62 at r4 (raw file):

"test-dev": "bazel test tfjs-data_test"

Nit: Should this be called "test-node"? We could add another "test-dev" target that uses the same command except with "ibazel", which automatically re-runs tests when inputs change (more of a "dev" experience).

Thinking about developer experience, we might also want targets for debugging the tests.

bazel run :tfjs-data_test --config=debug launches node and waits for a debugger to connect before running.
bazel run :tfjs-data_browser_test launches browser tests without starting them and waits for the user to click "debug".

(unfortunately, we can't just tell users to pass--config=debug whenever they want to debug becaues if you pass it to karma browser tests, that will debug the karma process instead of the tests in the browser)

tfjs-data/src/BUILD.bazel, line 110 at r4 (raw file):

        "@tensorflow/tfjs",
        "worker_threads",
        "util",

I had to add "fs" and "buffer" here as well.

tfjs-data/src/BUILD.bazel, line 116 at r4 (raw file):

        "//tfjs-data:package_json",

Might not need this

tfjs-data/src/util/deep_map.ts, line 100 at r4 (raw file):

    if (input.__proto__) {
      mappedIterable.__proto__ = input.__proto__;
    }

Nit: This looks like a change that should be tested, unless deep_map itself is only used for testing.

mattsoulanille · 2021-10-20T23:00:42Z

Build and test worked on Windows (after rebasing #5745 on top of this PR).

pyu10055

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

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