feat(sdk): Use google.protobuf.Value in v2 for passing parameters. #6804

neuromage · 2021-10-26T17:03:10Z

This PR introduce support for passing parameters using google.protobuf.Value in v2. Previously, parameters were passed using a custom Value proto in the IR which supported int, double and string. Switching to protobuf.google.Value allows us to pass JSON-able Python objects such as lists and dicts, and booleans in addition to scalar string and number values.

We should also look into using null to pass around None values, but I haven't thought this through just yet.

Fix tests
Remove print/debug lines

chensun · 2021-10-26T20:16:19Z

sdk/python/kfp/v2/compiler/compiler.py

+            # Two different types being compared. The only possible types are
+            # String, Boolean, Double and Integer. We'll promote the other type
+            # using the following precedence:
+            # String > Boolean > Double > Integer


I wasn't aware that CEL can work this way.
The rest of the change to support Protobuf.Value in condition operand is also very nice. I'm going to steal your code in my experimental fork :)

Go for it :-) I used this as my reference btw: https://github.com/google/cel-spec/blob/master/doc/langdef.md#list-of-standard-definitions

Update goldens, cleanup, all that stuff.

chensun

/lgtm
/approve

Thanks!

chensun · 2021-10-27T05:05:12Z

api/v2alpha1/python/kfp/pipeline_spec/__init__.py

@@ -11,3 +11,5 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+
+__path__ = __import__("pkgutil").extend_path(__path__, __name__)


I think this is probably unnecessary because kfp-pipeline-spec is a native namespace package when we set it up:

pipelines/api/v2alpha1/python/setup.py

Line 87 in 4360652

packages=setuptools.find_namespace_packages(include=['kfp.*']),

ref: https://packaging.python.org/guides/packaging-namespace-packages/#native-namespace-packages

In comparison, to make the existing kfp package a namespace package, we do have to include this in its init file:

pipelines/sdk/python/kfp/__init__.py

Lines 15 to 17 in 4360652

# `kfp` is a namespace package.

# https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages

__path__ = __import__("pkgutil").extend_path(__path__, __name__)

Also this change is not released, so it can't be needed for this PR, right?

You're right, left over while trying to get both namespace packages to work nicely while locally installed. Removed.

chensun · 2021-10-27T05:07:21Z

@Bobgy , the samples-v2 test has been quite flaky for a while, do you have some bandwidth to look into it?
Also, can you please help take a look to see if the v2-go test failure is something we need to address in this PR?
Thanks!

Bobgy · 2021-10-27T10:06:03Z

The go v2 test fails with

go modules are not tidy, run 'go mod tidy'.

Note, you may need to run the command in the v2 folder.

EDIT: I pushed a commit to fix this.

Bobgy · 2021-10-27T10:09:41Z

@Bobgy , the samples-v2 test has been quite flaky for a while, do you have some bandwidth to look into it?

I will work with @capri-xiyue on this.

Bobgy

Thank you for taking this complex refactoring!
Some nit pickings first.

It's very difficult to review, so I prefer we put getting all tests passing as the only criteria to merge. I will start investigating test failures tomorrow and send PRs to the branch directly.

Bobgy · 2021-10-27T10:42:56Z

api/v2alpha1/cache_key.proto

