Skip to content

Conversation

@bmd3k
Copy link
Contributor

@bmd3k bmd3k commented Nov 9, 2022

The upcoming upgrade to Angular 13+ introduces a number of bundling-related problems for our tests and applications. Angular has developed some internal tooling to help resolve these problems. This change uses Angular's internal spec_bundle() macro to allow us to continue to run Karma tests with Angular 13+.

Note: spec_bundle() is Angular-internal and not supported for external use. We use it at our own risk. Googlers, see the following documents for background and justification: http://go/angular-bazel-problems, http://go/tb-oss-bundling.

Our usage of spec_bundle() is largely guided/inspired by the usage in the angular/components repo. This is the version at HEAD as of yesterday:

Highlights.

  1. Run the following commands to install Angular's build tooling and Babel and then clean the environment:
  • yarn add https://github.com/angular/dev-infra-private-build-tooling-builds.git#fb42478534df7d48ec23a6834fea94a776cb89a0 @babel/core@^7.16.12 --dev
  • yarn run yarn-deduplicate
  • rm -rf node_modules; bazel clean --expunge; yarn;
  1. Automaticaly rename, at build-time, *test.ts files to *test.spec.ts files so that they are recognized by spec_bundle() as top-level test files to be executed. Otherwise they would be ignored and the tests not executed.
  2. Modify our build rules to produce correct compiler output and bundle them using extract_js_module_output() and spec_bundle().
  3. Cleanup files we no longer need. There are a number of shims and hacks that we needed for compatibility with karma test suite that are now unnecessary due to how the spec_bundle() tooling works.

@bmd3k bmd3k marked this pull request as ready for review November 10, 2022 16:16
@bmd3k bmd3k requested a review from JamesHollyer November 10, 2022 16:16
expect(scale.reverse([1000, 2000], [-100, 100], 100)).toBe(2000);

expect(scale.reverse([1000, 2000], [-100, 100], -101)).toBe(995);
expect(scale.reverse([1000, 2000], [-100, 100], -102)).toBe(990);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test started failing due to the migration. The underlying library doesn't seem to guarantee exact results -- it was returning 994 instead of 995 (maybe something to do with floating-point numbers?). Rather than just changing the expected value, I adjusted the test conditions so it continues to pass in a logical manner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.

Copy link
Contributor

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So glad to see that this is working! I tried my best to understand all the pieces. Still had some questions.

Also, I am not going to review the yarn.lock changes and just assume they all make sense.

expect(scale.reverse([1000, 2000], [-100, 100], 100)).toBe(2000);

expect(scale.reverse([1000, 2000], [-100, 100], -101)).toBe(995);
expect(scale.reverse([1000, 2000], [-100, 100], -102)).toBe(990);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood.

devmode_module = "esnext",
**kwargs)

