feat(sdk): Add BaseModel to component_spec data classes #6372

ji-yaqi · 2021-08-17T22:48:20Z

No description provided.

…ve_component

ji-yaqi · 2021-08-18T05:05:18Z

/retest

chensun

Thanks Yaqi. Left a couple nitpicks but overall looks good.

sdk/python/kfp/v2/compiler_cli_tests/test_data/experimental_v2_component.json

sdk/python/kfp/v2/components/experimental/base_component_test.py

sdk/python/kfp/v2/components/experimental/component_spec_test.py

sdk/python/kfp/v2/components/experimental/component_spec.py

chensun · 2021-08-18T07:29:39Z

sdk/python/kfp/v2/components/experimental/component_spec_test.py

+    default: null
+    type: str
+labels: null
+name: component_1


nit: can be done in a later PR, but the name better be on the top. (likely we need some custom sort logic).

sdk/python/kfp/v2/components/experimental/component_spec.py

chensun · 2021-08-23T07:28:23Z

sdk/python/kfp/components/_data_passing.py

-    Converter([bool], ['Boolean', 'Bool', 'bool'], _serialize_bool, _bool_deserializer_code, _bool_deserializer_definitions),
-    Converter([list], ['JsonArray', 'List', 'list'], _serialize_json, 'json.loads', 'import json'), # ! JSON map keys are always strings. Python converts all keys to strings without warnings
-    Converter([dict], ['JsonObject', 'Dictionary', 'Dict', 'dict'], _serialize_json, 'json.loads', 'import json'), # ! JSON map keys are always strings. Python converts all keys to strings without warnings
+    Converter([str,'str'], ['String', 'str'], _serialize_str, 'str', None),


Is this change needed?

Yes, this is needed to convert the type 'str' to actual type 'String'. Otherwise, the input as 'str' will be identified as None type

I'm not sure if this is the right place to change though. It looks to me the code build around this won't really make use of the added "type".

For example, type_name_to_type will only use the first item in this list:

pipelines/sdk/python/kfp/components/_data_passing.py

Line 122 in bdb6814

type_name_to_type = {type_name: converter.types[0] for converter in _converters for type_name in converter.type_names if converter.types}

And for type_to_type_name, you'll get both {str, 'String'} and {'str': 'String'}

pipelines/sdk/python/kfp/components/_data_passing.py

Line 121 in bdb6814

type_to_type_name = {typ: converter.type_names[0] for converter in _converters for typ in converter.types}

But then the key 'str' (with quotes) will never be used because type(value) will never return 'str'.

pipelines/sdk/python/kfp/components/_data_passing.py

Line 169 in bdb6814

type_name = type_to_type_name.get(type(value), type(value).__name__)

I assume you got an error without changing this? maybe we can sync offline and take a look at the callstack to see if there's a better way to address that.

chensun · 2021-08-23T07:29:59Z

sdk/python/kfp/v2/components/experimental/base_component_test.py

