-
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
Use Bazel and ts_library to build core and backend-cpu #5133
Use Bazel and ts_library to build core and backend-cpu #5133
Conversation
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.
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @lina128, @mattsoulanille, and @pyu10055)
tfjs-backend-cpu/package.json, line 6 at r6 (raw file):
Previously, mattsoulanille (Matthew Soulanille) wrote…
We currently distribute our
.js
outputs as esmodules and a separate node bundle, however, with Bazel, we'll distribute the.js
files as commonjs and new.mjs
files as esmodules. That way, we can skip bundling for node because node can just use the commonjs files.Also, since Bazel rewrites the commonjs module imports to use module names instead of relative paths, this change was required to make webgl karma tests pass. Webgl would import a file from
@tensorflow/tfjs-core/dist/...
which would then import@tensorflow/tfjs-core
(since bazel changed relative imports to module name imports), but since the package.json listed the node bundle as the main entrypoint, it would import the bundle instead oftfjs-core/dist/index.js
, which would result in duplicate definitions of classes. Specifically, this brokerouter_registry_test.ts
.
Thank you for the detailed explanation!
tensorflow/tfjs-models#757 should probably be merged before this PR. The current version of parcel-bundler that we use in models demos seems to have problems building with the Bazel outputs. Parcel 2 has no issues and neither does esbuild. |
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.
Ah, this reminds me, what's the testing plan for release with the new Bazel build? Will you use Verdaccio for release testing? Any change to publish-tfjs-ci.sh?
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @lina128, @mattsoulanille, and @pyu10055)
Release tests should run the same as before when we release, but I'll take a look (and try running them) to make sure. I've updated the release script to take into account how tfjs-core and tfjs-backend-cpu are published (I need to add this to the BAZEL_MIGRATION.md doc). |
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.
e2e/scripts/publish-tfjs-ci.sh uses yarn build-npm for-publish
and npm publish
, this probably needs to be changed?
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @lina128, @mattsoulanille, and @pyu10055)
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.
Good point! I changed publish-tfjs-ci.sh
to be aware of Bazel. Here's a branch where I tested verdaccio release tests and here's them passing on GCP.
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @lina128, @mattsoulanille, and @pyu10055)
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.
Reviewable status:
complete! 2 of 1 approvals obtained (waiting on @lina128 and @mattsoulanille)
…o bazel_ts_library
…o bazel_ts_library
This PR builds tfjs-core and tfjs-backend-cpu using Bazel and makes the build outputs available to the other packages in the monorepo.
New Bazel
tools
directoryThis PR adds a new
tools
directory that includes Bazel rules and macros for building TensorFlow.jstools/defaults.bzl
Defines default versions of
ts_library
andesbuild
that are specific to the tfjs repo.ts_library
is set to usees2017
and has itstsconfig.json
file set totsconfig_ts_library.json
.esbuild
has the correct esbuild binary for the user's platform pre-selected.esbuild
is used for tests since it is very fast.tools/tfjs_bundle.bzl
tfjs_bundle
is a macro to build bundles for tfjs. It defines severalrollup_bundle
targets and their minified variants:[name].es2017[.min].js
: A UMD es2017 bundle.[name][.min].js
: A UMD es5 bundle.[name].fesm[.min].js
: An es2017 flat es modules bundle.[name].node[.min].js
: A commonjs es5 node bundle. (Note: This may be removed)Rollup plugins and options are defined in the
rollup_template.config.js
file.tools/tfjs_web_test.bzl
tfjs_web_test
is a macro that defines severalkarma_web_test
targets, most of which are run on BrowserStack:[name]
: A localkarma_web_test
target that runs using chrome.browserstack_[browser_name]_[name]
: Akarma_web_test
that runs on BrowserStack using[browser_name]
.tools/karma_template.conf.js
browsers
keyword argument.tools/copy_to_dist
copy_to_dist
is a rule that creates symlinks for a given tree of files in another directory. It is used to copy thets_library
compilation results fromsrc
todist
to support creating an npm package with outputs indist
. It is also used to copy bundles fromtfjs_bundle
todist
(and any other files that need to be indist
).To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
Building
tfjs-core
andtfjs-backend-cpu
with BazelThere are several steps to building
tfjs-core
. Whiletfjs-core
itself does not depend on a backend, its tests requiretfjs-backend-cpu
, so we build them separately to avoid a circular dependency.The following Bazel targets build
tfjs-core
without tests://tfjs-core/src:tfjs-core_src_lib
: Builds all oftfjs-core
excluding tests and theindex.ts
entrypoint.module_name = "@tensorflow/tfjs-core/dist"
so that its files can be imported relative to that path. e.g.import @tensorflow/tfjs-core/dist/public/chained_ops/register_all_chained_ops
.//tfjs-core/src:tfjs-core_lib
: Buildsindex.ts
entrypoint and depends on//tfjs-core/src:tfjs-core_src_lib
.module_name = "@tensorflow/tfjs-core"
so it can be imported as@tensorflow/tfjs-core
.After
tfjs-core
is built,tfjs-backend-cpu
can be built by the following rules://tfjs-backend-cpu/src:tfjs-backend-cpu_src_lib
: Like with core, builds all oftfjs-backend-cpu
excluding tests and theindex.ts
entrypoint. Depends on//tfjs-core/src/tfjs-core_lib
.//tfjs-backend-cpu/src:tfjs-backend-cpu_lib
: Like with core, builds theindex.ts
entrypoint. These steps are similarly separated so thatmodule_name
can be set differently for each.With
tfjs-backend-cpu
built,tfjs-core
can be tested://tfjs-core/src/tfjs-core_test_lib
: Builds all the test files.//tfjs-core:tfjs-core_node_test
: Runstfjs-core_test_lib
tests in node.//tfjs-core/src:tfjs-core_test_bundle
: Builds a browser bundle for tests.//tfjs-core:tfjs-core_test
: Runs web tests using the bundle.With
tfjs-core
tests build,tfjs-backend-cpu
can be tested://tfjs-backend-cpu/src:tfjs-backend-cpu_test_lib
: Builds cpu tests.//tfjs-backend-cpu:tfjs-backend-cpu_test
: Runs cpu tests in node.Creating NPM packages
The
pkg_npm
rule is used to create an npm package fortfjs-core
, but there are a few steps that need to be done first://tfjs-core:tf-core
: Bundles//tfjs-core/src:tfjs-core_lib
usingtfjs_bundle
.//tfjs-core:copy_bundles
: Copies bundles todist/
(in bazel's output folder).//tfjs-core:copy_src_to_dist
: Copies the compiled results of//tfjs-core/src:tfjs-core[_src, _test]_lib
todist
.Once bundles are built and everything is copied, the npm package can be created:
//tfjs-core:tfjs-core_pkg
: Packages everything into an npm package.A similar process to the above is used to package
tfjs-backend-cpu
.Inter-operating with existing packages
Packages in the monorepo that don't yet use Bazel are linked to each other via
link:../package-name
dependencies in theirpackage.json
files. These link dependencies expect the outputs of building a package to appear in the package's directory, but Bazel doesn't allow that. Instead, Bazel places build artifacts in a location completely separate from the source files. This output directory can change, but there is a symlink to it nameddist
for easy access.This means we can just change the
link:../
dependencies to point to../dist/bin/tfjs-backend-cpu/tfjs-backend-cpu_pkg
, right? Unfortunately, we can't, because of how node resolve dependencies. Suppose we're buildingtfjs-layers
, which depends ontfjs-backend-cpu
. If we link to../dist/bin/tfjs-backend-cpu/tfjs-backend-cpu_pkg
, then whentfjs-backend-cpu
imports / requires@tensorflow/tfjs-core
, it moves up thedist/bin/tfjs-backend-cpu
directory tree and looks in the rootnode_modules
of thetfjs
repo. Due to some specifics of howts_library
works, thisnode_modules
contains an incomplete copy of@tensorflow/tfjs-core
that is created by Bazel for running node tests, so the import fails.We can't replace
link:../
withfile:../
because that would result in multiple imports of multiple copies of the same code, which creates several copies of things that are supposed to be singletons, likeio
handlers.To work around these issues, this PR creates a new top-level
link-package
that provides a place where bazel-built packages can belink:../
ed from. Itspackage.json
currently lists@tensorflow/tfjs-core
and@tensorflow/tfjs-backend-cpu
asfile:../
dependencies copied from../dist/bin/...
. With this new link package,tfjs-layers
and others can now list@tensorflow/tfjs-backend-cpu
aslink: ../link-package/node_modules/@tensorflow/tfjs-backend-cpu
. Now, whentfjs-backend-cpu
looks fortfjs-core
, it will find the correcttfjs-core
, since the link package has both installed in itsnode_modules
.The link package should never be published to npm, and will be removed once the monorepo fully builds with Bazel.
This change is