def tf_ng_web_test_suite(name, deps = [], **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused here. Why do all of our calls to tf_ng_web_test_suite already have a name specified when the old definition did not have name as a parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name argument was previously captured by the **kwargs argument and automatically passed down to karma_web_test_suite unmodified. Now that we are using name argument explicitly in tf_ng_web_test_suite then we have to explicitly list it in the arguments here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense. Thanks.

# top-level dep for spec_bundle().
srcs = [
"initialize_testbed.ts",
"initialize_testbed.spec.ts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems strange that this is the only file whose name actually changed instead of the programmatic changing of all the other test files. Is it simply because it does not follow the "_test.ts" naming convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly. I agree its a little weird.

Now that I think about it, I'd prefer to include a s.endswith("_testbed.ts") check in tf_ts_library. That solution is also a little imperfect but I think is more consistent with how we handle the other files.

Does that work for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I like this solution better.

@bmd3k bmd3k requested a review from JamesHollyer November 11, 2022 15:50
devmode_module = "esnext",
**kwargs)

def tf_ng_web_test_suite(name, deps = [], **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that makes sense. Thanks.

# top-level dep for spec_bundle().
srcs = [
"initialize_testbed.ts",
"initialize_testbed.spec.ts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I like this solution better.

@bmd3k bmd3k merged commit 2deca37 into tensorflow:master Nov 14, 2022
bmd3k added a commit that referenced this pull request Nov 21, 2022
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal app_bundle() macro to allow us to
continue to bundle Angular13+-compatible apps.

Note: app_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Note: We migrated our tests to use the similar spec_bundle() in
#6036.

Our usage of app_bundle() is largely guided/inspired by the usage in the
angular/components repo. This is the version at HEAD as of early
November:
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

We will now have two bundling targets: tf_js_binary(), which remains
unchanged; and tf_ng_prod_js_binary(), which is new and delegates to
app_bundle(). tf_ng_prod_js_binary() is, in practice, only used for one
bundle: The production Angular bundle at
//tensorboard/webapp:tb_webapp_binary. 

All other bundles, including our dev Angular bundle, continue to use
tf_js_binary().

Googlers, see detailed reasoning in http://go/tb-oss-bundling.

Highlights.
1. Run the following commands to install additional Angular build
tooling and terser and then clean the environment:
  * `yarn add @angular-devkit/build-angular@14.0.0-next.3
@bazel/terser@5.7.0 --dev`
  * `yarn run yarn-deduplicate`
  * `rm -rf node_modules; bazel clean --expunge; yarn;`

2. Patch the Angular build tooling to suit our code base:
  * Patch the esbuild config to point to old paths for tools in
    @angular/compiler-cli. When we upgrade @angular/compiler-cli to Angular
    13+ then we will be able to delete this portion of the patch.
  * Patch some of the babel-based optimization code to remove one
    particular plugin that is too aggressive in removing symbols. It is
    incompatible with the TensorBoard code base.

3. Add tf_ng_prod_js_binary, which delegates to app_bundle(), and use it
for //tensorboard/webapp:tb_webapp_binary.

Impact on "//tensorboard" build:
  * "//tensorboard/webapp:tb_webapp_binary" bundle size:
    * Before: 5,440,612 bytes
    * After: 4,790,003
  * "//tensorboard/webapp:tb_webapp_binary" bundle time:
    * Before: 1 or 2 seconds.
    * After: ~20 seconds.
  * This is an ok tradeoff since we continue to encourage engineers to use
     the faster "//tensorboard:dev" and its dev bundle for local development.

Impact on "//tensorboard:dev" target:
* No impact on either bundle size or bundle time since we continue to
   use tf_js_binary(). It remains big but fast.
* Note that when we update to Angular 13 we will have to change
   "//tensorboard:dev" to use Angular JIT compilation. Initial observations
   show that the JIT compilation does not meaningfully slow down app load
   or interaction times.
bmd3k added a commit that referenced this pull request Nov 22, 2022
Upgrade Angular to version 13 and related dependencies appropriately.

See: https://update.angular.io/?l=3&v=12.0-13.0
See:
https://medium.com/ngrx/announcing-ngrx-version-13-ivy-builds-feature-creators-improved-selectors-and-more-6a1a4c52c824

Highlights:

* Upgrade the following set of dependencies:
  * `@angular/*`: 13.3.X
  * `@ngrx/*`: 13.2.X
  * `rxjs`: 7.4.X
  * `zone.js` 0.11.6 or higher
  * `ngx-color-picker`: 12.0.1

* Some build tooling was previously upgraded in preparation for this
change:
  * #6036
  * #6049

* Some tests were previously cleaned up in preparation for this change:
  * #6056 

* No longer need ngcc as a post install step. All Angular-related
dependencies come with partially-compiled binaries.

* The tensorboard:dev Angular binary now includes the runtime JIT
compiler and no longer relies on AOT compilation. Googlers, see
http://go/tb-oss-bundling for justification.

* We can get rid of some of the patches to Angular build tooling that we
needed while we used an old version of @angular/compiler-cli.

* Impact on binaries and performance:
  * There isn't much impact on production binary sizes. The
'//tensorboard' tb_webapp_binary.js shrinks slightly from 2,574,467
bytes to 2,559,890 bytes.
  * The '//tensorboard:dev' tb_webapp_binary.js grows an entire 1MB, from
6,074,186 bytes to 7,005,529, likely because it now includes the Angular
compiler. It now relies on JIT compilation but there does not seem to be
noticeable impact on runtime performance. The development experience is
still relatively smooth.
qihach64 pushed a commit to qihach64/tensorboard that referenced this pull request Dec 19, 2022
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal spec_bundle() macro to allow us to
continue to run Karma tests with Angular 13+.

Note: spec_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Our usage of spec_bundle() is largely guided/inspired by the usage in
the angular/components repo. This is the version at HEAD as of
early November:
*
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

Highlights.
1. Run the following commands to install Angular's build tooling and
Babel and then clean the environment:
* `yarn add
https://github.com/angular/dev-infra-private-build-tooling-builds.git#fb42478534df7d48ec23a6834fea94a776cb89a0
@babel/core@^7.16.12 --dev`
* `yarn run yarn-deduplicate`
* `rm -rf node_modules; bazel clean --expunge; yarn;`
2. Automaticaly rename, at build-time, *test.ts files to *test.spec.ts
files so that they are recognized by spec_bundle() as top-level test
files to be executed. Otherwise they would be ignored and the tests not
executed.
3. Modify our build rules to produce correct compiler output and bundle
them using extract_js_module_output() and spec_bundle().
4. Cleanup files we no longer need. There are a number of shims and
hacks that we needed for compatibility with karma test suite that are
now unnecessary due to how the spec_bundle() tooling works.
qihach64 pushed a commit to qihach64/tensorboard that referenced this pull request Dec 19, 2022
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal app_bundle() macro to allow us to
continue to bundle Angular13+-compatible apps.

Note: app_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Note: We migrated our tests to use the similar spec_bundle() in
tensorflow#6036.

Our usage of app_bundle() is largely guided/inspired by the usage in the
angular/components repo. This is the version at HEAD as of early
November:
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

We will now have two bundling targets: tf_js_binary(), which remains
unchanged; and tf_ng_prod_js_binary(), which is new and delegates to
app_bundle(). tf_ng_prod_js_binary() is, in practice, only used for one
bundle: The production Angular bundle at
//tensorboard/webapp:tb_webapp_binary. 

All other bundles, including our dev Angular bundle, continue to use
tf_js_binary().

Googlers, see detailed reasoning in http://go/tb-oss-bundling.

Highlights.
1. Run the following commands to install additional Angular build
tooling and terser and then clean the environment:
  * `yarn add @angular-devkit/build-angular@14.0.0-next.3
@bazel/terser@5.7.0 --dev`
  * `yarn run yarn-deduplicate`
  * `rm -rf node_modules; bazel clean --expunge; yarn;`

2. Patch the Angular build tooling to suit our code base:
  * Patch the esbuild config to point to old paths for tools in
    @angular/compiler-cli. When we upgrade @angular/compiler-cli to Angular
    13+ then we will be able to delete this portion of the patch.
  * Patch some of the babel-based optimization code to remove one
    particular plugin that is too aggressive in removing symbols. It is
    incompatible with the TensorBoard code base.

3. Add tf_ng_prod_js_binary, which delegates to app_bundle(), and use it
for //tensorboard/webapp:tb_webapp_binary.

Impact on "//tensorboard" build:
  * "//tensorboard/webapp:tb_webapp_binary" bundle size:
    * Before: 5,440,612 bytes
    * After: 4,790,003
  * "//tensorboard/webapp:tb_webapp_binary" bundle time:
    * Before: 1 or 2 seconds.
    * After: ~20 seconds.
  * This is an ok tradeoff since we continue to encourage engineers to use
     the faster "//tensorboard:dev" and its dev bundle for local development.

Impact on "//tensorboard:dev" target:
* No impact on either bundle size or bundle time since we continue to
   use tf_js_binary(). It remains big but fast.
* Note that when we update to Angular 13 we will have to change
   "//tensorboard:dev" to use Angular JIT compilation. Initial observations
   show that the JIT compilation does not meaningfully slow down app load
   or interaction times.
qihach64 pushed a commit to qihach64/tensorboard that referenced this pull request Dec 19, 2022
)

Upgrade Angular to version 13 and related dependencies appropriately.

See: https://update.angular.io/?l=3&v=12.0-13.0
See:
https://medium.com/ngrx/announcing-ngrx-version-13-ivy-builds-feature-creators-improved-selectors-and-more-6a1a4c52c824

Highlights:

* Upgrade the following set of dependencies:
  * `@angular/*`: 13.3.X
  * `@ngrx/*`: 13.2.X
  * `rxjs`: 7.4.X
  * `zone.js` 0.11.6 or higher
  * `ngx-color-picker`: 12.0.1

* Some build tooling was previously upgraded in preparation for this
change:
  * tensorflow#6036
  * tensorflow#6049

* Some tests were previously cleaned up in preparation for this change:
  * tensorflow#6056 

* No longer need ngcc as a post install step. All Angular-related
dependencies come with partially-compiled binaries.

* The tensorboard:dev Angular binary now includes the runtime JIT
compiler and no longer relies on AOT compilation. Googlers, see
http://go/tb-oss-bundling for justification.

* We can get rid of some of the patches to Angular build tooling that we
needed while we used an old version of @angular/compiler-cli.

* Impact on binaries and performance:
  * There isn't much impact on production binary sizes. The
'//tensorboard' tb_webapp_binary.js shrinks slightly from 2,574,467
bytes to 2,559,890 bytes.
  * The '//tensorboard:dev' tb_webapp_binary.js grows an entire 1MB, from
6,074,186 bytes to 7,005,529, likely because it now includes the Angular
compiler. It now relies on JIT compilation but there does not seem to be
noticeable impact on runtime performance. The development experience is
still relatively smooth.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal spec_bundle() macro to allow us to
continue to run Karma tests with Angular 13+.

Note: spec_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Our usage of spec_bundle() is largely guided/inspired by the usage in
the angular/components repo. This is the version at HEAD as of
early November:
*
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

Highlights.
1. Run the following commands to install Angular's build tooling and
Babel and then clean the environment:
* `yarn add
https://github.com/angular/dev-infra-private-build-tooling-builds.git#fb42478534df7d48ec23a6834fea94a776cb89a0
@babel/core@^7.16.12 --dev`
* `yarn run yarn-deduplicate`
* `rm -rf node_modules; bazel clean --expunge; yarn;`
2. Automaticaly rename, at build-time, *test.ts files to *test.spec.ts
files so that they are recognized by spec_bundle() as top-level test
files to be executed. Otherwise they would be ignored and the tests not
executed.
3. Modify our build rules to produce correct compiler output and bundle
them using extract_js_module_output() and spec_bundle().
4. Cleanup files we no longer need. There are a number of shims and
hacks that we needed for compatibility with karma test suite that are
now unnecessary due to how the spec_bundle() tooling works.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal app_bundle() macro to allow us to
continue to bundle Angular13+-compatible apps.

Note: app_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Note: We migrated our tests to use the similar spec_bundle() in
tensorflow#6036.

Our usage of app_bundle() is largely guided/inspired by the usage in the
angular/components repo. This is the version at HEAD as of early
November:
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

We will now have two bundling targets: tf_js_binary(), which remains
unchanged; and tf_ng_prod_js_binary(), which is new and delegates to
app_bundle(). tf_ng_prod_js_binary() is, in practice, only used for one
bundle: The production Angular bundle at
//tensorboard/webapp:tb_webapp_binary. 

All other bundles, including our dev Angular bundle, continue to use
tf_js_binary().

Googlers, see detailed reasoning in http://go/tb-oss-bundling.

Highlights.
1. Run the following commands to install additional Angular build
tooling and terser and then clean the environment:
  * `yarn add @angular-devkit/build-angular@14.0.0-next.3
@bazel/terser@5.7.0 --dev`
  * `yarn run yarn-deduplicate`
  * `rm -rf node_modules; bazel clean --expunge; yarn;`

2. Patch the Angular build tooling to suit our code base:
  * Patch the esbuild config to point to old paths for tools in
    @angular/compiler-cli. When we upgrade @angular/compiler-cli to Angular
    13+ then we will be able to delete this portion of the patch.
  * Patch some of the babel-based optimization code to remove one
    particular plugin that is too aggressive in removing symbols. It is
    incompatible with the TensorBoard code base.

3. Add tf_ng_prod_js_binary, which delegates to app_bundle(), and use it
for //tensorboard/webapp:tb_webapp_binary.

Impact on "//tensorboard" build:
  * "//tensorboard/webapp:tb_webapp_binary" bundle size:
    * Before: 5,440,612 bytes
    * After: 4,790,003
  * "//tensorboard/webapp:tb_webapp_binary" bundle time:
    * Before: 1 or 2 seconds.
    * After: ~20 seconds.
  * This is an ok tradeoff since we continue to encourage engineers to use
     the faster "//tensorboard:dev" and its dev bundle for local development.

Impact on "//tensorboard:dev" target:
* No impact on either bundle size or bundle time since we continue to
   use tf_js_binary(). It remains big but fast.
* Note that when we update to Angular 13 we will have to change
   "//tensorboard:dev" to use Angular JIT compilation. Initial observations
   show that the JIT compilation does not meaningfully slow down app load
   or interaction times.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
)

Upgrade Angular to version 13 and related dependencies appropriately.

See: https://update.angular.io/?l=3&v=12.0-13.0
See:
https://medium.com/ngrx/announcing-ngrx-version-13-ivy-builds-feature-creators-improved-selectors-and-more-6a1a4c52c824

Highlights:

* Upgrade the following set of dependencies:
  * `@angular/*`: 13.3.X
  * `@ngrx/*`: 13.2.X
  * `rxjs`: 7.4.X
  * `zone.js` 0.11.6 or higher
  * `ngx-color-picker`: 12.0.1

* Some build tooling was previously upgraded in preparation for this
change:
  * tensorflow#6036
  * tensorflow#6049

* Some tests were previously cleaned up in preparation for this change:
  * tensorflow#6056 

* No longer need ngcc as a post install step. All Angular-related
dependencies come with partially-compiled binaries.

* The tensorboard:dev Angular binary now includes the runtime JIT
compiler and no longer relies on AOT compilation. Googlers, see
http://go/tb-oss-bundling for justification.

* We can get rid of some of the patches to Angular build tooling that we
needed while we used an old version of @angular/compiler-cli.

* Impact on binaries and performance:
  * There isn't much impact on production binary sizes. The
'//tensorboard' tb_webapp_binary.js shrinks slightly from 2,574,467
bytes to 2,559,890 bytes.
  * The '//tensorboard:dev' tb_webapp_binary.js grows an entire 1MB, from
6,074,186 bytes to 7,005,529, likely because it now includes the Angular
compiler. It now relies on JIT compilation but there does not seem to be
noticeable impact on runtime performance. The development experience is
still relatively smooth.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal spec_bundle() macro to allow us to
continue to run Karma tests with Angular 13+.

Note: spec_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Our usage of spec_bundle() is largely guided/inspired by the usage in
the angular/components repo. This is the version at HEAD as of
early November:
*
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

Highlights.
1. Run the following commands to install Angular's build tooling and
Babel and then clean the environment:
* `yarn add
https://github.com/angular/dev-infra-private-build-tooling-builds.git#fb42478534df7d48ec23a6834fea94a776cb89a0
@babel/core@^7.16.12 --dev`
* `yarn run yarn-deduplicate`
* `rm -rf node_modules; bazel clean --expunge; yarn;`
2. Automaticaly rename, at build-time, *test.ts files to *test.spec.ts
files so that they are recognized by spec_bundle() as top-level test
files to be executed. Otherwise they would be ignored and the tests not
executed.
3. Modify our build rules to produce correct compiler output and bundle
them using extract_js_module_output() and spec_bundle().
4. Cleanup files we no longer need. There are a number of shims and
hacks that we needed for compatibility with karma test suite that are
now unnecessary due to how the spec_bundle() tooling works.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
The upcoming upgrade to Angular 13+ introduces a number of
bundling-related problems for our tests and applications. Angular has
developed some internal tooling to help resolve these problems. This
change uses Angular's internal app_bundle() macro to allow us to
continue to bundle Angular13+-compatible apps.

Note: app_bundle() is Angular-internal and not supported for external
use. We use it at our own risk. Googlers, see the following documents
for background and justification: http://go/angular-bazel-problems,
http://go/tb-oss-bundling.

Note: We migrated our tests to use the similar spec_bundle() in
tensorflow#6036.

Our usage of app_bundle() is largely guided/inspired by the usage in the
angular/components repo. This is the version at HEAD as of early
November:
https://github.com/angular/components/blob/871f8f231a7a86a7a0778e345f4d517109c9a357/tools/defaults.bzl

We will now have two bundling targets: tf_js_binary(), which remains
unchanged; and tf_ng_prod_js_binary(), which is new and delegates to
app_bundle(). tf_ng_prod_js_binary() is, in practice, only used for one
bundle: The production Angular bundle at
//tensorboard/webapp:tb_webapp_binary. 

All other bundles, including our dev Angular bundle, continue to use
tf_js_binary().

Googlers, see detailed reasoning in http://go/tb-oss-bundling.

Highlights.
1. Run the following commands to install additional Angular build
tooling and terser and then clean the environment:
  * `yarn add @angular-devkit/build-angular@14.0.0-next.3
@bazel/terser@5.7.0 --dev`
  * `yarn run yarn-deduplicate`
  * `rm -rf node_modules; bazel clean --expunge; yarn;`

2. Patch the Angular build tooling to suit our code base:
  * Patch the esbuild config to point to old paths for tools in
    @angular/compiler-cli. When we upgrade @angular/compiler-cli to Angular
    13+ then we will be able to delete this portion of the patch.
  * Patch some of the babel-based optimization code to remove one
    particular plugin that is too aggressive in removing symbols. It is
    incompatible with the TensorBoard code base.

3. Add tf_ng_prod_js_binary, which delegates to app_bundle(), and use it
for //tensorboard/webapp:tb_webapp_binary.

Impact on "//tensorboard" build:
  * "//tensorboard/webapp:tb_webapp_binary" bundle size:
    * Before: 5,440,612 bytes
    * After: 4,790,003
  * "//tensorboard/webapp:tb_webapp_binary" bundle time:
    * Before: 1 or 2 seconds.
    * After: ~20 seconds.
  * This is an ok tradeoff since we continue to encourage engineers to use
     the faster "//tensorboard:dev" and its dev bundle for local development.

Impact on "//tensorboard:dev" target:
* No impact on either bundle size or bundle time since we continue to
   use tf_js_binary(). It remains big but fast.
* Note that when we update to Angular 13 we will have to change
   "//tensorboard:dev" to use Angular JIT compilation. Initial observations
   show that the JIT compilation does not meaningfully slow down app load
   or interaction times.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
)

