Skip to content

Commit

Permalink
fix(frontend): reduce list run latency (#10797)
Browse files Browse the repository at this point in the history 
* fix(frontend): reduce list run latency

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: quinnovator <jack@jq.codes>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
Co-authored-by: owmasch <owenmaschal0598@gmail.com>

* Handle multi-user deployments

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>

---------

Signed-off-by: droctothorpe <mythicalsunlight@gmail.com>
Co-authored-by: quinnovator <jack@jq.codes>
Co-authored-by: tarat44 <32471142+tarat44@users.noreply.github.com>
Co-authored-by: owmasch <owenmaschal0598@gmail.com>
  • Loading branch information
@droctothorpe @quinnovator
@tarat44 @owmasch
4 people committed May 27, 2024
1 parent 01ee4af commit 768ece4
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 66 deletions.
17 changes: 12 additions & 5 deletions frontend/src/pages/RecurringRunList.test.tsx
View file
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
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
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
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;

0 comments on commit 768ece4

Please sign in to comment.