chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters #5957

capri-xiyue · 2021-06-30T19:11:47Z

Description of your changes:
Fixed #5819
Fetch cache result from MLMD and put cache result into argo output artifacts/parameters

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

capri-xiyue · 2021-06-30T19:19:50Z

/test kubeflow-pipelines-v2-go-test

Bobgy · 2021-07-07T00:42:27Z

@capri-xiyue can you fix the bug in a separate PR?
We can do another e2e release after merging it.

capri-xiyue · 2021-07-07T01:45:44Z

/test kubeflow-pipeline-backend-test

…into issue_5819

backend/src/apiserver/filter/filter.go

capri-xiyue · 2021-07-08T03:11:37Z

@Bobgy It's ready for early review.
I verified in local that the cache works
This task reused artifacts of previous runs.

capri-xiyue · 2021-07-08T18:16:56Z

backend/src/v2/driver/main.go

 	"github.com/kubeflow/pipelines/backend/src/v2/common"
 	"github.com/kubeflow/pipelines/backend/src/v2/common/mlmd"
+	pb "github.com/kubeflow/pipelines/backend/src/v2/third_party/pipeline_spec"


backend/src/v2 uses out-dated pipelinespec. The fix of v2 package takes time. As a workaround, I temporarily pasted out-dated pipeline spec that v2 is using.

Thanks for the catch! I will update pipeline spec in a separate PR.

capri-xiyue · 2021-07-08T19:39:09Z

@Bobgy , this PR is ready for review. I disable caching in this PR. Will enabling caching in the PR of caching e2e test.

capri-xiyue · 2021-07-08T21:43:23Z

/test kubeflow-pipelines-samples-v2

capri-xiyue · 2021-07-08T22:15:50Z

/test kubeflow-pipelines-samples-v2

capri-xiyue · 2021-07-08T22:29:58Z

The sample-v2 test failure is not because of this PR.
The master also has the same issue #5997

Bobgy · 2021-07-09T01:23:53Z

backend/src/apiserver/resource/resource_manager_test.go

