From ee85d11be3a61b1629d96928f1754fd3e4d18b01 Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Wed, 23 Nov 2022 17:27:56 -0500 Subject: [PATCH 1/2] support primitive placeholders in fstring --- sdk/python/kfp/components/placeholders.py | 17 ++++- .../kfp/components/placeholders_test.py | 73 +++++++++++++++++++ sdk/python/kfp/components/structures.py | 6 +- 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/sdk/python/kfp/components/placeholders.py b/sdk/python/kfp/components/placeholders.py index 05aaded6fb7..9c6bf5db56f 100644 --- a/sdk/python/kfp/components/placeholders.py +++ b/sdk/python/kfp/components/placeholders.py @@ -29,9 +29,12 @@ def _to_string(self) -> str: raise NotImplementedError def __str__(self) -> str: - """Used for creating readable error messages when a placeholder doesn't - refer to an existing input or output.""" - return self.__class__.__name__ + """Enables use of placeholders in f-strings. + + To be overridden by container placeholders ConcatPlaceholder and + IfPresentPlaceholder, which cannot be used in an f-string. + """ + return self._to_string() def __eq__(self, other: Any) -> bool: """Used for comparing placeholders in tests.""" @@ -157,6 +160,10 @@ def _to_dict(self) -> Dict[str, Any]: def _to_string(self) -> str: return json.dumps(self._to_dict()) + def __str__(self) -> str: + raise ValueError( + f'Cannot use {self.__class__.__name__} in an f-string.') + class IfPresentPlaceholder(Placeholder): """Placeholder for handling cases where an input may or may not be passed. @@ -256,6 +263,10 @@ def _to_dict(self) -> Dict[str, Any]: def _to_string(self) -> str: return json.dumps(self._to_dict()) + def __str__(self) -> str: + raise ValueError( + f'Cannot use {self.__class__.__name__} in an f-string.') + _CONTAINER_PLACEHOLDERS = (IfPresentPlaceholder, ConcatPlaceholder) PRIMITIVE_INPUT_PLACEHOLDERS = (InputValuePlaceholder, InputPathPlaceholder, diff --git a/sdk/python/kfp/components/placeholders_test.py b/sdk/python/kfp/components/placeholders_test.py index 74d614663be..89b2b0db6a6 100644 --- a/sdk/python/kfp/components/placeholders_test.py +++ b/sdk/python/kfp/components/placeholders_test.py @@ -20,6 +20,8 @@ from kfp import compiler from kfp import dsl from kfp.components import placeholders +from kfp.dsl import Artifact +from kfp.dsl import Output class TestExecutorInputPlaceholder(parameterized.TestCase): @@ -396,3 +398,74 @@ def test_primitive_placeholder(self, placeholder: Any, expected: str): self.assertEqual( placeholders.convert_command_line_element_to_string_or_struct( placeholder), expected) + + +class OtherPlaceholderTests(parameterized.TestCase): + + def test_primitive_placeholder_can_be_used_in_fstring1(self): + + @dsl.container_component + def echo_bool(boolean: bool = True): + return dsl.ContainerSpec( + image='alpine', command=['sh', '-c', f'echo {boolean}']) + + self.assertEqual( + echo_bool.component_spec.implementation.container.command, + ['sh', '-c', "echo {{$.inputs.parameters['boolean']}}"]) + + def test_primitive_placeholder_can_be_used_in_fstring2(self): + + @dsl.container_component + def container_with_placeholder_in_fstring( + output_artifact: Output[Artifact], + text1: str, + ): + return dsl.ContainerSpec( + image='python:3.7', + command=[ + 'my_program', + f'prefix-{text1}', + f'{output_artifact.uri}/0', + ]) + + self.assertEqual( + container_with_placeholder_in_fstring.component_spec.implementation + .container.command, [ + 'my_program', + "prefix-{{$.inputs.parameters['text1']}}", + "{{$.outputs.artifacts['output_artifact'].uri}}/0", + ]) + + def test_cannot_use_concat_placeholder_in_f_string(self): + + with self.assertRaisesRegex( + ValueError, 'Cannot use ConcatPlaceholder in an f-string.'): + + @dsl.container_component + def container_with_placeholder_in_fstring( + text1: str, + text2: str, + ): + return dsl.ContainerSpec( + image='python:3.7', + command=[ + 'my_program', + f'another-prefix-{dsl.ConcatPlaceholder([text1, text2])}', + ]) + + def test_cannot_use_ifpresent_placeholder_in_f_string(self): + + with self.assertRaisesRegex( + ValueError, 'Cannot use IfPresentPlaceholder in an f-string.'): + + @dsl.container_component + def container_with_placeholder_in_fstring( + text1: str, + text2: str, + ): + return dsl.ContainerSpec( + image='python:3.7', + command=[ + 'echo', + f"another-prefix-{dsl.IfPresentPlaceholder(input_name='text1', then=['val'])}", + ]) diff --git a/sdk/python/kfp/components/structures.py b/sdk/python/kfp/components/structures.py index 34c1a8bb54d..96514031e4e 100644 --- a/sdk/python/kfp/components/structures.py +++ b/sdk/python/kfp/components/structures.py @@ -483,17 +483,17 @@ def check_placeholder_references_valid_io_name( elif isinstance(arg, placeholders.PRIMITIVE_INPUT_PLACEHOLDERS): if arg.input_name not in inputs_dict: raise ValueError( - f'Argument "{arg}" references nonexistant input: "{arg.input_name}".' + f'Argument "{arg.__class__.__name__}" references nonexistant input: "{arg.input_name}".' ) elif isinstance(arg, placeholders.PRIMITIVE_OUTPUT_PLACEHOLDERS): if arg.output_name not in outputs_dict: raise ValueError( - f'Argument "{arg}" references nonexistant output: "{arg.output_name}".' + f'Argument "{arg.__class__.__name__}" references nonexistant output: "{arg.output_name}".' ) elif isinstance(arg, placeholders.IfPresentPlaceholder): if arg.input_name not in inputs_dict: raise ValueError( - f'Argument "{arg}" references nonexistant input: "{arg.input_name}".' + f'Argument "{arg.__class__.__name__}" references nonexistant input: "{arg.input_name}".' ) all_normalized_args: List[placeholders.CommandLineElement] = [] From f2d84aeae7e7204319bf847bc5840fe4deabfdb0 Mon Sep 17 00:00:00 2001 From: connor-mccarthy Date: Wed, 23 Nov 2022 17:30:22 -0500 Subject: [PATCH 2/2] add read/write test component --- .../container_with_placeholder_in_fstring.py | 39 ++++++++++++++++ ...container_with_placeholder_in_fstring.yaml | 46 +++++++++++++++++++ sdk/python/test_data/test_data_config.yaml | 3 ++ 3 files changed, 88 insertions(+) create mode 100644 sdk/python/test_data/components/container_with_placeholder_in_fstring.py create mode 100644 sdk/python/test_data/components/container_with_placeholder_in_fstring.yaml diff --git a/sdk/python/test_data/components/container_with_placeholder_in_fstring.py b/sdk/python/test_data/components/container_with_placeholder_in_fstring.py new file mode 100644 index 00000000000..78103b7d00b --- /dev/null +++ b/sdk/python/test_data/components/container_with_placeholder_in_fstring.py @@ -0,0 +1,39 @@ +# Copyright 2022 The Kubeflow Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# 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. + +from kfp import dsl +from kfp.dsl import Artifact +from kfp.dsl import ContainerSpec +from kfp.dsl import Output + + +@dsl.container_component +def container_with_placeholder_in_fstring( + output_artifact: Output[Artifact], + text1: str = 'text!', +): + return ContainerSpec( + image='python:3.7', + command=[ + 'my_program', + f'prefix-{text1}', + f'{output_artifact.uri}/0', + ]) + + +if __name__ == '__main__': + from kfp import compiler + compiler.Compiler().compile( + pipeline_func=container_with_placeholder_in_fstring, + package_path=__file__.replace('.py', '.yaml')) diff --git a/sdk/python/test_data/components/container_with_placeholder_in_fstring.yaml b/sdk/python/test_data/components/container_with_placeholder_in_fstring.yaml new file mode 100644 index 00000000000..56c65bcc7df --- /dev/null +++ b/sdk/python/test_data/components/container_with_placeholder_in_fstring.yaml @@ -0,0 +1,46 @@ +components: + comp-container-with-placeholder-in-fstring: + executorLabel: exec-container-with-placeholder-in-fstring + inputDefinitions: + parameters: + text1: + defaultValue: text! + parameterType: STRING + outputDefinitions: + artifacts: + output_artifact: + artifactType: + schemaTitle: system.Artifact + schemaVersion: 0.0.1 +deploymentSpec: + executors: + exec-container-with-placeholder-in-fstring: + container: + command: + - my_program + - prefix-{{$.inputs.parameters['text1']}} + - '{{$.outputs.artifacts[''output_artifact''].uri}}/0' + image: python:3.7 +pipelineInfo: + name: container-with-placeholder-in-fstring +root: + dag: + tasks: + container-with-placeholder-in-fstring: + cachingOptions: + enableCache: true + componentRef: + name: comp-container-with-placeholder-in-fstring + inputs: + parameters: + text1: + componentInputParameter: text1 + taskInfo: + name: container-with-placeholder-in-fstring + inputDefinitions: + parameters: + text1: + defaultValue: text! + parameterType: STRING +schemaVersion: 2.1.0 +sdkVersion: kfp-2.0.0-beta.6 diff --git a/sdk/python/test_data/test_data_config.yaml b/sdk/python/test_data/test_data_config.yaml index a173fdf4677..d6128ebb97f 100644 --- a/sdk/python/test_data/test_data_config.yaml +++ b/sdk/python/test_data/test_data_config.yaml @@ -198,6 +198,9 @@ components: - module: container_with_if_placeholder name: container_with_if_placeholder execute: false + - module: container_with_placeholder_in_fstring + name: container_with_placeholder_in_fstring + execute: false v1_components: test_data_dir: sdk/python/test_data/v1_component_yaml read: true