From cc1bbaf515cb6109beb8a7f8ea93f2a2d2c24ae0 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 4 Aug 2023 17:34:22 +0000 Subject: [PATCH 01/10] stop making duplicate time series requests --- tensorboard/webapp/metrics/effects/index.ts | 82 ++++++++++--- .../metrics/effects/metrics_effects_test.ts | 115 ++++++++++++++++-- 2 files changed, 170 insertions(+), 27 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index d2f5684ace..c2fd5db024 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -19,6 +19,7 @@ import {forkJoin, merge, Observable, of} from 'rxjs'; import { catchError, throttleTime, + combineLatestWith, filter, map, mergeMap, @@ -68,6 +69,12 @@ const getCardFetchInfo = createSelector( const initAction = createAction('[Metrics Effects] Init'); +function parseRunIdFromSampledRunInfoName(eidRun: string): string { + if (!eidRun) return ''; + const [, ...runIdChunks] = eidRun.split('/'); + return runIdChunks.join('/'); +} + @Injectable() export class MetricsEffects implements OnInitEffects { constructor( @@ -76,6 +83,40 @@ export class MetricsEffects implements OnInitEffects { private readonly dataSource: MetricsDataSource ) {} + readonly tagToEid$: Observable>> = this.store + .select(selectors.getMetricsTagMetadata) + .pipe( + combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)), + map(([tagMetadata, runToEid]) => { + const imageTagToRuns = Object.fromEntries( + Object.entries(tagMetadata.images.tagRunSampledInfo).map( + ([tag, sampledRunInfo]) => { + const runIds = Object.keys(sampledRunInfo).map((runInfoKey) => + parseRunIdFromSampledRunInfoName(runInfoKey) + ); + return [tag, runIds]; + } + ) + ); + + const tagToEid: Record> = {}; + function mapTagsToEid(tagToRun: Record) { + Object.entries(tagToRun).forEach(([tag, runIds]) => { + if (!tagToEid[tag]) { + tagToEid[tag] = new Set(); + } + runIds.forEach((runId) => tagToEid[tag].add(runToEid[runId])); + }); + } + + mapTagsToEid(tagMetadata.scalars.tagToRuns); + mapTagsToEid(tagMetadata.histograms.tagToRuns); + mapTagsToEid(imageTagToRuns); + + return tagToEid; + }) + ); + /** @export */ ngrxOnInitEffects(): Action { return initAction(); @@ -195,23 +236,31 @@ export class MetricsEffects implements OnInitEffects { fetchInfos: CardFetchInfo[], experimentIds: string[] ) { - /** - * TODO(psybuzz): if 2 cards require the same data, we should dedupe instead of - * making 2 identical requests. - */ - const requests: TimeSeriesRequest[] = fetchInfos.map((fetchInfo) => { - const {plugin, tag, runId, sample} = fetchInfo; - const partialRequest: TimeSeriesRequest = isSingleRunPlugin(plugin) - ? {plugin, tag, runId: runId!} - : {plugin, tag, experimentIds}; - if (sample !== undefined) { - partialRequest.sample = sample; - } - return partialRequest; - }); - // Fetch and handle responses. - return of(requests).pipe( + return this.tagToEid$.pipe( + map((tagToEid): TimeSeriesRequest[] => { + const requests = fetchInfos.map((fetchInfo) => { + const {plugin, tag, runId, sample} = fetchInfo; + const filteredEids = experimentIds.filter((eid) => + tagToEid[tag]?.has(eid) + ); + + const partialRequest: TimeSeriesRequest = isSingleRunPlugin(plugin) + ? {plugin, tag, runId: runId!} + : {plugin, tag, experimentIds: filteredEids}; + if (sample !== undefined) { + partialRequest.sample = sample; + } + return partialRequest; + }); + const uniqueRequests = new Set( + requests.map((request) => JSON.stringify(request)) + ); + + return Array.from(uniqueRequests).map( + (serialized) => JSON.parse(serialized) as TimeSeriesRequest + ); + }), tap((requests) => { this.store.dispatch(actions.multipleTimeSeriesRequested({requests})); }), @@ -302,4 +351,5 @@ export class MetricsEffects implements OnInitEffects { export const TEST_ONLY = { getCardFetchInfo, initAction, + parseRunIdFromSampledRunInfoName, }; diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index 2851c19a7d..9512c821d4 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -89,8 +89,52 @@ describe('metrics effects', () => { selectors.getMetricsTooltipSort, TooltipSort.ALPHABETICAL ); + + overrideTagMetadata(); + overrideRunToEid(); }); + function overrideTagMetadata() { + store.overrideSelector(selectors.getMetricsTagMetadata, { + scalars: { + tagDescriptions: {} as any, + tagToRuns: { + tagA: ['run1'], + tagB: ['run2', 'run3'], + tagC: ['run4', 'run5'], + tagD: ['run6'], + }, + }, + histograms: { + tagDescriptions: {} as any, + tagToRuns: { + tagA: ['run1'], + tagB: ['run4'], + }, + }, + images: { + tagDescriptions: {}, + tagRunSampledInfo: { + tagC: { + 'defaultExperimentId/run1': {} as any, + 'exp1/run3': {} as any, + }, + }, + }, + }); + } + + function overrideRunToEid() { + store.overrideSelector(selectors.getRunIdToExperimentId, { + run1: 'exp1', + run2: 'exp1', + run3: 'exp2', + run4: 'defaultExperimentId', + run5: 'defaultExperimentId', + run6: 'defaultExperimentId', + }); + } + afterEach(() => { store?.resetSelectors(); }); @@ -365,15 +409,13 @@ describe('metrics effects', () => { actions$.next(reloadAction()); expect(fetchTagMetadataSpy).toHaveBeenCalled(); - expect(fetchTimeSeriesSpy).toHaveBeenCalledTimes(2); + expect(fetchTimeSeriesSpy).toHaveBeenCalledTimes(1); expect(actualActions).toEqual([ actions.metricsTagMetadataRequested(), actions.metricsTagMetadataLoaded({ tagMetadata: buildDataSourceTagMetadata(), }), - // Currently we expect 2x the same requests if the cards are the same. - // Ideally we should dedupe requests for the same info. actions.multipleTimeSeriesRequested({ requests: [ { @@ -381,19 +423,11 @@ describe('metrics effects', () => { tag: 'tagA', experimentIds: ['exp1'], }, - { - plugin: PluginType.SCALARS as MultiRunPluginType, - tag: 'tagA', - experimentIds: ['exp1'], - }, ], }), actions.fetchTimeSeriesLoaded({ response: buildTimeSeriesResponse(), }), - actions.fetchTimeSeriesLoaded({ - response: buildTimeSeriesResponse(), - }), ]); }); @@ -487,6 +521,8 @@ describe('metrics effects', () => { it('does not re-fetch time series, until a valid experiment id', () => { // Reset any `getExperimentIdsFromRoute` overrides above. store.resetSelectors(); + overrideTagMetadata(); + overrideRunToEid(); store.overrideSelector(getActivePlugin, METRICS_PLUGIN_ID); store.overrideSelector( selectors.getVisibleCardIdSet, @@ -510,6 +546,43 @@ describe('metrics effects', () => { expect(fetchTimeSeriesSpy).toHaveBeenCalledTimes(2); }); + + it('does not send requests to experiments lacking a cards tag', () => { + store.overrideSelector(getActivePlugin, METRICS_PLUGIN_ID); + store.overrideSelector(selectors.getExperimentIdsFromRoute, [ + 'exp1', + 'exp2', + ]); + store.overrideSelector( + selectors.getVisibleCardIdSet, + new Set(['card1', 'card2']) + ); + provideCardFetchInfo([ + {id: 'card1', tag: 'tagA'}, + {id: 'card2', tag: 'tagB'}, + ]); + store.refreshState(); + + const effectFetchTimeSeriesSpy = spyOn( + effects as any, + 'fetchTimeSeries' + ).and.stub(); + + actions$.next(coreActions.manualReload()); + + expect(effectFetchTimeSeriesSpy).toHaveBeenCalledTimes(2); + expect(effectFetchTimeSeriesSpy).toHaveBeenCalledWith({ + plugin: 'scalars', + tag: 'tagA', + experimentIds: ['exp1'], + }); + + expect(effectFetchTimeSeriesSpy).toHaveBeenCalledWith({ + plugin: 'scalars', + tag: 'tagB', + experimentIds: ['exp1', 'exp2'], + }); + }); }); describe('loadTimeSeriesForVisibleCardsWithoutData', () => { @@ -778,4 +851,24 @@ describe('metrics effects', () => { } }); }); + + describe('#utilities', () => { + describe('parseRunIdFromSampledRunInfoName', () => { + it('removes prefixed experiment id', () => { + expect( + TEST_ONLY.parseRunIdFromSampledRunInfoName('experimentId/someRun') + ).toEqual('someRun'); + }); + + it('preserves "/" characters in run names', () => { + expect( + TEST_ONLY.parseRunIdFromSampledRunInfoName('experimentId/some/run') + ).toEqual('some/run'); + }); + + it('returns an empty string when an empty string is provided', () => { + expect(TEST_ONLY.parseRunIdFromSampledRunInfoName('')).toEqual(''); + }); + }); + }); }); From 94b102ae8ab3036c32ee1776b5a66a8cbc077b61 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 17 Aug 2023 20:21:41 +0000 Subject: [PATCH 02/10] use plugintype rather than referencing plugins directly --- tensorboard/webapp/metrics/effects/index.ts | 50 ++++++++++++++------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index c2fd5db024..b9b9da7fc6 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -27,6 +27,7 @@ import { take, tap, withLatestFrom, + shareReplay, } from 'rxjs/operators'; import * as routingActions from '../../app_routing/actions'; import {State} from '../../app_state'; @@ -43,13 +44,17 @@ import { TagMetadata, TimeSeriesRequest, TimeSeriesResponse, + SampledPluginType, + isSampledPlugin, + NonSampledPluginType, + SampledTagMetadata, } from '../data_source/index'; import { getCardLoadState, getCardMetadata, getMetricsTagMetadataLoadState, } from '../store'; -import {CardId, CardMetadata} from '../types'; +import {CardId, CardMetadata, PluginType} from '../types'; export type CardFetchInfo = CardMetadata & { id: CardId; @@ -75,6 +80,15 @@ function parseRunIdFromSampledRunInfoName(eidRun: string): string { return runIdChunks.join('/'); } +function sampledPluginToTagRunPairs(plugin: SampledTagMetadata) { + return Object.fromEntries( + Object.entries(plugin.tagRunSampledInfo).map(([tag, sampledRunInfo]) => { + const runIds = Object.keys(sampledRunInfo); + return [tag, runIds]; + }) + ); +} + @Injectable() export class MetricsEffects implements OnInitEffects { constructor( @@ -83,22 +97,14 @@ export class MetricsEffects implements OnInitEffects { private readonly dataSource: MetricsDataSource ) {} + /** + * Computes a record of tag to the experiments it appears in. + */ readonly tagToEid$: Observable>> = this.store .select(selectors.getMetricsTagMetadata) .pipe( combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)), map(([tagMetadata, runToEid]) => { - const imageTagToRuns = Object.fromEntries( - Object.entries(tagMetadata.images.tagRunSampledInfo).map( - ([tag, sampledRunInfo]) => { - const runIds = Object.keys(sampledRunInfo).map((runInfoKey) => - parseRunIdFromSampledRunInfoName(runInfoKey) - ); - return [tag, runIds]; - } - ) - ); - const tagToEid: Record> = {}; function mapTagsToEid(tagToRun: Record) { Object.entries(tagToRun).forEach(([tag, runIds]) => { @@ -109,12 +115,23 @@ export class MetricsEffects implements OnInitEffects { }); } - mapTagsToEid(tagMetadata.scalars.tagToRuns); - mapTagsToEid(tagMetadata.histograms.tagToRuns); - mapTagsToEid(imageTagToRuns); + for (const pluginType in tagMetadata) { + if (isSampledPlugin(pluginType as PluginType)) { + const tagRunPairs = sampledPluginToTagRunPairs( + tagMetadata[pluginType as SampledPluginType] + ); + mapTagsToEid(tagRunPairs); + continue; + } + + mapTagsToEid( + tagMetadata[pluginType as NonSampledPluginType].tagToRuns + ); + } return tagToEid; - }) + }), + shareReplay(1) ); /** @export */ @@ -238,6 +255,7 @@ export class MetricsEffects implements OnInitEffects { ) { // Fetch and handle responses. return this.tagToEid$.pipe( + take(1), map((tagToEid): TimeSeriesRequest[] => { const requests = fetchInfos.map((fetchInfo) => { const {plugin, tag, runId, sample} = fetchInfo; From 581461882043ec39c20e0024df85741c26abe58f Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 17 Aug 2023 21:15:55 +0000 Subject: [PATCH 03/10] refactor tagToEid + use plugin type --- tensorboard/webapp/metrics/effects/index.ts | 20 ++++++++++--------- .../metrics/effects/metrics_effects_test.ts | 20 ------------------- 2 files changed, 11 insertions(+), 29 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index b9b9da7fc6..6c1e01eb7b 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -74,13 +74,7 @@ const getCardFetchInfo = createSelector( const initAction = createAction('[Metrics Effects] Init'); -function parseRunIdFromSampledRunInfoName(eidRun: string): string { - if (!eidRun) return ''; - const [, ...runIdChunks] = eidRun.split('/'); - return runIdChunks.join('/'); -} - -function sampledPluginToTagRunPairs(plugin: SampledTagMetadata) { +function sampledPluginToTagRunIdPairs(plugin: SampledTagMetadata) { return Object.fromEntries( Object.entries(plugin.tagRunSampledInfo).map(([tag, sampledRunInfo]) => { const runIds = Object.keys(sampledRunInfo); @@ -99,6 +93,15 @@ export class MetricsEffects implements OnInitEffects { /** * Computes a record of tag to the experiments it appears in. + * + * The computation is done by translating Plugin -> Tag -> Run -> ExpId + * Unfortunately Sampled and NonSampled plugins store the Tag -> Run relationship + * differently. + * + * SampledPlugins contain Record + * NonSampledPlugins have Record + * + * To handle this inconsistency the SampledPlugins datascructure is simplified and inverted. */ readonly tagToEid$: Observable>> = this.store .select(selectors.getMetricsTagMetadata) @@ -117,7 +120,7 @@ export class MetricsEffects implements OnInitEffects { for (const pluginType in tagMetadata) { if (isSampledPlugin(pluginType as PluginType)) { - const tagRunPairs = sampledPluginToTagRunPairs( + const tagRunPairs = sampledPluginToTagRunIdPairs( tagMetadata[pluginType as SampledPluginType] ); mapTagsToEid(tagRunPairs); @@ -369,5 +372,4 @@ export class MetricsEffects implements OnInitEffects { export const TEST_ONLY = { getCardFetchInfo, initAction, - parseRunIdFromSampledRunInfoName, }; diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index 9512c821d4..9584c2aae5 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -851,24 +851,4 @@ describe('metrics effects', () => { } }); }); - - describe('#utilities', () => { - describe('parseRunIdFromSampledRunInfoName', () => { - it('removes prefixed experiment id', () => { - expect( - TEST_ONLY.parseRunIdFromSampledRunInfoName('experimentId/someRun') - ).toEqual('someRun'); - }); - - it('preserves "/" characters in run names', () => { - expect( - TEST_ONLY.parseRunIdFromSampledRunInfoName('experimentId/some/run') - ).toEqual('some/run'); - }); - - it('returns an empty string when an empty string is provided', () => { - expect(TEST_ONLY.parseRunIdFromSampledRunInfoName('')).toEqual(''); - }); - }); - }); }); From 20bf2f02410b7d8df6db36885df50916dc4abe0b Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 17 Aug 2023 21:43:33 +0000 Subject: [PATCH 04/10] minor test updates --- tensorboard/webapp/metrics/effects/metrics_effects_test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index 9584c2aae5..9fc624ca94 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -560,6 +560,8 @@ describe('metrics effects', () => { provideCardFetchInfo([ {id: 'card1', tag: 'tagA'}, {id: 'card2', tag: 'tagB'}, + {id: 'card3', tag: 'tagC'}, + {id: 'card4', tag: 'tagD'}, ]); store.refreshState(); From 4d264a5677a8ac84268dd8cf7a417783fb9d0fe3 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 18 Aug 2023 19:27:42 +0000 Subject: [PATCH 05/10] finished writing tests, discovered a bug - nonsampled data is fetched by run not eid and should be handled differently --- tensorboard/webapp/metrics/effects/BUILD | 1 + tensorboard/webapp/metrics/effects/index.ts | 109 +++++---- .../metrics/effects/metrics_effects_test.ts | 210 +++++++++++++++--- 3 files changed, 255 insertions(+), 65 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/BUILD b/tensorboard/webapp/metrics/effects/BUILD index b0979fc828..e148c6b79f 100644 --- a/tensorboard/webapp/metrics/effects/BUILD +++ b/tensorboard/webapp/metrics/effects/BUILD @@ -17,6 +17,7 @@ tf_ng_module( "//tensorboard/webapp/metrics/data_source", "//tensorboard/webapp/metrics/store", "//tensorboard/webapp/types", + "//tensorboard/webapp/util:types", "@npm//@angular/core", "@npm//@ngrx/effects", "@npm//@ngrx/store", diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index 6c1e01eb7b..2773d84434 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -53,8 +53,10 @@ import { getCardLoadState, getCardMetadata, getMetricsTagMetadataLoadState, + TagMetadata as StoreTagMetadata, } from '../store'; import {CardId, CardMetadata, PluginType} from '../types'; +import {DeepReadonly} from '../../util/types'; export type CardFetchInfo = CardMetadata & { id: CardId; @@ -83,6 +85,35 @@ function sampledPluginToTagRunIdPairs(plugin: SampledTagMetadata) { ); } +function generateTagToEidMapping( + tagMetadata: DeepReadonly, + runToEid: Record +): Record> { + const tagToEid: Record> = {}; + function mapTagsToEid(tagToRun: Record) { + Object.entries(tagToRun).forEach(([tag, runIds]) => { + if (!tagToEid[tag]) { + tagToEid[tag] = new Set(); + } + runIds.forEach((runId) => tagToEid[tag].add(runToEid[runId])); + }); + } + + for (const pluginType in tagMetadata) { + if (isSampledPlugin(pluginType as PluginType)) { + const tagRunPairs = sampledPluginToTagRunIdPairs( + tagMetadata[pluginType as SampledPluginType] + ); + mapTagsToEid(tagRunPairs); + continue; + } + + mapTagsToEid(tagMetadata[pluginType as NonSampledPluginType].tagToRuns); + } + + return tagToEid; +} + @Injectable() export class MetricsEffects implements OnInitEffects { constructor( @@ -108,31 +139,7 @@ export class MetricsEffects implements OnInitEffects { .pipe( combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)), map(([tagMetadata, runToEid]) => { - const tagToEid: Record> = {}; - function mapTagsToEid(tagToRun: Record) { - Object.entries(tagToRun).forEach(([tag, runIds]) => { - if (!tagToEid[tag]) { - tagToEid[tag] = new Set(); - } - runIds.forEach((runId) => tagToEid[tag].add(runToEid[runId])); - }); - } - - for (const pluginType in tagMetadata) { - if (isSampledPlugin(pluginType as PluginType)) { - const tagRunPairs = sampledPluginToTagRunIdPairs( - tagMetadata[pluginType as SampledPluginType] - ); - mapTagsToEid(tagRunPairs); - continue; - } - - mapTagsToEid( - tagMetadata[pluginType as NonSampledPluginType].tagToRuns - ); - } - - return tagToEid; + return generateTagToEidMapping(tagMetadata, runToEid); }), shareReplay(1) ); @@ -260,20 +267,44 @@ export class MetricsEffects implements OnInitEffects { return this.tagToEid$.pipe( take(1), map((tagToEid): TimeSeriesRequest[] => { - const requests = fetchInfos.map((fetchInfo) => { - const {plugin, tag, runId, sample} = fetchInfo; - const filteredEids = experimentIds.filter((eid) => - tagToEid[tag]?.has(eid) - ); + console.log('fetchInfos', fetchInfos); + const requests = fetchInfos + .map((fetchInfo) => { + const {plugin, tag, runId, sample} = fetchInfo; - const partialRequest: TimeSeriesRequest = isSingleRunPlugin(plugin) - ? {plugin, tag, runId: runId!} - : {plugin, tag, experimentIds: filteredEids}; - if (sample !== undefined) { - partialRequest.sample = sample; - } - return partialRequest; - }); + if (isSingleRunPlugin(plugin)) { + if (!runId) { + return; + } + return { + plugin, + tag, + runId, + sample, + }; + } + + console.log( + tag, + 'tagToEid', + Object.fromEntries( + Object.entries(tagToEid).map(([key, value]) => [ + key, + Array.from(value), + ]) + ) + ); + const filteredEids = experimentIds.filter((eid) => + tagToEid[tag]?.has(eid) + ); + if (!filteredEids.length) { + return; + } + + return {plugin, tag, experimentIds: filteredEids}; + }) + .filter(Boolean); + console.log('requests', requests); const uniqueRequests = new Set( requests.map((request) => JSON.stringify(request)) ); @@ -372,4 +403,6 @@ export class MetricsEffects implements OnInitEffects { export const TEST_ONLY = { getCardFetchInfo, initAction, + sampledPluginToTagRunIdPairs, + generateTagToEidMapping, }; diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index 9fc624ca94..e4dee4d1e6 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -38,6 +38,7 @@ import { TagMetadata, TimeSeriesRequest, TimeSeriesResponse, + SampledTagMetadata, } from '../data_source'; import {getMetricsTagMetadataLoadState} from '../store'; import { @@ -50,7 +51,7 @@ import { import {CardId, TooltipSort} from '../types'; import {CardFetchInfo, MetricsEffects, TEST_ONLY} from './index'; -describe('metrics effects', () => { +fdescribe('metrics effects', () => { let dataSource: MetricsDataSource; let effects: MetricsEffects; let store: MockStore; @@ -97,7 +98,7 @@ describe('metrics effects', () => { function overrideTagMetadata() { store.overrideSelector(selectors.getMetricsTagMetadata, { scalars: { - tagDescriptions: {} as any, + tagDescriptions: {}, tagToRuns: { tagA: ['run1'], tagB: ['run2', 'run3'], @@ -106,7 +107,7 @@ describe('metrics effects', () => { }, }, histograms: { - tagDescriptions: {} as any, + tagDescriptions: {}, tagToRuns: { tagA: ['run1'], tagB: ['run4'], @@ -116,8 +117,8 @@ describe('metrics effects', () => { tagDescriptions: {}, tagRunSampledInfo: { tagC: { - 'defaultExperimentId/run1': {} as any, - 'exp1/run3': {} as any, + run1: {maxSamplesPerStep: 1}, + run3: {maxSamplesPerStep: 1}, }, }, }, @@ -547,7 +548,7 @@ describe('metrics effects', () => { expect(fetchTimeSeriesSpy).toHaveBeenCalledTimes(2); }); - it('does not send requests to experiments lacking a cards tag', () => { + fit('does not send requests to experiments lacking a cards tag', () => { store.overrideSelector(getActivePlugin, METRICS_PLUGIN_ID); store.overrideSelector(selectors.getExperimentIdsFromRoute, [ 'exp1', @@ -555,35 +556,64 @@ describe('metrics effects', () => { ]); store.overrideSelector( selectors.getVisibleCardIdSet, - new Set(['card1', 'card2']) + new Set(['card1', 'card2', 'card3', 'card4', 'card5', 'card6']) ); provideCardFetchInfo([ - {id: 'card1', tag: 'tagA'}, - {id: 'card2', tag: 'tagB'}, - {id: 'card3', tag: 'tagC'}, - {id: 'card4', tag: 'tagD'}, + {id: 'card1', tag: 'tagA', plugin: PluginType.SCALARS}, + { + id: 'card2', + tag: 'tagA', + plugin: PluginType.HISTOGRAMS, + runId: 'run1', + }, + {id: 'card3', tag: 'tagB', plugin: PluginType.SCALARS}, + // Fetch info should not be provided for tagB histogram data because there + // is no histogram data associated with both tagB and either exp1, or exp2 + {id: 'card4', tag: 'tagC'}, + {id: 'card5', tag: 'tagD'}, + { + id: 'card6', + tag: 'tagC', + plugin: PluginType.IMAGES, + runId: 'run3', + }, ]); store.refreshState(); - const effectFetchTimeSeriesSpy = spyOn( - effects as any, - 'fetchTimeSeries' - ).and.stub(); + const requests: TimeSeriesRequest[] = []; + spyOn(effects as any, 'fetchTimeSeries').and.callFake( + (request: TimeSeriesRequest) => { + console.log('request', request); + requests.push(request); + } + ); actions$.next(coreActions.manualReload()); - expect(effectFetchTimeSeriesSpy).toHaveBeenCalledTimes(2); - expect(effectFetchTimeSeriesSpy).toHaveBeenCalledWith({ - plugin: 'scalars', - tag: 'tagA', - experimentIds: ['exp1'], - }); - - expect(effectFetchTimeSeriesSpy).toHaveBeenCalledWith({ - plugin: 'scalars', - tag: 'tagB', - experimentIds: ['exp1', 'exp2'], - }); + expect(requests).toEqual([ + { + plugin: PluginType.SCALARS, + tag: 'tagA', + experimentIds: ['exp1'], + }, + { + plugin: PluginType.HISTOGRAMS, + runId: 'run1', + tag: 'tagA', + }, + { + plugin: PluginType.SCALARS, + tag: 'tagB', + experimentIds: ['exp1', 'exp2'], + }, + // No requests should be sent for card 3 or 4 because all their + // runs are associated with another experiment. + { + plugin: PluginType.IMAGES, + tag: 'tagC', + runId: 'run3', + }, + ]); }); }); @@ -853,4 +883,130 @@ describe('metrics effects', () => { } }); }); + + describe('utilities', () => { + it('sampledPluginToTagRunIdPairs flattens sampled plugin tag to run mapping', () => { + const plugin: SampledTagMetadata = { + tagDescriptions: {}, + tagRunSampledInfo: { + tagA: { + run1: {maxSamplesPerStep: 1}, + run2: {maxSamplesPerStep: 1}, + }, + tagB: { + run3: {maxSamplesPerStep: 1}, + }, + }, + }; + expect(TEST_ONLY.sampledPluginToTagRunIdPairs(plugin)).toEqual({ + tagA: ['run1', 'run2'], + tagB: ['run3'], + }); + }); + + describe('generateTagToEidMapping', () => { + it('maps image plugin data', () => { + const runToEid = { + run1: 'eid1', + run2: 'eid2', + run3: 'eid1', + }; + const tagMetadata = { + images: { + tagDescriptions: {}, + tagRunSampledInfo: { + tagA: { + run1: {maxSamplesPerStep: 1}, + run2: {maxSamplesPerStep: 1}, + }, + tagB: { + run3: {maxSamplesPerStep: 1}, + }, + }, + }, + }; + expect( + TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + ).toEqual({ + tagA: new Set(['eid1', 'eid2']), + tagB: new Set(['eid1']), + }); + }); + + it('maps scalar data', () => { + const runToEid = { + run1: 'eid1', + run2: 'eid1', + run3: 'eid2', + }; + const tagMetadata = { + scalars: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + tagB: ['run2', 'run3'], + }, + }, + }; + + expect( + TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + ).toEqual({ + tagA: new Set(['eid1']), + tagB: new Set(['eid1', 'eid2']), + }); + }); + + it('maps histogram data', () => { + const runToEid = { + run1: 'eid1', + run2: 'eid1', + run3: 'eid2', + }; + const tagMetadata = { + histograms: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + tagB: ['run2', 'run3'], + }, + }, + }; + + expect( + TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + ).toEqual({ + tagA: new Set(['eid1']), + tagB: new Set(['eid1', 'eid2']), + }); + }); + + it('tags with multiple data types are additive', () => { + const runToEid = { + run1: 'eid1', + run2: 'eid2', + }; + const tagMetadata = { + scalars: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + }, + }, + histograms: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run2'], + }, + }, + }; + + expect( + TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + ).toEqual({ + tagA: new Set(['eid1', 'eid2']), + }); + }); + }); + }); }); From 3eea3982fa378fd85b3886c60d837f44285819fd Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 18 Aug 2023 19:33:54 +0000 Subject: [PATCH 06/10] fix the issue with sampled plugins. This is still subtly wrong - histograms and scalars should not be entangled --- tensorboard/webapp/metrics/effects/index.ts | 46 +++-------------- .../metrics/effects/metrics_effects_test.ts | 51 ++++++++----------- 2 files changed, 27 insertions(+), 70 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index 2773d84434..c2fec1fe04 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -76,16 +76,7 @@ const getCardFetchInfo = createSelector( const initAction = createAction('[Metrics Effects] Init'); -function sampledPluginToTagRunIdPairs(plugin: SampledTagMetadata) { - return Object.fromEntries( - Object.entries(plugin.tagRunSampledInfo).map(([tag, sampledRunInfo]) => { - const runIds = Object.keys(sampledRunInfo); - return [tag, runIds]; - }) - ); -} - -function generateTagToEidMapping( +function generateNonSampledTagToEidMapping( tagMetadata: DeepReadonly, runToEid: Record ): Record> { @@ -101,10 +92,6 @@ function generateTagToEidMapping( for (const pluginType in tagMetadata) { if (isSampledPlugin(pluginType as PluginType)) { - const tagRunPairs = sampledPluginToTagRunIdPairs( - tagMetadata[pluginType as SampledPluginType] - ); - mapTagsToEid(tagRunPairs); continue; } @@ -126,20 +113,14 @@ export class MetricsEffects implements OnInitEffects { * Computes a record of tag to the experiments it appears in. * * The computation is done by translating Plugin -> Tag -> Run -> ExpId - * Unfortunately Sampled and NonSampled plugins store the Tag -> Run relationship - * differently. - * - * SampledPlugins contain Record - * NonSampledPlugins have Record * - * To handle this inconsistency the SampledPlugins datascructure is simplified and inverted. + * Sampled plugins are ignored because they are associated with runs, not experiments. */ - readonly tagToEid$: Observable>> = this.store - .select(selectors.getMetricsTagMetadata) - .pipe( + readonly nonSampledTagToEid$: Observable>> = + this.store.select(selectors.getMetricsTagMetadata).pipe( combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)), map(([tagMetadata, runToEid]) => { - return generateTagToEidMapping(tagMetadata, runToEid); + return generateNonSampledTagToEidMapping(tagMetadata, runToEid); }), shareReplay(1) ); @@ -264,10 +245,9 @@ export class MetricsEffects implements OnInitEffects { experimentIds: string[] ) { // Fetch and handle responses. - return this.tagToEid$.pipe( + return this.nonSampledTagToEid$.pipe( take(1), map((tagToEid): TimeSeriesRequest[] => { - console.log('fetchInfos', fetchInfos); const requests = fetchInfos .map((fetchInfo) => { const {plugin, tag, runId, sample} = fetchInfo; @@ -284,16 +264,6 @@ export class MetricsEffects implements OnInitEffects { }; } - console.log( - tag, - 'tagToEid', - Object.fromEntries( - Object.entries(tagToEid).map(([key, value]) => [ - key, - Array.from(value), - ]) - ) - ); const filteredEids = experimentIds.filter((eid) => tagToEid[tag]?.has(eid) ); @@ -304,7 +274,6 @@ export class MetricsEffects implements OnInitEffects { return {plugin, tag, experimentIds: filteredEids}; }) .filter(Boolean); - console.log('requests', requests); const uniqueRequests = new Set( requests.map((request) => JSON.stringify(request)) ); @@ -403,6 +372,5 @@ export class MetricsEffects implements OnInitEffects { export const TEST_ONLY = { getCardFetchInfo, initAction, - sampledPluginToTagRunIdPairs, - generateTagToEidMapping, + generateNonSampledTagToEidMapping, }; diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index e4dee4d1e6..b6eb725330 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -548,7 +548,7 @@ fdescribe('metrics effects', () => { expect(fetchTimeSeriesSpy).toHaveBeenCalledTimes(2); }); - fit('does not send requests to experiments lacking a cards tag', () => { + it('does not send requests to experiments lacking a cards tag', () => { store.overrideSelector(getActivePlugin, METRICS_PLUGIN_ID); store.overrideSelector(selectors.getExperimentIdsFromRoute, [ 'exp1', @@ -583,7 +583,6 @@ fdescribe('metrics effects', () => { const requests: TimeSeriesRequest[] = []; spyOn(effects as any, 'fetchTimeSeries').and.callFake( (request: TimeSeriesRequest) => { - console.log('request', request); requests.push(request); } ); @@ -885,27 +884,8 @@ fdescribe('metrics effects', () => { }); describe('utilities', () => { - it('sampledPluginToTagRunIdPairs flattens sampled plugin tag to run mapping', () => { - const plugin: SampledTagMetadata = { - tagDescriptions: {}, - tagRunSampledInfo: { - tagA: { - run1: {maxSamplesPerStep: 1}, - run2: {maxSamplesPerStep: 1}, - }, - tagB: { - run3: {maxSamplesPerStep: 1}, - }, - }, - }; - expect(TEST_ONLY.sampledPluginToTagRunIdPairs(plugin)).toEqual({ - tagA: ['run1', 'run2'], - tagB: ['run3'], - }); - }); - - describe('generateTagToEidMapping', () => { - it('maps image plugin data', () => { + describe('generateNonSampledTagToEidMapping', () => { + it('does not map image plugin data', () => { const runToEid = { run1: 'eid1', run2: 'eid2', @@ -926,11 +906,11 @@ fdescribe('metrics effects', () => { }, }; expect( - TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) - ).toEqual({ - tagA: new Set(['eid1', 'eid2']), - tagB: new Set(['eid1']), - }); + TEST_ONLY.generateNonSampledTagToEidMapping( + tagMetadata as any, + runToEid + ) + ).toEqual({}); }); it('maps scalar data', () => { @@ -950,7 +930,10 @@ fdescribe('metrics effects', () => { }; expect( - TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + TEST_ONLY.generateNonSampledTagToEidMapping( + tagMetadata as any, + runToEid + ) ).toEqual({ tagA: new Set(['eid1']), tagB: new Set(['eid1', 'eid2']), @@ -974,7 +957,10 @@ fdescribe('metrics effects', () => { }; expect( - TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + TEST_ONLY.generateNonSampledTagToEidMapping( + tagMetadata as any, + runToEid + ) ).toEqual({ tagA: new Set(['eid1']), tagB: new Set(['eid1', 'eid2']), @@ -1002,7 +988,10 @@ fdescribe('metrics effects', () => { }; expect( - TEST_ONLY.generateTagToEidMapping(tagMetadata as any, runToEid) + TEST_ONLY.generateNonSampledTagToEidMapping( + tagMetadata as any, + runToEid + ) ).toEqual({ tagA: new Set(['eid1', 'eid2']), }); From 0d741af142aa830c10705079b652074771aa74bb Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 18 Aug 2023 20:55:34 +0000 Subject: [PATCH 07/10] only remap multi run plugins --- tensorboard/webapp/metrics/effects/index.ts | 15 +++--- .../metrics/effects/metrics_effects_test.ts | 53 ++++--------------- 2 files changed, 16 insertions(+), 52 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index c2fec1fe04..b88a6643f1 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -44,10 +44,7 @@ import { TagMetadata, TimeSeriesRequest, TimeSeriesResponse, - SampledPluginType, - isSampledPlugin, NonSampledPluginType, - SampledTagMetadata, } from '../data_source/index'; import { getCardLoadState, @@ -76,7 +73,7 @@ const getCardFetchInfo = createSelector( const initAction = createAction('[Metrics Effects] Init'); -function generateNonSampledTagToEidMapping( +function generateMultiRunTagsToEidMapping( tagMetadata: DeepReadonly, runToEid: Record ): Record> { @@ -91,7 +88,7 @@ function generateNonSampledTagToEidMapping( } for (const pluginType in tagMetadata) { - if (isSampledPlugin(pluginType as PluginType)) { + if (isSingleRunPlugin(pluginType as PluginType)) { continue; } @@ -116,11 +113,11 @@ export class MetricsEffects implements OnInitEffects { * * Sampled plugins are ignored because they are associated with runs, not experiments. */ - readonly nonSampledTagToEid$: Observable>> = + readonly multiRunTagsToEid$: Observable>> = this.store.select(selectors.getMetricsTagMetadata).pipe( combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)), map(([tagMetadata, runToEid]) => { - return generateNonSampledTagToEidMapping(tagMetadata, runToEid); + return generateMultiRunTagsToEidMapping(tagMetadata, runToEid); }), shareReplay(1) ); @@ -245,7 +242,7 @@ export class MetricsEffects implements OnInitEffects { experimentIds: string[] ) { // Fetch and handle responses. - return this.nonSampledTagToEid$.pipe( + return this.multiRunTagsToEid$.pipe( take(1), map((tagToEid): TimeSeriesRequest[] => { const requests = fetchInfos @@ -372,5 +369,5 @@ export class MetricsEffects implements OnInitEffects { export const TEST_ONLY = { getCardFetchInfo, initAction, - generateNonSampledTagToEidMapping, + generateMultiRunTagsToEidMapping, }; diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index b6eb725330..8dbe49ce10 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -51,7 +51,7 @@ import { import {CardId, TooltipSort} from '../types'; import {CardFetchInfo, MetricsEffects, TEST_ONLY} from './index'; -fdescribe('metrics effects', () => { +describe('metrics effects', () => { let dataSource: MetricsDataSource; let effects: MetricsEffects; let store: MockStore; @@ -884,7 +884,7 @@ fdescribe('metrics effects', () => { }); describe('utilities', () => { - describe('generateNonSampledTagToEidMapping', () => { + describe('generateMultiRunTagsToEidMapping', () => { it('does not map image plugin data', () => { const runToEid = { run1: 'eid1', @@ -906,21 +906,21 @@ fdescribe('metrics effects', () => { }, }; expect( - TEST_ONLY.generateNonSampledTagToEidMapping( + TEST_ONLY.generateMultiRunTagsToEidMapping( tagMetadata as any, runToEid ) ).toEqual({}); }); - it('maps scalar data', () => { + it('does not map histogram data', () => { const runToEid = { run1: 'eid1', run2: 'eid1', run3: 'eid2', }; const tagMetadata = { - scalars: { + histograms: { tagDescriptions: {}, tagToRuns: { tagA: ['run1'], @@ -930,24 +930,21 @@ fdescribe('metrics effects', () => { }; expect( - TEST_ONLY.generateNonSampledTagToEidMapping( + TEST_ONLY.generateMultiRunTagsToEidMapping( tagMetadata as any, runToEid ) - ).toEqual({ - tagA: new Set(['eid1']), - tagB: new Set(['eid1', 'eid2']), - }); + ).toEqual({}); }); - it('maps histogram data', () => { + it('maps scalar data', () => { const runToEid = { run1: 'eid1', run2: 'eid1', run3: 'eid2', }; const tagMetadata = { - histograms: { + scalars: { tagDescriptions: {}, tagToRuns: { tagA: ['run1'], @@ -957,7 +954,7 @@ fdescribe('metrics effects', () => { }; expect( - TEST_ONLY.generateNonSampledTagToEidMapping( + TEST_ONLY.generateMultiRunTagsToEidMapping( tagMetadata as any, runToEid ) @@ -966,36 +963,6 @@ fdescribe('metrics effects', () => { tagB: new Set(['eid1', 'eid2']), }); }); - - it('tags with multiple data types are additive', () => { - const runToEid = { - run1: 'eid1', - run2: 'eid2', - }; - const tagMetadata = { - scalars: { - tagDescriptions: {}, - tagToRuns: { - tagA: ['run1'], - }, - }, - histograms: { - tagDescriptions: {}, - tagToRuns: { - tagA: ['run2'], - }, - }, - }; - - expect( - TEST_ONLY.generateNonSampledTagToEidMapping( - tagMetadata as any, - runToEid - ) - ).toEqual({ - tagA: new Set(['eid1', 'eid2']), - }); - }); }); }); }); From 02ee0fd13524221738608f1358fb673bb02a7c2e Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 24 Aug 2023 18:44:24 +0000 Subject: [PATCH 08/10] inline helper function --- tensorboard/webapp/metrics/effects/index.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index b88a6643f1..c7352e07c4 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -78,21 +78,19 @@ function generateMultiRunTagsToEidMapping( runToEid: Record ): Record> { const tagToEid: Record> = {}; - function mapTagsToEid(tagToRun: Record) { - Object.entries(tagToRun).forEach(([tag, runIds]) => { - if (!tagToEid[tag]) { - tagToEid[tag] = new Set(); - } - runIds.forEach((runId) => tagToEid[tag].add(runToEid[runId])); - }); - } - for (const pluginType in tagMetadata) { if (isSingleRunPlugin(pluginType as PluginType)) { continue; } - mapTagsToEid(tagMetadata[pluginType as NonSampledPluginType].tagToRuns); + Object.entries( + tagMetadata[pluginType as NonSampledPluginType].tagToRuns + ).forEach(([tag, runIds]) => { + if (!tagToEid[tag]) { + tagToEid[tag] = new Set(); + } + runIds.forEach((runId) => tagToEid[tag].add(runToEid[runId])); + }); } return tagToEid; From bcd88ae1d0e40a38c1bdb52cfacd1e83e021fad7 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 24 Aug 2023 21:04:43 +0000 Subject: [PATCH 09/10] add a detailed comment and change to using withLatestFrom --- tensorboard/webapp/metrics/effects/index.ts | 99 +++++++++++++-------- 1 file changed, 60 insertions(+), 39 deletions(-) diff --git a/tensorboard/webapp/metrics/effects/index.ts b/tensorboard/webapp/metrics/effects/index.ts index c7352e07c4..9ab1a95fbd 100644 --- a/tensorboard/webapp/metrics/effects/index.ts +++ b/tensorboard/webapp/metrics/effects/index.ts @@ -109,7 +109,27 @@ export class MetricsEffects implements OnInitEffects { * * The computation is done by translating Plugin -> Tag -> Run -> ExpId * - * Sampled plugins are ignored because they are associated with runs, not experiments. + * There are essentially 3 kinds of plugins but here we really only care about + * Single Run vs Multi Run + * 1) Non-sampled multi-run plugins + * 2) Non-sampled single-run plugins + * 3) Sampled single-run plugins + * Note: There are no sampled multi run plugins + * + * TensorBoard generates cards for the based on the Tag -> Run relationship it + * recieves from the `/timeseries/tags` response. + * + * As these cards become visible this effect is invoked to fetch data for the + * cards using the `/timeseries/timeSeries` endpoint. One request is made for each + * kind of data being shown for each experiment being viewed. + * + * Because runs can only be associated with a single experiment only a single + * request is required for single run plugin data. + * + * Multi run plugins can contain runs from multiple experiments (if they contain + * the same tag) and thus may require up to N requests where N is the number of + * experiments. This mapping from tag to experiment id is used to ensure we do + * not make unnecessary requests fetch data for experiments without the relevant tag. */ readonly multiRunTagsToEid$: Observable>> = this.store.select(selectors.getMetricsTagMetadata).pipe( @@ -237,46 +257,42 @@ export class MetricsEffects implements OnInitEffects { private fetchTimeSeriesForCards( fetchInfos: CardFetchInfo[], - experimentIds: string[] + experimentIds: string[], + multiRunTagsToEid: Record> ) { - // Fetch and handle responses. - return this.multiRunTagsToEid$.pipe( - take(1), - map((tagToEid): TimeSeriesRequest[] => { - const requests = fetchInfos - .map((fetchInfo) => { - const {plugin, tag, runId, sample} = fetchInfo; - - if (isSingleRunPlugin(plugin)) { - if (!runId) { - return; - } - return { - plugin, - tag, - runId, - sample, - }; - } + const requests = fetchInfos + .map((fetchInfo) => { + const {plugin, tag, runId, sample} = fetchInfo; - const filteredEids = experimentIds.filter((eid) => - tagToEid[tag]?.has(eid) - ); - if (!filteredEids.length) { - return; - } + if (isSingleRunPlugin(plugin)) { + if (!runId) { + return; + } + return { + plugin, + tag, + runId, + sample, + }; + } - return {plugin, tag, experimentIds: filteredEids}; - }) - .filter(Boolean); - const uniqueRequests = new Set( - requests.map((request) => JSON.stringify(request)) + const filteredEids = experimentIds.filter((eid) => + multiRunTagsToEid[tag]?.has(eid) ); + if (!filteredEids.length) { + return; + } - return Array.from(uniqueRequests).map( - (serialized) => JSON.parse(serialized) as TimeSeriesRequest - ); - }), + return {plugin, tag, sample, experimentIds: filteredEids}; + }) + .filter(Boolean); + + const uniqueRequests = Array.from( + new Set(requests.map((request) => JSON.stringify(request))) + ).map((serialized) => JSON.parse(serialized) as TimeSeriesRequest); + + // Fetch and handle responses. + return of(uniqueRequests).pipe( tap((requests) => { this.store.dispatch(actions.multipleTimeSeriesRequested({requests})); }), @@ -319,10 +335,15 @@ export class MetricsEffects implements OnInitEffects { withLatestFrom( this.store .select(selectors.getExperimentIdsFromRoute) - .pipe(filter((experimentIds) => experimentIds !== null)) + .pipe(filter((experimentIds) => experimentIds !== null)), + this.multiRunTagsToEid$ ), - mergeMap(([fetchInfos, experimentIds]) => { - return this.fetchTimeSeriesForCards(fetchInfos, experimentIds!); + mergeMap(([fetchInfos, experimentIds, multiRunTagsToEid]) => { + return this.fetchTimeSeriesForCards( + fetchInfos, + experimentIds!, + multiRunTagsToEid + ); }) ); From 11e772900880a79dbc67782e9ff2534d5f3a9eab Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Thu, 24 Aug 2023 21:14:14 +0000 Subject: [PATCH 10/10] add additional test --- .../metrics/effects/metrics_effects_test.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index 8dbe49ce10..d31cebbfa0 100644 --- a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts +++ b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts @@ -963,6 +963,52 @@ describe('metrics effects', () => { tagB: new Set(['eid1', 'eid2']), }); }); + + it('only maps scalar data', () => { + const runToEid = { + run1: 'eid1', + run2: 'eid1', + run3: 'eid2', + }; + const tagMetadata = { + scalars: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + tagB: ['run2', 'run3'], + }, + }, + histograms: { + tagDescriptions: {}, + tagToRuns: { + tagC: ['run4'], + tagD: ['run5', 'run6'], + }, + }, + images: { + tagDescriptions: {}, + tagRunSampledInfo: { + tagE: { + run7: {maxSamplesPerStep: 1}, + run8: {maxSamplesPerStep: 1}, + }, + tagF: { + run9: {maxSamplesPerStep: 1}, + }, + }, + }, + }; + + expect( + TEST_ONLY.generateMultiRunTagsToEidMapping( + tagMetadata as any, + runToEid + ) + ).toEqual({ + tagA: new Set(['eid1']), + tagB: new Set(['eid1', 'eid2']), + }); + }); }); }); });