Add pipeline version api methods #2338

jingzhang36 · 2019-10-09T10:20:11Z

add api methods that support pipeline versions.
remove one unused func.

This change is

jingzhang36 · 2019-10-11T01:22:56Z

/test kubeflow-pipeline-e2e-test

jingzhang36 · 2019-10-11T01:25:28Z

/assign @IronPan
/cc @james-jwu

jingzhang36 · 2019-10-12T02:04:07Z

/assign @IronPan

IronPan · 2019-10-15T00:26:25Z

backend/api/pipeline.proto

+
+  rpc ListPipelineVersions(ListPipelineVersionsRequest) returns (ListPipelineVersionsResponse) {
+    option (google.api.http) = {
+      get: "/apis/v1beta1/pipeline_versions/pipeline/{resource_key.id}"


this URL looks not right. as reference can u check URL to list run by experiment?

changed api path

Another related api path question. If we merge this PR, users can call api server to directly manipulate (create, delete, get, list) versions in spite that they can't do that from FE. Would that be ok, say, considered as a too-early or premature exposure?

You could, in implementation, return not implemented.

ah, I commented out the http point in pipeline.proto, so i can leave the method implementation in its expected final state. BTW, by doing the comment-out, the curl returns "not found".

IronPan · 2019-10-15T04:25:33Z

backend/src/apiserver/resource/client_manager_fake.go

@@ -25,6 +25,7 @@ import (

 const (
 	DefaultFakeUUID = "123e4567-e89b-12d3-a456-426655440000"
+	fakeUUIDOne     = "123e4567-e89b-12d3-a456-426655440001"


IronPan · 2019-10-15T04:26:07Z

backend/src/apiserver/resource/resource_manager.go

@@ -814,3 +814,101 @@ func (r *ResourceManager) MarkSampleLoaded() error {
 func (r *ResourceManager) getDefaultSA() string {
 	return common.GetStringConfigWithDefault(defaultPipelineRunnerServiceAccountEnvVar, defaultPipelineRunnerServiceAccount)
 }
+
+func (r *ResourceManager) CreatePipelineVersion(apiVersion *api.PipelineVersion, pipelineFile []byte) (*model.PipelineVersion, error) {
+	// Extract the parameter from the pipeline


parameter -> parameters

IronPan · 2019-10-15T04:39:59Z

backend/src/apiserver/server/pipeline_server.go

+			"Invalid Pipeline URL %v. Please specify a valid URL",
+			request.Version.PackageUrl.PipelineUrl)
+	}
+	pipelineUrl := request.Version.PackageUrl.PipelineUrl


move to line 129

curious: move before validating the url? logically no issue. but assign to pipelineUrl will be simply a waste, if the later check doesn't pass. so why not assign after two checks have passed?

it's more for code sanity than memory optimization. later doesn't matter too much in this case.

IronPan · 2019-10-15T04:44:22Z

backend/src/apiserver/server/pipeline_server.go

+	if err != nil || resp.StatusCode != http.StatusOK {
+		return nil, util.NewInternalServerError(err,
+			`Failed to download the pipeline from %v.
+			Please double check the URL is valid and can be accessed by the


seems line breaks too eagerly (<80 char) and breaks the sentence
see best practice
https://github.com/golang/go/wiki/CodeReviewComments#line-length

please change other places in the PR too

i'll try, but given that the guideline for breaking is basically a break here feeling natural (or unnatural), I am inclined to think that many calls are subjective. E.g., I feel it's easy to read to have each input parameter in a separate line, if they can't fit all in a single line. So I know how many parameters are there in first glimpse, especially there are many of them.

That being said. Back to go style, what I'll do is: if gofmt doesn't break lines I'll never add breaks manually.

here it's breaking in the middle of a sentence.

IronPan · 2019-10-15T04:45:00Z

backend/src/apiserver/server/pipeline_server.go

+			pipelineUrl)
+	}
+	pipelineFileName := path.Base(pipelineUrl)
+	pipelineFile, err := ReadPipelineFile(


ditto this is not a natural line break

IronPan · 2019-10-15T04:50:08Z

backend/src/apiserver/storage/pipeline_store.go

+				"Parameters":     newPipelineVersion.Parameters,
+				"PipelineId":     newPipelineVersion.PipelineId,
+				"Status":         string(newPipelineVersion.Status),
+				"CodeSourceUrl":  ""}).


why empty string?

... Thanks for the catch! Forgot about this place after the initial quick draft. Added unit test including this field.

IronPan · 2019-10-15T04:54:46Z

backend/src/apiserver/storage/pipeline_store.go

+	return nil
+}
+
+func (s *PipelineStore) GetPipelineVersion(versionId string) (*model.PipelineVersion, error) {


can you remind me why we need two methods? get and get status

the latter is used in test to quickly fetch a pipeline (or pipeline version) in certain status? e.g., when testing deletion, db is updated, minio failed; and pipeline in db should be in deleting status. so get pipeline or pipeline version with deleting status to verify.

IronPan · 2019-10-15T04:55:33Z

backend/src/apiserver/storage/pipeline_store.go

+	sql, args, err := sq.
+		Select(pipelineVersionColumns...).
+		From("pipeline_versions").
+		Where(sq.Eq{"UUID": versionId}).


can you use and statement instead of chaining where for better readability

jingzhang36 · 2019-10-15T07:17:25Z

/assign @IronPan

jingzhang36 · 2019-10-17T02:30:21Z

/assign @IronPan

IronPan · 2019-10-17T06:59:35Z

/lgtm
/approve

k8s-ci-robot · 2019-10-17T06:59:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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:

~~backend/OWNERS~~ [IronPan]

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

jingzhang36 · 2019-10-17T07:19:24Z

/test kubeflow-pipeline-sample-test

Signed-off-by: Dan Sun <dsun20@bloomberg.net>

jingzhang36 added 3 commits October 9, 2019 14:27

add version api

087f2ab

unit tests

3972ab9

remove debug fmt

e2c2def

k8s-ci-robot requested review from IronPan and neuromage October 9, 2019 10:20

k8s-ci-robot added the size/XXL label Oct 9, 2019

jingzhang36 added 5 commits October 10, 2019 11:13

remove unused func

b329fe0

remove another unused method

a488eef

formatting

c97d39c

remove unused consts

be61038

some comments

20de892

k8s-ci-robot assigned IronPan Oct 11, 2019

k8s-ci-robot requested a review from james-jwu October 11, 2019 01:25

jingzhang36 added 5 commits October 11, 2019 17:02

build

386e6af

unit tests

cd8043d

unit tests

380a302

formatting

546e9d3

unit tests

22404e8

IronPan reviewed Oct 15, 2019

View reviewed changes

address comments

4802241

jingzhang36 added 4 commits October 16, 2019 14:46

address comment

9408182

address comment

48f0697

add comment

9ed59b8

Merge remote-tracking branch 'origin/master' into api-version

f83218f

k8s-ci-robot added the lgtm label Oct 17, 2019

k8s-ci-robot added the approved label Oct 17, 2019

k8s-ci-robot merged commit f66af1f into kubeflow:master Oct 17, 2019

Bobgy mentioned this pull request Jan 21, 2020

Support the versioning of artifacts and/or pipelines. #467

Closed

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023

Add annotation check for the deployment mode (kubeflow#2338)

133eceb

Signed-off-by: Dan Sun <dsun20@bloomberg.net>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pipeline version api methods #2338

Add pipeline version api methods #2338

jingzhang36 commented Oct 9, 2019 •

edited

Loading

jingzhang36 commented Oct 11, 2019

jingzhang36 commented Oct 11, 2019

jingzhang36 commented Oct 12, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

jingzhang36 Oct 15, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 16, 2019 •

edited

Loading

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 16, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019 •

edited

Loading

IronPan Oct 15, 2019

jingzhang36 Oct 16, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019 •

edited

Loading

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

IronPan Oct 15, 2019

jingzhang36 Oct 15, 2019

jingzhang36 commented Oct 15, 2019

jingzhang36 commented Oct 17, 2019

IronPan commented Oct 17, 2019

k8s-ci-robot commented Oct 17, 2019

jingzhang36 commented Oct 17, 2019

Add pipeline version api methods #2338

Add pipeline version api methods #2338

Conversation

jingzhang36 commented Oct 9, 2019 • edited Loading

jingzhang36 commented Oct 11, 2019

jingzhang36 commented Oct 11, 2019

jingzhang36 commented Oct 12, 2019

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

jingzhang36 Oct 16, 2019 • 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

jingzhang36 Oct 15, 2019 • 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

jingzhang36 Oct 15, 2019 • 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

jingzhang36 commented Oct 15, 2019

jingzhang36 commented Oct 17, 2019

IronPan commented Oct 17, 2019

k8s-ci-robot commented Oct 17, 2019

jingzhang36 commented Oct 17, 2019

jingzhang36 commented Oct 9, 2019 •

edited

Loading

jingzhang36 Oct 16, 2019 •

edited

Loading

jingzhang36 Oct 15, 2019 •

edited

Loading

jingzhang36 Oct 15, 2019 •

edited

Loading