feat(backend): Merge kfp-tekton backend code #10678

rimolive · 2024-04-08T18:37:07Z

Description of your changes:
Closes #10488

Merge all the kfp-tekton compiler code to pipelines.

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.

rimolive · 2024-04-08T18:37:22Z

/hold

Still working on the tests

backend/third_party_licenses/tekton-driver.csv

Tomcli · 2024-04-08T19:03:01Z

backend/src/v2/objectstore/object_store.go


 	// TODO: Verify/add support for file:///.
-	if ms[1] != "gs://" && ms[1] != "s3://" && ms[1] != "minio://" {
+	if ms[1] != "gs://" && ms[1] != "s3://" && ms[1] != "minio://" && ms[1] != "mem://" {


mem:// here is to support in memory storage for launcher tests under environments that doesn't have access to public gcs endpoints

Tomcli · 2024-04-08T19:06:36Z

backend/src/agent/persistence/client/pipeline_client.go

 	"google.golang.org/grpc/metadata"

 	api "github.com/kubeflow/pipelines/backend/api/v1beta1/go_client"
+	apiv2 "github.com/kubeflow/pipelines/backend/api/v2beta1/go_client"


We move report client to use the new v2 api

Tomcli · 2024-04-08T19:06:59Z

backend/src/agent/persistence/main.go

 	numWorkerName                         = "numWorker"
 	clientQPSFlagName                     = "clientQPS"
 	clientBurstFlagName                   = "clientBurst"
+	executionTypeFlagName                 = "executionType"


This new flag is for changing executionType, default is Argo

rimolive · 2024-04-09T12:41:32Z

/test kubeflow-pipeline-e2e-test

rimolive · 2024-04-09T13:26:50Z

/test kubeflow-pipeline-e2e-test

rimolive · 2024-04-09T14:30:11Z

@Tomcli @chensun I need a help in here. The e2e tests that are failling are job tests, and I just ran locally but they're passing. Did I miss someting?

Tomcli

Thanks @rimolive for resolving the tests. I don't have access the the backend test, but it seems like only the api integration test is failing.

chensun · 2024-04-09T21:31:42Z

The failure are on jobs, aka scheduled workflow:

integration-test-dg828-1458653792: === RUN   TestJobApi/TestJobApis
integration-test-dg828-1458653792:     job_api_test.go:374: 
integration-test-dg828-1458653792:         	Error Trace:	/go/src/github.com/kubeflow/pipelines/backend/test/integration/job_api_test.go:374
integration-test-dg828-1458653792:         	Error:      	Expected nil, but got: &errors.errorString{s:"expected runs to be length 1, got: 0"}
integration-test-dg828-1458653792:         	Test:       	TestJobApi/TestJobApis
integration-test-dg828-1458653792:     job_api_test.go:395: 
integration-test-dg828-1458653792:         	Error Trace:	/go/src/github.com/kubeflow/pipelines/backend/test/integration/job_api_test.go:395
integration-test-dg828-1458653792:         	Error:      	Expected nil, but got: &errors.errorString{s:"expected runs to be length 1, got: 0"}
integration-test-dg828-1458653792:         	Test:       	TestJobApi/TestJobApis
integration-test-dg828-1458653792: === RUN   TestJobApi/TestJobApis_SwfNotFound
integration-test-dg828-1458653792: === RUN   TestJobApi/TestJobApis_noCatchupOption
integration-test-dg828-1458653792:     job_api_test.go:512: 
integration-test-dg828-1458653792:         	Error Trace:	/go/src/github.com/kubeflow/pipelines/backend/test/integration/job_api_test.go:512
integration-test-dg828-1458653792:         	Error:      	Expected nil, but got: &errors.errorString{s:"expected runsWhenCatchupTrue with periodic schedule to be 2, got: 0"}
integration-test-dg828-1458653792:         	Test:       	TestJobApi/TestJobApis_noCatchupOption
integration-test-dg828-1458653792:     job_api_test.go:540: 
integration-test-dg828-1458653792:         	Error Trace:	/go/src/github.com/kubeflow/pipelines/backend/test/integration/job_api_test.go:540
integration-test-dg828-1458653792:         	Error:      	Expected nil, but got: &errors.errorString{s:"expected runsWhenCatchupFalse with periodic schedule to be 1, got: 0"}
integration-test-dg828-1458653792:         	Test:       	TestJobApi/TestJobApis_noCatchupOption
integration-test-dg828-1458653792: --- FAIL: TestJobApi (176.21s)
integration-test-dg828-1458653792:     --- FAIL: TestJobApi/TestJobApis (93.56s)
integration-test-dg828-1458653792:     --- PASS: TestJobApi/TestJobApis_SwfNotFound (0.50s)
integration-test-dg828-1458653792:     --- FAIL: TestJobApi/TestJobApis_noCatchupOption (81.82s)

Not sure how you tested this locally, consider adding debug logs to the test functions?
https://github.com/kubeflow/pipelines/blob/ac399315e66d6ed2666dc9dbaecbce4938f87356/backend/test/integration/job_api_test.go#L156C27-L156C38

backend/src/agent/persistence/client/pipeline_client.go

backend/Dockerfile.scheduledworkflow

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>

rimolive · 2024-04-12T20:39:25Z

/unhold

@Tomcli @chensun Finally I got the tests passing but unfortunately I needed to follow Tommy's recommentation to remove the scheduled workflow and rethink the kfp-tekton code for that.

Feel free to lgtm/approve, but take into consideration that this will be a partial feature. I believe it's not a big deal given that historically we could handle partial feature implementations to make progress.

Tomcli

thanks @rimolive for fixing some of the tests and dependencies.
/lgtm

For the scheduledworkflow, we probably need to also update the api test to use the new swf v2 client. Let's do that in a follow up PR so it won't block the core KFP feature.

chensun · 2024-04-15T17:14:38Z

/unhold

@Tomcli @chensun Finally I got the tests passing but unfortunately I needed to follow Tommy's recommentation to remove the scheduled workflow and rethink the kfp-tekton code for that.

Feel free to lgtm/approve, but take into consideration that this will be a partial feature. I believe it's not a big deal given that historically we could handle partial feature implementations to make progress.

@rimolive Just to confirm, by partial feature you meant scheduledworkflow doesn't work on kfp-tekton yet, but there's no regression to the feature on kfp-argo, right?

Tomcli · 2024-04-15T17:16:13Z

/unhold
@Tomcli @chensun Finally I got the tests passing but unfortunately I needed to follow Tommy's recommentation to remove the scheduled workflow and rethink the kfp-tekton code for that.
Feel free to lgtm/approve, but take into consideration that this will be a partial feature. I believe it's not a big deal given that historically we could handle partial feature implementations to make progress.

@rimolive Just to confirm, by partial feature you meant scheduledworkflow doesn't work on kfp-tekton yet, but there's no regression to the feature on kfp-argo, right?

Yes, there's no regression on all the kfp-argo feature.

chensun · 2024-04-15T18:00:30Z

backend/Dockerfile.persistenceagent

 ENV NUM_WORKERS 2
 ENV LOG_LEVEL info

-CMD persistence_agent --logtostderr=true --namespace=${NAMESPACE} --ttlSecondsAfterWorkflowFinish=${TTL_SECONDS_AFTER_WORKFLOW_FINISH} --numWorker ${NUM_WORKERS} --logLevel=${LOG_LEVEL}


This seems to be an unintentional change? I see the default value remains the same, but we should keep this customization capability.

Nice catch! I will rollback that change.

Actually, this line is completely wrong, it should have the EXECUTIONTYPE env var set by default and send as a flag. I'm fixing it.

chensun · 2024-04-15T18:02:47Z

backend/src/agent/persistence/main.go

 	// k8s.io/client-go/rest/config.go#RESTClientFor
 	flag.Float64Var(&clientQPS, clientQPSFlagName, 5, "The maximum QPS to the master from this client.")
 	flag.IntVar(&clientBurst, clientBurstFlagName, 10, "Maximum burst for throttle from this client.")
+	flag.StringVar(&executionType, executionTypeFlagName, "Workflow", "Custom Resource's name of the backend Orchestration Engine")


nit: Workflow is too generic as the executionType, should we name the default "ArgoWorkflow" instead?

This code uses the kind field of the pipeline to find its execution type. Changing to ArgoWorkflow would break the code.

We can think of a better way to do this, maybe using annotation. However, I think this is out of scope of this PR.

chensun · 2024-04-15T18:04:10Z

backend/src/agent/persistence/worker/swf_saver.go


 	}

+	// TODO: wait for up stream to officially update to v2beta1


nit: The comment needs some update--there's no upstream once this is merged in to kfp repro.

But v2beta1 is already released, right? If so, we can just remove the comment.

chensun · 2024-04-15T18:06:38Z

backend/src/apiserver/resource/resource_manager.go

+			return nil, util.NewInternalServerError(util.NewInvalidInputError("ScheduledWorkflow doesn't exist: %s", job.K8SName), "Failed to create a run due to invalid name")
+		}
+		executionSpec.SetOwnerReferences(swf)
+		// executionSpec.SetExecutionName(run.Description)


