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

fix(frontend): reduce list run latency #10797

Merged
merged 2 commits into from
May 27, 2024
Merged

fix(frontend): reduce list run latency #10797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4c69e9
fix(frontend): reduce list run latency
droctothorpe May 7, 2024
cffecf8
Handle multi-user deployments
droctothorpe May 16, 2024
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
17 changes: 12 additions & 5 deletions frontend/src/pages/RecurringRunList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('RecurringRunList', () => {
const onErrorSpy = jest.fn();
const listRecurringRunsSpy = jest.spyOn(Apis.recurringRunServiceApi, 'listRecurringRuns');
const getRecurringRunSpy = jest.spyOn(Apis.recurringRunServiceApi, 'getRecurringRun');
const getExperimentSpy = jest.spyOn(Apis.experimentServiceApiV2, 'getExperiment');
const listExperimentsSpy = jest.spyOn(Apis.experimentServiceApiV2, 'listExperiments');
// We mock this because it uses toLocaleDateString, which causes mismatches between local and CI
// test environments
const formatDateStringSpy = jest.spyOn(Utils, 'formatDateString');
Expand Down Expand Up @@ -78,7 +78,7 @@ describe('RecurringRunList', () => {
}),
);

getExperimentSpy.mockImplementation(() => ({ display_name: 'some experiment' }));
listExperimentsSpy.mockImplementation(() => ({ display_name: 'some experiment' }));
}