@@ -26,6 +27,7 @@ message CacheKey {
  map<string, RuntimeArtifact> outputArtifactsSpec = 3;
  map<string, string> outputParametersSpec=4;
  ContainerSpec containerSpec=5;
+  map<string, google.protobuf.Value> input_parameter_values = 6;


nit: the other fields use camel case

Thanks, but proto style guide is explicitly underscore_separated_names for fields. This is what's used everywhere else as well:
https://developers.google.com/protocol-buffers/docs/style#message_and_field_names

My suggestion is to adopt the right style here for long term health, and consider replacing the camel case ones in a future PR.

Bobgy · 2021-10-27T10:51:38Z

sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_loops.py

@@ -27,6 +27,8 @@ def args_generator_op() -> List[str]:
 def print_op(msg: str):
    print(msg)

+    dsl.ParallelFor


nit: is this a typo?

Good catch, fixed.

google-oss-robot · 2021-10-27T20:39:49Z

[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:

~~api/OWNERS~~ [chensun]
~~samples/OWNERS~~ [chensun]
~~sdk/OWNERS~~ [chensun]
~~v2/OWNERS~~ [chensun]

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

chensun · 2021-10-27T20:43:56Z

/lgtm

Thanks!

Bobgy · 2021-10-27T23:38:29Z

There are still some failures with v2 compatible / v2 samples that we need to fix.
I think it's also worth discussing -- how do we develop v2 going forward?
The current pain point seems to be that, when a pipeline spec change is introduced, the SDK and backend have to be changed at the same time, so this one single PR will be huge (and requires someone who knows both SDK and backend to implement).

Will it work out better if the SDK changes can be merged first gated by a temporary hidden flag? If the flag is enabled, all parameters are changed to protobuf.Value.
Then people working on the backend can try to write their PRs separately with the hidden flag turned on in their PR.
When the backend PR is merged, we clean up the hidden flag and legacy code.

Another option I can think of is what we are doing now: let people from SDK/backend work together on a single PR. We can let the PR author give PR branch push permission to other collaborators, so they work together on the PR focusing on SDK/backend.

What do you think?

Bobgy · 2021-10-28T02:33:02Z

I'm examining failure of a v2 engine hello world sample: https://4e18c21c9d33d20f-dot-datalab-vm-staging.googleusercontent.com/#/runs/details/9aafca2c-a958-4e23-85e9-e3c517ead3fb.

…ll refactorings

Bobgy · 2021-10-28T03:51:47Z

Last failure fixed, new failure: https://4e18c21c9d33d20f-dot-datalab-vm-staging.googleusercontent.com/#/runs/details/c3721d7a-83b5-4742-9efe-c171205bd366

Bobgy · 2021-10-28T07:26:48Z

v2 hello world is passing: https://4e18c21c9d33d20f-dot-datalab-vm-staging.googleusercontent.com/#/runs/details/85863efb-ff41-43a3-b506-b7aac70982c7
Some tests are still failing.

chensun · 2021-10-28T17:59:02Z

/lgtm

/test kubeflow-pipelines-samples-v2

chensun · 2021-10-28T18:08:58Z

/test kubeflow-pipelines-samples-v2

…protobuf-value

chensun · 2021-10-28T21:21:28Z

/lgtm

Thank you, @neuromage and @Bobgy

…ubeflow#6804) * Use google.protobuf.Value in v2 for passing parameters. * retest samples. * Fix tests. * Update release, more cleanup. * Use github.com/kubeflow/pipelines/api from same repo. * Run go mod tidy * chore: go mod tidy * fix v2 compile error and clean up unused code * pr comments. * update goldens * Fix metadata recording. * Update kfp mlmd client. * fix test again * another try. * chore: migrate v2 DAG driver input parameters to protobuf.Value + small refactorings * fix v2 launcher + clean up * fix a compile error * fix a few more tests * fix number parsing * clean up * disable cache_v2 test. Co-authored-by: Yuan Gong <[email protected]>

Use google.protobuf.Value in v2 for passing parameters.

4966b0b

neuromage requested review from chensun and capri-xiyue October 26, 2021 17:03

google-cla bot added the cla: yes label Oct 26, 2021

google-oss-robot added the size/XL label Oct 26, 2021

google-oss-robot requested review from Bobgy and ji-yaqi October 26, 2021 17:03

neuromage changed the title ~~feat(sdk): Use google.protobuf.Value in v2 for passing parameters.~~ [WIP] feat(sdk): Use google.protobuf.Value in v2 for passing parameters. Oct 26, 2021

google-oss-robot added the do-not-merge/work-in-progress label Oct 26, 2021

chensun reviewed Oct 26, 2021

View reviewed changes

retest samples.

8f14fb1

google-oss-robot added size/XXL and removed size/XL labels Oct 26, 2021

neuromage added 3 commits October 26, 2021 17:20

Fix tests.

728f922

Merge branch 'master' into protobuf-value.

5b13589

Update goldens, cleanup, all that stuff.

Update release, more cleanup.

cf3407d

neuromage changed the title ~~[WIP] feat(sdk): Use google.protobuf.Value in v2 for passing parameters.~~ feat(sdk): Use google.protobuf.Value in v2 for passing parameters. Oct 27, 2021

google-oss-robot removed the do-not-merge/work-in-progress label Oct 27, 2021

neuromage added 2 commits October 26, 2021 19:44

Use github.com/kubeflow/pipelines/api from same repo.

7e6d18f

Run go mod tidy

bde385f

chensun reviewed Oct 27, 2021

View reviewed changes

google-oss-robot assigned chensun Oct 27, 2021

google-oss-robot added lgtm approved labels Oct 27, 2021

Bobgy reviewed Oct 27, 2021

View reviewed changes

chore: go mod tidy

ac5f6e3

google-oss-robot removed the lgtm label Oct 27, 2021

neuromage added 3 commits October 27, 2021 12:27

update goldens

9b5e012

Fix metadata recording.

c8d49a2

Update kfp mlmd client.

de1f943

google-oss-robot added the lgtm label Oct 27, 2021

fix test again

bfc380b

google-oss-robot removed the lgtm label Oct 27, 2021

another try.

ad3e7f9

chore: migrate v2 DAG driver input parameters to protobuf.Value + sma…

917d979

…ll refactorings

Bobgy added 2 commits October 28, 2021 06:32

fix v2 launcher + clean up

b0718ec

fix a compile error

1df554a

Bobgy added 3 commits October 28, 2021 08:43

fix a few more tests

24360f2

fix number parsing

7b9a681

clean up

547747e

google-oss-robot added the lgtm label Oct 28, 2021

neuromage added 2 commits October 28, 2021 13:33

disable cache_v2 test.

f2426b0

Merge branch 'protobuf-value' of github.com:neuromage/pipelines into …

35f8448

…protobuf-value

google-oss-robot removed the lgtm label Oct 28, 2021

google-oss-robot added the lgtm label Oct 28, 2021

google-oss-robot merged commit 0be57c3 into kubeflow:master Oct 28, 2021

capri-xiyue mentioned this pull request Oct 29, 2021

[bug] reenable cache v2 and importer v2 e2e test #6838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): Use google.protobuf.Value in v2 for passing parameters. #6804

feat(sdk): Use google.protobuf.Value in v2 for passing parameters. #6804

neuromage commented Oct 26, 2021 •

edited

Loading

chensun Oct 26, 2021

neuromage Oct 27, 2021

chensun left a comment

chensun Oct 27, 2021

neuromage Oct 27, 2021

chensun commented Oct 27, 2021

Bobgy commented Oct 27, 2021 •

edited

Loading

Bobgy commented Oct 27, 2021

Bobgy left a comment

Bobgy Oct 27, 2021

neuromage Oct 27, 2021

Bobgy Oct 27, 2021

neuromage Oct 27, 2021

google-oss-robot commented Oct 27, 2021

chensun commented Oct 27, 2021

Bobgy commented Oct 27, 2021 •

edited

Loading

Bobgy commented Oct 28, 2021

Bobgy commented Oct 28, 2021

Bobgy commented Oct 28, 2021

chensun commented Oct 28, 2021

chensun commented Oct 28, 2021

chensun commented Oct 28, 2021 •

edited

Loading

	# `kfp` is a namespace package.
	# https://packaging.python.org/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages
	__path__ = __import__("pkgutil").extend_path(__path__, __name__)

feat(sdk): Use google.protobuf.Value in v2 for passing parameters. #6804

feat(sdk): Use google.protobuf.Value in v2 for passing parameters. #6804

Conversation

neuromage commented Oct 26, 2021 • edited Loading

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

chensun commented Oct 27, 2021

Bobgy commented Oct 27, 2021 • edited Loading

Bobgy commented Oct 27, 2021

Bobgy 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

google-oss-robot commented Oct 27, 2021

chensun commented Oct 27, 2021

Bobgy commented Oct 27, 2021 • edited Loading

Bobgy commented Oct 28, 2021

Bobgy commented Oct 28, 2021

Bobgy commented Oct 28, 2021

chensun commented Oct 28, 2021

chensun commented Oct 28, 2021

chensun commented Oct 28, 2021 • edited Loading

neuromage commented Oct 26, 2021 •

edited

Loading

Bobgy commented Oct 27, 2021 •

edited

Loading

Bobgy commented Oct 27, 2021 •

edited

Loading

chensun commented Oct 28, 2021 •

edited

Loading