[MLMD][Lineage] Integrate Lineage View into KFP #2918

kwasi · 2020-01-27T23:50:37Z

/assign @kwasi
/cc @avdaredevil
/area front-end

Changes

Adds LineageView from kubeflow/frontend to ArtifactDetails
Changes ArtifactDetails and ExecutionDetails to use the common MLMD gRPC client and utils from kubeflow/frontend
Fixes [MLMD] Integrate LineageView into KFP #2907

Validation

Deployment: https://2de286866441b3ac-dot-us-west1.notebooks.googleusercontent.com/

This change is

* Update licenses to make gen_licenses script pass in Docker build * Revert docker build dev change

…age-in-kfp-wip-merge-latest

k8s-ci-robot · 2020-01-27T23:50:40Z

@kwasi: The label(s) area/metadata cannot be applied, because the repository doesn't have them

In response to this:

/assign @kwasi
/cc @avdaredevil
/area front-end
/area metadata

Changes

Adds LineageView from kubeflow/frontend to ArtifactDetails

Changes ArtifactDetails and ExecutionDetails to use the common MLMD gRPC client and utils from kubeflow/frontend

Fixes [MLMD] Integrate LineageView into KFP #2907

Validation

Deployment: https://2de286866441b3ac-dot-us-west1.notebooks.googleusercontent.com/

Hold

/hold

Need to investigate pipeline_versions issue

Waiting for dependent changes to be merged in to kubeflow/frontend

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-01-27T23:50:58Z

@kwasi: The label(s) area/metadata cannot be applied, because the repository doesn't have them

In response to this:

/assign @kwasi
/cc @avdaredevil
/area front-end
/area metadata

Changes

Adds LineageView from kubeflow/frontend to ArtifactDetails

Changes ArtifactDetails and ExecutionDetails to use the common MLMD gRPC client and utils from kubeflow/frontend

Fixes [MLMD] Integrate LineageView into KFP #2907

Validation

Deployment: https://2de286866441b3ac-dot-us-west1.notebooks.googleusercontent.com/

Hold

/hold

Need to investigate pipeline_versions issue

Waiting for dependent changes to be merged in to kubeflow/frontend

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

avdaredevil

Looks good, few comments.

avdaredevil · 2020-01-28T01:10:10Z

frontend/src/components/ResourceInfo.tsx