nit: remove?

chensun · 2024-04-15T18:11:48Z

backend/src/common/util/execution_spec.go

+		if executionSpecStr, ok := wfr.Spec.(string); ok {
+			return NewPipelineRunFromScheduleWorkflowSpecBytesJSON([]byte(executionSpecStr))
+		}
+		// fall back to Argo WorkflowSpec, need to marshal back to json string then unmarshal to


nit: update comment

chensun · 2024-04-15T18:13:44Z

backend/src/common/util/execution_spec_test.go

 	assert.Empty(t, execSpec)
 	assert.Error(t, err)
-	assert.EqualError(t, err, "InternalServerError: type:PipelineRun: ExecutionType is not supported")
+	assert.EqualError(t, err, "Invalid input error: not PipelineRun struct")


nit: there should be a better error message

It follows the same message for Argo workflow: https://github.com/kubeflow/pipelines/blob/master/backend/src/common/util/workflow.go#L94

chensun · 2024-04-15T18:14:26Z

backend/src/common/util/pipelinerun.go

+// Get converts this object to a workflowapi.Workflow.
+// func (pr *PipelineRun) Get() *pipelineapi.PipelineRun {
+// 	return pr.PipelineRun
+// }


nit: remove

chensun · 2024-04-15T18:16:37Z

