From a8ebc5e1613a391d80be2ce639ef6d6891a7a171 Mon Sep 17 00:00:00 2001 From: Yaqi Date: Mon, 29 Nov 2021 13:57:52 -0800 Subject: [PATCH 1/3] fix(sdk): fix load_test --- .../compiler_cli_tests/compiler_cli_tests.py | 5 +- .../pipeline_with_if_placeholder.json | 134 +++++++++--------- sdk/python/kfp/v2/components/structures.py | 5 +- .../kfp/v2/components/yaml_component.py | 2 +- 4 files changed, 70 insertions(+), 76 deletions(-) diff --git a/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py b/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py index fdae3cdf2a1..c6c51f12ba3 100644 --- a/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py +++ b/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py @@ -107,9 +107,8 @@ def test_pipeline_with_importer(self): # def test_pipeline_with_ontology(self): # self._test_compile_py_to_json('pipeline_with_ontology') - # TODO: re-enable the test, debug load_component_from_file error - # def test_pipeline_with_if_placeholder(self): - # self._test_compile_py_to_json('pipeline_with_if_placeholder') + def test_pipeline_with_if_placeholder(self): + self._test_compile_py_to_json('pipeline_with_if_placeholder') # TODO: re-enable the test, debug load_component_from_file error # def test_pipeline_with_concat_placeholder(self): diff --git a/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_if_placeholder.json b/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_if_placeholder.json index a3e0df000bb..32665fd1e22 100644 --- a/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_if_placeholder.json +++ b/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_if_placeholder.json @@ -1,84 +1,80 @@ { - "pipelineSpec": { - "components": { - "comp-component-with-optional-inputs": { - "executorLabel": "exec-component-with-optional-inputs", - "inputDefinitions": { - "parameters": { - "optional_input_1": { - "parameterType": "STRING" - }, - "required_input": { - "parameterType": "STRING" - } + "components": { + "comp-component-with-optional-inputs": { + "executorLabel": "exec-component-with-optional-inputs", + "inputDefinitions": { + "parameters": { + "optional_input_1": { + "parameterType": "STRING" + }, + "required_input": { + "parameterType": "STRING" } } } - }, - "deploymentSpec": { - "executors": { - "exec-component-with-optional-inputs": { - "container": { - "args": [ - "--arg0", - "{{$.inputs.parameters['required_input']}}", - "--arg1", - "{{$.inputs.parameters['optional_input_1']}}", - "--arg3", - "default value" - ], - "image": "gcr.io/my-project/my-image" - } + } + }, + "defaultPipelineRoot": "dummy_root", + "deploymentSpec": { + "executors": { + "exec-component-with-optional-inputs": { + "container": { + "args": [ + "--arg0", + "{{$.inputs.parameters['required_input']}}", + "--arg1", + "{{$.inputs.parameters['optional_input_1']}}", + "--arg3", + "default value" + ], + "image": "gcr.io/my-project/my-image" } } - }, - "pipelineInfo": { - "name": "one-step-pipeline-with-if-placeholder" - }, - "root": { - "dag": { - "tasks": { - "component-with-optional-inputs": { - "cachingOptions": { - "enableCache": true - }, - "componentRef": { - "name": "comp-component-with-optional-inputs" - }, - "inputs": { - "parameters": { - "optional_input_1": { - "componentInputParameter": "input1" - }, - "required_input": { - "componentInputParameter": "input0" - } + } + }, + "pipelineInfo": { + "name": "one-step-pipeline-with-if-placeholder" + }, + "root": { + "dag": { + "tasks": { + "component-with-optional-inputs": { + "cachingOptions": { + "enableCache": true + }, + "componentRef": { + "name": "comp-component-with-optional-inputs" + }, + "inputs": { + "parameters": { + "optional_input_1": { + "componentInputParameter": "input1" + }, + "required_input": { + "componentInputParameter": "input0" } - }, - "taskInfo": { - "name": "component-with-optional-inputs" } - } - } - }, - "inputDefinitions": { - "parameters": { - "input0": { - "parameterType": "STRING" - }, - "input1": { - "parameterType": "STRING" }, - "input2": { - "parameterType": "STRING" + "taskInfo": { + "name": "component-with-optional-inputs" } } } }, - "schemaVersion": "2.1.0", - "sdkVersion": "kfp-1.8.6" + "inputDefinitions": { + "parameters": { + "input0": { + "parameterType": "STRING" + }, + "input1": { + "parameterType": "STRING" + }, + "input2": { + "parameterType": "STRING" + } + } + } }, - "runtimeConfig": { - "gcsOutputDirectory": "dummy_root" - } + "schemaVersion": "2.1.0", + "sdkVersion": "kfp-1.8.9" } \ No newline at end of file diff --git a/sdk/python/kfp/v2/components/structures.py b/sdk/python/kfp/v2/components/structures.py index 5f6c04f662f..f088cbc0806 100644 --- a/sdk/python/kfp/v2/components/structures.py +++ b/sdk/python/kfp/v2/components/structures.py @@ -374,8 +374,8 @@ def _check_valid_placeholder_reference(cls, valid_inputs: Sequence[str], if arg.if_structure.input_name not in valid_inputs: raise ValueError( f'Argument "{arg}" references non-existing input.') - for placeholder in itertools.chain(arg.if_structure.then, - arg.if_structure.otherwise): + for placeholder in itertools.chain(arg.if_structure.then or [], + arg.if_structure.otherwise or []): cls._check_valid_placeholder_reference(valid_inputs, valid_outputs, placeholder) @@ -574,7 +574,6 @@ def load_from_component_yaml(cls, component_yaml: str) -> 'ComponentSpec': Returns: Component spec in the form of V2 ComponentSpec. """ - json_component = yaml.safe_load(component_yaml) try: return ComponentSpec.parse_obj(json_component) diff --git a/sdk/python/kfp/v2/components/yaml_component.py b/sdk/python/kfp/v2/components/yaml_component.py index bbe20e4ae3c..1a0e009b8f8 100644 --- a/sdk/python/kfp/v2/components/yaml_component.py +++ b/sdk/python/kfp/v2/components/yaml_component.py @@ -43,7 +43,7 @@ def load_component_from_file(file_path: str) -> base_component.BaseComponent: file_path: A string containing path to the YAML file. """ with open(file_path, 'rb') as component_stream: - return load_component_from_text(component_stream) + return load_component_from_text(component_stream.read()) def load_component_from_url(url: str, From 5ebc1cb1f6f0899424495143c7627c6670461c84 Mon Sep 17 00:00:00 2001 From: Yaqi Date: Mon, 29 Nov 2021 14:17:20 -0800 Subject: [PATCH 2/3] concat placeholder --- .../compiler_cli_tests/compiler_cli_tests.py | 7 +- .../pipeline_with_concat_placeholder.json | 94 +++++++++---------- 2 files changed, 48 insertions(+), 53 deletions(-) diff --git a/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py b/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py index c6c51f12ba3..5c90df01f06 100644 --- a/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py +++ b/sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py @@ -110,11 +110,10 @@ def test_pipeline_with_importer(self): def test_pipeline_with_if_placeholder(self): self._test_compile_py_to_json('pipeline_with_if_placeholder') - # TODO: re-enable the test, debug load_component_from_file error - # def test_pipeline_with_concat_placeholder(self): - # self._test_compile_py_to_json('pipeline_with_concat_placeholder') + def test_pipeline_with_concat_placeholder(self): + self._test_compile_py_to_json('pipeline_with_concat_placeholder') - # TODO: re-enable the test, debug load_component_from_file error + # TODO: re-enable the test, debug add_node_selector_constraint error # def test_pipeline_with_resource_spec(self): # self._test_compile_py_to_json('pipeline_with_resource_spec') diff --git a/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_concat_placeholder.json b/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_concat_placeholder.json index 4eb955c2f4b..780ba6bc193 100644 --- a/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_concat_placeholder.json +++ b/sdk/python/kfp/v2/compiler_cli_tests/test_data/pipeline_with_concat_placeholder.json @@ -1,63 +1,59 @@ { - "pipelineSpec": { - "components": { - "comp-component-with-concat-placeholder": { - "executorLabel": "exec-component-with-concat-placeholder", - "inputDefinitions": { - "parameters": { - "input_prefix": { - "parameterType": "STRING" - } + "components": { + "comp-component-with-concat-placeholder": { + "executorLabel": "exec-component-with-concat-placeholder", + "inputDefinitions": { + "parameters": { + "input_prefix": { + "parameterType": "STRING" } } } - }, - "deploymentSpec": { - "executors": { - "exec-component-with-concat-placeholder": { - "container": { - "args": [ - "--arg0", - "{{$.inputs.parameters['input_prefix']}}some value" - ], - "image": "gcr.io/my-project/my-image" - } + } + }, + "defaultPipelineRoot": "dummy_root", + "deploymentSpec": { + "executors": { + "exec-component-with-concat-placeholder": { + "container": { + "args": [ + "--arg0", + "{{$.inputs.parameters['input_prefix']}}some value" + ], + "image": "gcr.io/my-project/my-image" } } - }, - "pipelineInfo": { - "name": "one-step-pipeline-with-concat-placeholder" - }, - "root": { - "dag": { - "tasks": { - "component-with-concat-placeholder": { - "cachingOptions": { - "enableCache": true - }, - "componentRef": { - "name": "comp-component-with-concat-placeholder" - }, - "inputs": { - "parameters": { - "input_prefix": { - "runtimeValue": { - "constant": "some prefix:" - } + } + }, + "pipelineInfo": { + "name": "one-step-pipeline-with-concat-placeholder" + }, + "root": { + "dag": { + "tasks": { + "component-with-concat-placeholder": { + "cachingOptions": { + "enableCache": true + }, + "componentRef": { + "name": "comp-component-with-concat-placeholder" + }, + "inputs": { + "parameters": { + "input_prefix": { + "runtimeValue": { + "constant": "some prefix:" } } - }, - "taskInfo": { - "name": "component-with-concat-placeholder" } + }, + "taskInfo": { + "name": "component-with-concat-placeholder" } } } - }, - "schemaVersion": "2.1.0", - "sdkVersion": "kfp-1.8.6" + } }, - "runtimeConfig": { - "gcsOutputDirectory": "dummy_root" - } + "schemaVersion": "2.1.0", + "sdkVersion": "kfp-1.8.9" } \ No newline at end of file From a990f9ab988bdf847adf801733d9725a93131aaa Mon Sep 17 00:00:00 2001 From: Yaqi Date: Mon, 29 Nov 2021 14:26:52 -0800 Subject: [PATCH 3/3] release notes --- sdk/RELEASE.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/RELEASE.md b/sdk/RELEASE.md index 5109fdf6bcf..a5ec828971c 100644 --- a/sdk/RELEASE.md +++ b/sdk/RELEASE.md @@ -30,6 +30,8 @@ * Depends on `google-auth>=1.6.1,<3` [\#6939](https://github.com/kubeflow/pipelines/pull/6939) * Change otherwise to else in yaml [\#6952](https://github.com/kubeflow/pipelines/pull/6952) * Avoid pydantic bug on Union type [\#6957](https://github.com/kubeflow/pipelines/pull/6957) +* Fix bug for if and concat placeholders [\#6978](https://github.com/kubeflow/pipelines/pull/6978) + ## Documentation Updates