From 0517114dc2b365a4a6d95424af6157ead774eff3 Mon Sep 17 00:00:00 2001 From: Riley Bauer <34456002+rileyjbauer@users.noreply.github.com> Date: Thu, 15 Aug 2019 12:28:35 -0700 Subject: [PATCH] Reduce getPipeline calls in RunList (#1852) * Skips calling getPipeline in RunList if the pipeline name is in the pipeline_spec * Update fixed data to include pipeline names in pipeline specs * Remove redundant getRuns call --- frontend/mock-backend/fixed-data.ts | 22 ++++-- frontend/src/apis/job/api.ts | 12 +++ frontend/src/apis/run/api.ts | 12 +++ frontend/src/lib/RunUtils.ts | 10 ++- frontend/src/pages/ExperimentDetails.tsx | 10 +-- frontend/src/pages/NewRun.tsx | 4 +- frontend/src/pages/PipelineDetails.tsx | 2 +- frontend/src/pages/RunList.test.tsx | 19 +++-- frontend/src/pages/RunList.tsx | 32 +++++--- .../pages/__snapshots__/RunList.test.tsx.snap | 75 ++++++++++++++++++- 10 files changed, 163 insertions(+), 35 deletions(-) diff --git a/frontend/mock-backend/fixed-data.ts b/frontend/mock-backend/fixed-data.ts index 9a4f010f7b8..8aac362733d 100644 --- a/frontend/mock-backend/fixed-data.ts +++ b/frontend/mock-backend/fixed-data.ts @@ -136,6 +136,7 @@ const jobs: ApiJob[] = [ } ], pipeline_id: pipelines[0].id, + pipeline_name: pipelines[0].name, }, resource_references: [{ key: { @@ -177,6 +178,7 @@ const jobs: ApiJob[] = [ } ], pipeline_id: pipelines[1].id, + pipeline_name: pipelines[1].name, }, resource_references: [{ key: { @@ -221,6 +223,7 @@ const jobs: ApiJob[] = [ } ], pipeline_id: pipelines[2].id, + pipeline_name: pipelines[2].name, }, resource_references: [{ key: { @@ -290,7 +293,8 @@ const runs: ApiRunDetail[] = [ { name: 'paramName1', value: 'paramVal1' }, { name: 'paramName2', value: 'paramVal2' }, ], - pipeline_id: '8fbe3bd6-a01f-11e8-98d0-529269fb1459', + pipeline_id: pipelines[0].id, + pipeline_name: pipelines[0].name, }, resource_references: [{ key: { @@ -327,7 +331,8 @@ const runs: ApiRunDetail[] = [ { name: 'paramName1', value: 'paramVal1' }, { name: 'paramName2', value: 'paramVal2' }, ], - pipeline_id: '8fbe3bd6-a01f-11e8-98d0-529269fb1459', + pipeline_id: pipelines[0].id, + pipeline_name: pipelines[0].name, }, resource_references: [{ key: { @@ -360,7 +365,8 @@ const runs: ApiRunDetail[] = [ { name: 'paramName1', value: 'paramVal1' }, { name: 'paramName2', value: 'paramVal2' }, ], - pipeline_id: '8fbe41b2-a01f-11e8-98d0-529269fb1459', + pipeline_id: pipelines[2].id, + pipeline_name: pipelines[2].name, }, resource_references: [{ key: { @@ -393,7 +399,8 @@ const runs: ApiRunDetail[] = [ { name: 'paramName1', value: 'paramVal1' }, { name: 'paramName2', value: 'paramVal2' }, ], - pipeline_id: '8fbe42f2-a01f-11e8-98d0-529269fb1459', + pipeline_id: pipelines[3].id, + pipeline_name: pipelines[3].name, }, scheduled_at: new Date('2018-06-17T22:58:23.000Z'), status: 'Failed', @@ -439,7 +446,8 @@ const runs: ApiRunDetail[] = [ { name: 'paramName1', value: 'paramVal1' }, { name: 'paramName2', value: 'paramVal2' }, ], - pipeline_id: '8fbe3f78-a01f-11e8-98d0-529269fb1459', + pipeline_id: pipelines[1].id, + pipeline_name: pipelines[1].name, }, resource_references: [{ key: { @@ -488,7 +496,8 @@ const runs: ApiRunDetail[] = [ { name: 'paramName1', value: 'paramVal1' }, { name: 'paramName2', value: 'paramVal2' }, ], - pipeline_id: '8fbe3f78-a01f-11e8-98d0-529269fb1459', + pipeline_id: pipelines[1].id, + pipeline_name: pipelines[1].name, }, resource_references: [{ key: { @@ -603,6 +612,7 @@ function generateNRuns(): ApiRunDetail[] { { name: 'paramName2', value: 'paramVal2' }, ], pipeline_id: 'Some-pipeline-id-' + i, + pipeline_name: 'Kubeflow Pipeline number ' + i, }, resource_references: [{ key: { diff --git a/frontend/src/apis/job/api.ts b/frontend/src/apis/job/api.ts index 359cf6f889a..f278e2b7db5 100644 --- a/frontend/src/apis/job/api.ts +++ b/frontend/src/apis/job/api.ts @@ -274,6 +274,12 @@ export interface ApiPipelineSpec { * @memberof ApiPipelineSpec */ pipeline_id?: string; + /** + * Optional output field. The name of the pipeline. Not empty if the pipeline id is not empty. + * @type {string} + * @memberof ApiPipelineSpec + */ + pipeline_name?: string; /** * Optional input field. The marshalled raw argo JSON workflow. This will be deprecated when pipeline_manifest is in use. * @type {string} @@ -337,6 +343,12 @@ export interface ApiResourceReference { * @memberof ApiResourceReference */ key?: ApiResourceKey; + /** + * The name of the resource that referred to. + * @type {string} + * @memberof ApiResourceReference + */ + name?: string; /** * Required field. The relationship from referred resource to the object. * @type {ApiRelationship} diff --git a/frontend/src/apis/run/api.ts b/frontend/src/apis/run/api.ts index f43656460aa..e75829739e9 100644 --- a/frontend/src/apis/run/api.ts +++ b/frontend/src/apis/run/api.ts @@ -156,6 +156,12 @@ export interface ApiPipelineSpec { * @memberof ApiPipelineSpec */ pipeline_id?: string; + /** + * Optional output field. The name of the pipeline. Not empty if the pipeline id is not empty. + * @type {string} + * @memberof ApiPipelineSpec + */ + pipeline_name?: string; /** * Optional input field. The marshalled raw argo JSON workflow. This will be deprecated when pipeline_manifest is in use. * @type {string} @@ -267,6 +273,12 @@ export interface ApiResourceReference { * @memberof ApiResourceReference */ key?: ApiResourceKey; + /** + * The name of the resource that referred to. + * @type {string} + * @memberof ApiResourceReference + */ + name?: string; /** * Required field. The relationship from referred resource to the object. * @type {ApiRelationship} diff --git a/frontend/src/lib/RunUtils.ts b/frontend/src/lib/RunUtils.ts index b7c26ccef15..69548dbbe98 100644 --- a/frontend/src/lib/RunUtils.ts +++ b/frontend/src/lib/RunUtils.ts @@ -51,7 +51,11 @@ function getPipelineId(run?: ApiRun | ApiJob): string | null { return (run && run.pipeline_spec && run.pipeline_spec.pipeline_id) || null; } -function getPipelineSpec(run?: ApiRun | ApiJob): string | null { +function getPipelineName(run?: ApiRun | ApiJob): string | null { + return (run && run.pipeline_spec && run.pipeline_spec.pipeline_name) || null; +} + +function getWorkflowManifest(run?: ApiRun | ApiJob): string | null { return (run && run.pipeline_spec && run.pipeline_spec.workflow_manifest) || null; } @@ -123,6 +127,7 @@ function getRecurringRunId(run?: ApiRun): string { return ''; } +// TODO: This file needs tests export default { extractMetricMetadata, getAllExperimentReferences, @@ -131,7 +136,8 @@ export default { getParametersFromRun, getParametersFromRuntime, getPipelineId, - getPipelineSpec, + getPipelineName, getRecurringRunId, + getWorkflowManifest, runsToMetricMetadataMap, }; diff --git a/frontend/src/pages/ExperimentDetails.tsx b/frontend/src/pages/ExperimentDetails.tsx index defc7cc320a..3f0d87c12ea 100644 --- a/frontend/src/pages/ExperimentDetails.tsx +++ b/frontend/src/pages/ExperimentDetails.tsx @@ -222,7 +222,11 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> { } public async refresh(): Promise { - return this.load(); + await this.load(); + if (this._runlistRef.current) { + await this._runlistRef.current.refresh(); + } + return; } public async componentDidMount(): Promise { @@ -272,10 +276,6 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> { await this.showPageError(`Error: failed to retrieve experiment: ${experimentId}.`, err); logger.error(`Error loading experiment: ${experimentId}`, err); } - - if (this._runlistRef.current) { - this._runlistRef.current.refresh(); - } } private _selectionChanged(selectedIds: string[]): void { diff --git a/frontend/src/pages/NewRun.tsx b/frontend/src/pages/NewRun.tsx index 0bc7e8f50a0..ef5a1d5607b 100644 --- a/frontend/src/pages/NewRun.tsx +++ b/frontend/src/pages/NewRun.tsx @@ -489,7 +489,7 @@ class NewRun extends Page<{}, NewRunState> { try { runWithEmbeddedPipeline = await Apis.runServiceApi.getRun(embeddedPipelineRunId); - embeddedPipelineSpec = RunUtils.getPipelineSpec(runWithEmbeddedPipeline.run); + embeddedPipelineSpec = RunUtils.getWorkflowManifest(runWithEmbeddedPipeline.run); } catch (err) { await this.showPageError( `Error: failed to retrieve the specified run: ${embeddedPipelineRunId}.`, err); @@ -538,7 +538,7 @@ class NewRun extends Page<{}, NewRunState> { const referencePipelineId = RunUtils.getPipelineId(originalRun); // This corresponds to a run where the pipeline has not been uploaded, such as runs started from // the CLI or notebooks - const embeddedPipelineSpec = RunUtils.getPipelineSpec(originalRun); + const embeddedPipelineSpec = RunUtils.getWorkflowManifest(originalRun); if (referencePipelineId) { try { pipeline = await Apis.pipelineServiceApi.getPipeline(referencePipelineId); diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx index 15f8642a6ab..17cc78986b3 100644 --- a/frontend/src/pages/PipelineDetails.tsx +++ b/frontend/src/pages/PipelineDetails.tsx @@ -258,7 +258,7 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> { // Convert the run's pipeline spec to YAML to be displayed as the pipeline's source. try { - const pipelineSpec = JSON.parse(RunUtils.getPipelineSpec(runDetails.run) || '{}'); + const pipelineSpec = JSON.parse(RunUtils.getWorkflowManifest(runDetails.run) || '{}'); try { templateString = JsYaml.safeDump(pipelineSpec); } catch (err) { diff --git a/frontend/src/pages/RunList.test.tsx b/frontend/src/pages/RunList.test.tsx index 1bdc2bd9f82..0d7b5af9b87 100644 --- a/frontend/src/pages/RunList.test.tsx +++ b/frontend/src/pages/RunList.test.tsx @@ -310,7 +310,16 @@ describe('RunList', () => { }); it('shows pipeline name', async () => { - mockNRuns(1, { run: { pipeline_spec: { pipeline_id: 'test-pipeline-id' } } }); + mockNRuns(1, { run: { pipeline_spec: { pipeline_id: 'test-pipeline-id', pipeline_name: 'pipeline name' } } }); + const props = generateProps(); + tree = shallow(); + await (tree.instance() as RunListTest)._loadRuns({}); + expect(props.onError).not.toHaveBeenCalled(); + expect(tree).toMatchSnapshot(); + }); + + it('retrieves pipeline from backend to display name if not in spec', async () => { + mockNRuns(1, { run: { pipeline_spec: { pipeline_id: 'test-pipeline-id' /* no pipeline_name */ } } }); getPipelineSpy.mockImplementationOnce(() => ({ name: 'test pipeline' })); const props = generateProps(); tree = shallow(); @@ -372,19 +381,19 @@ describe('RunList', () => { }); it('renders pipeline name as link to its details page', () => { - expect(getMountedInstance()._pipelineCustomRenderer({ value: { displayName: 'test pipeline', id: 'pipeline-id', showLink: false }, id: 'run-id' })).toMatchSnapshot(); + expect(getMountedInstance()._pipelineCustomRenderer({ value: { displayName: 'test pipeline', id: 'pipeline-id', usePlaceholder: false }, id: 'run-id' })).toMatchSnapshot(); }); it('handles no pipeline id given', () => { - expect(getMountedInstance()._pipelineCustomRenderer({ value: { displayName: 'test pipeline', showLink: false }, id: 'run-id' })).toMatchSnapshot(); + expect(getMountedInstance()._pipelineCustomRenderer({ value: { displayName: 'test pipeline', usePlaceholder: false }, id: 'run-id' })).toMatchSnapshot(); }); it('shows "View pipeline" button if pipeline is embedded in run', () => { - expect(getMountedInstance()._pipelineCustomRenderer({ value: { displayName: 'test pipeline', id: 'pipeline-id', showLink: true }, id: 'run-id' })).toMatchSnapshot(); + expect(getMountedInstance()._pipelineCustomRenderer({ value: { displayName: 'test pipeline', id: 'pipeline-id', usePlaceholder: true }, id: 'run-id' })).toMatchSnapshot(); }); it('handles no pipeline name', () => { - expect(getMountedInstance()._pipelineCustomRenderer({ value: { /* no displayName */ showLink: true }, id: 'run-id' })).toMatchSnapshot(); + expect(getMountedInstance()._pipelineCustomRenderer({ value: { /* no displayName */ usePlaceholder: true }, id: 'run-id' })).toMatchSnapshot(); }); it('renders pipeline name as link to its details page', () => { diff --git a/frontend/src/pages/RunList.tsx b/frontend/src/pages/RunList.tsx index 711a02cb664..e033cb81560 100644 --- a/frontend/src/pages/RunList.tsx +++ b/frontend/src/pages/RunList.tsx @@ -38,7 +38,7 @@ interface PipelineInfo { displayName?: string; id?: string; runId?: string; - showLink: boolean; + usePlaceholder: boolean; } interface RecurringRunInfo { @@ -191,17 +191,17 @@ class RunList extends React.PureComponent { public _pipelineCustomRenderer: React.FC> = (props: CustomRendererProps) => { // If the getPipeline call failed or a run has no pipeline, we display a placeholder. - if (!props.value || (!props.value.showLink && !props.value.id)) { + if (!props.value || (!props.value.usePlaceholder && !props.value.id)) { return
-
; } const search = new URLParser(this.props).build({ [QUERY_PARAMS.fromRunId]: props.id }); - const url = props.value.showLink ? + const url = props.value.usePlaceholder ? RoutePage.PIPELINE_DETAILS.replace(':' + RouteParams.pipelineId + '?', '') + search : RoutePage.PIPELINE_DETAILS.replace(':' + RouteParams.pipelineId, props.value.id || ''); return ( e.stopPropagation()} to={url}> - {props.value.showLink ? '[View pipeline]' : props.value.displayName} + {props.value.usePlaceholder ? '[View pipeline]' : props.value.displayName} ); } @@ -357,15 +357,23 @@ class RunList extends React.PureComponent { private async _getAndSetPipelineNames(displayRun: DisplayRun): Promise { const pipelineId = RunUtils.getPipelineId(displayRun.run); if (pipelineId) { - try { - const pipeline = await Apis.pipelineServiceApi.getPipeline(pipelineId); - displayRun.pipeline = { displayName: pipeline.name || '', id: pipelineId, showLink: false }; - } catch (err) { - // This could be an API exception, or a JSON parse exception. - displayRun.error = 'Failed to get associated pipeline: ' + await errorToMessage(err); + let pipelineName = RunUtils.getPipelineName(displayRun.run); + if (!pipelineName) { + try { + const pipeline = await Apis.pipelineServiceApi.getPipeline(pipelineId); + pipelineName = pipeline.name || ''; + } catch (err) { + displayRun.error = 'Failed to get associated pipeline: ' + await errorToMessage(err); + return; + } } - } else if (!!RunUtils.getPipelineSpec(displayRun.run)) { - displayRun.pipeline = { showLink: true }; + displayRun.pipeline = { + displayName: pipelineName, + id: pipelineId, + usePlaceholder: false + }; + } else if (!!RunUtils.getWorkflowManifest(displayRun.run)) { + displayRun.pipeline = { usePlaceholder: true }; } } diff --git a/frontend/src/pages/__snapshots__/RunList.test.tsx.snap b/frontend/src/pages/__snapshots__/RunList.test.tsx.snap index 074546ab29a..e80c1cb38ab 100644 --- a/frontend/src/pages/__snapshots__/RunList.test.tsx.snap +++ b/frontend/src/pages/__snapshots__/RunList.test.tsx.snap @@ -7745,6 +7745,77 @@ exports[`RunList renders the empty experience 1`] = ` `; +exports[`RunList retrieves pipeline from backend to display name if not in spec 1`] = ` +
+ +
+`; + exports[`RunList shows "View pipeline" button if pipeline is embedded in run 1`] = `