metrics: separate default settings from override #5030

stephanwlee · 2021-06-01T21:23:41Z

Background: currently, from view's and deep linker's perspective, there
is no way to discern a setting that was overridden vs. default. This
makes the deep linking particularly finicky since, if a different
application provides a different default for smoothing, for example, it
will show up in the deep link as it does not match OSS's default 0.6.

Change: this change separates default away from overrides while making
it largely transparent to views.

Background: currently, from view's and deep linker's perspective, there is no way to discern a setting that was overridden vs. default. This makes the deep linking particularly finicky since, if a different application provides a different default for smoothing, for example, it will show up in the deep link as it does not match OSS's default 0.6. Change: this change separates default away from overrides while making it largely transparent to views.

psybuzz · 2021-06-01T21:46:03Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

        imageContrastInMilli: contrastInMilli,
      },
    };
  }),
  on(actions.metricsResetImageBrightness, (state) => {
+    const settingOverrides = {...state.settingOverrides};
+    delete settingOverrides['imageBrightnessInMilli'];


Double checking, does this work with property renaming, or do we need the 'declare' keyword on export interface MetricsSettings, similar to other interfaces we protect?

Thanks, you are correct. I do not intend to use declare just for sake of the delete so I worked around it.
I meant to test this before mailing this change out but I have forgotten it.

In my test:

interface IFoo { apple: number; banana: number; }; const foo: Partial<IFoo> = { apple: 1, banana: 2 }; function Foo(foo: Partial<IFoo>) { delete foo['apple']; console.log(foo); } Foo(foo);

// built var a={g:1,h:2};delete a.apple;console.log(a);

JSCompiler is unable to understand delete and the property name here.

psybuzz · 2021-06-01T21:51:58Z

tensorboard/webapp/metrics/store/metrics_selectors.ts

 );

 /**
 * Settings.
 */
+export const getMetricsSettingOverrides = createSelector(
+  selectMetricsState,
+  (state): MetricsState['settingOverrides'] => {


I think Partial<MetricsSettings> as the return type would be more readable, to not require a jump to definition here.

psybuzz · 2021-06-01T21:52:55Z

tensorboard/webapp/routes/dashboard_deeplink_provider.ts

-          return [{key: SMOOTHING_KEY, value: String(smoothing)}];
+      store.select(selectors.getMetricsSettingOverrides).pipe(
+        map((settingOverrides) => {
+          if (Number.isFinite(settingOverrides.scalarSmoothing)) {


scalarSmoothing is never infinite, is it? Is a typeof settingOverrides.scalarSmoothing === 'undefined' sufficient?

While you are correct that I am abusing the Number.isFinite(undefined), I am also using it for Number.isFinite(NaN) === false)

I see. Then separately, I think it would be better if metrics_reducer would be responsible for discarding NaNs, so that store-readers would not have to do this isFinite check. Right now, the settings pane does not do this check when using the selector.

PR tensorflow#5030 divided up the default setting from overrides and, in selector, we have mixed two values in selectors to make a concrete settings value. When reset, overriden values were set to `undefined` and caused the settings to be very wrong. ```ts // This resolves to {a: undefined} {...{a: 1}, ...{a: undefined}} ``` Object spread (or `assign`) does not skip a property whose value is `undefined` and causes settingOverride with undefined value to take precedence over the default value which, in the end, resolves the image brightness/contrast values to `undefined` unlike what the types indicated. In the end, this is a fault of the type system since the interface is defined as `Partial<>` and a value should really not have `undefined` value. To illustrate, ```ts interface Foo { bar: number; } // Legal; `bar` property may not exist const foo2: Partial<Foo> = { }; // Legal; `bar` is defined as `number` but here, it is `undefined` // yet it is legal. const foo3: Partial<Foo> = { bar: undefined }; ``` TypeScript 4.4 has a way to property guard against `undefined` value in a `Partial<>` type, `"exactOptionalPropertyTypes"` but TensorBoard app currently violates the type definition so we will enable the tsconfig in a subsequent changes.

PR #5030 divided up the default setting from overrides and, in selector, we have mixed two values in selectors to make a concrete settings value. When reset, overriden values were set to `undefined` and caused the settings to be very wrong. ```ts // This resolves to {a: undefined} {...{a: 1}, ...{a: undefined}} ``` Object spread (or `assign`) does not skip a property whose value is `undefined` and causes settingOverride with undefined value to take precedence over the default value which, in the end, resolves the image brightness/contrast values to `undefined` unlike what the types indicated. In the end, this is a fault of the type system since the interface is defined as `Partial<>` and a value should really not have `undefined` value. To illustrate, ```ts interface Foo { bar: number; } // Legal; `bar` property may not exist const foo2: Partial<Foo> = { }; // Legal; `bar` is defined as `number` but here, it is `undefined` // yet it is legal. const foo3: Partial<Foo> = { bar: undefined }; ``` TypeScript 4.4 has a way to property guard against `undefined` value in a `Partial<>` type, `"exactOptionalPropertyTypes"` but TensorBoard app currently violates the type definition so we will enable the tsconfig in a subsequent changes.

PR tensorflow#5030 divided up the default setting from overrides and, in selector, we have mixed two values in selectors to make a concrete settings value. When reset, overriden values were set to `undefined` and caused the settings to be very wrong. ```ts // This resolves to {a: undefined} {...{a: 1}, ...{a: undefined}} ``` Object spread (or `assign`) does not skip a property whose value is `undefined` and causes settingOverride with undefined value to take precedence over the default value which, in the end, resolves the image brightness/contrast values to `undefined` unlike what the types indicated. In the end, this is a fault of the type system since the interface is defined as `Partial<>` and a value should really not have `undefined` value. To illustrate, ```ts interface Foo { bar: number; } // Legal; `bar` property may not exist const foo2: Partial<Foo> = { }; // Legal; `bar` is defined as `number` but here, it is `undefined` // yet it is legal. const foo3: Partial<Foo> = { bar: undefined }; ``` TypeScript 4.4 has a way to property guard against `undefined` value in a `Partial<>` type, `"exactOptionalPropertyTypes"` but TensorBoard app currently violates the type definition so we will enable the tsconfig in a subsequent changes.

stephanwlee requested a review from psybuzz June 1, 2021 21:23

google-cla bot added the cla: yes label Jun 1, 2021

psybuzz approved these changes Jun 1, 2021

View reviewed changes

cr

f6516e5

stephanwlee merged commit da66ad4 into tensorflow:master Jun 2, 2021

stephanwlee deleted the smoothing branch June 2, 2021 16:48

stephanwlee mentioned this pull request Dec 11, 2021

timeseries: fix setting reset causing weird behavior #5458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metrics: separate default settings from override #5030

metrics: separate default settings from override #5030

stephanwlee commented Jun 1, 2021

psybuzz Jun 1, 2021

stephanwlee Jun 2, 2021

psybuzz Jun 1, 2021

psybuzz Jun 1, 2021

stephanwlee Jun 2, 2021

psybuzz Jun 2, 2021

metrics: separate default settings from override #5030

metrics: separate default settings from override #5030

Conversation

stephanwlee commented Jun 1, 2021

psybuzz Jun 1, 2021

Choose a reason for hiding this comment

stephanwlee Jun 2, 2021

Choose a reason for hiding this comment

psybuzz Jun 1, 2021

Choose a reason for hiding this comment

psybuzz Jun 1, 2021

Choose a reason for hiding this comment

stephanwlee Jun 2, 2021

Choose a reason for hiding this comment

psybuzz Jun 2, 2021

Choose a reason for hiding this comment