diff --git a/tensorboard/webapp/metrics/effects/BUILD b/tensorboard/webapp/metrics/effects/BUILD index b0979fc828f..e148c6b79f3 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 d2f5684ace3..9ab1a95fbd4 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, @@ -26,6 +27,7 @@ import { take, tap, withLatestFrom, + shareReplay, } from 'rxjs/operators'; import * as routingActions from '../../app_routing/actions'; import {State} from '../../app_state'; @@ -42,13 +44,16 @@ import { TagMetadata, TimeSeriesRequest, TimeSeriesResponse, + NonSampledPluginType, } from '../data_source/index'; import { getCardLoadState, getCardMetadata, getMetricsTagMetadataLoadState, + TagMetadata as StoreTagMetadata, } from '../store'; -import {CardId, CardMetadata} from '../types'; +import {CardId, CardMetadata, PluginType} from '../types'; +import {DeepReadonly} from '../../util/types'; export type CardFetchInfo = CardMetadata & { id: CardId; @@ -68,6 +73,29 @@ const getCardFetchInfo = createSelector( const initAction = createAction('[Metrics Effects] Init'); +function generateMultiRunTagsToEidMapping( + tagMetadata: DeepReadonly, + runToEid: Record +): Record> { + const tagToEid: Record> = {}; + for (const pluginType in tagMetadata) { + if (isSingleRunPlugin(pluginType as PluginType)) { + continue; + } + + 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; +} + @Injectable() export class MetricsEffects implements OnInitEffects { constructor( @@ -76,6 +104,42 @@ export class MetricsEffects implements OnInitEffects { private readonly dataSource: MetricsDataSource ) {} + /** + * Computes a record of tag to the experiments it appears in. + * + * The computation is done by translating Plugin -> Tag -> Run -> ExpId + * + * 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( + combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)), + map(([tagMetadata, runToEid]) => { + return generateMultiRunTagsToEidMapping(tagMetadata, runToEid); + }), + shareReplay(1) + ); + /** @export */ ngrxOnInitEffects(): Action { return initAction(); @@ -193,25 +257,42 @@ export class MetricsEffects implements OnInitEffects { private fetchTimeSeriesForCards( fetchInfos: CardFetchInfo[], - experimentIds: string[] + experimentIds: string[], + multiRunTagsToEid: Record> ) { - /** - * 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; - }); + const requests = fetchInfos + .map((fetchInfo) => { + const {plugin, tag, runId, sample} = fetchInfo; + + if (isSingleRunPlugin(plugin)) { + if (!runId) { + return; + } + return { + plugin, + tag, + runId, + sample, + }; + } + + const filteredEids = experimentIds.filter((eid) => + multiRunTagsToEid[tag]?.has(eid) + ); + if (!filteredEids.length) { + return; + } + + 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(requests).pipe( + return of(uniqueRequests).pipe( tap((requests) => { this.store.dispatch(actions.multipleTimeSeriesRequested({requests})); }), @@ -254,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 + ); }) ); @@ -302,4 +388,5 @@ export class MetricsEffects implements OnInitEffects { export const TEST_ONLY = { getCardFetchInfo, initAction, + generateMultiRunTagsToEidMapping, }; diff --git a/tensorboard/webapp/metrics/effects/metrics_effects_test.ts b/tensorboard/webapp/metrics/effects/metrics_effects_test.ts index 2851c19a7d7..d31cebbfa00 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 { @@ -89,8 +90,52 @@ describe('metrics effects', () => { selectors.getMetricsTooltipSort, TooltipSort.ALPHABETICAL ); + + overrideTagMetadata(); + overrideRunToEid(); }); + function overrideTagMetadata() { + store.overrideSelector(selectors.getMetricsTagMetadata, { + scalars: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + tagB: ['run2', 'run3'], + tagC: ['run4', 'run5'], + tagD: ['run6'], + }, + }, + histograms: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + tagB: ['run4'], + }, + }, + images: { + tagDescriptions: {}, + tagRunSampledInfo: { + tagC: { + run1: {maxSamplesPerStep: 1}, + run3: {maxSamplesPerStep: 1}, + }, + }, + }, + }); + } + + function overrideRunToEid() { + store.overrideSelector(selectors.getRunIdToExperimentId, { + run1: 'exp1', + run2: 'exp1', + run3: 'exp2', + run4: 'defaultExperimentId', + run5: 'defaultExperimentId', + run6: 'defaultExperimentId', + }); + } + afterEach(() => { store?.resetSelectors(); }); @@ -365,15 +410,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 +424,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 +522,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 +547,73 @@ 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', 'card3', 'card4', 'card5', 'card6']) + ); + provideCardFetchInfo([ + {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 requests: TimeSeriesRequest[] = []; + spyOn(effects as any, 'fetchTimeSeries').and.callFake( + (request: TimeSeriesRequest) => { + requests.push(request); + } + ); + + actions$.next(coreActions.manualReload()); + + 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', + }, + ]); + }); }); describe('loadTimeSeriesForVisibleCardsWithoutData', () => { @@ -778,4 +882,133 @@ describe('metrics effects', () => { } }); }); + + describe('utilities', () => { + describe('generateMultiRunTagsToEidMapping', () => { + it('does not map 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.generateMultiRunTagsToEidMapping( + tagMetadata as any, + runToEid + ) + ).toEqual({}); + }); + + it('does not map histogram data', () => { + const runToEid = { + run1: 'eid1', + run2: 'eid1', + run3: 'eid2', + }; + const tagMetadata = { + histograms: { + tagDescriptions: {}, + tagToRuns: { + tagA: ['run1'], + tagB: ['run2', 'run3'], + }, + }, + }; + + expect( + TEST_ONLY.generateMultiRunTagsToEidMapping( + tagMetadata as any, + runToEid + ) + ).toEqual({}); + }); + + 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.generateMultiRunTagsToEidMapping( + tagMetadata as any, + runToEid + ) + ).toEqual({ + tagA: new Set(['eid1']), + 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']), + }); + }); + }); + }); });