Add necessary data types to api and database to support pipeline version. #1873

jingzhang36 · 2019-08-17T02:52:59Z

Mostly based on Yang's branch at https://github.com/IronPan/pipelines/tree/kfpci/.

Backward compatible.
Major intention is to add data types/fields and some util methods to access those new types/fields. API method setting/using those fields will be in another PR, in order to avoid huge PRs.

In addition to the unit tests and integration tests, I tested on my kfp deployment,
(1) all runs and pipelines from before the upgrade survive the upgrade and function as usual (e.g., view, delete)
(2) new pipelines can be added properly after upgrade and runs (one time and recurring) are created on these pipelines. Then deletion of these pipelines behave as usual too.
(3) all deletion of pipelines lead to the removal of versions as well.
(4) create new experiment, and create run under this experiment

This change is

on Yang's branch at https://github.com/IronPan/pipelines/tree/kfpci/. Backward compatible.

jingzhang36 · 2019-08-20T23:00:31Z

/assign @IronPan
/assign @neuromage

jingzhang36 · 2019-08-21T03:46:33Z

/retest

jingzhang36 · 2019-08-21T23:23:37Z

/assign @IronPan
/assing @neuromage

IronPan · 2019-08-23T09:02:44Z

backend/api/pipeline.proto

+  string version_id = 2;
+}
+
+message ListPipelineVersionsRequest {


we should consider adding pagination for list version.

IronPan · 2019-08-23T09:07:42Z

backend/api/pipeline.proto

+  repeated Parameter parameters = 4;
+
+  // Input.
+  oneof code_source {


should they be modeled as oneof?
the code source is where the source code is located. the url is where the compiled package stored. they are not mutually exclusive.

I'll move url out of code source then. Another note: if we have both source and package as optional fields, there is a possibility that the two are inconsistent with each other (and we won't validate the provided source and package when those get set, since the validation work is not so trivial and probably not worthwhile). In case of that, we probably need to make one having priority over the other...

Also, if we allow both code source and package source to be present at the same time. Should we also allow multiple code sources? So we don't need oneof in CodeSource type.

IronPan · 2019-08-23T09:39:17Z

backend/src/apiserver/model/pipeline_version.go

+type PipelineVersion struct {
+	UUID           string `gorm:"column:VersionUUID; not null; primary_key"`
+	CreatedAtInSec int64  `gorm:"column:VersionCreatedAtInSec; not null"`
+	Name           string `gorm:"column:VersionName; not null"`


maybe we've discussed about this. the name of the version should be unique for a given pipeline for caching purpose. can we enforce it through db schema?

yes, we did. I missed it in the change..... added "unique"

IronPan · 2019-08-23T09:41:06Z

backend/src/apiserver/model/pipeline_version.go

+	RepoName  string `gorm:"column:RepoName"`
+	CommitSHA string `gorm:"column:CommitSHA"`
+
+	URL string `gorm:"column:URL`


same as api definition. url doesn't belong to code source.

IronPan · 2019-08-23T09:43:34Z

backend/src/apiserver/model/pipeline_version.go

+type CodeSource struct {
+	// PipelineVersion can be based on a git commit or a package stored at some
+	// url.
+	// All fields below are optional.


for storing oneof fields in DB should we instead use just two columns? code source type and the streamed json of the codesource?
This will make DB schema much cleaner and extensible.

I get rid of the oneof from api type definition. So kept 3 cols here for now. Please let me if we still want 2 columns instead: one is the combination of repo name and commit; the other is url.

IronPan · 2019-08-23T09:45:32Z

backend/src/apiserver/model/pipeline_version.go

+	// Set size to 65535 so it will be stored as longtext.
+	// https://dev.mysql.com/doc/refman/8.0/en/column-count-limit.html
+	Parameters string                `gorm:"column:VersionParameters; not null; size:65535"`
+	PipelineId string                `gorm:"column:PipelineId; not null"`


consider add foreign key for cascade delete.

jingzhang36 · 2019-08-27T03:30:07Z

/assign @IronPan
/assign @neuromage

IronPan · 2019-08-28T16:37:06Z

backend/api/pipeline.proto

@@ -136,6 +137,32 @@ message GetTemplateResponse {
  string template = 1;
 }

+message GetPipelineVersionTemplateRequest{


Is it intended to add API definition in a separate change?

We should use resource reference for flexible pipeline <-> version mapping
https://github.com/kubeflow/pipelines/blob/master/backend/api/resource_reference.proto

Please check experiment <-> job and experiment <-> run as examples to see how this is being used.

Api PipelineVersion definition will use resource reference to point to Pipeline. Model/Storage PipelineVersion will use foreign key of pipeline id.

jingzhang36 · 2019-09-19T15:44:15Z

/assign @IronPan

IronPan · 2019-09-19T16:55:50Z

backend/api/pipeline.proto

@@ -136,6 +138,32 @@ message GetTemplateResponse {
  string template = 1;
 }

+message GetPipelineVersionTemplateRequest{
+  string pipeline_id = 1;


pipeline id is not necessary here.

IronPan · 2019-09-19T17:07:25Z

backend/api/pipeline.proto

+}
+
+message CreatePipelineVersionRequest {
+  string pipeline_id = 1;


use ResourceReference?

IronPan · 2019-09-19T17:07:34Z

backend/api/pipeline.proto

+}
+
+message GetPipelineVersionRequest {
+  string pipeline_id = 1;


pipeline ID is not needed here.

IronPan · 2019-09-19T17:08:12Z

backend/api/pipeline.proto

+}
+
+message ListPipelineVersionsRequest {
+  string pipeline_id = 1;


Use ResourceKey instead.

IronPan · 2019-09-19T17:11:44Z

backend/api/pipeline.proto

+  repeated Parameter parameters = 4;
+
+  // Input. Optional. Pipeline version code sources.
+  repeated string code_source_urls = 5;


does this need to be repeated?

We allow multiple sources. In api, I use repeated field and in db, I used ";" separated string for multiple sources.

Should code always be single source of truth?

Let me try to summarize: (1) in storage/model, we have a single code source url (2) in api, we'll have to have a package url and a code source url in PipelineVersion message, because the package url we need to read file from for CreatePipelineVersionRequest. But the package url is not stored in db, so not in storage/model definition...

So I changed to a single code source url and a single package url in API, and a single code source in model.

IronPan · 2019-09-19T18:33:30Z

backend/src/apiserver/storage/pipeline_store.go

-	_, err = s.db.Exec(sql, args...)
+	sqlPipelineVersions, argsPipelineVersions, err := sq.
+		Insert("pipeline_versions").
+		SetMap(


format the code

IronPan · 2019-09-19T18:36:07Z

backend/src/apiserver/storage/pipeline_store.go

 	newPipeline := *p
 	now := s.time.Now().Unix()
 	newPipeline.CreatedAtInSec = now
+	newPipeline.DefaultVersion.CreatedAtInSec = now


the defaultversion is not guarantee to exist. this might cause nil pointer exception.

Added a check and if it's nil, create an implicit one.
Note:
(1) in the final release, CreatePipeline in pipline_store shouldn't create version. But in the intermediate status, CreatePipeline has to create an implicit version with pipeline
(2) in the intermediate status, if CreatePipeline is called from resource_manager, this field shall not be nil, since I the pipeline struct constructed there contains default version.

IronPan · 2019-09-19T18:36:33Z

backend/src/apiserver/storage/pipeline_store.go

 		ToSql()
 	if err != nil {
 		return nil, util.NewInternalServerError(err, "Failed to create query to insert pipeline to pipeline table: %v",
 			err.Error())
 	}
-	_, err = s.db.Exec(sql, args...)
+	sqlPipelineVersions, argsPipelineVersions, err := sq.


what if pipeline doesnt have default version?

In the final release, the default version in CreatePipeline shall be nil. But here, CreatePipeline will create an implicit version without the request specifying it.

IronPan · 2019-09-19T18:37:53Z

backend/src/apiserver/storage/pipeline_store.go

 	id, err := s.uuid.NewRandom()
 	if err != nil {
 		return nil, util.NewInternalServerError(err, "Failed to create a pipeline id.")
 	}
 	newPipeline.UUID = id.String()
+	newPipeline.DefaultVersion.PipelineId = id.String()


could you split the code to store a new pipeline and a new version? not mingle them together

They need to be in a single transaction anyway? A further note: in the final release, we wouldn't insert/create versions here at all. CreatePipeline will only handle pipeline (as a container/bag) and has no insertion/creation of versions when everything is in its final status.

IronPan · 2019-09-19T18:38:31Z

backend/src/apiserver/storage/run_store.go

@@ -17,6 +17,7 @@ package storage
 import (
 	"database/sql"
 	"fmt"
+


This empty line is not added manually. Auto formatting inserted it.

jingzhang36 · 2019-09-20T02:15:02Z

/cc @james-jwu FYI

k8s-ci-robot · 2019-09-20T02:15:04Z

@jingzhang36: GitHub didn't allow me to request PR reviews from the following users: FYI.

Note that only kubeflow members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @james-jwu FYI

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.

jingzhang36 · 2019-09-23T04:01:51Z

/assign @IronPan

…o make them more separate.

…lineVersionRequest

jingzhang36 · 2019-09-25T09:43:26Z

/retest

jingzhang36 · 2019-09-25T10:58:45Z

/test kubeflow-pipeline-sample-test

jingzhang36 · 2019-09-25T15:49:33Z

/assign @IronPan

IronPan · 2019-09-26T06:09:09Z

backend/api/pipeline.proto

+  // Input. Optional. Pipeline version code source.
+  string code_source_url = 5;
+
+  // Input. Required. Pipeline version package url.


this should be optional. Same as the Url in here
https://github.com/kubeflow/pipelines/pull/1873/files#diff-84b54249a5b54df88b95c2eed5c3f7c3R187

IronPan · 2019-09-26T06:12:26Z

/approve
Just a small comment

k8s-ci-robot · 2019-09-26T06:12:34Z

[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

IronPan · 2019-09-26T06:22:16Z

/lgtm

jingzhang36 added 2 commits August 17, 2019 10:42

Add necessary data types/tables for pipeline version. Mostly based

bd122ec

on Yang's branch at https://github.com/IronPan/pipelines/tree/kfpci/. Backward compatible.

Modified comment

dfe6588

k8s-ci-robot requested review from IronPan and neuromage August 17, 2019 02:53

k8s-ci-robot added the size/L label Aug 17, 2019

jingzhang36 changed the title ~~Add necessary data types and proto messages for pipeline version.~~ Add necessary data types to api and database to support pipeline version. Aug 19, 2019

jingzhang36 added 6 commits August 20, 2019 01:50

Modify api converter according with new pipeline (version) definition

14cd856

Change pipeline_store for DefaultVersionId field

e980617

Add pipeline spec to pipeline version

586f5ce

Merge branch 'master' into cicd_storage

8205623

fix model converter

30d9616

fix a comment

bc47bed

k8s-ci-robot assigned IronPan and neuromage Aug 20, 2019

IronPan reviewed Aug 23, 2019

View reviewed changes

Add foreign key, pagination of list request, refactor code source

fc0c1a8

Refactor code source

b835319

IronPan reviewed Aug 28, 2019

View reviewed changes

jingzhang36 added 6 commits September 7, 2019 01:06

Foreign key

0e17a3d

Merge branch 'master' into cicd_storage

899d42b

Change code source and package source type

509a8db

Fix ; separator

fc60a57

Add versions table and modify existing pipeline apis

3d47da3

Merge remote-tracking branch 'origin/master' into cicd_storage

fea90d4

k8s-ci-robot added size/XXL and removed size/L labels Sep 16, 2019

Revise if condition

a3fe5ed

IronPan reviewed Sep 19, 2019

View reviewed changes

k8s-ci-robot requested a review from james-jwu September 20, 2019 02:15

jingzhang36 added 2 commits September 23, 2019 01:09

Address comments

1171e09

address more comments

b95eed2

jingzhang36 added 7 commits September 23, 2019 17:18

Rearrange pipeline and version related parts inside CreatePipeline, t…

a004d96

…o make them more separate.

Merge remote-tracking branch 'origin/master' into cicd_storage

05a7601

Add package url to pipeline version. Required when calling CreatePipe…

cd2d678

…lineVersionRequest

Single code source url; remove pipeline id as sorting field; reformat

7f90853

Merge remote-tracking branch 'origin/master' into cicd_storage

3ea7beb

Merge remote-tracking branch 'origin/cicd_storage' into cicd_storage

0198ab0

resolve remote branch and local branch diff

4a29213

jingzhang36 added 2 commits September 25, 2019 18:11

remove unused func

5e69126

Remove an empty line

0b5eb15

IronPan reviewed Sep 26, 2019

View reviewed changes

k8s-ci-robot added the approved label Sep 26, 2019

k8s-ci-robot added the lgtm label Sep 26, 2019

k8s-ci-robot merged commit 7aaecb1 into kubeflow:master Sep 26, 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

Handle Kubeflow based migrations (kubeflow#1873)

ba651d1

Add necessary data types to api and database to support pipeline version. #1873

Add necessary data types to api and database to support pipeline version. #1873

Conversation

jingzhang36 commented Aug 17, 2019 • edited Loading

jingzhang36 commented Aug 20, 2019

jingzhang36 commented Aug 21, 2019

jingzhang36 commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingzhang36 Aug 26, 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 commented Aug 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingzhang36 commented Sep 19, 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

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 Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingzhang36 Sep 25, 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

jingzhang36 Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingzhang36 Sep 23, 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 Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingzhang36 commented Sep 20, 2019

k8s-ci-robot commented Sep 20, 2019

jingzhang36 commented Sep 23, 2019

jingzhang36 commented Sep 25, 2019

jingzhang36 commented Sep 25, 2019

jingzhang36 commented Sep 25, 2019

Choose a reason for hiding this comment

IronPan commented Sep 26, 2019

k8s-ci-robot commented Sep 26, 2019

IronPan commented Sep 26, 2019

jingzhang36 commented Aug 17, 2019 •

edited

Loading

jingzhang36 Aug 26, 2019 •

edited

Loading

jingzhang36 Sep 20, 2019 •

edited

Loading

jingzhang36 Sep 25, 2019 •

edited

Loading

jingzhang36 Sep 23, 2019 •

edited

Loading

jingzhang36 Sep 23, 2019 •

edited

Loading

jingzhang36 Sep 23, 2019 •

edited

Loading