From 6afb91b902651c95b62cf2e818daf6f04234b1e2 Mon Sep 17 00:00:00 2001 From: Alexey Volkov Date: Tue, 17 Sep 2019 13:07:34 -0700 Subject: [PATCH] SDK - Fix pipeline metadata serialization (#2137) Two PRs have been merged that turned out to be slightly incompatible. This PR fixes the failing tests. Root causes: * The pipeline parameter default values were not properly serialized when constructing the metadata object. * The `ParameterMeta` class did not validate the default value type, so the lack of serialization has not been caught. The `ParameterMeta` was replaced by `InputSpec` which has strict type validation. * Previously we did not have samples with complex pipeline parameter default values (e.g. lists) that could trigger the failures. Then two samples were added that had complex default values. * Travis does not re-run tests before merging * Prow does not re-run Travis tests before merging --- sdk/python/kfp/dsl/_metadata.py | 6 ++++++ sdk/python/tests/compiler/testdata/pipelineparams.yaml | 2 +- sdk/python/tests/compiler/testdata/recursive_while.yaml | 2 +- sdk/python/tests/compiler/testdata/withitem_basic.yaml | 2 +- sdk/python/tests/compiler/testdata/withitem_nested.yaml | 2 +- sdk/python/tests/compiler/testdata/withparam_global.yaml | 2 +- .../tests/compiler/testdata/withparam_global_dict.yaml | 4 ++-- sdk/python/tests/dsl/component_tests.py | 2 +- sdk/python/tests/dsl/pipeline_tests.py | 2 +- 9 files changed, 15 insertions(+), 9 deletions(-) diff --git a/sdk/python/kfp/dsl/_metadata.py b/sdk/python/kfp/dsl/_metadata.py index f2a7c028b9c..88848003e49 100644 --- a/sdk/python/kfp/dsl/_metadata.py +++ b/sdk/python/kfp/dsl/_metadata.py @@ -14,6 +14,7 @@ import warnings from .types import BaseType, _check_valid_type_dict, _instance_to_dict +from ..components._data_passing import serialize_value from ..components._structures import ComponentSpec, InputSpec, OutputSpec @@ -67,6 +68,8 @@ def _extract_component_metadata(func): arg_default = arg_default.value if arg in annotations: arg_type = _annotation_to_typemeta(annotations[arg]) + if arg_default is not None: + arg_default = serialize_value(arg_default, type_name=str(arg_type) if arg_type else None) # TODO: Improve _annotation_to_typemeta or just replace the whole function with kfp.component._python_op._extract_component_interface inputs.append(InputSpec(name=arg, type=arg_type, default=arg_default)) # Outputs outputs = [] @@ -124,7 +127,10 @@ def _extract_pipeline_metadata(func): if isinstance(schema_object, str): # In case the property value for the schema validator is a string instead of a dict. schema_object = json.loads(schema_object) + # Only validating non-serialized values validate(instance=arg_default, schema=schema_object) + if arg_default is not None: + arg_default = serialize_value(arg_default, type_name=str(arg_type) if arg_type else None) # TODO: Improve _annotation_to_typemeta or just replace the whole function with kfp.component._python_op._extract_component_interface inputs.append(InputSpec(name=arg, type=arg_type, default=arg_default)) #TODO: add descriptions to the metadata diff --git a/sdk/python/tests/compiler/testdata/pipelineparams.yaml b/sdk/python/tests/compiler/testdata/pipelineparams.yaml index 2e71241b3ea..859d228b01c 100644 --- a/sdk/python/tests/compiler/testdata/pipelineparams.yaml +++ b/sdk/python/tests/compiler/testdata/pipelineparams.yaml @@ -17,7 +17,7 @@ metadata: annotations: pipelines.kubeflow.org/pipeline_spec: '{"description": "A pipeline with multiple pipeline params.", "inputs": [{"default": "latest", "name": - "tag"}, {"default": 10, "name": "sleep_ms"}], "name": "PipelineParams"}' + "tag"}, {"default": "10", "name": "sleep_ms"}], "name": "PipelineParams"}' generateName: pipelineparams- spec: entrypoint: pipelineparams diff --git a/sdk/python/tests/compiler/testdata/recursive_while.yaml b/sdk/python/tests/compiler/testdata/recursive_while.yaml index d7101ef1fd1..6169a5c8257 100644 --- a/sdk/python/tests/compiler/testdata/recursive_while.yaml +++ b/sdk/python/tests/compiler/testdata/recursive_while.yaml @@ -3,7 +3,7 @@ kind: Workflow metadata: annotations: pipelines.kubeflow.org/pipeline_spec: '{"description": "shows how to use dsl.Condition.", - "inputs": [{"default": 12, "name": "maxVal"}], + "inputs": [{"default": "12", "name": "maxVal"}], "name": "pipeline flip coin"}' generateName: pipeline-flip-coin- spec: diff --git a/sdk/python/tests/compiler/testdata/withitem_basic.yaml b/sdk/python/tests/compiler/testdata/withitem_basic.yaml index 0cbf73eab21..cac186ee6a4 100644 --- a/sdk/python/tests/compiler/testdata/withitem_basic.yaml +++ b/sdk/python/tests/compiler/testdata/withitem_basic.yaml @@ -2,7 +2,7 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: annotations: - pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": 10, "name": "my_pipe_param"}], + pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "10", "name": "my_pipe_param"}], "name": "my-pipeline"}' generateName: my-pipeline- spec: diff --git a/sdk/python/tests/compiler/testdata/withitem_nested.yaml b/sdk/python/tests/compiler/testdata/withitem_nested.yaml index 7a5eb3e85b5..9c35f7e0fa8 100644 --- a/sdk/python/tests/compiler/testdata/withitem_nested.yaml +++ b/sdk/python/tests/compiler/testdata/withitem_nested.yaml @@ -2,7 +2,7 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: annotations: - pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": 10, "name": "my_pipe_param"}], + pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "10", "name": "my_pipe_param"}], "name": "my-pipeline"}' generateName: my-pipeline- spec: diff --git a/sdk/python/tests/compiler/testdata/withparam_global.yaml b/sdk/python/tests/compiler/testdata/withparam_global.yaml index b8ebf38d067..b3c1cbc9d38 100644 --- a/sdk/python/tests/compiler/testdata/withparam_global.yaml +++ b/sdk/python/tests/compiler/testdata/withparam_global.yaml @@ -2,7 +2,7 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: annotations: - pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": [3, 5, 7, 9], "name": + pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "[3, 5, 7, 9]", "name": "loopidy_doop"}], "name": "my-pipeline"}' generateName: my-pipeline- spec: diff --git a/sdk/python/tests/compiler/testdata/withparam_global_dict.yaml b/sdk/python/tests/compiler/testdata/withparam_global_dict.yaml index e1de5e26066..0c59a14dadc 100644 --- a/sdk/python/tests/compiler/testdata/withparam_global_dict.yaml +++ b/sdk/python/tests/compiler/testdata/withparam_global_dict.yaml @@ -2,8 +2,8 @@ apiVersion: argoproj.io/v1alpha1 kind: Workflow metadata: annotations: - pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": [{"a": 1, "b": - 2}, {"a": 10, "b": 20}], "name": "loopidy_doop"}], "name": "my-pipeline"}' + pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "[{\"a\": 1, \"b\": + 2}, {\"a\": 10, \"b\": 20}]", "name": "loopidy_doop"}], "name": "my-pipeline"}' generateName: my-pipeline- spec: arguments: diff --git a/sdk/python/tests/dsl/component_tests.py b/sdk/python/tests/dsl/component_tests.py index 3b2ee009c3b..c22ea25f29a 100644 --- a/sdk/python/tests/dsl/component_tests.py +++ b/sdk/python/tests/dsl/component_tests.py @@ -37,7 +37,7 @@ def componentA(a: {'ArtifactA': {'file_type': 'csv'}}, b: Integer() = 12, c: {'A golden_meta = ComponentSpec(name='componentA', inputs=[], outputs=[]) golden_meta.inputs.append(InputSpec(name='a', type={'ArtifactA': {'file_type': 'csv'}})) - golden_meta.inputs.append(InputSpec(name='b', type={'Integer': {'openapi_schema_validator': {"type": "integer"}}}, default=12)) + golden_meta.inputs.append(InputSpec(name='b', type={'Integer': {'openapi_schema_validator': {"type": "integer"}}}, default="12")) golden_meta.inputs.append(InputSpec(name='c', type={'ArtifactB': {'path_type':'file', 'file_type': 'tsv'}}, default='gs://hello/world')) golden_meta.outputs.append(OutputSpec(name='model', type={'Integer': {'openapi_schema_validator': {"type": "integer"}}})) diff --git a/sdk/python/tests/dsl/pipeline_tests.py b/sdk/python/tests/dsl/pipeline_tests.py index 0f9ff08c812..df7d7c377ef 100644 --- a/sdk/python/tests/dsl/pipeline_tests.py +++ b/sdk/python/tests/dsl/pipeline_tests.py @@ -72,7 +72,7 @@ def my_pipeline1(a: {'Schema': {'file_type': 'csv'}}='good', b: Integer()=12): golden_meta = ComponentSpec(name='p1', description='description1', inputs=[]) golden_meta.inputs.append(InputSpec(name='a', type={'Schema': {'file_type': 'csv'}}, default='good')) - golden_meta.inputs.append(InputSpec(name='b', type={'Integer': {'openapi_schema_validator': {"type": "integer"}}}, default=12)) + golden_meta.inputs.append(InputSpec(name='b', type={'Integer': {'openapi_schema_validator': {"type": "integer"}}}, default="12")) pipeline_meta = _extract_pipeline_metadata(my_pipeline1) self.assertEqual(pipeline_meta, golden_meta) \ No newline at end of file