backend/src/common/util/pipelinerun.go

+	// if newWF.Spec.ActiveDeadlineSeconds != nil && *newWF.Spec.ActiveDeadlineSeconds == 0 {
+	// 	// if it was terminated, unset the deadline
+	// 	newWF.Spec.ActiveDeadlineSeconds = nil
+	// }


nit: remove all the dead code.

chensun · 2024-04-15T18:18:38Z

backend/src/common/util/pipelinerun.go

+func readTaskRunMetricsJSONOrEmpty(
+	runID string, nodeStatus pipelineapi.PipelineRunTaskRunStatus,
+	retrieveArtifact RetrieveArtifact) (string, error) {
+	// Tekton doesn't support any artifact spec, artifact records are done by our custom metadata writers:


ditto: remove dead code

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>

google-oss-prow · 2024-04-15T21:47:02Z

@rimolive: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
kubeflow-pipelines-samples-v2	`39991f2`	link	false	`/test kubeflow-pipelines-samples-v2`

Details

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. I understand the commands that are listed here.

chensun

/lgtm
/approve

Thanks!

google-oss-prow · 2024-04-15T23:56:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun, Tomcli

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [chensun]

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

google-oss-prow bot added the size/XXL label Apr 8, 2024

google-oss-prow bot added the do-not-merge/hold label Apr 8, 2024

google-oss-prow bot requested review from Tomcli and chensun April 8, 2024 18:37

Tomcli reviewed Apr 8, 2024

View reviewed changes

backend/third_party_licenses/tekton-driver.csv Outdated Show resolved Hide resolved

Tomcli reviewed Apr 8, 2024

View reviewed changes

Tomcli reviewed Apr 9, 2024

View reviewed changes

backend/src/agent/persistence/client/pipeline_client.go Outdated Show resolved Hide resolved

Tomcli reviewed Apr 11, 2024

View reviewed changes

backend/Dockerfile.scheduledworkflow Outdated Show resolved Hide resolved

Merge kfp-tekton backend code

90cfbb4

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>

google-oss-prow bot removed the do-not-merge/hold label Apr 12, 2024

Tomcli approved these changes Apr 12, 2024

View reviewed changes

google-oss-prow bot assigned Tomcli Apr 12, 2024

google-oss-prow bot added the lgtm label Apr 12, 2024

chensun requested changes Apr 15, 2024

View reviewed changes

google-oss-prow bot removed the lgtm label Apr 15, 2024

Add swf work

39991f2

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>

chensun approved these changes Apr 15, 2024

View reviewed changes

google-oss-prow bot assigned chensun Apr 15, 2024

google-oss-prow bot added the lgtm label Apr 15, 2024

google-oss-prow bot added the approved label Apr 15, 2024

google-oss-prow bot merged commit 60a443e into kubeflow:master Apr 16, 2024


		}

		// TODO: wait for up stream to officially update to v2beta1

feat(backend): Merge kfp-tekton backend code #10678

feat(backend): Merge kfp-tekton backend code #10678

Uh oh!

Conversation

rimolive commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rimolive commented Apr 8, 2024

Uh oh!

Uh oh!

Tomcli Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomcli Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tomcli Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rimolive commented Apr 9, 2024

Uh oh!

rimolive commented Apr 9, 2024

Uh oh!

rimolive commented Apr 9, 2024

Uh oh!

Tomcli left a comment

Choose a reason for hiding this comment

Uh oh!

chensun commented Apr 9, 2024

Uh oh!

Uh oh!

Uh oh!

rimolive commented Apr 12, 2024

Uh oh!

Tomcli left a comment

Choose a reason for hiding this comment

Uh oh!

chensun commented Apr 15, 2024

Uh oh!

Tomcli commented Apr 15, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-prow bot commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chensun left a comment

Choose a reason for hiding this comment

Uh oh!

google-oss-prow bot commented Apr 15, 2024

Uh oh!

rimolive commented Apr 8, 2024 •

edited

Loading

Tomcli Apr 8, 2024 •

edited

Loading

Tomcli Apr 8, 2024 •

edited

Loading

Tomcli Apr 8, 2024 •

edited

Loading

google-oss-prow bot commented Apr 15, 2024 •

edited

Loading