function getMountedInstance(): RecurringRunList {
Expand All @@ -93,7 +93,7 @@ describe('RecurringRunList', () => {
onErrorSpy.mockClear();
listRecurringRunsSpy.mockClear();
getRecurringRunSpy.mockClear();
getExperimentSpy.mockClear();
listExperimentsSpy.mockClear();
});

afterEach(async () => {
Expand Down Expand Up @@ -613,7 +613,14 @@ describe('RecurringRunList', () => {
mockNRecurringRuns(1, {
experiment_id: 'test-experiment-id',
});
getExperimentSpy.mockImplementationOnce(() => ({ display_name: 'test experiment' }));
listExperimentsSpy.mockImplementationOnce(() => ({
experiments: [
{
experiment_id: 'test-experiment-id',
display_name: 'test experiment',
},
],
}));
const props = generateProps();
tree = shallow(<RecurringRunList {...props} />);
await (tree.instance() as RecurringRunListTest)._loadRecurringRuns({});
Expand Down Expand Up @@ -682,7 +689,7 @@ describe('RecurringRunList', () => {
mockNRecurringRuns(1, {
experiment_id: 'test-experiment-id',
});
getExperimentSpy.mockImplementationOnce(() => ({ display_name: 'test experiment' }));
listExperimentsSpy.mockImplementationOnce(() => ({ display_name: 'test experiment' }));
const props = generateProps();
props.hideExperimentColumn = true;
tree = shallow(<RecurringRunList {...props} />);
Expand Down
67 changes: 40 additions & 27 deletions frontend/src/pages/RecurringRunList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
V2beta1RecurringRunStatus,
V2beta1Trigger,
} from 'src/apisv2beta1/recurringrun';
import { V2beta1ListExperimentsResponse } from 'src/apisv2beta1/experiment';

interface DisplayRecurringRun {
experiment?: ExperimentInfo;
Expand Down Expand Up @@ -269,10 +270,47 @@ class RecurringRunList extends React.PureComponent<RecurringRunListProps, Recurr
private async _setColumns(
displayRecurringRuns: DisplayRecurringRun[],
): Promise<DisplayRecurringRun[]> {
let experimentsResponse: V2beta1ListExperimentsResponse;
let experimentsGetError: string;
try {
if (!this.props.namespaceMask) {
// Single-user mode.
experimentsResponse = await Apis.experimentServiceApiV2.listExperiments();
} else {
// Multi-user mode.
experimentsResponse = await Apis.experimentServiceApiV2.listExperiments(
undefined,
undefined,
undefined,
undefined,
this.props.namespaceMask,
);
}
} catch (error) {
experimentsGetError = 'Failed to get associated experiment: ' + (await errorToMessage(error));
}

return Promise.all(
displayRecurringRuns.map(async displayRecurringRun => {
displayRecurringRuns.map(displayRecurringRun => {
if (!this.props.hideExperimentColumn) {
await this._getAndSetExperimentNames(displayRecurringRun);
const experimentId = displayRecurringRun.recurringRun.experiment_id;

if (experimentId) {
const experiment = experimentsResponse?.experiments?.find(
e => e.experiment_id === displayRecurringRun.recurringRun.experiment_id,
);
// If matching experiment id not found (typically because it has
// been deleted), set display name to "-".
const displayName = experiment?.display_name || '-';
if (experimentsGetError) {
displayRecurringRun.error = experimentsGetError;
} else {
displayRecurringRun.experiment = {
displayName: displayName,
id: experimentId,
};
}
}
}
return displayRecurringRun;
}),
Expand Down Expand Up @@ -301,31 +339,6 @@ class RecurringRunList extends React.PureComponent<RecurringRunListProps, Recurr
}),
);
}

/**
* For the given DisplayRecurringRun, get its recurring run and retrieve the Experiment ID
* if it has one, then use that Experiment ID to fetch its associated Experiment and attach
* that Experiment's name to the DisplayRecurringRun. If the recurring run has no Experiment ID,
* then the corresponding DisplayRecurringRun will show '-'.
*/
private async _getAndSetExperimentNames(displayRecurringRun: DisplayRecurringRun): Promise<void> {
const experimentId = displayRecurringRun.recurringRun.experiment_id;
if (experimentId) {
let experimentName;
try {
const experiment = await Apis.experimentServiceApiV2.getExperiment(experimentId);
experimentName = experiment.display_name || '';
} catch (err) {
displayRecurringRun.error =
'Failed to get associated experiment: ' + (await errorToMessage(err));
return;
}
displayRecurringRun.experiment = {
displayName: experimentName,
id: experimentId,
};
}
}
}

export default RecurringRunList;
25 changes: 16 additions & 9 deletions frontend/src/pages/RunList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('RunList', () => {
const listRunsSpy = jest.spyOn(Apis.runServiceApiV2, 'listRuns');
const getRunSpy = jest.spyOn(Apis.runServiceApiV2, 'getRun');
const getPipelineVersionSpy = jest.spyOn(Apis.pipelineServiceApiV2, 'getPipelineVersion');
const getExperimentSpy = jest.spyOn(Apis.experimentServiceApiV2, 'getExperiment');
const listExperimentsSpy = jest.spyOn(Apis.experimentServiceApiV2, 'listExperiments');
// We mock this because it uses toLocaleDateString, which causes mismatches between local and CI
// test enviroments
const formatDateStringSpy = jest.spyOn(Utils, 'formatDateString');
Expand Down Expand Up @@ -97,7 +97,7 @@ describe('RunList', () => {
);

getPipelineVersionSpy.mockImplementation(() => ({ display_name: 'some pipeline version' }));
getExperimentSpy.mockImplementation(() => ({ display_name: 'some experiment' }));
listExperimentsSpy.mockImplementation(() => ({ display_name: 'some experiment' }));
}

function getMountedInstance(): RunList {
Expand All @@ -117,7 +117,7 @@ describe('RunList', () => {
onErrorSpy.mockClear();
listRunsSpy.mockClear();
getRunSpy.mockClear();
getExperimentSpy.mockClear();
listExperimentsSpy.mockClear();
});

afterEach(async () => {
Expand Down Expand Up @@ -302,7 +302,7 @@ describe('RunList', () => {
mockNRuns(1, {
experiment_id: 'test-experiment-id',
});
TestUtils.makeErrorResponseOnce(getExperimentSpy, 'bad stuff happened');
TestUtils.makeErrorResponseOnce(listExperimentsSpy, 'bad stuff happened');
const props = generateProps();

render(
Expand All @@ -312,7 +312,7 @@ describe('RunList', () => {
);
await waitFor(() => {
expect(listRunsSpy).toHaveBeenCalled();
expect(getExperimentSpy).toHaveBeenCalled();
expect(listExperimentsSpy).toHaveBeenCalled();
});

screen.findByText('Failed to get associated experiment: bad stuff happened');
Expand Down Expand Up @@ -488,7 +488,14 @@ describe('RunList', () => {
mockNRuns(1, {
experiment_id: 'test-experiment-id',
});
getExperimentSpy.mockImplementationOnce(() => ({ display_name: 'test experiment' }));
listExperimentsSpy.mockImplementationOnce(() => ({
experiments: [
{
experiment_id: 'test-experiment-id',
display_name: 'test experiment',
},
],
}));
const props = generateProps();
render(
<CommonTestWrapper>
Expand All @@ -497,7 +504,7 @@ describe('RunList', () => {
);
await waitFor(() => {
expect(listRunsSpy).toHaveBeenCalled();
expect(getExperimentSpy).toHaveBeenCalled();
expect(listExperimentsSpy).toHaveBeenCalled();
});

screen.getByText('test experiment');
Expand All @@ -507,7 +514,7 @@ describe('RunList', () => {
mockNRuns(1, {
experiment_id: 'test-experiment-id',
});
getExperimentSpy.mockImplementationOnce(() => ({ display_name: 'test experiment' }));
listExperimentsSpy.mockImplementationOnce(() => ({ display_name: 'test experiment' }));
const props = generateProps();
props.hideExperimentColumn = true;
render(
Expand All @@ -517,7 +524,7 @@ describe('RunList', () => {
);
await waitFor(() => {
expect(listRunsSpy).toHaveBeenCalled();
expect(getExperimentSpy).toHaveBeenCalledTimes(0);
expect(listExperimentsSpy).toHaveBeenCalledTimes(0);
});

expect(screen.queryByText('test experiment')).toBeNull();
Expand Down
63 changes: 38 additions & 25 deletions frontend/src/pages/RunList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import CustomTable, { Column, Row, CustomRendererProps } from 'src/components/Cu
import Metric from 'src/components/Metric';
import { MetricMetadata, ExperimentInfo } from 'src/lib/RunUtils';
import { V2beta1Run, V2beta1RuntimeState, V2beta1RunStorageState } from 'src/apisv2beta1/run';
import { V2beta1ListExperimentsResponse } from 'src/apisv2beta1/experiment';
import { Apis, RunSortKeys, ListRequest } from 'src/lib/Apis';
import { Link, RouteComponentProps } from 'react-router-dom';
import { V2beta1Filter, V2beta1PredicateOperation } from 'src/apisv2beta1/filter';
Expand Down Expand Up @@ -405,14 +406,50 @@ class RunList extends React.PureComponent<RunListProps, RunListState> {
}

private async _setColumns(displayRuns: DisplayRun[]): Promise<DisplayRun[]> {
let experimentsResponse: V2beta1ListExperimentsResponse;
let experimentsGetError: string;
try {
if (!this.props.namespaceMask) {
// Single-user mode.
experimentsResponse = await Apis.experimentServiceApiV2.listExperiments();
} else {
// Multi-user mode.
experimentsResponse = await Apis.experimentServiceApiV2.listExperiments(
undefined,
undefined,
undefined,
undefined,
this.props.namespaceMask,
);
}
} catch (error) {
experimentsGetError = 'Failed to get associated experiment: ' + (await errorToMessage(error));
}

return Promise.all(
displayRuns.map(async displayRun => {
this._setRecurringRun(displayRun);

await this._getAndSetPipelineVersionNames(displayRun);

if (!this.props.hideExperimentColumn) {
await this._getAndSetExperimentNames(displayRun);
const experimentId = displayRun.run.experiment_id;

if (experimentId) {
const experiment = experimentsResponse?.experiments?.find(
e => e.experiment_id === displayRun.run.experiment_id,
);
// If matching experiment id not found (typically because it has been deleted), set display name to "-".
const displayName = experiment?.display_name || '-';
if (experimentsGetError) {
displayRun.error = experimentsGetError;
} else {
displayRun.experiment = {
displayName: displayName,
id: experimentId,
};
}
}
}
return displayRun;
}),
Expand Down Expand Up @@ -478,30 +515,6 @@ class RunList extends React.PureComponent<RunListProps, RunListState> {
: { usePlaceholder: true };
}
}

/**
* For the given DisplayRun, get its ApiRun and retrieve that ApiRun's Experiment ID if it has
* one, then use that Experiment ID to fetch its associated Experiment and attach that
* Experiment's name to the DisplayRun. If the ApiRun has no Experiment ID, then the corresponding
* DisplayRun will show '-'.
*/
private async _getAndSetExperimentNames(displayRun: DisplayRun): Promise<void> {
const experimentId = displayRun.run.experiment_id;
if (experimentId) {
let experimentName;
try {
const experiment = await Apis.experimentServiceApiV2.getExperiment(experimentId);
experimentName = experiment.display_name || '';
} catch (err) {
displayRun.error = 'Failed to get associated experiment: ' + (await errorToMessage(err));
return;
}
displayRun.experiment = {
displayName: experimentName,
id: experimentId,
};
}
}
}

export default RunList;