fix(backend): fix simple loop bug #7578

Linchin · 2022-04-18T22:36:27Z

Description of your changes:
Fix bug that simple loop does not work. It only does not work when there is static input.

Note: need to change line #110 in backend/src/v2/compiler/argocompiler/argo.go to use the ml-pipeline-test gcr address.

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.

chensun · 2022-04-19T17:08:25Z

backend/src/v2/compiler/argocompiler/argo.go

@@ -107,7 +107,7 @@ func Compile(jobArg *pipelinespec.PipelineJob, opts *Options) (*wfapi.Workflow,
 		wf:        wf,
 		templates: make(map[string]*wfapi.Template),
 		// TODO(chensun): release process and update the images.
-		driverImage:   "gcr.io/ml-pipeline-test/dev/kfp-driver:latest",
+		driverImage:   "gcr.io/ling-kfp/dev/kfp-driver:latest",


This should exist only in your local change. And this seems to be the cause of the unit test failure in presubmit.

Should I upload my driver image onto the "gcr.io/ml-pipeline-test/dev/kfp-driver:latest" address? If I don't do that, the presubmit test would not pass.

Not yet. Was it failing on samples-v2 test in presubmit? In that case, we can update the image when this PR is close the ready to merge state.

Reverted it.

chensun · 2022-04-19T17:10:45Z

backend/src/v2/driver/driver.go

-			return execution, report(fmt.Errorf("cannot find input parameter"))
+		// Check the items type of parameterIterator:
+		// It can be "inputParameter" or "Raw"
+		value := structpb.NewNullValue()


nit: this null value initialization seems to be unnecessary?

I think it is necessary. The variable needs to be declared outside the switch/case statement, then it can be assigned value inside the switch/case statements. If the variables were declared inside, it would not be accessible outside the statement. (Ref: https://stackoverflow.com/questions/21481288/variable-not-declared-inside-a-if-else-statement)

The variable needs to be declared outside the switch/case statement, then it can be assigned value inside the switch/case statements.

You can still declare it inside a switch case/branch if it's not reused across multiple branches and not used outside the switch scope--didn't check if that's the case.
That said, my comments was more on the initialization part than the declaration part.

It is being used outside the switch scope, so I guess it needs to be declared outside as well. Although I don't have to assign it a null value - I can just declare its type, I don't find a method in structpb that just declares a Variable type. It looks like they need to initialize some value inside the variable.

It turned out that I can use var value *structpb.Value for the declaration.

chensun · 2022-04-19T17:49:01Z

backend/src/v2/driver/driver.go

+		// Check the items type of parameterIterator:
+		// It can be "inputParameter" or "Raw"
+		value := structpb.NewNullValue()
+		var ok bool


nit: probably more common to use short variable declaration. E.g.:

value, ok := ... if !ok { ... }

or

if value, ok := ...; !ok { ... }

Got it, I will declare value and ok in the same line.

Remove the declaration for ok?

The problem is, in line 454 (just 4 lines below), we need to assign values to both value and ok. Thus ok has to be created beforehand.

No, you could have

var value *structpb.Value value, ok := ...

Short variable declaration works in this case.

It still reports error though,

What error specifically?

probably because value, ok := ... line is not directly under the declaration of value, but inside a switch/case.

I don't think this would matter.
You can probably play around here: https://go.dev/tour/basics/10

See an example here: https://go.dev/play/p/-LdL96A5GUV
If I use the := declaration inside the switch/case block, the value of value that was assigned inside won't be accessible from outside.

That's expected. But we were saying to keep the value declaration as-is (we already settled on that one from another comment where I asked to remove the null value assignment and only keep the declaration). The comment above was to omit the ok declaration on this line, because I don't think you need to read ok outside the switch.

Does that mean I declare ok inside the switch?

you're right on the value part, just a nitpick let's still move ok declaration inside the switch so it's close to where it is used.

backend/src/v2/driver/driver.go

chensun · 2022-04-19T17:56:23Z

backend/src/v2/driver/driver.go

+		var ok bool
+		switch iterator.GetItems().GetKind().(type) {
+		case *pipelinespec.ParameterIteratorSpec_ItemsSpec_InputParameter:
+			glog.Infof("ParameterIterator type: %T", iterator.GetItems().GetKind())


nit: feels to me a bit excessive. Consider reducing some logs? (They may be helpful for debug process, but would they be necessary to log?)

Makes sense! Will remove these.

remove this and the below logs?

OK, I will remove them.

chensun · 2022-04-19T17:57:14Z

backend/src/v2/driver/driver.go

+			if !ok {
+				return execution, report(fmt.Errorf("cannot find input parameter"))
+			}
+			glog.Infof("inputParameter value: %+v", value)


ditto: excessive logs (and below)

backend/src/v2/driver/driver.go

Linchin · 2022-04-19T23:19:03Z

@chensun Updated the PR

change the driver image back to the kfp container registry.

Linchin · 2022-04-20T21:11:08Z

@chensun updated the PR

Linchin · 2022-04-29T21:35:33Z

/retest

chensun

/lgtm
/approve

Thanks!

google-oss-prow · 2022-05-04T18:43:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

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~~ [chensun]
~~samples/OWNERS~~ [chensun]

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

Linchin · 2022-05-04T20:23:54Z

\retest

Linchin · 2022-05-04T20:50:54Z

/test kubeflow-pipelines-samples-v2

Linchin · 2022-05-05T01:09:38Z

/test kubeflow-pipelines-samples-v2

* support IR YAML format in API * Check the error message and return false if it is not nil * update error message * fixed simple loop but need cleaning up * Deleted debug logs * remove logs and fix some format * fix static_loop_arguments * change the driver image change the driver image back to the kfp container registry. * change variable declaration * remove logs * remove log * move `ok` definition * change test file for debug purpose * change test for debug purpose * update sample test for static loop * update test file, remove code for debug

Linchin and others added 7 commits March 31, 2022 20:20

support IR YAML format in API

31f0cc2

Check the error message and return false if it is not nil

bf63df8

Merge branch 'kubeflow:master' into lingqing

b01b26b

update error message

4d97abf

Merge branch 'kubeflow:master' into lingqing

9046f9b

fixed simple loop but need cleaning up

927d025

Deleted debug logs

5276047

google-oss-prow bot added the size/M label Apr 18, 2022

google-oss-prow bot requested review from chensun and zijianjoy April 18, 2022 22:36

Linchin changed the title ~~Lingqing~~ fix(backend): fix simple loop bug Apr 18, 2022

chensun reviewed Apr 19, 2022

View reviewed changes

Linchin added 2 commits April 19, 2022 23:00

remove logs and fix some format

483248f

Merge branch 'kubeflow:master' into lingqing

f42c3cf

chensun mentioned this pull request Apr 20, 2022

fix(sample): fix static_loop_arguments #7588

Closed

1 task

Linchin added 5 commits April 20, 2022 16:36

Merge branch 'kubeflow:master' into lingqing

07d2dc1

Merge branch 'kubeflow:master' into lingqing

85cdfbf

fix static_loop_arguments

51c6c8c

change the driver image

02eb7b1

change the driver image back to the kfp container registry.

change variable declaration

04d5b2c

Linchin added 4 commits April 20, 2022 22:20

remove logs

ca6def8

remove log

4ebab0a

Merge branch 'kubeflow:master' into lingqing

21bbae3

move ok definition

a48e3ba

Linchin added 3 commits May 3, 2022 20:35

change test file for debug purpose

bafad39

change test for debug purpose

a04c8cf

update sample test for static loop

e5d8c4b

update test file, remove code for debug

06e6402

chensun reviewed May 4, 2022

View reviewed changes

google-oss-prow bot assigned chensun May 4, 2022

google-oss-prow bot added the lgtm label May 4, 2022

google-oss-prow bot added the approved label May 4, 2022

google-oss-prow bot merged commit 1180e10 into kubeflow:master May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(backend): fix simple loop bug #7578

fix(backend): fix simple loop bug #7578

Linchin commented Apr 18, 2022 •

edited

Loading

chensun Apr 19, 2022

Linchin Apr 19, 2022 •

edited

Loading

chensun Apr 19, 2022

Linchin Apr 20, 2022

chensun Apr 19, 2022

Linchin Apr 19, 2022

chensun Apr 19, 2022

Linchin Apr 19, 2022

Linchin Apr 21, 2022

chensun Apr 19, 2022

Linchin Apr 19, 2022

chensun Apr 20, 2022

Linchin Apr 20, 2022 •

edited

Loading

chensun Apr 21, 2022

chensun Apr 21, 2022

Linchin Apr 21, 2022

chensun Apr 22, 2022

Linchin Apr 22, 2022

chensun Apr 22, 2022

chensun Apr 19, 2022

Linchin Apr 19, 2022

chensun Apr 20, 2022

Linchin Apr 20, 2022

chensun Apr 19, 2022

Linchin commented Apr 19, 2022

Linchin commented Apr 20, 2022

Linchin commented Apr 29, 2022

chensun left a comment

google-oss-prow bot commented May 4, 2022

Linchin commented May 4, 2022

Linchin commented May 4, 2022

Linchin commented May 5, 2022

fix(backend): fix simple loop bug #7578

fix(backend): fix simple loop bug #7578

Conversation

Linchin commented Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

Linchin Apr 19, 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

Linchin Apr 20, 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

Linchin commented Apr 19, 2022

Linchin commented Apr 20, 2022

Linchin commented Apr 29, 2022

chensun left a comment

Choose a reason for hiding this comment

google-oss-prow bot commented May 4, 2022

Linchin commented May 4, 2022

Linchin commented May 4, 2022

Linchin commented May 5, 2022

Linchin commented Apr 18, 2022 •

edited

Loading

Linchin Apr 19, 2022 •

edited

Loading

Linchin Apr 20, 2022 •

edited

Loading