From 7ad061e9aba03be8bbdd8fe164a7c7c911a652f4 Mon Sep 17 00:00:00 2001 From: E Date: Tue, 6 Oct 2020 12:53:43 -0700 Subject: [PATCH 1/2] feat: persist pinned cards in URL with DeepLinkProvider --- tensorboard/webapp/BUILD | 1 + .../webapp/metrics/store/metrics_selectors.ts | 8 + .../metrics/store/metrics_selectors_test.ts | 29 +++ tensorboard/webapp/routes/BUILD | 44 ++++ .../webapp/routes/core_deeplink_provider.ts | 162 +++++++++++++ .../routes/core_deeplink_provider_test.ts | 227 ++++++++++++++++++ tensorboard/webapp/routes/index.ts | 2 + 7 files changed, 473 insertions(+) create mode 100644 tensorboard/webapp/routes/core_deeplink_provider.ts create mode 100644 tensorboard/webapp/routes/core_deeplink_provider_test.ts diff --git a/tensorboard/webapp/BUILD b/tensorboard/webapp/BUILD index 14d718b558..91ae224a3f 100644 --- a/tensorboard/webapp/BUILD +++ b/tensorboard/webapp/BUILD @@ -259,6 +259,7 @@ tf_ng_web_test_suite( "//tensorboard/webapp/plugins/text_v2/effects:effects_test_lib", "//tensorboard/webapp/plugins/text_v2/store:store_test_lib", "//tensorboard/webapp/reloader:test_lib", + "//tensorboard/webapp/routes:routes_test_lib", "//tensorboard/webapp/runs/data_source:runs_data_source_test", "//tensorboard/webapp/runs/effects:effects_test", "//tensorboard/webapp/runs/store:store_test", diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index cbb82afeaf..a33c4e4a77 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -20,6 +20,7 @@ import {DeepReadonly} from '../../util/types'; import { CardId, CardIdWithMetadata, + CardUniqueInfo, CardMetadata, HistogramMode, NonPinnedCardId, @@ -238,6 +239,13 @@ export const getCardPinnedState = createSelector( } ); +export const getUnresolvedImportedPinnedCards = createSelector( + selectMetricsState, + (state: MetricsState): CardUniqueInfo[] => { + return state.unresolvedImportedPinnedCards; + } +); + /** * Settings. */ diff --git a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts index f22a959091..63b50da96f 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts @@ -512,6 +512,35 @@ describe('metrics selectors', () => { }); }); + describe('getUnresolvedImportedPinnedCards', () => { + it('returns false if no card exists', () => { + selectors.getUnresolvedImportedPinnedCards.release(); + + const state = appStateFromMetricsState( + buildMetricsState({ + unresolvedImportedPinnedCards: [ + {plugin: PluginType.SCALARS, tag: 'accuracy'}, + { + plugin: PluginType.IMAGES, + tag: 'output', + runId: 'exp1/run1', + sample: 5, + }, + ], + }) + ); + expect(selectors.getUnresolvedImportedPinnedCards(state)).toEqual([ + {plugin: PluginType.SCALARS, tag: 'accuracy'}, + { + plugin: PluginType.IMAGES, + tag: 'output', + runId: 'exp1/run1', + sample: 5, + }, + ]); + }); + }); + describe('settings', () => { it('returns tooltipSort when called getMetricsTooltipSort', () => { selectors.getMetricsTooltipSort.release(); diff --git a/tensorboard/webapp/routes/BUILD b/tensorboard/webapp/routes/BUILD index 0a18dbc776..18935709a9 100644 --- a/tensorboard/webapp/routes/BUILD +++ b/tensorboard/webapp/routes/BUILD @@ -10,9 +10,53 @@ tf_ts_library( "index.ts", ], deps = [ + ":core_deeplink_provider", "//tensorboard/webapp/app_routing:route_config", "//tensorboard/webapp/app_routing:types", "//tensorboard/webapp/tb_wrapper", "@npm//@angular/core", ], ) + +tf_ts_library( + name = "core_deeplink_provider", + srcs = [ + "core_deeplink_provider.ts", + ], + deps = [ + "//tensorboard/webapp:app_state", + "//tensorboard/webapp:selectors", + "//tensorboard/webapp/app_routing:deep_link_provider", + "//tensorboard/webapp/app_routing:route_config", + "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/metrics:types", + "//tensorboard/webapp/metrics/data_source:types", + "//tensorboard/webapp/tb_wrapper", + "@npm//@angular/core", + "@npm//@ngrx/store", + "@npm//rxjs", + ], +) + +tf_ts_library( + name = "routes_test_lib", + testonly = True, + srcs = [ + "core_deeplink_provider_test.ts", + ], + deps = [ + ":core_deeplink_provider", + "//tensorboard/webapp:app_state", + "//tensorboard/webapp:selectors", + "//tensorboard/webapp/angular:expect_angular_core_testing", + "//tensorboard/webapp/angular:expect_ngrx_store_testing", + "//tensorboard/webapp/app_routing:deep_link_provider", + "//tensorboard/webapp/app_routing:types", + "//tensorboard/webapp/metrics:test_lib", + "//tensorboard/webapp/metrics/data_source:types", + "@npm//@angular/core", + "@npm//@ngrx/store", + "@npm//@types/jasmine", + "@npm//rxjs", + ], +) diff --git a/tensorboard/webapp/routes/core_deeplink_provider.ts b/tensorboard/webapp/routes/core_deeplink_provider.ts new file mode 100644 index 0000000000..038cac78fa --- /dev/null +++ b/tensorboard/webapp/routes/core_deeplink_provider.ts @@ -0,0 +1,162 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {Injectable} from '@angular/core'; +import {Store} from '@ngrx/store'; +import {DeepLinkProvider} from '../app_routing/deep_link_provider'; +import {SerializableQueryParams} from '../app_routing/types'; +import { + CardUniqueInfo, + URLDeserializedState as MetricsURLDeserializedState, +} from '../metrics/types'; +import { + isSampledPlugin, + isSingleRunPlugin, + isPluginType, +} from '../metrics/data_source/types'; +import {combineLatest, Observable} from 'rxjs'; +import {map} from 'rxjs/operators'; + +import {State} from '../app_state'; +import * as selectors from '../selectors'; + +export type DeserializedState = MetricsURLDeserializedState; + +/** + * Provides deeplinking for the core dashboards page. + */ +@Injectable() +export class CoreDeepLinkProvider extends DeepLinkProvider { + private getMetricsPinnedCards( + store: Store + ): Observable { + return combineLatest([ + store.select(selectors.getPinnedCardsWithMetadata), + store.select(selectors.getUnresolvedImportedPinnedCards), + ]).pipe( + map(([pinnedCards, unresolvedImportedPinnedCards]) => { + if (!pinnedCards.length && !unresolvedImportedPinnedCards.length) { + return []; + } + + const pinnedCardsToStore = pinnedCards.map( + ({plugin, tag, sample, runId}) => { + const info = {plugin, tag} as CardUniqueInfo; + if (isSingleRunPlugin(plugin)) { + info.runId = runId!; + } + if (isSampledPlugin(plugin)) { + info.sample = sample!; + } + return info; + } + ); + // Intentionally order unresolved cards last, so that cards pinned by + // the user in this session have priority. + const cardsToStore = [ + ...pinnedCardsToStore, + ...unresolvedImportedPinnedCards, + ]; + return [{key: 'pinnedCards', value: JSON.stringify(cardsToStore)}]; + }) + ); + } + + serializeStateToQueryParams( + store: Store + ): Observable { + return this.getMetricsPinnedCards(store); + } + + deserializeQueryParams( + queryParams: SerializableQueryParams + ): DeserializedState { + let pinnedCards = null; + for (const {key, value} of queryParams) { + if (key === 'pinnedCards') { + pinnedCards = pinnedCards || extractPinnedCardsFromURLText(value); + } + } + return { + metrics: { + pinnedCards: pinnedCards || [], + }, + }; + } +} + +function extractPinnedCardsFromURLText( + urlText: string +): CardUniqueInfo[] | null { + // Check that the URL text parses. + let object; + try { + object = JSON.parse(urlText) as unknown; + } catch { + return null; + } + if (!Array.isArray(object)) { + return null; + } + + const result = []; + for (const item of object) { + // Validate types. + const isPluginString = typeof item.plugin === 'string'; + const isRunString = typeof item.runId === 'string'; + const isSampleNumber = typeof item.sample === 'number'; + const isTagTypeValid = typeof item.tag === 'string'; + const isRunTypeValid = isRunString || typeof item.runId === 'undefined'; + const isSampleTypeValid = + isSampleNumber || typeof item.sample === 'undefined'; + if ( + !isPluginString || + !isTagTypeValid || + !isRunTypeValid || + !isSampleTypeValid + ) { + continue; + } + + // Required fields and range errors. + if (!isPluginType(item.plugin)) { + continue; + } + if (!item.tag) { + continue; + } + if (isRunString && (!item.runId || !isSingleRunPlugin(item.plugin))) { + continue; + } + if (isSampleNumber) { + if (!isSampledPlugin(item.plugin)) { + continue; + } + if (!Number.isInteger(item.sample) || item.sample < 0) { + continue; + } + } + + // Assemble result. + const resultItem = {plugin: item.plugin, tag: item.tag} as CardUniqueInfo; + if (isRunString) { + resultItem.runId = item.runId; + } + if (isSampleNumber) { + resultItem.sample = item.sample; + } + result.push(resultItem); + } + return result; +} diff --git a/tensorboard/webapp/routes/core_deeplink_provider_test.ts b/tensorboard/webapp/routes/core_deeplink_provider_test.ts new file mode 100644 index 0000000000..edb126aedb --- /dev/null +++ b/tensorboard/webapp/routes/core_deeplink_provider_test.ts @@ -0,0 +1,227 @@ +/* Copyright 2020 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ +import {TestBed} from '@angular/core/testing'; +import {Store} from '@ngrx/store'; +import {MockStore, provideMockStore} from '@ngrx/store/testing'; +import {Observable} from 'rxjs'; +import {skip} from 'rxjs/operators'; + +import * as selectors from '../selectors'; +import {DeepLinkProvider} from '../app_routing/deep_link_provider'; +import {SerializableQueryParams} from '../app_routing/types'; +import {State} from '../app_state'; +import {appStateFromMetricsState, buildMetricsState} from '../metrics/testing'; +import {PluginType} from '../metrics/data_source/types'; +import {CoreDeepLinkProvider} from './core_deeplink_provider'; + +describe('core deeplink provider', () => { + let store: MockStore; + let provider: DeepLinkProvider; + let queryParamsSerialized: SerializableQueryParams[]; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + providers: [ + provideMockStore({ + initialState: { + ...appStateFromMetricsState(buildMetricsState()), + }, + }), + ], + }).compileComponents(); + + store = TestBed.inject>(Store) as MockStore; + queryParamsSerialized = []; + + provider = new CoreDeepLinkProvider(); + provider + .serializeStateToQueryParams(store) + .pipe( + // Skip the initial bootstrap. + skip(1) + ) + .subscribe((queryParams) => { + queryParamsSerialized.push(queryParams); + }); + }); + + describe('time series', () => { + it('serializes pinned card state when store updates', () => { + store.overrideSelector(selectors.getPinnedCardsWithMetadata, [ + { + cardId: 'card1', + plugin: PluginType.SCALARS, + tag: 'accuracy', + runId: null, + }, + ]); + store.overrideSelector(selectors.getUnresolvedImportedPinnedCards, [ + { + plugin: PluginType.SCALARS, + tag: 'loss', + }, + ]); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + { + key: 'pinnedCards', + value: + '[{"plugin":"scalars","tag":"accuracy"},{"plugin":"scalars","tag":"loss"}]', + }, + ]); + + store.overrideSelector(selectors.getPinnedCardsWithMetadata, [ + { + cardId: 'card1', + plugin: PluginType.SCALARS, + tag: 'accuracy2', + runId: null, + }, + ]); + store.overrideSelector(selectors.getUnresolvedImportedPinnedCards, [ + { + plugin: PluginType.SCALARS, + tag: 'loss2', + }, + ]); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual([ + { + key: 'pinnedCards', + value: + '[{"plugin":"scalars","tag":"accuracy2"},{"plugin":"scalars","tag":"loss2"}]', + }, + ]); + }); + + it('serializes nothing when states are empty', () => { + store.overrideSelector(selectors.getPinnedCardsWithMetadata, []); + store.overrideSelector(selectors.getUnresolvedImportedPinnedCards, []); + store.refreshState(); + + expect(queryParamsSerialized[queryParamsSerialized.length - 1]).toEqual( + [] + ); + }); + + it('deserializes empty pinned cards', () => { + const state = provider.deserializeQueryParams([]); + + expect(state).toEqual({metrics: {pinnedCards: []}}); + }); + + it('deserializes valid pinned cards', () => { + const state = provider.deserializeQueryParams([ + { + key: 'pinnedCards', + value: + '[{"plugin":"scalars","tag":"accuracy"},{"plugin":"images","tag":"loss","runId":"exp1/123","sample":5}]', + }, + ]); + + expect(state).toEqual({ + metrics: { + pinnedCards: [ + {plugin: PluginType.SCALARS, tag: 'accuracy'}, + { + plugin: PluginType.IMAGES, + tag: 'loss', + runId: 'exp1/123', + sample: 5, + }, + ], + }, + }); + }); + + it('sanitizes pinned cards on deserialization', () => { + const cases = [ + { + serializedValue: 'blah[{"plugin":"scalars","tag":"accuracy"}]', + expectedPinnedCards: [], + }, + { + // no plugin + serializedValue: + '[{"tag":"loss"},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // unknown plugin + serializedValue: + '[{"plugin":"unknown","tag":"loss"},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // tag is not a string + serializedValue: + '[{"plugin":"scalars","tag":5},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // tag is empty + serializedValue: + '[{"plugin":"scalars","tag":""},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // runId is not a string + serializedValue: + '[{"plugin":"images","tag":"loss","runId":123},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // runId is empty + serializedValue: + '[{"plugin":"images","tag":"loss","runId":""},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // sample provided with non-sampled plugin + serializedValue: + '[{"plugin":"scalars","tag":"loss","sample":5},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // sample is not a number + serializedValue: + '[{"plugin":"images","tag":"loss","sample":"5"},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // sample is not an integer + serializedValue: + '[{"plugin":"images","tag":"loss","sample":5.5},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + { + // sample is negative + serializedValue: + '[{"plugin":"images","tag":"loss","sample":-5},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, + ]; + for (const {serializedValue, expectedPinnedCards} of cases) { + const state = provider.deserializeQueryParams([ + {key: 'pinnedCards', value: serializedValue}, + ]); + + expect(state).toEqual({metrics: {pinnedCards: expectedPinnedCards}}); + } + }); + }); +}); diff --git a/tensorboard/webapp/routes/index.ts b/tensorboard/webapp/routes/index.ts index d19f868816..cc087a27e1 100644 --- a/tensorboard/webapp/routes/index.ts +++ b/tensorboard/webapp/routes/index.ts @@ -17,6 +17,7 @@ import {Component, Type} from '@angular/core'; import {TensorBoardWrapperComponent} from '../tb_wrapper/tb_wrapper_component'; import {RouteDef} from '../app_routing/route_config_types'; import {RouteKind} from '../app_routing/types'; +import {CoreDeepLinkProvider} from './core_deeplink_provider'; export function routesFactory(): RouteDef[] { return [ @@ -25,6 +26,7 @@ export function routesFactory(): RouteDef[] { path: '/', ngComponent: TensorBoardWrapperComponent as Type, defaultRoute: true, + deepLinkProvider: new CoreDeepLinkProvider(), }, ]; } From 29d4cd2ce6da025b236a2d8a1aebfb22a0d2c758 Mon Sep 17 00:00:00 2001 From: E Date: Fri, 16 Oct 2020 13:31:37 -0700 Subject: [PATCH 2/2] ac --- .../metrics/store/metrics_selectors_test.ts | 2 +- .../webapp/routes/core_deeplink_provider.ts | 19 ++++++++++++++----- .../routes/core_deeplink_provider_test.ts | 8 +++++++- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts index 63b50da96f..2095d710d3 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts @@ -513,7 +513,7 @@ describe('metrics selectors', () => { }); describe('getUnresolvedImportedPinnedCards', () => { - it('returns false if no card exists', () => { + it('returns unresolved imported pinned cards', () => { selectors.getUnresolvedImportedPinnedCards.release(); const state = appStateFromMetricsState( diff --git a/tensorboard/webapp/routes/core_deeplink_provider.ts b/tensorboard/webapp/routes/core_deeplink_provider.ts index 038cac78fa..374720042b 100644 --- a/tensorboard/webapp/routes/core_deeplink_provider.ts +++ b/tensorboard/webapp/routes/core_deeplink_provider.ts @@ -85,7 +85,8 @@ export class CoreDeepLinkProvider extends DeepLinkProvider { let pinnedCards = null; for (const {key, value} of queryParams) { if (key === 'pinnedCards') { - pinnedCards = pinnedCards || extractPinnedCardsFromURLText(value); + pinnedCards = extractPinnedCardsFromURLText(value); + break; } } return { @@ -116,13 +117,13 @@ function extractPinnedCardsFromURLText( const isPluginString = typeof item.plugin === 'string'; const isRunString = typeof item.runId === 'string'; const isSampleNumber = typeof item.sample === 'number'; - const isTagTypeValid = typeof item.tag === 'string'; + const isTagString = typeof item.tag === 'string'; const isRunTypeValid = isRunString || typeof item.runId === 'undefined'; const isSampleTypeValid = isSampleNumber || typeof item.sample === 'undefined'; if ( !isPluginString || - !isTagTypeValid || + !isTagString || !isRunTypeValid || !isSampleTypeValid ) { @@ -136,8 +137,16 @@ function extractPinnedCardsFromURLText( if (!item.tag) { continue; } - if (isRunString && (!item.runId || !isSingleRunPlugin(item.plugin))) { - continue; + if (isSingleRunPlugin(item.plugin)) { + // A single run plugin must specify a non-empty run. + if (!item.runId) { + continue; + } + } else { + // A multi run plugin must not specify a run. + if (item.runId) { + continue; + } } if (isSampleNumber) { if (!isSampledPlugin(item.plugin)) { diff --git a/tensorboard/webapp/routes/core_deeplink_provider_test.ts b/tensorboard/webapp/routes/core_deeplink_provider_test.ts index edb126aedb..a11aa25f46 100644 --- a/tensorboard/webapp/routes/core_deeplink_provider_test.ts +++ b/tensorboard/webapp/routes/core_deeplink_provider_test.ts @@ -15,7 +15,6 @@ limitations under the License. import {TestBed} from '@angular/core/testing'; import {Store} from '@ngrx/store'; import {MockStore, provideMockStore} from '@ngrx/store/testing'; -import {Observable} from 'rxjs'; import {skip} from 'rxjs/operators'; import * as selectors from '../selectors'; @@ -151,6 +150,7 @@ describe('core deeplink provider', () => { it('sanitizes pinned cards on deserialization', () => { const cases = [ { + // malformed URL value serializedValue: 'blah[{"plugin":"scalars","tag":"accuracy"}]', expectedPinnedCards: [], }, @@ -190,6 +190,12 @@ describe('core deeplink provider', () => { '[{"plugin":"images","tag":"loss","runId":""},{"plugin":"scalars","tag":"default"}]', expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], }, + { + // runId provided with multi-run plugin + serializedValue: + '[{"plugin":"scalars","tag":"loss","runId":"123"},{"plugin":"scalars","tag":"default"}]', + expectedPinnedCards: [{plugin: PluginType.SCALARS, tag: 'default'}], + }, { // sample provided with non-sampled plugin serializedValue: