-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Update packages to compile to ESM modules by default #3112
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! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @kangyizhang, @lina128, @nsthorat, @pyu10055, and @tafsiri)
tfjs-backend-cpu/src/shared.ts, line 19 at r4 (raw file):
// Shared kernel impls for use in other backends. export {transposeImpl as transposeImpl} from './kernels/Transpose_impl';
Can it be export {transposeImpl} from './kernels/Transpose_impl';
tfjs-backend-webgl/src/setup_test.ts, line 46 at r4 (raw file):
// Import and run tests from core. // tslint:disable-next-line:no-imports-from-dist import '@tensorflow/tfjs-core/dist/tests';
optional: This uses a different pattern to import core tests than cpu. Can you use the same pattern as cpu? I think have a run_test.ts in webgl's top level directory will be perfect. I can do this in a separate PR after this PR is merged.
Amazing work. Thank you Yannick! |
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.
Great work, thanks. One high level question, is it possible to share the code between webgl and cpu (symlink or something), instead of having the npm dependency?
Reviewed 40 of 88 files at r1, 1 of 2 files at r2, 3 of 6 files at r3, 15 of 22 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @kangyizhang, @nsthorat, @pyu10055, and @tafsiri)
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.
@pyu10055 In our local setup Na changed the npm packages refer to each other using symlinks (the link:// scheme in package json). When published they should use package names to resolve correctly and share code in different environments (browser and node).
Reviewed 88 of 88 files at r1, 2 of 2 files at r2, 6 of 6 files at r3, 17 of 22 files at r4, 1 of 2 files at r5.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, @kangyizhang, @lina128, and @nsthorat)
tfjs-backend-cpu/src/shared.ts, line 19 at r4 (raw file):
Previously, lina128 (Na Li) wrote…
Can it be export {transposeImpl} from './kernels/Transpose_impl';
Done. good catch.
tfjs-backend-webgl/src/setup_test.ts, line 46 at r4 (raw file):
Previously, lina128 (Na Li) wrote…
optional: This uses a different pattern to import core tests than cpu. Can you use the same pattern as cpu? I think have a run_test.ts in webgl's top level directory will be perfect. I can do this in a separate PR after this PR is merged.
Let's discuss this after this merges. I was under the assumption that the cpu backend uses run_tests because it only runs tests in node. Whereas this uses karma to load this file. We should decide on the patterns used in either case. Note I also think we should not have top level scripts like run_tests, these should go in scripts subfolder imo. It makes managing the tsconfigs easier.
Switch default compile (tsc) options from CommonJS (modules) + ES5 (javascript target version) to ESM (modulex) + ES2017 (javascript target version).
This PR also turns the union package into something releasable (with backend-cpu and backend-webgl.
Rollup
Rollup is used to produce a number of new bundles across the packages:
Note that because package.main now points to tf-[package-name].node.js. Rollup must be run before that package can be imported by other packages in node.js (i.e. all of our scripts and infra).
dist/
dist/
(excluding the files listed above) contains the output from tsc, which is now ESM+ES2017 code. This means that any toolchain that does a “dist import” needs to be able to handle ESM+ES2017.This has been enabled in the build and test scripts using two options
In environments that require commonjs+es5 (i.e. node) we use tsc with a different tsconfig (tsconfig.test.json) to compile the javascript down to the level expected by that tool.
In karma driven test runs we use karma-typescript-es6-transform to transpile ES2017 to ES5, babel polyfills are included as well since that is required by the output of the transform.
You will notice many references to tsconfig.test.json or more local tsconfigs geared towards running scripts and tests rather than the main tsconfig, which is geared towards producing ESM.
This PR also includes some general unification/updating of rollup configs, karma configs and related dependencies. We now get a bundle visualization for every package.
Package.module
package.module now points to dist/index.js (ESM+ES2017). This means any consumer (including end users) that uses the module entry needs to be able to handle that format. We definitely need ESM but can be more flexible on what javascript version we target. I chose 2017 for this PR to lay the groundwork for what we want to do in 3.x. But we can choose any version between ES5 and ES2017+ and the setup in this PR should just work.
Tree shaking
The primary motivation for this is to enable tree-shaking. An example of that is pulling a single function from another package. The graph below shows the bundle visualization for tfjs-backend-webgl, which imports a function
transposeImpl
from tfjs-backend-cpu. Notice the tiny sliver at the top of the image that represents that single function being pulled in.Bundle Size
One motivation for pushing on enabling ES2017 now is bundle size due to less transpilation. Right off the bat we see savings:
Other changes
The ES5 bundle for the union package now includes polyfills, so it will work in IE11.
Other notes
Usage docs have not been updated (e.g. using core needs a backend). I plan on doing this after doing 2.x pre-release and testing these packages through some of our other code (examples, models, etc).
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is