-            .filter(k => k[0] !== '__ALL_META__')
-            .map(k => (
+          {propertyMap.getEntryList()
+          // TODO: __ALL_META__ is something of a hack, is redundant, and can be ignored


Suggested change

// TODO: __ALL_META__ is something of a hack, is redundant, and can be ignored

// TODO: __ALL_META__ is something of a hack, is redundant, and can be ignored

avdaredevil · 2020-01-28T01:10:42Z

frontend/src/components/ResourceInfo.tsx

+          {propertyMap.getEntryList()
+          // TODO: __ALL_META__ is something of a hack, is redundant, and can be ignored
+            .filter((k:any) => k[0] !== '__ALL_META__')
+            // @ts-ignore


Something more specific perhaps?

avdaredevil · 2020-01-28T01:11:06Z

frontend/src/lib/Utils.tsx

 import { CustomTableRow, css } from '../components/CustomTableRow';
-import { ServiceError } from '../generated/src/apis/metadata/metadata_store_service_pb_service';
+// import { ServiceError } from '../generated/src/apis/metadata/metadata_store_service_pb_service';


Did you want to remove this line?

frontend/src/lib/Utils.tsx

avdaredevil · 2020-01-28T01:11:28Z

frontend/src/lib/Utils.tsx

-}
+// export function serviceErrorToString(error: ServiceError): string {
+//   return `Error: ${error.message}. Code: ${error.code}`;
+// }


frontend/src/pages/ArtifactDetails.tsx

avdaredevil · 2020-01-28T01:15:17Z

frontend/src/pages/ExecutionDetails.tsx

@@ -260,6 +264,7 @@ class SectionIO extends Component<
      }
      const data: ArtifactInfo = {
        id,
+        // @ts-ignore


More specific?

frontend/src/pages/ExecutionList.tsx

This PR has no changes to the Status page, but the snapshot fails locally for me unless I update the snapshot. Reverting to the initial state to try to fix build failures.

kwasi · 2020-01-28T07:44:44Z

I updated the KFP cluster to the latest version of the API server image as recommended by @jingzhang36 and confirmed that Pipeline creation is fixed on the server. The latest version of this code is pushed to the shared cluster.

k8s-ci-robot · 2020-01-28T07:45:43Z

@kwasi: The label(s) area/metadata cannot be applied, because the repository doesn't have them

In response to this:

/assign @kwasi
/cc @avdaredevil
/area front-end
/area metadata

Changes

Adds LineageView from kubeflow/frontend to ArtifactDetails

Changes ArtifactDetails and ExecutionDetails to use the common MLMD gRPC client and utils from kubeflow/frontend

Fixes [MLMD] Integrate LineageView into KFP #2907

Validation

Deployment: https://2de286866441b3ac-dot-us-west1.notebooks.googleusercontent.com/

Hold

/hold

Waiting for dependent changes to be merged in to kubeflow/frontend

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kwasi

@Bobgy Thanks for the detailed review! The fixes to the error handling and ArtifactList loading will make a big difference.

Bobgy

@kwasi Thank you! The changes look awesome.

Some last minor comments, you can ask someone else from the team to lgtm when you feel ready with this.
/approve

frontend/src/pages/ArtifactDetails.tsx

frontend/src/pages/ArtifactList.tsx

frontend/src/pages/ExecutionDetails.tsx

frontend/src/pages/ExecutionList.tsx

k8s-ci-robot · 2020-01-30T08:04:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avdaredevil, Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kwasi · 2020-01-30T19:39:20Z

/test kubeflow-pipeline-sample-test

kwasi · 2020-01-30T19:40:00Z

PR is ready for final approvals and merging. Built a new image and deployed to https://2de286866441b3ac-dot-us-west1.notebooks.googleusercontent.com/

avdaredevil · 2020-01-30T21:22:37Z

/lgtm

avdaredevil · 2020-01-30T21:22:58Z

/hold cancel

kwasi · 2020-01-30T21:27:33Z

/test kubeflow-pipeline-sample-test

kwasi · 2020-01-30T21:27:57Z

/retest

…ineage-in-kfp-wip-merge-latest

kwasi · 2020-01-30T22:32:47Z

/retest

kwasi · 2020-01-30T23:35:42Z

/retest

avdaredevil · 2020-01-30T23:55:16Z

/lgtm

kwasi · 2020-01-30T23:55:22Z

Tests are now getting past CreateContainer before they fail so I'll give it some time then retest again.

kwasi · 2020-01-31T00:11:25Z

/retest

Bobgy · 2020-01-31T00:46:14Z

/retest

Bobgy · 2020-01-31T01:22:21Z

@kwasi I'm not sure how much efforts it would be -- can we move mlmd client related imports to a subpath of '@kubeflow/frontend'. Maybe '@kubeflow/frontend/metadata/client'.

there may be name collisions with other imports from '@kubeflow/frontend'
it's easier to find all client related imports

We can do this in a separate PR if you think the proposal is reasonable.

kwasi · 2020-01-31T01:36:44Z

@Bobgy We can definitely do that. I made an issue to track it with a link to your comment.

@ts-ignore

* Minimal change to verify that the new image will work on Marketplace * Very rough working copy of LineageView in KFP * Update kubeflow-frontend dependency * Fix lint errors * Update to latest version of kubeflow-frontend * Update licenses to make gen_licenses script pass in Docker build * Revert docker build dev change * Bump kubeflow/frontend hash to latest version with pre-built library * Remove debug string * Replace metadata APIs with versions from kubeflow/frontend * Use latest dev version of kubeflow/frontend package * Pass route builder to ArtifactDetails for building details pages routes * Review ResourcesInfo.tsx * Review changes part 2 * Revert change to Status snapshot This PR has no changes to the Status page, but the snapshot fails locally for me unless I update the snapshot. Reverting to the initial state to try to fix build failures. * Revert Status snapshot to master * Remove unneeded @ts-ignore * Bump to latest mainline kubeflow/frontend hash * Respond to comments PR comments * Scope kubeflow/frontend to @kubeflow/frontend in package.json * Use explicit tab name ordering * Make ArtifactDetails class functions members to avoid binding * Add missing await when loading ArtifactList Fixes issue where loading spinner hides immediately before artifact list loads. * Fix casing on global constants in ArtifactDetails * Improve readability on ExecutionList.tsx * Remove `()!` pattern caused by not having @types/google-protobuf * Remove a stray @ts-ignore * Remove side effects from getRowsFrom{Artifacts,Executions} * Add error handling back to Artifact and Execution pages * Fix error message handling on getArtifacts() * Fix ArtifactList response * Check for potentially undefined `code` in serviceErrorToString * Restore missing error handlers

kwasi added 11 commits January 8, 2020 11:56

Minimal change to verify that the new image will work on Marketplace

afadec7

Very rough working copy of LineageView in KFP

84b26ec

Update kubeflow-frontend dependency

439fb71

Fix lint errors

0a1b712

Update to latest version of kubeflow-frontend

8380bd6

* Update licenses to make gen_licenses script pass in Docker build * Revert docker build dev change

Bump kubeflow/frontend hash to latest version with pre-built library

11b98df

Remove debug string

040d231

Merge branch 'master' of https://github.com/kwasi/pipelines into line…

ab09edd

…age-in-kfp-wip-merge-latest

Replace metadata APIs with versions from kubeflow/frontend

fb9a42f

Use latest dev version of kubeflow/frontend package

64c5f5d

Pass route builder to ArtifactDetails for building details pages routes

dc57612

k8s-ci-robot assigned kwasi Jan 27, 2020

k8s-ci-robot added do-not-merge/work-in-progress do-not-merge/hold labels Jan 27, 2020

k8s-ci-robot added the area/front-end label Jan 27, 2020

k8s-ci-robot requested a review from avdaredevil January 27, 2020 23:50

k8s-ci-robot added the size/XXL label Jan 27, 2020

avdaredevil reviewed Jan 28, 2020

View reviewed changes

kwasi added 5 commits January 27, 2020 21:55

Review ResourcesInfo.tsx

05ffa28

Review changes part 2

9fbafb3

Revert change to Status snapshot

db31466

This PR has no changes to the Status page, but the snapshot fails locally for me unless I update the snapshot. Reverting to the initial state to try to fix build failures.

Revert Status snapshot to master

8834e28

Remove unneeded @ts-ignore

77d685f

kwasi marked this pull request as ready for review January 28, 2020 07:46

k8s-ci-robot removed the do-not-merge/work-in-progress label Jan 28, 2020

jessiezcc requested a review from Bobgy January 28, 2020 18:15

Fix ArtifactList response

82ccb80

kwasi commented Jan 30, 2020

View reviewed changes

Bobgy approved these changes Jan 30, 2020

View reviewed changes

k8s-ci-robot added the approved label Jan 30, 2020

kwasi added 2 commits January 30, 2020 11:09

Check for potentially undefined code in serviceErrorToString

a2a4d6b

Restore missing error handlers

a2f4476

k8s-ci-robot added the lgtm label Jan 30, 2020

k8s-ci-robot removed the do-not-merge/hold label Jan 30, 2020

Merge branch 'master' of https://github.com/kubeflow/pipelines into l…

073460f

…ineage-in-kfp-wip-merge-latest

k8s-ci-robot removed the lgtm label Jan 30, 2020

k8s-ci-robot added the lgtm label Jan 30, 2020

k8s-ci-robot merged commit 0ad6a55 into kubeflow:master Jan 31, 2020

kwasi mentioned this pull request Jan 31, 2020

[MLMD] Move mlmd clients to a subdirectory kubeflow/frontend#25

Open

Bobgy mentioned this pull request Feb 8, 2020

Refactor frontend to use promise based grpc-web metadata api #2196

Closed

	// TODO: __ALL_META__ is something of a hack, is redundant, and can be ignored
	// TODO: __ALL_META__ is something of a hack, is redundant, and can be ignored

[MLMD][Lineage] Integrate Lineage View into KFP #2918

[MLMD][Lineage] Integrate Lineage View into KFP #2918

Conversation

kwasi commented Jan 27, 2020 • edited Loading

Changes

Validation

k8s-ci-robot commented Jan 27, 2020

Changes

Validation

Hold

k8s-ci-robot commented Jan 27, 2020 • edited by jlewi Loading

Changes

Validation

Hold

avdaredevil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwasi commented Jan 28, 2020 • edited Loading

k8s-ci-robot commented Jan 28, 2020 • edited by jlewi Loading

Changes

Validation

Hold

kwasi left a comment

Choose a reason for hiding this comment

Bobgy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 30, 2020

kwasi commented Jan 30, 2020

kwasi commented Jan 30, 2020

avdaredevil commented Jan 30, 2020

avdaredevil commented Jan 30, 2020

kwasi commented Jan 30, 2020

kwasi commented Jan 30, 2020

kwasi commented Jan 30, 2020

kwasi commented Jan 30, 2020

avdaredevil commented Jan 30, 2020

kwasi commented Jan 30, 2020

kwasi commented Jan 31, 2020

Bobgy commented Jan 31, 2020

Bobgy commented Jan 31, 2020 • edited Loading

kwasi commented Jan 31, 2020

kwasi commented Jan 27, 2020 •

edited

Loading

k8s-ci-robot commented Jan 27, 2020 •

edited by jlewi

Loading

kwasi commented Jan 28, 2020 •

edited

Loading

k8s-ci-robot commented Jan 28, 2020 •

edited by jlewi

Loading

Bobgy commented Jan 31, 2020 •

edited

Loading