Upgrade Angular to version 13 and related dependencies appropriately.

See: https://update.angular.io/?l=3&v=12.0-13.0
See:
https://medium.com/ngrx/announcing-ngrx-version-13-ivy-builds-feature-creators-improved-selectors-and-more-6a1a4c52c824

Highlights:

* Upgrade the following set of dependencies:
  * `@angular/*`: 13.3.X
  * `@ngrx/*`: 13.2.X
  * `rxjs`: 7.4.X
  * `zone.js` 0.11.6 or higher
  * `ngx-color-picker`: 12.0.1

* Some build tooling was previously upgraded in preparation for this
change:
  * tensorflow#6036
  * tensorflow#6049

* Some tests were previously cleaned up in preparation for this change:
  * tensorflow#6056 

* No longer need ngcc as a post install step. All Angular-related
dependencies come with partially-compiled binaries.

* The tensorboard:dev Angular binary now includes the runtime JIT
compiler and no longer relies on AOT compilation. Googlers, see
http://go/tb-oss-bundling for justification.

* We can get rid of some of the patches to Angular build tooling that we
needed while we used an old version of @angular/compiler-cli.

* Impact on binaries and performance:
  * There isn't much impact on production binary sizes. The
'//tensorboard' tb_webapp_binary.js shrinks slightly from 2,574,467
bytes to 2,559,890 bytes.
  * The '//tensorboard:dev' tb_webapp_binary.js grows an entire 1MB, from
6,074,186 bytes to 7,005,529, likely because it now includes the Angular
compiler. It now relies on JIT compilation but there does not seem to be
noticeable impact on runtime performance. The development experience is
still relatively smooth.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants