From d5cd3d1b732a4b517540c3ae883e2373da21d22f Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Mon, 31 Oct 2022 16:42:48 -0700 Subject: [PATCH 1/5] support single element IfPresentPlaceholders that contain a primitive placeholder --- sdk/python/kfp/components/pipeline_task.py | 17 +++++- .../kfp/components/placeholders_test.py | 57 ++++++++++++++++++- sdk/python/kfp/components/structures.py | 18 +++++- 3 files changed, 88 insertions(+), 4 deletions(-) diff --git a/sdk/python/kfp/components/pipeline_task.py b/sdk/python/kfp/components/pipeline_task.py index e1a710a20a6..4e7572cc13d 100644 --- a/sdk/python/kfp/components/pipeline_task.py +++ b/sdk/python/kfp/components/pipeline_task.py @@ -514,7 +514,22 @@ def check_primitive_placeholder_is_used_for_correct_io_type( f'"{outputs_dict[output_name].type}" cannot be paired with ' f'{arg.__class__.__name__}.') elif isinstance(arg, placeholders.IfPresentPlaceholder): - for arg in itertools.chain(arg.then or [], arg.else_ or []): + all_normalized_args: List[Union[str, placeholders.Placeholder]] = [] + if arg.then is None: + pass + elif isinstance(arg.then, list): + all_normalized_args.extend(arg.then) + else: + all_normalized_args.append(arg.then) + + if arg.else_ is None: + pass + elif isinstance(arg.else_, list): + all_normalized_args.extend(arg.else_) + else: + all_normalized_args.append(arg.else_) + + for arg in all_normalized_args: check_primitive_placeholder_is_used_for_correct_io_type( inputs_dict, outputs_dict, arg) elif isinstance(arg, placeholders.ConcatPlaceholder): diff --git a/sdk/python/kfp/components/placeholders_test.py b/sdk/python/kfp/components/placeholders_test.py index 2ab5502c439..3ca1f987c38 100644 --- a/sdk/python/kfp/components/placeholders_test.py +++ b/sdk/python/kfp/components/placeholders_test.py @@ -12,8 +12,10 @@ # See the License for the specific language governing permissions and # limitations under the License. """Contains tests for kfp.components.placeholders.""" +import os +import tempfile from typing import Any - +from kfp import compiler from absl.testing import parameterized from kfp.components import placeholders @@ -89,7 +91,7 @@ def test(self): "{{$.outputs.artifacts['output1'].metadata}}") -class TestIfPresentPlaceholderStructure(parameterized.TestCase): +class TestIfPresentPlaceholder(parameterized.TestCase): @parameterized.parameters([ (placeholders.IfPresentPlaceholder( @@ -140,6 +142,57 @@ def test_with_primitive_placeholders( self.assertEqual(placeholder_obj._to_string(), placeholder) + def test_if_present_with_single_element_simple_can_be_compiled(self): + + @dsl.container_component + def container_component(a: str): + return dsl.ContainerSpec( + image='alpine', + command=[ + placeholders.IfPresentPlaceholder( + input_name='a', then='b', else_='c') + ]) + + with tempfile.TemporaryDirectory() as tempdir: + output_yaml = os.path.join(tempdir, 'component.yaml') + compiler.Compiler().compile( + pipeline_func=container_component, package_path=output_yaml) + + def test_if_present_with_single_element_parameter_reference_can_be_compiled( + self): + + @dsl.container_component + def container_component(a: str): + return dsl.ContainerSpec( + image='alpine', + command=[ + placeholders.IfPresentPlaceholder( + input_name='a', then=a, else_='c') + ]) + + with tempfile.TemporaryDirectory() as tempdir: + output_yaml = os.path.join(tempdir, 'component.yaml') + compiler.Compiler().compile( + pipeline_func=container_component, package_path=output_yaml) + + def test_if_present_with_single_element_artifact_reference_can_be_compiled( + self): + + @dsl.container_component + def container_component(a: dsl.Input[dsl.Artifact]): + return dsl.ContainerSpec( + image='alpine', + command=[ + placeholders.IfPresentPlaceholder( + input_name='a', then=a.path, else_='c') + ]) + + with tempfile.TemporaryDirectory() as tempdir: + output_yaml = os.path.join(tempdir, 'component.yaml') + compiler.Compiler().compile( + pipeline_func=container_component, package_path=output_yaml) + + class TestConcatPlaceholder(parameterized.TestCase): @parameterized.parameters([ diff --git a/sdk/python/kfp/components/structures.py b/sdk/python/kfp/components/structures.py index f997da1166b..e29a79468f4 100644 --- a/sdk/python/kfp/components/structures.py +++ b/sdk/python/kfp/components/structures.py @@ -493,7 +493,23 @@ def check_placeholder_references_valid_io_name( raise ValueError( f'Argument "{arg}" references nonexistant input: "{arg.input_name}".' ) - for arg in itertools.chain(arg.then or [], arg.else_ or []): + + all_normalized_args: List[Union[str, placeholders.Placeholder]] = [] + if arg.then is None: + pass + elif isinstance(arg.then, list): + all_normalized_args.extend(arg.then) + else: + all_normalized_args.append(arg.then) + + if arg.else_ is None: + pass + elif isinstance(arg.else_, list): + all_normalized_args.extend(arg.else_) + else: + all_normalized_args.append(arg.else_) + + for arg in all_normalized_args: check_placeholder_references_valid_io_name(inputs_dict, outputs_dict, arg) elif isinstance(arg, placeholders.ConcatPlaceholder): From e66fdc75229841088ed65ee7778b332524bdee89 Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Mon, 31 Oct 2022 16:44:33 -0700 Subject: [PATCH 2/5] block list then and else_ inside concat --- sdk/python/kfp/components/placeholders.py | 33 +++++ .../kfp/components/placeholders_test.py | 120 ++++++++++++++++-- sdk/python/kfp/components/structures_test.py | 30 +---- 3 files changed, 145 insertions(+), 38 deletions(-) diff --git a/sdk/python/kfp/components/placeholders.py b/sdk/python/kfp/components/placeholders.py index cde3969ef11..c51352012cf 100644 --- a/sdk/python/kfp/components/placeholders.py +++ b/sdk/python/kfp/components/placeholders.py @@ -142,6 +142,9 @@ def container_with_concat_placeholder(text1: str, text2: Output[Dataset], """ def __init__(self, items: List['CommandLineElement']) -> None: + for item in items: + if isinstance(item, IfPresentPlaceholder): + item._validate_then_and_else_are_only_single_element() self.items = items def _to_dict(self) -> Dict[str, Any]: @@ -198,6 +201,36 @@ def __init__( self.then = then self.else_ = else_ + def _validate_then_and_else_are_only_single_element(self) -> None: + """Rercursively validate that then and else contain only a single + element. + + This method should only be called by a ConcatPlaceholder, which + cannot have an IfPresentPlaceholder with a list in either 'then' + or 'else_'. + """ + + # the illegal state + if isinstance(self.then, list) or isinstance(self.else_, list): + raise ValueError( + f'Cannot use {IfPresentPlaceholder.__name__} within {ConcatPlaceholder.__name__} when `then` and `else_` arguments to {IfPresentPlaceholder.__name__} are lists. Please use a single element for `then` and `else_` only.' + ) + + # check that there is no illegal state found recursively + if isinstance(self.then, ConcatPlaceholder): + for item in self.then.items: + if isinstance(item, IfPresentPlaceholder): + item._validate_then_and_else_are_only_single_element() + elif isinstance(self.then, IfPresentPlaceholder): + self.then._validate_then_and_else_are_only_single_element() + + if isinstance(self.else_, ConcatPlaceholder): + for item in self.else_.items: + if isinstance(item, IfPresentPlaceholder): + item._validate_then_and_else_are_only_single_element() + elif isinstance(self.else_, IfPresentPlaceholder): + self.else_._validate_then_and_else_are_only_single_element() + def _to_dict(self) -> Dict[str, Any]: struct = { 'IfPresent': { diff --git a/sdk/python/kfp/components/placeholders_test.py b/sdk/python/kfp/components/placeholders_test.py index 3ca1f987c38..74d614663be 100644 --- a/sdk/python/kfp/components/placeholders_test.py +++ b/sdk/python/kfp/components/placeholders_test.py @@ -15,8 +15,10 @@ import os import tempfile from typing import Any -from kfp import compiler + from absl.testing import parameterized +from kfp import compiler +from kfp import dsl from kfp.components import placeholders @@ -141,7 +143,6 @@ def test_with_primitive_placeholders( placeholder: str): self.assertEqual(placeholder_obj._to_string(), placeholder) - def test_if_present_with_single_element_simple_can_be_compiled(self): @dsl.container_component @@ -235,16 +236,6 @@ class TestContainerPlaceholdersTogether(parameterized.TestCase): ]), """{"Concat": ["a", {"Concat": ["b", {"Concat": ["c", "{{$.inputs.parameters['input2']}}"]}]}]}""" ), - (placeholders.ConcatPlaceholder([ - 'a', - placeholders.ConcatPlaceholder([ - 'b', - placeholders.IfPresentPlaceholder( - input_name='output1', then=['then'], else_=['something']) - ]) - ]), - '{"Concat": ["a", {"Concat": ["b", {"IfPresent": {"InputName": "output1", "Then": ["then"], "Else": ["something"]}}]}]}' - ), (placeholders.ConcatPlaceholder([ 'a', placeholders.ConcatPlaceholder([ @@ -280,10 +271,111 @@ class TestContainerPlaceholdersTogether(parameterized.TestCase): """{"Concat": ["a", {"IfPresent": {"InputName": "output1", "Then": {"Concat": ["--", "flag"]}, "Else": "{{$.inputs.artifacts['input2'].path}}"}}, "c"]}""" ), ]) - def test(self, placeholder_obj: placeholders.IfPresentPlaceholder, - placeholder: str): + def test_valid(self, placeholder_obj: placeholders.IfPresentPlaceholder, + placeholder: str): self.assertEqual(placeholder_obj._to_string(), placeholder) + def test_only_single_element_ifpresent_inside_concat_outer(self): + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + placeholders.ConcatPlaceholder([ + 'b', + placeholders.IfPresentPlaceholder( + input_name='output1', then=['then'], else_=['something']) + ]) + + def test_only_single_element_ifpresent_inside_concat_recursive(self): + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + placeholders.ConcatPlaceholder([ + 'a', + placeholders.ConcatPlaceholder([ + 'b', + placeholders.IfPresentPlaceholder( + input_name='output1', + then=['then'], + else_=['something']) + ]) + ]) + + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + placeholders.ConcatPlaceholder([ + 'a', + placeholders.ConcatPlaceholder([ + 'b', + placeholders.IfPresentPlaceholder( + input_name='output1', + then=placeholders.ConcatPlaceholder([ + placeholders.IfPresentPlaceholder( + input_name='a', then=['b']) + ]), + else_='something') + ]) + ]) + + def test_only_single_element_in_nested_ifpresent_inside_concat(self): + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + dsl.ConcatPlaceholder([ + 'my-prefix-', + dsl.IfPresentPlaceholder( + input_name='input1', + then=[ + dsl.IfPresentPlaceholder( + input_name='input1', + then=dsl.ConcatPlaceholder(['infix-', 'value'])) + ]) + ]) + + def test_recursive_nested_placeholder_validation_does_not_exit_when_first_valid_then_is_found( + self): + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + dsl.ConcatPlaceholder([ + 'my-prefix-', + dsl.IfPresentPlaceholder( + input_name='input1', + then=dsl.IfPresentPlaceholder( + input_name='input1', + then=[dsl.ConcatPlaceholder(['infix-', 'value'])])) + ]) + + def test_only_single_element_in_nested_ifpresent_inside_concat_with_outer_ifpresent( + self): + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + dsl.IfPresentPlaceholder( + input_name='input_1', + then=dsl.ConcatPlaceholder([ + 'my-prefix-', + dsl.IfPresentPlaceholder( + input_name='input1', + then=dsl.IfPresentPlaceholder( + input_name='input1', + then=[dsl.ConcatPlaceholder(['infix-', 'value'])])) + ])) + + def test_valid_then_but_invalid_else(self): + with self.assertRaisesRegex( + ValueError, + f'Please use a single element for `then` and `else_` only\.'): + dsl.ConcatPlaceholder([ + 'my-prefix-', + dsl.IfPresentPlaceholder( + input_name='input1', + then=dsl.IfPresentPlaceholder( + input_name='input1', + then='single-element', + else_=['one', 'two'])) + ]) + class TestConvertCommandLineElementToStringOrStruct(parameterized.TestCase): diff --git a/sdk/python/kfp/components/structures_test.py b/sdk/python/kfp/components/structures_test.py index bd345e90b67..260d0b78ac0 100644 --- a/sdk/python/kfp/components/structures_test.py +++ b/sdk/python/kfp/components/structures_test.py @@ -105,15 +105,8 @@ - if: cond: isPresent: input_prefix - else: - - --arg2 - - default - - concat: - - --arg1 - - {inputValue: input_prefix} - then: - - --arg1 - - {inputValue: input_prefix} + then: {inputValue: input_prefix} + else: default image: alpine inputs: - {name: input_prefix, optional: false, type: String} @@ -129,21 +122,10 @@ '--arg1', placeholders.IfPresentPlaceholder( input_name='input_prefix', - then=[ - '--arg1', - placeholders.InputValuePlaceholder( - input_name='input_prefix'), - ], - else_=[ - '--arg2', - 'default', - placeholders.ConcatPlaceholder(items=[ - '--arg1', - placeholders.InputValuePlaceholder( - input_name='input_prefix'), - ]), - ]), - ]) + then=placeholders.InputValuePlaceholder( + input_name='input_prefix'), + else_='default'), + ]), ])), inputs={'input_prefix': structures.InputSpec(type='String')}, ) From be2728c59fa3c720af7e526a83cfbe9f3b98852d Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Wed, 2 Nov 2022 11:33:50 -0700 Subject: [PATCH 3/5] support single element IfPresent in v1 --- sdk/python/kfp/components/placeholders.py | 31 ++++++++++++++++------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/sdk/python/kfp/components/placeholders.py b/sdk/python/kfp/components/placeholders.py index c51352012cf..3d249073cfd 100644 --- a/sdk/python/kfp/components/placeholders.py +++ b/sdk/python/kfp/components/placeholders.py @@ -364,18 +364,31 @@ def maybe_convert_v1_yaml_placeholder_to_v2_placeholder( elif 'if' in arg: if_ = arg['if'] input_name = utils.sanitize_input_name(if_['cond']['isPresent']) - then_ = if_['then'] - else_ = if_.get('else', []) - return IfPresentPlaceholder( - input_name=input_name, - then=[ + then = if_['then'] + else_ = if_.get('else') + + if isinstance(then, list): + then = [ maybe_convert_v1_yaml_placeholder_to_v2_placeholder( - val, component_dict=component_dict) for val in then_ - ], - else_=[ + val, component_dict=component_dict) for val in then + ] + else: + then = maybe_convert_v1_yaml_placeholder_to_v2_placeholder( + then, component_dict=component_dict) + + if else_ is None: + pass + elif isinstance(else_, list): + else_ = [ maybe_convert_v1_yaml_placeholder_to_v2_placeholder( val, component_dict=component_dict) for val in else_ - ]) + ] + else: + maybe_convert_v1_yaml_placeholder_to_v2_placeholder( + else_, component_dict=component_dict) + + return IfPresentPlaceholder( + input_name=input_name, then=then, else_=else_) elif 'concat' in arg: From f2f1a7de513b9be2127c2cc3ff922d49f289a380 Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Wed, 2 Nov 2022 11:36:43 -0700 Subject: [PATCH 4/5] clean up typing --- sdk/python/kfp/components/pipeline_task.py | 4 ++-- sdk/python/kfp/components/placeholders.py | 6 +----- sdk/python/kfp/components/structures.py | 2 +- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/sdk/python/kfp/components/pipeline_task.py b/sdk/python/kfp/components/pipeline_task.py index 4e7572cc13d..c0e1a1ba5fe 100644 --- a/sdk/python/kfp/components/pipeline_task.py +++ b/sdk/python/kfp/components/pipeline_task.py @@ -474,7 +474,7 @@ def my_pipeline(): def check_primitive_placeholder_is_used_for_correct_io_type( inputs_dict: Dict[str, structures.InputSpec], outputs_dict: Dict[str, structures.OutputSpec], - arg: Union[placeholders.Placeholder, Any], + arg: Union[placeholders.CommandLineElement, Any], ): """Validates input/output placeholders refer to an input/output with an appropriate type for the placeholder. This should only apply to components @@ -514,7 +514,7 @@ def check_primitive_placeholder_is_used_for_correct_io_type( f'"{outputs_dict[output_name].type}" cannot be paired with ' f'{arg.__class__.__name__}.') elif isinstance(arg, placeholders.IfPresentPlaceholder): - all_normalized_args: List[Union[str, placeholders.Placeholder]] = [] + all_normalized_args: List[placeholders.CommandLineElement] = [] if arg.then is None: pass elif isinstance(arg.then, list): diff --git a/sdk/python/kfp/components/placeholders.py b/sdk/python/kfp/components/placeholders.py index 3d249073cfd..2a0f5346c3b 100644 --- a/sdk/python/kfp/components/placeholders.py +++ b/sdk/python/kfp/components/placeholders.py @@ -265,11 +265,7 @@ def _to_string(self) -> str: OutputPathPlaceholder, OutputUriPlaceholder, OutputMetadataPlaceholder) -CommandLineElement = Union[str, IfPresentPlaceholder, ConcatPlaceholder, - InputValuePlaceholder, InputPathPlaceholder, - InputUriPlaceholder, InputMetadataPlaceholder, - OutputParameterPlaceholder, OutputPathPlaceholder, - OutputUriPlaceholder, OutputMetadataPlaceholder] +CommandLineElement = Union[str, Placeholder] def convert_command_line_element_to_string( diff --git a/sdk/python/kfp/components/structures.py b/sdk/python/kfp/components/structures.py index e29a79468f4..6ec76df69bb 100644 --- a/sdk/python/kfp/components/structures.py +++ b/sdk/python/kfp/components/structures.py @@ -494,7 +494,7 @@ def check_placeholder_references_valid_io_name( f'Argument "{arg}" references nonexistant input: "{arg.input_name}".' ) - all_normalized_args: List[Union[str, placeholders.Placeholder]] = [] + all_normalized_args: List[placeholders.CommandLineElement] = [] if arg.then is None: pass elif isinstance(arg.then, list): From 6f27c981fcdfd1119b6195a6ea526839b7841254 Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Thu, 3 Nov 2022 15:10:58 -0700 Subject: [PATCH 5/5] skip loading batch_predict_job yaml --- components/test_load_all_components.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/test_load_all_components.sh b/components/test_load_all_components.sh index 00f44282f0f..a79d53698dd 100755 --- a/components/test_load_all_components.sh +++ b/components/test_load_all_components.sh @@ -35,6 +35,9 @@ SKIP_COMPONENT_FILES = [ "./contrib/XGBoost/Cross_validation_for_regression/from_CSV/component.yaml", "./contrib/XGBoost/Train_regression_and_calculate_metrics/from_CSV/component.yaml", "./contrib/XGBoost/Train_and_cross-validate_regression/from_CSV/component.yaml", + # TODO: This component uses invalid placeholders. Updated when migrating GCPC to v2. + "./google-cloud/google_cloud_pipeline_components/aiplatform/batch_predict_job/component.yaml", + "./google-cloud/google_cloud_pipeline_components/v1/batch_predict_job/component.yaml" ] for component_file in sys.stdin: