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.

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

Conversation

rileyajones
Copy link
Contributor

Motivation for features / changes

Whenever a card appears on the time series dashboard we make a request to the fetch data for the card for each experiment being viewed. Because not all cards contain data from all experiments being viewed this sometimes results in unnecessary requests being dispatched.

@rileyajones rileyajones requested a review from bmd3k August 7, 2023 16:40
@rileyajones rileyajones marked this pull request as ready for review August 7, 2023 16:40
Copy link
Contributor

@bmd3k bmd3k left a comment

Choose a reason for hiding this comment

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

There is a lot about this PR that is confusing. I'm worried you've introduced a bunch of new subtle bugs.

This is a critical piece of code so please take a deep look at what you've written and rigourously test it (manually and with unit tests).

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.

@@ -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}
        }
      }
    },
  }
}

// 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.

}
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

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.

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.


expect(effectFetchTimeSeriesSpy).toHaveBeenCalledTimes(2);
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.

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

tagDescriptions: {} as any,
tagToRuns: {
tagA: ['run1'],
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.

});

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.

Copy link
Contributor Author

@rileyajones rileyajones left a comment

Choose a reason for hiding this comment

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

I'll add some additional tests to this but wanted to leave some preliminary comments.

combineLatestWith(this.store.select(selectors.getRunIdToExperimentId)),
map(([tagMetadata, runToEid]) => {
const imageTagToRuns = Object.fromEntries(
Object.entries(tagMetadata.images.tagRunSampledInfo).map(
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.

@@ -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 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}
        }
      }
    },
  }
}

// Fetch and handle responses.
return of(requests).pipe(
return this.tagToEid$.pipe(
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.

}
return partialRequest;
});
const uniqueRequests = new Set(
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

histograms: {
tagDescriptions: {} as any,
tagToRuns: {
tagA: ['run1'],
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.

tagDescriptions: {} as any,
tagToRuns: {
tagA: ['run1'],
tagB: ['run2', 'run3'],
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.

@rileyajones rileyajones requested a review from bmd3k August 21, 2023 17:36
runToEid: Record<string, string>
): Record<string, Set<string>> {
const tagToEid: Record<string, Set<string>> = {};
function mapTagsToEid(tagToRun: Record<string, readonly string[]>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think it would be clearer if you just inlined the contents of this function in the for loop (at L95). You would possibly even save some lines of code. You only use it once, after all.

}
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.

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?

*
* The computation is done by translating Plugin -> Tag -> Run -> ExpId
*
* Sampled plugins are ignored because they are associated with runs, not experiments.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sampled plugins are not the only things ignored here. Really it's any single-run plugins that are ignored. Would be good to fix that in the documentation.

I think its also worth explaining the real motivation for the change here - otherwise it is hard to understand why we would bother doing this for scalars and why we wouldn't do this for the others:

We want to eliminate unnecessary requests for experiment+tag combinations where the experiment does not actually contain the tag. In case of single-run plugins we assume that every given request for expeirment+run+tag is already valid, since they originate from cards for that experiment+run+tag combination.

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 updated this comment and added a detail description of the problem and how observable is being used to solve it.

// Fetch and handle responses.
return of(requests).pipe(
return this.multiRunTagsToEid$.pipe(
take(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use withLatestFrom instead? The caller of this function uses withLatestFrom so that might be a natural place to tie in this observable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that works.

plugin,
tag,
runId,
sample,
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that sample only exists for single run plugins. Is that guaranteed by the data model? - single run vs sampled are theoretically orthoganal considerations. (I realize in practice that there are no multi-run, sampled plugins but the old code handles this case fine so I assume that is intentional).

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 sample to the multi run plugin request. It shouldn't ever come up, but the typing does allow for it so it's close to free to include.

tagDescriptions: {} as any,
tagToRuns: {
tagA: ['run1'],
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.

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

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.

Ah ya, fair enough. That makes sense.

).toEqual({});
});

it('maps scalar data', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding one additional test that includes tagMetadata for all of scalars, histograms, and images?

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 think it's a little redundant but I've added one to be safe.

@bmd3k bmd3k self-requested a review September 20, 2023 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants