Reduce getPipeline calls in RunList (kubeflow#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

@@ -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: {

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}

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}

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

frontend/src/pages/ExperimentDetails.tsx

@@ -222,7 +222,11 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> {
   }
 
   public async refresh(): Promise<void> {
-    return this.load();
+    await this.load();
+    if (this._runlistRef.current) {
+      await this._runlistRef.current.refresh();
+    }
+    return;
   }
 
   public async componentDidMount(): Promise<void> {
@@ -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 {

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);

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) {

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(<RunList {...props} />);
+    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(<RunList {...props} />);
@@ -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', () => {

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<RunListProps, RunListState> {
 
   public _pipelineCustomRenderer: React.FC<CustomRendererProps<PipelineInfo>> = (props: CustomRendererProps<PipelineInfo>) => {
     // 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 <div>-</div>;
     }
     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 (
       <Link className={commonCss.link} onClick={(e) => e.stopPropagation()}
         to={url}>
-        {props.value.showLink ? '[View pipeline]' : props.value.displayName}
+        {props.value.usePlaceholder ? '[View pipeline]' : props.value.displayName}
       </Link>
     );
   }
@@ -357,15 +357,23 @@ class RunList extends React.PureComponent<RunListProps, RunListState> {
   private async _getAndSetPipelineNames(displayRun: DisplayRun): Promise<void> {
     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 };
     }
   }