@@ -412,7 +412,11 @@ func TestCreateRun_ThroughPipelineID(t *testing.T) {
 	expectedRuntimeWorkflow.Annotations = map[string]string{util.AnnotationKeyRunName: "run1"}
 	expectedRuntimeWorkflow.Spec.Arguments.Parameters = []v1alpha1.Parameter{{Name: "param1", Value: v1alpha1.AnyStringPtr("world")}}
 	expectedRuntimeWorkflow.Spec.ServiceAccountName = defaultPipelineRunnerServiceAccount
-
+	expectedRuntimeWorkflow.Spec.PodMetadata = &v1alpha1.Metadata{


Adding this to every test makes tests harder to read, shall we add the label consistently in AddMetadata method on 410 line?

Or another approach is changing the AddMetadata method to remove runtime metadata. In all tests, we remove test unrelated metadata from generated workflow before comparison with expectation

Bobgy · 2021-07-09T01:55:23Z

v2/cacheutils/cache.go

+}
+
+func (c *Client) CreateExecutionCache(ctx context.Context, task *api.Task) error {


Does this need to be in cache package? I think putting task records is theoretically unrelated to cache, just that cache uses these records

The cache package has the Client(Currently the client is only used for storing cache entries). Therefore, I put it in the cache package. If later, we decide to put task records for every task(not just for tasks where cache is enabled), I think it makes sense to move it to other package like kfp package.

Bobgy · 2021-07-09T01:57:35Z

v2/cacheutils/cache.go

+}
+
+func GetOutputParamsFromCachedExecution(cachedExecution *ml_metadata.Execution) (map[string]string, error) {


nit: consider avoiding stutter in naming?
GetOutputParams(cachedExecution ...)

Bobgy · 2021-07-09T02:21:57Z

v2/component/launcher.go

+	}
+	MLMDOutputArtifactByName := make(map[string]*pb.Artifact)
+	for _, artifact := range MLMDOutputArtifacts {
+		name := extractNameFromURI(*artifact.Uri)


Name should be taken from the event that connects artifact and the original execution.
See event.path

No. I printed the event.path. It records the id of the artifact which will change every time.

@capri-xiyue strange, did you check event.artifactId or event.path?
Refer to code

pipelines/v2/metadata/client.go

Line 236 in ad419cd

Path: eventPath(oa.Name),

I printed the wrong one. Fixed it in the refactor PR.

v2/metadata/client.go

Bobgy · 2021-07-09T03:50:17Z

/retest

capri-xiyue · 2021-07-09T05:02:26Z

/test kubeflow-pipelines-samples-v2

capri-xiyue · 2021-07-09T06:13:36Z

I will submit another PR to resolve the comments.

google-oss-robot · 2021-07-09T06:14:40Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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:

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

capri-xiyue added 17 commits June 11, 2021 17:24

added model and storage layer for task

a4798be

added create task api

b5805e2

added api to list tasks

3594e89

modified task proto and fixed nits

7f1f7a9

renamed variable

ad8f16c

fixed ut

ee1e6d7

fixed UT

7b29f2d

added UT for api_converter and resource manager

155d50c

added UT for api_converter and resource manager

5a1ec6d

fixed BE UT

a68b0e2

added task storage layer UT

41d6cfe

merge with master and resolve conflicts

7776460

changed UT

9f06ff5

fixed foreign key typo

ca9d83a

added some draft code for replay argo result

16851db

added kfp client in v2

fe323b7

merge with master and resolve conflicts

23c06e4

google-oss-robot added the do-not-merge/work-in-progress label Jun 30, 2021

google-oss-robot requested review from Ark-kun and Bobgy June 30, 2021 19:11

google-oss-robot added the size/XXL label Jun 30, 2021

capri-xiyue requested review from Bobgy and removed request for Bobgy and Ark-kun June 30, 2021 19:11

google-cla bot added the cla: yes label Jun 30, 2021

deleted unused code

634c028

capri-xiyue added 2 commits June 30, 2021 12:21

run go mode

8bcda3a

upgraded go version in go v2 test

6d34968

capri-xiyue changed the title ~~WIP chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters~~ chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters Jun 30, 2021

Merge branch 'master' into issue_5819

e5613e3

capri-xiyue added 2 commits July 6, 2021 22:02

resolve conflicts with master

885ecc5

Merge branch 'issue_5819' of https://github.com/capri-xiyue/pipelines …

46b809c

…into issue_5819

capri-xiyue commented Jul 7, 2021

View reviewed changes

backend/src/apiserver/filter/filter.go Outdated Show resolved Hide resolved

capri-xiyue added 3 commits July 7, 2021 20:00

fixed cache bug

b9090c6

added cache info

d734aaa

fixed unused test

411042f

capri-xiyue added 2 commits July 8, 2021 11:07

fixed backend test

22d485c

fixed v2 build

f2f3e66

capri-xiyue commented Jul 8, 2021

View reviewed changes

refactored launcher

60c181b

capri-xiyue removed the do-not-merge/work-in-progress label Jul 8, 2021

removed unused code

b6186d1

Bobgy reviewed Jul 9, 2021

View reviewed changes

capri-xiyue added 2 commits July 8, 2021 22:52

merge with master

bb088a1

fixed typo

f67004f

capri-xiyue added lgtm approved labels Jul 9, 2021

google-oss-robot merged commit 724e5b4 into kubeflow:master Jul 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters #5957

chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters #5957

capri-xiyue commented Jun 30, 2021

capri-xiyue commented Jun 30, 2021

Bobgy commented Jul 7, 2021

capri-xiyue commented Jul 7, 2021

capri-xiyue commented Jul 8, 2021

capri-xiyue Jul 8, 2021

Bobgy Jul 9, 2021

capri-xiyue commented Jul 8, 2021

capri-xiyue commented Jul 8, 2021

capri-xiyue commented Jul 8, 2021

capri-xiyue commented Jul 8, 2021 •

edited

Loading

Bobgy Jul 9, 2021

Bobgy Jul 9, 2021

Bobgy Jul 9, 2021

capri-xiyue Jul 9, 2021

Bobgy Jul 9, 2021

capri-xiyue Jul 9, 2021

Bobgy Jul 9, 2021

capri-xiyue Jul 9, 2021

Bobgy Jul 10, 2021

capri-xiyue Jul 10, 2021

Bobgy commented Jul 9, 2021

capri-xiyue commented Jul 9, 2021

capri-xiyue commented Jul 9, 2021

google-oss-robot commented Jul 9, 2021

		}

		func (c Client) CreateExecutionCache(ctx context.Context, task api.Task) error {

		}

		func GetOutputParamsFromCachedExecution(cachedExecution *ml_metadata.Execution) (map[string]string, error) {

chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters #5957

chore(v2): Fetch cache result from MLMD and put cache result into argo output artifacts/parameters #5957

Conversation

capri-xiyue commented Jun 30, 2021

capri-xiyue commented Jun 30, 2021

Bobgy commented Jul 7, 2021

capri-xiyue commented Jul 7, 2021

capri-xiyue commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

capri-xiyue commented Jul 8, 2021

capri-xiyue commented Jul 8, 2021

capri-xiyue commented Jul 8, 2021

capri-xiyue commented Jul 8, 2021 • edited Loading

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

Choose a reason for hiding this comment

Bobgy commented Jul 9, 2021

capri-xiyue commented Jul 9, 2021

capri-xiyue commented Jul 9, 2021

google-oss-robot commented Jul 9, 2021

capri-xiyue commented Jul 8, 2021 •

edited

Loading