Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug Fix: Stop making duplicate time series requests #6529

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Bug Fix: Stop making duplicate time series requests #6529

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cc1bbaf
stop making duplicate time series requests
rileyajones Aug 4, 2023
f79b718
Merge branch 'master' into smart-timeseries-fetch
rileyajones Aug 17, 2023
94b102a
use plugintype rather than referencing plugins directly
rileyajones Aug 17, 2023
5814618
refactor tagToEid + use plugin type
rileyajones Aug 17, 2023
20bf2f0
minor test updates
rileyajones Aug 17, 2023
4d264a5
finished writing tests, discovered a bug - nonsampled data is fetched…
rileyajones Aug 18, 2023
3eea398
fix the issue with sampled plugins. This is still subtly wrong - hist…
rileyajones Aug 18, 2023
0d741af
only remap multi run plugins
rileyajones Aug 18, 2023
02ee0fd
inline helper function
rileyajones Aug 24, 2023
bcd88ae
add a detailed comment and change to using withLatestFrom
rileyajones Aug 24, 2023
11e7729
add additional test
rileyajones Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 66 additions & 16 deletions tensorboard/webapp/metrics/effects/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {forkJoin, merge, Observable, of} from 'rxjs';
import {
catchError,
throttleTime,
combineLatestWith,
filter,
map,
mergeMap,
Expand Down Expand Up @@ -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('/');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you are trying to parse here. A comment would be helpful.

Is it this part highlighted in red:
image

Is this the same format as the run names for tagMetadata.scalars.tagToRuns and tagMetadata.histograms.tagToRuns? If so, why handle it differently for this case?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found a way to avoid doing this parsing but the structure still needs to be different.
I'll add a block comment explaining this.

The structure of SampledTagMetadata is quite different from non sampled

The NonSampledPlugins map from run to tag while the SampledPlugin(s) map from tag to run

Sampled

image

Non Sampled

image

Rough Sketch

Here is a rough sketch with only the relevant parts

{
  tagMetadata: {
    scalars: {
      runTagInfo: {
        runId: ['tag1', 'tag2',]
      },
    },
    images: {
      tagRunSampledInfo: {
        tag: {
          runId: {maxSamplesPerStep: number}
        }
      }
    },
  }
}

return runIdChunks.join('/');
}

@Injectable()
export class MetricsEffects implements OnInitEffects {
constructor(
Expand All @@ -76,6 +83,40 @@ export class MetricsEffects implements OnInitEffects {
private readonly dataSource: MetricsDataSource
) {}

readonly tagToEid$: Observable<Record<string, Set<string>>> = 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we handle plugins and sampled vs non-sampled more generically?

Ideally the code is unaware of the set of plugin types (it doesn't know about images, scalars, or histograms). Ideally the code is unaware of which plugin types are sampled and which are not.

There is isSampledPlugin function to help with this, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I thought the additional loop that approach required harmed readability a bit but I've gone ahead and refactored to use it.

([tag, sampledRunInfo]) => {
const runIds = Object.keys(sampledRunInfo).map((runInfoKey) =>
parseRunIdFromSampledRunInfoName(runInfoKey)
);
return [tag, runIds];
}
)
);

const tagToEid: Record<string, Set<string>> = {};
function mapTagsToEid(tagToRun: Record<string, readonly string[]>) {
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();
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than piping this.tagToEid$ can we just get the latest value? I'm a little worried about subtle bugs when tagToEid$ changes for whatever unpredicatable reason and the pipe kicks off a new set of requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a take(1) I could use a subject instead or maybe a subscription? Let me know if you'd prefer something else.

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an actual problem you are trying to solve here? I don't see a test for this and I didn't see anything about it in the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am attempting to address the TODO left by psybuzz

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "if 2 cards require the same data" happen in practice? If it does, is it a source of problems? If not, especially given that this code is critical, do we need to be making unnecessary changes? Also, it's not clear to me that you wrote a test for this particular change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an existing test which verified this was doing the wrong thing which I updated. See the comment that I removed from line 375 of metrics_effects_test

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}));
}),
Expand Down Expand Up @@ -302,4 +351,5 @@ export class MetricsEffects implements OnInitEffects {
export const TEST_ONLY = {
getCardFetchInfo,
initAction,
parseRunIdFromSampledRunInfoName,
};
115 changes: 104 additions & 11 deletions tensorboard/webapp/metrics/effects/metrics_effects_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
bmd3k marked this conversation as resolved.
Show resolved Hide resolved
tagB: ['run2', 'run3'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no references to 'run2' thru 'run6' or 'tagC' and 'tagD' or 'defaultExperimentId' anywhere else in this test as far as I can tell.

Maybe just mock the minimum amount of data you need for existing tests to pass and override at a more detailed level only for your new tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some additional data to the state to ensure it did not result in additional requests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some comments acknowledging which data is unnecessary and why you include it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After your last comment I added some additional tests and ensured that all of the data is being used. In particular the test does not send requests to experiments lacking a cards tag references every tag.

tagC: ['run4', 'run5'],
tagD: ['run6'],
},
},
histograms: {
tagDescriptions: {} as any,
tagToRuns: {
tagA: ['run1'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it valid for there to be duplicate tags across scalars/histograms? Doesn't seem to make sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything that prohibits this? Tags can appear in multiple experiments and they could have different data being logged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ya, fair enough. That makes sense.

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();
});
Expand Down Expand Up @@ -365,35 +409,25 @@ 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: [
{
plugin: PluginType.SCALARS as MultiRunPluginType,
tag: 'tagA',
experimentIds: ['exp1'],
},
{
plugin: PluginType.SCALARS as MultiRunPluginType,
tag: 'tagA',
experimentIds: ['exp1'],
},
],
}),
actions.fetchTimeSeriesLoaded({
response: buildTimeSeriesResponse(),
}),
actions.fetchTimeSeriesLoaded({
response: buildTimeSeriesResponse(),
}),
]);
});

Expand Down Expand Up @@ -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,
Expand All @@ -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({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's extremely hard to reason why the test concludes that these should be the requests sent. Some of the key test data (like in overrideTagMetadata and overrideRunToEid) are setup far from here. Maybe it would be helpful to leave a comment about all the requests that could have been made and identify why certain requests were filtered out.

plugin: 'scalars',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also be fetching plugin: 'histograms', tag: 'tagA', experimentIds: ['exp1']?

tag: 'tagA',
experimentIds: ['exp1'],
});

expect(effectFetchTimeSeriesSpy).toHaveBeenCalledWith({
plugin: 'scalars',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the complexity of the logic you've written it would be good to see more rigourous testing. A single test case doesn't really seem to cut it.

A couple that pop up in my head:

  • A test case where some image requests make it through the new filter.
  • A test case where some histogram requests make it through the new filter.

tag: 'tagB',
experimentIds: ['exp1', 'exp2'],
});
});
});

describe('loadTimeSeriesForVisibleCardsWithoutData', () => {
Expand Down Expand Up @@ -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('');
});
});
});
});