-            component_spec.OutputSpec(name='output1', type=str),
-        ],
+        inputs={
+            'input1': component_spec.InputSpec(type="str"),


nit: be consistent with quote sign.

chensun · 2021-08-24T07:39:54Z

sdk/python/kfp/components/_data_passing.py

-    Converter([bool], ['Boolean', 'Bool', 'bool'], _serialize_bool, _bool_deserializer_code, _bool_deserializer_definitions),
-    Converter([list], ['JsonArray', 'List', 'list'], _serialize_json, 'json.loads', 'import json'), # ! JSON map keys are always strings. Python converts all keys to strings without warnings
-    Converter([dict], ['JsonObject', 'Dictionary', 'Dict', 'dict'], _serialize_json, 'json.loads', 'import json'), # ! JSON map keys are always strings. Python converts all keys to strings without warnings
+    Converter([str,'str'], ['String', 'str'], _serialize_str, 'str', None),


I'm not sure if this is the right place to change though. It looks to me the code build around this won't really make use of the added "type".

For example, type_name_to_type will only use the first item in this list:

pipelines/sdk/python/kfp/components/_data_passing.py

Line 122 in bdb6814

type_name_to_type = {type_name: converter.types[0] for converter in _converters for type_name in converter.type_names if converter.types}

And for type_to_type_name, you'll get both {str, 'String'} and {'str': 'String'}

pipelines/sdk/python/kfp/components/_data_passing.py

Line 121 in bdb6814

type_to_type_name = {typ: converter.type_names[0] for converter in _converters for typ in converter.types}

But then the key 'str' (with quotes) will never be used because type(value) will never return 'str'.

pipelines/sdk/python/kfp/components/_data_passing.py

Line 169 in bdb6814

type_name = type_to_type_name.get(type(value), type(value).__name__)

I assume you got an error without changing this? maybe we can sync offline and take a look at the callstack to see if there's a better way to address that.

chensun · 2021-08-24T07:40:00Z

sdk/python/kfp/v2/components/experimental/base_component_test.py

@@ -80,7 +80,7 @@ def test_instantiate_component_omitting_arguments_with_default(
        arguments={
            'input1': 'hello',
            'input2': 100,
-            'input3': 3.14,
+            'input3': '3.14',


why is this a string? (yet the integer default is not a string)

As discussed offline, it's a default field which gets parsed to string originally, will think about future improvement.

It might be a good idea to only support string-serialized data here.
This data must be serialized at some point to a string that becomes a part of the command-line or a file.
It might be better to serialize as early as possible.
Otherwise, the serialization happens every time the pipeline is executed and the serialized data can be different every time (e.g. we Python <3.6 was serializing dicts to JSON with random key ordering).

This applies to default values as well. KFP v1's create_component_from_func serializes all default values to strings at the time of the component creation. So every component instance in every pipeline has the same serialized value.

chensun · 2021-08-24T07:41:47Z

sdk/python/kfp/v2/components/experimental/component_spec.py

-  default: Optional[Any] = None
+  # TODO(ji-yaqi): Add logic to cast default value into the specified type.
+  type: str
+  default: Union[str, int, float, bool, dict, list] = None


nit: Optional[Union[...]] = None

Optional[...] == Union[..., None]

chensun · 2021-08-24T07:43:56Z

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

-            component_spec.OutputSpec(name='output1', type=str),
-        ],
+        inputs={
+            'input1': component_spec.InputSpec(type="str")


nit: single quote for consistency.

chensun

/lgtm

chensun · 2021-08-25T06:38:42Z

sdk/python/kfp/v2/components/experimental/base_component_test.py

+        inputs={
+            'input1': component_spec.InputSpec(type='String'),
+            'input2': component_spec.InputSpec(type='int'),
+            'input3': component_spec.InputSpec(type='float', default=3.14),


nit: maybe Integer?

chensun · 2021-08-25T20:47:32Z

/lgtm

ji-yaqi · 2021-08-25T20:59:42Z

/approve

google-oss-robot · 2021-08-25T20:59:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ji-yaqi

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:

~~sdk/OWNERS~~ [ji-yaqi]

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

Ark-kun · 2021-08-26T02:59:05Z

sdk/python/kfp/v2/components/experimental/component_spec.py

-    raise NotImplementedError
+    with open(output_file, 'a') as output_file:
+        json_component = self.json(exclude_unset=True, exclude_none=True)
+        yaml_file = yaml.safe_dump(json.loads(json_component))


Better use kfp.components._yaml_utils.dump_yaml(...)

pipelines/sdk/python/kfp/components/_yaml_utils.py

Line 33 in 8f10bcd

def dump_yaml(data):

Ark-kun · 2021-08-26T03:01:01Z

sdk/python/kfp/v2/components/experimental/component_spec_test.py

+    open_mock = mock_open()
+    expected_yaml = textwrap.dedent("""\
+        implementation:
+          commands:


Kubernetes and Google Cloud use command + args naming.

Ark-kun · 2021-08-26T03:03:10Z

sdk/python/kfp/v2/components/experimental/component_spec_test.py

+          - 'set -ex
+
+            echo "$0" > "$1"'
+          - name: input1


What does name mean? How does it differentiate between inputs and outputs (e.g. when the component is transforming Model input to Model output)?

Ark-kun · 2021-08-26T03:04:26Z

sdk/python/kfp/v2/components/experimental/component_spec_test.py

+                commands=[
+                    'sh',
+                    '-c',
+                    'set -ex\necho "$0" > "$1"',


JFYI: The resulting file will have \n at the end. And unlike Argo, Vertex Pipelines will currently preserve that end-of-line character and put it on the command-line. Sometimes this is not desired.

ji-yaqi added 2 commits August 16, 2021 14:56

pydantic setup

614e924

switch to union types

1a6778e

google-oss-robot requested review from Ark-kun and chensun August 17, 2021 22:48

google-cla bot added the cla: yes label Aug 17, 2021

google-oss-robot added the size/L label Aug 17, 2021

ji-yaqi added 4 commits August 17, 2021 15:48

Merge branch 'master' into save_component

d278cbd

Fix base_component tests

808a401

Merge branch 'save_component' of github.com:ji-yaqi/pipelines into sa…

bb45e6d

…ve_component

switch to pydantic for python3.6 compat and fix tests

e8541f1

chensun reviewed Aug 18, 2021

View reviewed changes

chensun requested review from neuromage and removed request for Ark-kun August 18, 2021 07:36

ji-yaqi changed the title ~~feat(sdk): Add yaml_model to component_spec data classes~~ feat(sdk): Add BaseModel to component_spec data classes Aug 18, 2021

ji-yaqi added 3 commits August 18, 2021 15:37

Fix tests

7ce96c4

use str for type

6785f8e

fix tests

432fc7f

ji-yaqi force-pushed the save_component branch from 95d5ec6 to 432fc7f Compare August 19, 2021 18:09

chensun reviewed Aug 23, 2021

View reviewed changes

Consistent quote sign

bdb6814

chensun reviewed Aug 24, 2021

View reviewed changes

Address comments

cd2ca37

ji-yaqi force-pushed the save_component branch from 8301fd3 to cd2ca37 Compare August 24, 2021 23:37

chensun reviewed Aug 25, 2021

View reviewed changes

google-oss-robot assigned chensun Aug 25, 2021

google-oss-robot added the lgtm label Aug 25, 2021

Update type

6afbef4

google-oss-robot removed the lgtm label Aug 25, 2021

google-oss-robot added the lgtm label Aug 25, 2021

google-oss-robot added the approved label Aug 25, 2021

ji-yaqi merged commit 65174cd into kubeflow:master Aug 25, 2021

ji-yaqi deleted the save_component branch August 25, 2021 21:00

Ark-kun reviewed Aug 26, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): Add BaseModel to component_spec data classes #6372

feat(sdk): Add BaseModel to component_spec data classes #6372

ji-yaqi commented Aug 17, 2021

ji-yaqi commented Aug 18, 2021

chensun left a comment

chensun Aug 18, 2021

chensun Aug 23, 2021 •

edited

Loading

ji-yaqi Aug 23, 2021

chensun Aug 24, 2021

chensun Aug 23, 2021

chensun Aug 24, 2021

chensun Aug 24, 2021

ji-yaqi Aug 24, 2021

Ark-kun Aug 26, 2021

chensun Aug 24, 2021

chensun Aug 24, 2021

chensun left a comment

chensun Aug 25, 2021

chensun commented Aug 25, 2021

ji-yaqi commented Aug 25, 2021

google-oss-robot commented Aug 25, 2021

Ark-kun Aug 26, 2021

Ark-kun Aug 26, 2021

Ark-kun Aug 26, 2021

Ark-kun Aug 26, 2021

feat(sdk): Add BaseModel to component_spec data classes #6372

feat(sdk): Add BaseModel to component_spec data classes #6372

Conversation

ji-yaqi commented Aug 17, 2021

ji-yaqi commented Aug 18, 2021

chensun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun Aug 23, 2021 • 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

chensun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chensun commented Aug 25, 2021

ji-yaqi commented Aug 25, 2021

google-oss-robot commented Aug 25, 2021

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 Aug 23, 2021 •

edited

Loading