feat(backend): Implement RuntimeConfig in backend #8085

Linchin · 2022-07-30T08:24:36Z

Description of your changes:

Implemented for runs only.

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.

IronPan · 2022-08-03T16:43:47Z

backend/src/apiserver/resource/model_converter.go

 		runDetail.PipelineSpecManifest = manifest
+		runDetail.PipelineSpec.RuntimeConfig.Parameters = params
+		runDetail.PipelineSpec.RuntimeConfig.PipelineRoot = run.GetPipelineSpec().GetRuntimeConfig().GetPipelineRoot()


could you gofmt the code?

IronPan · 2022-08-03T16:46:17Z

backend/src/apiserver/model/pipeline_spec.go

@@ -32,4 +32,7 @@ type PipelineSpec struct {

 	// Store parameters key-value pairs as serialized string.
 	Parameters string `gorm:"column:Parameters; size:65535"`
+
+	// Runtime config of the pipeline
+	RuntimeConfig


nit - can the parameters be stored in the field above, and just add a new field for storing pipeline root?
the DB schema doesn't have to be 1:1 mapping with API schema

and no matter which way to go, call out in the comment which fields are v1 only and v2 only.

This was discussed in the design review. The main reason behind it is to eliminate the confusion between v1 and v2 parameters. I also added comments to clarify the difference between the two Parameters fields.

IronPan · 2022-08-03T16:49:50Z

backend/src/apiserver/resource/model_converter.go

+	for key := range runtimeConfig.GetParameters() {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)


why sorting the keys?

Since the Parameters is a map, the marshalled JSON may be of any random order. I sorted the keys for mainly two reasons:

To make comparisons of strings in testing possible

Ensure the marshalled JSONs are the same whenever the Parameters are the same

for #1, as a eng practice, the test shouldn't affect the source code. can you unmarshal the string field and compare without ordering in test code?
for #2, is that necessary for the BE to work properly?

+1 to both points raised by Yang.

In fact, I don't see how sorting the list of keys affects the serialization of the dict.

for #1, since we are marshalling the dict into a string, the key-value pairs may be marshalled in any random order. This makes the resulting string possibly different every time, thus making testing by comparing the strings directly impossible. An alternative to comparing strings is to unmarshal the strings back into maps, and compare the maps instead. WDYT?

yeah i think that's one way to do this correctly.

IronPan · 2022-08-03T17:00:35Z

backend/src/apiserver/resource/model_converter.go

@@ -253,28 +256,31 @@ func runtimeConfigToModelParameters(runtimeConfig *api.PipelineSpec_RuntimeConfi
 		return "", nil
 	}
 	var params []v1alpha1.Parameter


the code is confusing to me. the runtime config is a v2 proto, but here we marshall the runtime config into a string through a v1alpha1 API proto.

Can we start refactoring these code and separate out the v1 and v2 code path, making them hermetically avoiding referring to the proto in the other version?

Yes, that's a good point! I will update the code accordingly.

api/v2alpha1/go/pipelinespec/pipeline_spec.pb.go

chensun · 2022-08-04T07:17:49Z

backend/src/apiserver/resource/model_converter.go

+	for key := range runtimeConfig.GetParameters() {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)


+1 to both points raised by Yang.

In fact, I don't see how sorting the list of keys affects the serialization of the dict.

backend/src/apiserver/server/api_converter.go

chensun · 2022-08-04T07:28:03Z

backend/src/apiserver/server/api_converter.go

@@ -148,14 +152,45 @@ func toApiParameters(paramsString string) ([]*api.Parameter, error) {
 	return apiParams, nil
 }

+func toApiRuntimeParameters(paramsString string) (map[string]*structpb.Value, error) {


We should have unit tests for newly added methods.

There is a new unit test involving this method, though there is no individual unit test for this single method. Should I add unit tests at this level?

chensun · 2022-08-04T07:31:37Z

backend/src/apiserver/server/api_converter.go

@@ -148,14 +152,45 @@ func toApiParameters(paramsString string) ([]*api.Parameter, error) {
 	return apiParams, nil
 }

+func toApiRuntimeParameters(paramsString string) (map[string]*structpb.Value, error) {
+	if paramsString == "" {
+		return make(map[string]*structpb.Value), nil


Is it necessary to return an empty dict?

chensun · 2022-08-04T07:34:14Z

backend/src/apiserver/server/api_converter.go

@@ -148,14 +152,45 @@ func toApiParameters(paramsString string) ([]*api.Parameter, error) {
 	return apiParams, nil
 }

+func toApiRuntimeParameters(paramsString string) (map[string]*structpb.Value, error) {


Also we talked about this offline, why not just have toApiRuntimeConfig that returns api.PipelineSpec_RuntimeConfig?

We indeed talked about it offline, but that was for parseXXX functions in run_store.go (link). Here toApiRuntimeParameters() is analogous to toApiParameters() above (link).

chensun · 2022-08-04T07:36:34Z

backend/src/apiserver/server/run_server_test.go

@@ -81,6 +86,71 @@ func TestCreateRun(t *testing.T) {
 	assert.Equal(t, expectedRunDetail, *runDetail)
 }

+/*


chensun · 2022-08-04T08:34:48Z

backend/src/apiserver/server/api_converter.go

+	}
+	runtimeParams := make(map[string]*structpb.Value)
+	for k, v := range runtimeParamsStringsMap {
+		protoValue := structpb.NewStringValue("")


why NewStringValue?

Just to initialize the variable to be later marshalled into.

chensun · 2022-08-04T08:38:23Z

backend/src/apiserver/server/api_converter_test.go

+			WorkflowManifest: "workflow123",
+		},
+	}
+	assert.Equal(t, expectedApiRun.String(), apiRun.String())


no need for Sting()

Added explanation for usage of String(). Some fields in ApiRuns are only used for protobuff, and may be different although the fields we use are the same. Using string representation avoids the issue.

This reverts commit 251c3a9.

chensun

/lgtm

backend/src/apiserver/server/api_converter.go

chensun · 2022-08-05T22:24:04Z

backend/src/apiserver/server/api_converter_test.go

+
+	// Test all parameters types converted to model.RuntimeConfig.Parameters, which is string type
+	v2RuntimeParams := map[string]*structpb.Value{
+		"param2": structpb.NewStringValue("world"),


nit: why starting with 2?

I'm not sure why, but all test cases in api_converter_test.go have parameters starting at 2.

Not a big deal yet it just reads odd. I really don't see why this needs to be repeated or reinforced.

backend/src/apiserver/server/api_converter_test.go

chensun

/lgtm

chensun · 2022-08-08T23:39:34Z

backend/src/apiserver/server/api_converter_test.go

+
+	// Test all parameters types converted to model.RuntimeConfig.Parameters, which is string type
+	v2RuntimeParams := map[string]*structpb.Value{
+		"param2": structpb.NewStringValue("world"),


Not a big deal yet it just reads odd. I really don't see why this needs to be repeated or reinforced.

IronPan · 2022-08-10T05:58:05Z

backend/src/apiserver/resource/model_converter.go

 		modelJob.PipelineSpecManifest = manifest
+		modelJob.PipelineSpec.RuntimeConfig.Parameters = params
+		modelJob.PipelineSpec.RuntimeConfig.PipelineRoot = job.GetPipelineSpec().GetRuntimeConfig().GetPipelineRoot()


could you gofmt the code?

I did gofmt for the single file but nothing changed. I will create a new PR to run the command over the entire backend codebase, so we only have changes related to runtimeconfig in this PR.

IronPan · 2022-08-10T06:02:41Z

backend/src/apiserver/resource/model_converter.go

@@ -253,34 +255,19 @@ func runtimeConfigToModelParameters(runtimeConfig *api.PipelineSpec_RuntimeConfi
 	if runtimeConfig == nil {
 		return "", nil
 	}
-	var params util.SpecParameters
+	// Use structpb to marshal protobuff value, store them in a map, then marshal the entire map into a single new string


I wonder if this is this necessary. Can you just json marshal the runtimeConfig.Parameters of type map[string]*structpb.Value without marshaling the value first?

That's right. I updated the code accordingly.

IronPan · 2022-08-10T06:06:26Z

backend/src/apiserver/server/api_converter.go

+		runtimeParams = make(map[string]*structpb.Value)
+		for k, v := range runtimeParamsStringsMap {
+			var protoValue structpb.Value
+			err := protoValue.UnmarshalJSON([]byte(v))
+			if err != nil {
+				return nil, util.NewInternalServerError(err, fmt.Sprintf("Cannot unmarshal string %+v to structpb.Value", v))
+			}
+			runtimeParams[k] = &protoValue


same here, for unmarshal, is it possible to do it in 1 step instead of unmarshal the value first then unmarshal the map?

I don't think it was necessary. I have updated the code accordingly.

IronPan · 2022-08-10T06:15:39Z

backend/src/apiserver/server/api_converter.go

+	// v1 parameters
 	params, err := toApiParameters(run.Parameters)
 	if err != nil {
 		return &api.Run{
 			Id:    run.UUID,
 			Error: err.Error(),
 		}
 	}
+	// v2 RuntimeConfig
+	runtimeConfig, err := toApiRuntimeConfig(run.PipelineSpec.RuntimeConfig)
+	if err != nil {
+		return &api.Run{
+			Id:    run.UUID,
+			Error: err.Error(),
+		}
+	}


I'm still feeling a bit strong about this.
we should really create a new v2 pipeline_spec.proto instead of reusing v1 API in
https://github.com/kubeflow/pipelines/blob/master/backend/api/pipeline_spec.proto

To note, anything we released to post Beta is subject to the formal deprecation policy. If I understand correctly this will be released as part of the next V1 release which is already GA'ed? (Or is the API proto already released?)

that being said i'm ok with merge this and do a quick follow up PR to properly break down the code before the next backward compatible promised release.

Yes, the current API that has been released (which is going to be V1 going forward) already contains RuntimeConfig field. FE uses RuntimeConfig to pass parameters and pipeline roots, where only parameters are stored in the backend DB. Then backend returns the run without using the RuntimeConfig field. The change here just makes sure the RuntimeConfig info coming from UI is stored, and then can be returned via RuntimeConfig. Shall we remove RuntimeConfig from V1 entirely after releasing V2?

Yes let's call out that in the proto comment, and do that when adding v2 api.

IronPan · 2022-08-11T22:19:49Z

/lgtm
/approve

google-oss-prow · 2022-08-11T22:20:22Z

[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

* add runtime config model * add run store and tests * add runtime_config to model converter and tests * add runtime_config to api converter and its tests * change api server and related tests * remove v2 runtime_config test * add runtimeconfig to upgrade test * fix test values * upgrade test debug * tests * add more info for debug * use NullString instead of String, remove debug * fix type conversion * change function and add unit tests * run go fmt * Add comments for model * marshal params using v2 structpb values * fix small bug * Revert "run go fmt" This reverts commit 251c3a9. * No longer sort keys * test values and explain comparison using .String() * func toApiRuntimeConfig * tests updates * add api converter tests * change test * fix format * change test * simplify marshalling parameters

Linchin added 2 commits July 30, 2022 07:37

add runtime config model

b25feef

add run store and tests

22c2898

google-oss-prow bot requested review from chensun and zijianjoy July 30, 2022 08:24

google-oss-prow bot added the size/L label Jul 30, 2022

Linchin added 12 commits July 30, 2022 08:31

add runtime_config to model converter and tests

6686090

add runtime_config to api converter and its tests

3818b7f

change api server and related tests

ed7c8f7

remove v2 runtime_config test

4ca2b65

add runtimeconfig to upgrade test

991e2df

fix test values

81fa878

upgrade test debug

e0d299e

tests

96a0ef7

add more info for debug

f222a2b

use NullString instead of String, remove debug

caaa994

fix type conversion

a02bac7

change function and add unit tests

555b2a9

google-oss-prow bot added size/XL and removed size/L labels Aug 2, 2022

IronPan reviewed Aug 3, 2022

View reviewed changes

Linchin added 3 commits August 3, 2022 19:06

run go fmt

251c3a9

Add comments for model

7efead2

marshal params using v2 structpb values

d407102

google-oss-prow bot added size/XXL and removed size/XL labels Aug 4, 2022

fix small bug

d608da4

chensun requested changes Aug 4, 2022

View reviewed changes

Revert "run go fmt"

562aad8

This reverts commit 251c3a9.

google-oss-prow bot added size/XL and removed size/XXL labels Aug 4, 2022

Linchin added 4 commits August 4, 2022 23:54

tests updates

af1b682

add api converter tests

b7f0a03

Merge remote-tracking branch 'origin/master'

461a508

change test

b2e5bdc

Linchin requested review from IronPan and chensun August 5, 2022 19:49

chensun reviewed Aug 5, 2022

View reviewed changes

google-oss-prow bot assigned chensun Aug 5, 2022

google-oss-prow bot added the lgtm label Aug 5, 2022

fix format

916de30

google-oss-prow bot removed the lgtm label Aug 8, 2022

Linchin added 2 commits August 8, 2022 22:10

Merge remote-tracking branch 'origin/master'

92b7595

change test

ef4e588

chensun reviewed Aug 8, 2022

View reviewed changes

google-oss-prow bot added the lgtm label Aug 8, 2022

IronPan reviewed Aug 10, 2022

View reviewed changes

simplify marshalling parameters

e5d08c1

google-oss-prow bot removed the lgtm label Aug 10, 2022

Linchin requested review from chensun and IronPan August 11, 2022 00:26

google-oss-prow bot assigned IronPan Aug 11, 2022

google-oss-prow bot added the lgtm label Aug 11, 2022

google-oss-prow bot added the approved label Aug 11, 2022

google-oss-prow bot merged commit 2b556ec into master Aug 11, 2022

google-oss-prow bot deleted the lingqing-runtimeconfig-0730 branch August 11, 2022 23:21

Linchin mentioned this pull request Aug 16, 2022

feat(backend): Implement RuntimeConfig in backend for jobs(RecurringRun) #8152

Merged

1 task

feat(backend): Implement RuntimeConfig in backend #8085

feat(backend): Implement RuntimeConfig in backend #8085

Conversation

Linchin commented Jul 30, 2022 • 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

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

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

chensun 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

chensun 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IronPan commented Aug 11, 2022

google-oss-prow bot commented Aug 11, 2022

Linchin commented Jul 30, 2022 •

edited

Loading