Add WebGL escape hatch #5494

JamesHollyer · 2022-01-11T01:28:24Z

While we hope our error catching is sufficient to automate the fallback to SVG when WebGL fails we still want to give users the ability to force the use of SVG. This is not something we want most users to do. This should only be used if WebGL is failing and out fallback is not working.

This CL adds a secret query parameter that will allow users in the know to force the use of SVG. It also adds a secret parameter to stop forcing SVG and return to the default behavior.

bmd3k · 2022-01-11T16:22:45Z

Ultimately, like all state in our Angular app, the source of truth for the user's decision to use WebGL vs SVG should be the in-memory ngrx state. On app load we would construct the ngrx-specific value taking into consideration the value from the query parameters and from local settings. Normally we would represent this sort of state as a "feature flag":

tensorboard/tensorboard/webapp/feature_flag/store/feature_flag_reducers.ts

Line 46 in 367839f

on(globalSettingsLoaded, (state, {partialSettings}) => {

That line, specifically, shows how we read "feature flags" from "persistent settings", which is the abstraction on top of local storage:

https://github.com/tensorflow/tensorboard/tree/master/tensorboard/webapp/persistent_settings

This is where we read some feature flag overrides from query parameters:

https://github.com/tensorflow/tensorboard/blob/367839f30de3968eff5fecb8b38e1d5782434b6b/tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

And, finally, I believe this is an example of how you could take the value from the ngrx state and update the local storage via (via persistent settings):

tensorboard/tensorboard/webapp/feature_flag/feature_flag_module.ts

Line 34 in 367839f

export function getThemeSettingSelector() {

bmd3k

I think ForceSVGDataSource is missing from these changes. Can you add it?

bmd3k · 2022-01-19T19:52:04Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects.ts

@@ -33,6 +34,7 @@ export class FeatureFlagEffects {
      combineLatestWith(this.store.select(getIsAutoDarkModeAllowed)),
      map(([, isDarkModeAllowed]) => {
        const features = this.dataSource.getFeatures(isDarkModeAllowed);
+        features.forceSVG = this.ForceSVGDataSource.getAndUpdateForceSVGFlag();


Data source layer is ideally sort of dumb and just reads/writes data without trying to make any decisions. The decision making (aka business logic) is ideally in the effects and reducers layers.

My recommendation:

Rely on TBFeatureFlagDataSource.getFeatures() to read the value of the forceSVG query parameter. If it follows the pattern of other query parameters then it should return empty value for forceSVG if it is not specified in the URL.

Introduce ForceSVGDataSource.getForceSVG() that returns the value from local storage or null if there is none.

Write logic in the Effects layer to determine the actual value of ForceSVG based on (1) and (2). The value is possibly empty (in which case, in practice, the app will rely on the default in the ngrx state). Include this value in the partialFeatureFlagsLoaded.

Introduce ForceSVGDataSoruce.updateForceSVG() to write the value to local storage, if appropriate.

This all sounds good. However, if we are only pulling from localStorage for the ForceSVGDataSource it does not need all the overhead of an injectable class. I am going to change it to force_svg_util and just have a getForceSvg and updateForceSvg functions as standalone functions. Does that sound good to you?

In this particular case, I can empathise with the temptation to avoid using Angular's Dependency Injection (DI). Why you might think this way:

force_svg_util itself currently has no DI-able dependencies (although I think it's hard to be certain that will be true forever)

we don't foresee wanting to to DI some other implementation of ForceSVGDataSource -- we think it will always be a local storage thing, unlike persistent settings which might one day be stored on the server.

Having said that, I think it's good habit/hygiene to make this a DI-able object like any other object in our system that reads/writes to a data source. I agree there is some overhead but it is pretty standard Angular stuff. It will also give you standard options/patterns when you write tests - how to Mock, how to Fake/Stub.

I made it injectable again.

I also added tests.

PTAL.

tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html

tensorboard/webapp/webapp_data_source/BUILD

tensorboard/webapp/webapp_data_source/force_SVG_data_source.ts

bmd3k · 2022-01-19T20:09:47Z

tensorboard/webapp/webapp_data_source/BUILD

@@ -91,6 +91,7 @@ tf_ng_module(
 tf_ng_module(
    name = "feature_flag",
    srcs = [
+        "force_SVG_data_source.ts",


Over time we've been trying to move things out of webapp_data_source and into feature-specific folders. Ideally force_svg_data_source and tb_feature_flag_data_source and tb_feature_flag_module would be in the feature_flag folder.

Could we put force_svg_data_source there immediately?

Would you be interested in following up with cleanup PRs to move tb_feature_flag_data_source and tb_feature_flag_module there as well? (disclosure: this may be a bit difficult because of the fact that classes in our internal repo reference these classes)

Moved and renamed. Yea I can probably do that cleanup. I will look into what it will take to make that happen. Maybe make a copy of it temporarily until we can switch internal code over then remove.

bmd3k · 2022-01-21T14:36:32Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

@@ -90,6 +91,10 @@ export class QueryParamsFeatureFlagDataSource
        params.get(ENABLE_TIME_NAMESPACED_STATE) !== 'false';
    }

+    if (params.has(FORCE_SVG_RENDERER)) {


Can you add tests for this?

bmd3k · 2022-01-21T14:38:44Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html

@@ -102,7 +102,7 @@
  ></mat-spinner>
  <line-chart
    [disableUpdate]="!isCardVisible"
-    [preferredRendererType]="RendererType.SVG"
+    [preferredRendererType]="forceSvg ? RendererType.SVG : RendererType.WEBGL"


Can you add tests for this?

bmd3k · 2022-01-21T14:40:02Z

tensorboard/webapp/feature_flag/types.ts

@@ -50,4 +50,6 @@ export interface FeatureFlags {
  // Whether to enable time-namespaced state and how it impacts how user
  // settings are kept during navigation.
  enabledTimeNamespacedState: boolean;
+
+  forceSvg: boolean;


Document? Worth mentioning this is for TimeSeries Scalars cards only (I think?).

bmd3k · 2022-01-21T14:42:36Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts

 import {FeatureFlagEffects} from './feature_flag_effects';

-describe('feature_flag_effects', () => {
+fdescribe('feature_flag_effects', () => {


Dangling fdescribe here.

doh! removed.

bmd3k · 2022-01-21T14:43:42Z

tensorboard/webapp/feature_flag/force_svg_data_source_module.ts

@@ -0,0 +1,22 @@
+/* Copyright 2020 The TensorFlow Authors. All Rights Reserved.


bmd3k · 2022-01-21T15:04:44Z

tensorboard/webapp/feature_flag/force_svg_data_source_test.ts

+    removeItemSpy = spyOn(window.localStorage, 'removeItem').and.stub();
+    dataSource = TestBed.inject(ForceSvgDataSource);
+  });
+  describe('#getForceSVG', () => {


Nit: Newline before this line.

bmd3k · 2022-01-21T15:16:09Z

tensorboard/webapp/feature_flag/force_svg_data_source.ts

+limitations under the License.
+==============================================================================*/
+import {Injectable} from '@angular/core';
+import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types';


Rather than sharing the query parameter key perhaps we should use a name more like the one used by PersistentSettingsDataSource:

https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts;l=22;drc=b0f27571ce3e6ce322b9f1b91a4e8773f9ca47e0

Prefixed with '_tb_'.

That makes more sense. Changed.

bmd3k · 2022-01-21T15:17:15Z

tensorboard/webapp/feature_flag/force_svg_data_source_test.ts

+  });
+
+  describe('updateForceSVG', () => {
+    it('Creates localStorage entry with key forceSVG when passed truthy', () => {


nit 'Creates' => 'creates'

bmd3k · 2022-01-21T15:18:08Z

tensorboard/webapp/feature_flag/force_svg_data_source_test.ts

+
+  describe('updateForceSVG', () => {
+    it('Creates localStorage entry with key forceSVG when passed truthy', () => {
+      let dataSource = new ForceSvgDataSource();


Use dataSource you get from the TestBed instead?

I did not mean to leave this line in there. Removed.

bmd3k · 2022-01-21T15:18:20Z

tensorboard/webapp/feature_flag/force_svg_data_source_test.ts

+      );
+    });
+    it('calls localStorage.removeItem with key forceSVG', () => {
+      let dataSource = new ForceSvgDataSource();


Use dataSource you get from the TestBed instead?

JamesHollyer

Thanks Brian. Let me know if there is anything else you can see to improve this.

JamesHollyer · 2022-01-21T19:45:50Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts

 import {FeatureFlagEffects} from './feature_flag_effects';

-describe('feature_flag_effects', () => {
+fdescribe('feature_flag_effects', () => {


doh! removed.

JamesHollyer · 2022-01-21T19:46:01Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts

+      expect(updateSpy).toHaveBeenCalledOnceWith(true);
+    });
+
+    fit('gets forceSVG flag from ForceSvgDataSource when getFeatures returns no value for forceSvg', () => {


JamesHollyer · 2022-01-21T19:51:36Z

tensorboard/webapp/feature_flag/effects/feature_flag_effects_test.ts

+      expect(updateSpy).toHaveBeenCalledOnceWith(true);
+    });
+
+    fit('gets forceSVG flag from ForceSvgDataSource when getFeatures returns no value for forceSvg', () => {


JamesHollyer · 2022-01-21T20:00:38Z

tensorboard/webapp/feature_flag/force_svg_data_source.ts

+
+@Injectable()
+export class ForceSvgDataSource {
+  constructor() {}


I would love to do that. I see it as a useless abstraction. I am not a fan of useless abstractions. The basic localStorage API works great by itself(I might be a bit biased because my last team owned that API :P)

JamesHollyer · 2022-01-21T20:02:33Z

tensorboard/webapp/feature_flag/force_svg_data_source_module.ts

@@ -0,0 +1,22 @@
+/* Copyright 2020 The TensorFlow Authors. All Rights Reserved.


JamesHollyer · 2022-01-21T20:10:25Z

tensorboard/webapp/feature_flag/types.ts

@@ -50,4 +50,6 @@ export interface FeatureFlags {
  // Whether to enable time-namespaced state and how it impacts how user
  // settings are kept during navigation.
  enabledTimeNamespacedState: boolean;
+
+  forceSvg: boolean;


JamesHollyer · 2022-01-21T21:01:39Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html

@@ -102,7 +102,7 @@
  ></mat-spinner>
  <line-chart
    [disableUpdate]="!isCardVisible"
-    [preferredRendererType]="RendererType.SVG"
+    [preferredRendererType]="forceSvg ? RendererType.SVG : RendererType.WEBGL"


JamesHollyer · 2022-01-21T21:02:01Z

tensorboard/webapp/webapp_data_source/tb_feature_flag_data_source.ts

@@ -90,6 +91,10 @@ export class QueryParamsFeatureFlagDataSource
        params.get(ENABLE_TIME_NAMESPACED_STATE) !== 'false';
    }

+    if (params.has(FORCE_SVG_RENDERER)) {


JamesHollyer · 2022-01-21T21:02:30Z

tensorboard/webapp/feature_flag/force_svg_data_source.ts

+
+  updateForceSvgFlag(forceSvgFlag: boolean) {
+    if (forceSvgFlag) {
+      localStorage.setItem(FORCE_SVG_RENDERER, 'true');


I like that. Changed.

JamesHollyer · 2022-01-21T21:11:52Z

tensorboard/webapp/feature_flag/force_svg_data_source.ts

+limitations under the License.
+==============================================================================*/
+import {Injectable} from '@angular/core';
+import {FORCE_SVG_RENDERER} from '../webapp_data_source/tb_feature_flag_data_source_types';


That makes more sense. Changed.

bmd3k · 2022-01-24T12:19:05Z

tensorboard/webapp/feature_flag/types.ts

@@ -50,6 +50,7 @@ export interface FeatureFlags {
  // Whether to enable time-namespaced state and how it impacts how user
  // settings are kept during navigation.
  enabledTimeNamespacedState: boolean;
-
+  // Flag for the escape hatch from webGL. This only effects the TimeSeries


Nit: webGL => WebGL

There is logic to fallback to SVG if we receive an error from WebGL. However, if some users are having trouble with WebGL and we are not catching the error we wanted an escape hatch for users to force the use of SVG. This PR adds that escape hatch as a hidden query parameter that users can add. Once added it will be stored locally until turned off which can be done with another query parameter.

JamesHollyer requested a review from bmd3k January 11, 2022 01:28

bmd3k reviewed Jan 19, 2022

View reviewed changes

bmd3k self-requested a review January 19, 2022 19:26

bmd3k reviewed Jan 19, 2022

View reviewed changes

JamesHollyer requested a review from bmd3k January 19, 2022 23:32

bmd3k reviewed Jan 21, 2022

View reviewed changes

JamesHollyer commented Jan 21, 2022

View reviewed changes

bmd3k approved these changes Jan 24, 2022

View reviewed changes

JamesHollyer added 9 commits January 24, 2022 15:45

Add WebGL escape hatch

fc42102

mock getForceSvgFeatureFlag in all scalar card tests.

31a5370

add forceSVGDataSource file

4f9dec7

clean up forceSVGDataSource

7c57f78

Code Review 1

7e647de

Make ForceSVGDataSource injectable and add tests

6a2f128

Code Review 2

07e41d4

Capitalize WebGL nit

0f54bb0

mock out getForceSvgFeatureFlagselector before all scalar card tests

8f8472a

JamesHollyer force-pushed the AddEscapeHatch branch from 31f70e5 to 8f8472a Compare January 24, 2022 23:50

JamesHollyer merged commit 7a2a169 into tensorflow:master Jan 25, 2022

		@@ -0,0 +1,22 @@
		/* Copyright 2020 The TensorFlow Authors. All Rights Reserved.

Add WebGL escape hatch #5494

Add WebGL escape hatch #5494

Conversation

JamesHollyer commented Jan 11, 2022

bmd3k commented Jan 11, 2022

bmd3k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesHollyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment