Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(sdk.v2): Fix regression on optional inputs #6905

Merged
merged 2 commits into from
Nov 15, 2021
Merged

fix(sdk.v2): Fix regression on optional inputs #6905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3592075
Fix regression on optional inputs
chensun Nov 13, 2021
72b88b9
add release note
chensun Nov 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
* Fix importer ignoring reimport setting, and switch to Protobuf.Value for import uri [\#6827](https://github.com/kubeflow/pipelines/pull/6827)
* Fix display name support for groups [\#6832](https://github.com/kubeflow/pipelines/pull/6832)
* Remove redundant check in set_gpu_limit [\#6866](https://github.com/kubeflow/pipelines/pull/6866)
* Fix regression on optional inputs [\#6905](https://github.com/kubeflow/pipelines/pull/6905)

## Documentation Updates

Expand Down
9 changes: 4 additions & 5 deletions sdk/python/kfp/v2/compiler_cli_tests/compiler_cli_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,11 @@ def test_pipeline_with_nested_conditions_yaml(self):
def test_pipeline_with_loops(self):
self._test_compile_py_to_json('pipeline_with_loops')

# TODO: re-enable the test, fix optional input support
# TODO: re-enable the test, fix regression on loop
# def test_pipeline_with_nested_loops(self):
# self._test_compile_py_to_json('pipeline_with_nested_loops')

# TODO: re-enable the test, fix optional input support
# TODO: re-enable the test, fix regression on loop
# def test_pipeline_with_loops_and_conditions(self):
# self._test_compile_py_to_json('pipeline_with_loops_and_conditions')

Expand Down Expand Up @@ -176,9 +176,8 @@ def test_pipeline_with_exit_handler(self):
# def test_pipeline_with_env(self):
# self._test_compile_py_to_json('pipeline_with_env')

# TODO: re-enable the test, fix optional input support
# def test_v2_component_with_optional_inputs(self):
# self._test_compile_py_to_json('v2_component_with_optional_inputs')
def test_v2_component_with_optional_inputs(self):
self._test_compile_py_to_json('v2_component_with_optional_inputs')

def test_pipeline_with_gcpc_types(self):
self._test_compile_py_to_json('pipeline_with_gcpc_types')
Expand Down
8 changes: 0 additions & 8 deletions sdk/python/kfp/v2/compiler_cli_tests/test_data/lightweight_python_functions_v2_pipeline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@
},
"message": {
"parameterType": "STRING"
},
"num_steps": {
"parameterType": "NUMBER_INTEGER"
}
}
},
Expand Down Expand Up @@ -223,11 +220,6 @@
"outputParameterKey": "output_parameter_path",
"producerTask": "preprocess"
}
},
"num_steps": {
"runtimeValue": {
"constant": 100.0
}
}
}
},
Expand Down
131 changes: 63 additions & 68 deletions sdk/python/kfp/v2/compiler_cli_tests/test_data/v2_component_with_optional_inputs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,82 +1,77 @@
{
"pipelineSpec": {
"components": {
"comp-component-op": {
"executorLabel": "exec-component-op",
"inputDefinitions": {
"parameters": {
"input1": {
"parameterType": "STRING"
},
"input2": {
"parameterType": "STRING"
}
"components": {
"comp-component-op": {
"executorLabel": "exec-component-op",
"inputDefinitions": {
"parameters": {
"input1": {
"parameterType": "STRING"
},
"input2": {
"parameterType": "STRING"
}
}
}
},
"deploymentSpec": {
"executors": {
"exec-component-op": {
"container": {
"args": [
"--executor_input",
"{{$}}",
"--function_to_execute",
"component_op"
],
"command": [
"sh",
"-c",
"\nif ! [ -x \"$(command -v pip)\" ]; then\n python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location 'kfp==1.8.6' && \"$0\" \"$@\"\n",
"sh",
"-ec",
"program_path=$(mktemp -d)\nprintf \"%s\" \"$0\" > \"$program_path/ephemeral_component.py\"\npython3 -m kfp.v2.components.executor_main --component_module_path \"$program_path/ephemeral_component.py\" \"$@\"\n",
"\nimport kfp\nfrom kfp.v2 import dsl\nfrom kfp.v2.dsl import *\nfrom typing import *\n\ndef component_op(\n input1: str = 'default value',\n input2: Optional[str] = None,\n input3: Optional[str] = None,\n):\n print(f'input1: {input1}, type: {type(input1)}')\n print(f'input2: {input2}, type: {type(input2)}')\n print(f'input3: {input3}, type: {type(input3)}')\n\n"
],
"image": "python:3.7"
}
}
},
"deploymentSpec": {
"executors": {
"exec-component-op": {
"container": {
"args": [
"--executor_input",
"{{$}}",
"--function_to_execute",
"component_op"
],
"command": [
"sh",
"-c",
"\nif ! [ -x \"$(command -v pip)\" ]; then\n python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location 'kfp==1.8.9' && \"$0\" \"$@\"\n",
"sh",
"-ec",
"program_path=$(mktemp -d)\nprintf \"%s\" \"$0\" > \"$program_path/ephemeral_component.py\"\npython3 -m kfp.v2.components.executor_main --component_module_path \"$program_path/ephemeral_component.py\" \"$@\"\n",
"\nimport kfp\nfrom kfp.v2 import dsl\nfrom kfp.v2.dsl import *\nfrom typing import *\n\ndef component_op(\n input1: str = 'default value',\n input2: Optional[str] = None,\n input3: Optional[str] = None,\n):\n print(f'input1: {input1}, type: {type(input1)}')\n print(f'input2: {input2}, type: {type(input2)}')\n print(f'input3: {input3}, type: {type(input3)}')\n\n"
],
"image": "python:3.7"
}
}
},
"pipelineInfo": {
"name": "v2-component-optional-input"
},
"root": {
"dag": {
"tasks": {
"component-op": {
"cachingOptions": {
"enableCache": true
},
"componentRef": {
"name": "comp-component-op"
},
"inputs": {
"parameters": {
"input1": {
"runtimeValue": {
"constant": "Hello"
}
},
"input2": {
"runtimeValue": {
"constant": "World"
}
}
},
"pipelineInfo": {
"name": "v2-component-optional-input"
},
"root": {
"dag": {
"tasks": {
"component-op": {
"cachingOptions": {
"enableCache": true
},
"componentRef": {
"name": "comp-component-op"
},
"inputs": {
"parameters": {
"input1": {
"runtimeValue": {
"constant": "Hello"
}
},
"input2": {
"runtimeValue": {
"constant": "World"
}
}
},
"taskInfo": {
"name": "component-op"
}
},
"taskInfo": {
"name": "component-op"
}
}
}
},
"schemaVersion": "2.1.0",
"sdkVersion": "kfp-1.8.6"
}
},
"runtimeConfig": {
"gcsOutputDirectory": "dummy_root"
}
"schemaVersion": "2.1.0",
"sdkVersion": "kfp-1.8.9"
}
15 changes: 5 additions & 10 deletions sdk/python/kfp/v2/components/base_component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,23 +62,18 @@ def __call__(self, *args, **kwargs) -> pipeline_task.PipelineTask:
f'{self.name}() got multiple values for argument "{k}".')
task_inputs[k] = v

# Fill in default value if there was no user provided value
for input_name, input_spec in (self.component_spec.inputs or
{}).items():
if input_spec.default is not None and input_name not in task_inputs:
task_inputs[input_name] = input_spec.default

missing_arguments = [
input_name for input_name in (self.component_spec.inputs or {})
if input_name not in task_inputs
input_name for input_name, input_spec in (
self.component_spec.inputs or {}).items()
if input_name not in task_inputs and not input_spec.optional
]
if missing_arguments:
argument_or_arguments = 'argument' if len(
missing_arguments) == 1 else 'arguments'
arguments = ','.join(missing_arguments)
arguments = ', '.join(missing_arguments)

raise TypeError(
f'{self.name}() missing {len(missing_arguments)} required positional '
f'{self.name}() missing {len(missing_arguments)} required '
f'{argument_or_arguments}: {arguments}.')

return pipeline_task.create_pipeline_task(
Expand Down
30 changes: 17 additions & 13 deletions sdk/python/kfp/v2/components/base_component_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ def execute(self, *args, **kwargs):
],
),
inputs={
'input1': structures.InputSpec(type='String'),
'input2': structures.InputSpec(type='Integer'),
'input3': structures.InputSpec(type='Float', default=3.14),
'input1':
structures.InputSpec(type='String'),
'input2':
structures.InputSpec(type='Integer'),
'input3':
structures.InputSpec(type='Float', default=3.14),
'input4':
structures.InputSpec(type='Optional[Float]', default=None),
},
outputs={
'output1': structures.OutputSpec(name='output1', type='String'),
Expand All @@ -59,14 +64,15 @@ class BaseComponentTest(unittest.TestCase):
def test_instantiate_component_with_keyword_arguments(
self, mock_create_pipeline_task):

component_op(input1='hello', input2=100, input3=1.23)
component_op(input1='hello', input2=100, input3=1.23, input4=3.21)

mock_create_pipeline_task.assert_called_once_with(
component_spec=component_op.component_spec,
arguments={
'input1': 'hello',
'input2': 100,
'input3': 1.23,
'input4': 3.21,
})

@patch.object(pipeline_task, 'create_pipeline_task', autospec=True)
Expand All @@ -80,33 +86,31 @@ def test_instantiate_component_omitting_arguments_with_default(
arguments={
'input1': 'hello',
'input2': 100,
'input3': '3.14',
})

def test_instantiate_component_with_positional_arugment(self):
with self.assertRaisesRegex(
TypeError,
'Components must be instantiated using keyword arguments. Positional '
'parameters are not allowed \(found 3 such parameters for component '
'"component_1"\).'):
'Components must be instantiated using keyword arguments.'
' Positional parameters are not allowed \(found 3 such'
' parameters for component "component_1"\).'):
component_op('abc', 1, 2.3)

def test_instantiate_component_with_unexpected_keyword_arugment(self):
with self.assertRaisesRegex(
TypeError,
'component_1\(\) got an unexpected keyword argument "input4".'):
component_op(input1='abc', input2=1, input3=2.3, input4='extra')
'component_1\(\) got an unexpected keyword argument "input0".'):
component_op(input1='abc', input2=1, input3=2.3, input0='extra')

def test_instantiate_component_with_missing_arugments(self):
with self.assertRaisesRegex(
TypeError,
'component_1\(\) missing 1 required positional argument: input1.'
):
'component_1\(\) missing 1 required argument: input1.'):
component_op(input2=1)

with self.assertRaisesRegex(
TypeError,
'component_1\(\) missing 2 required positional arguments: input1,input2.'
'component_1\(\) missing 2 required arguments: input1, input2.'
):
component_op()

Expand Down
23 changes: 11 additions & 12 deletions sdk/python/kfp/v2/components/component_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,19 +226,18 @@ def extract_component_interface(func: Callable) -> structures.ComponentSpec:
else:
io_name = _maybe_make_unique(io_name, input_names)
input_names.add(io_name)
input_spec = structures.InputSpec(
type=type_struct, description=doc_dict.get(parameter.name))
if parameter.default is not inspect.Parameter.empty:
# input_spec.optional = True
if parameter.default is not None:
outer_type_name = list(type_struct.keys())[0] if isinstance(
type_struct, dict) else type_struct
try:
input_spec.default = parameter.default
except Exception as ex:
warnings.warn(
'Could not serialize the default value of the'
' parameter "{}". {}'.format(parameter.name, ex))
input_spec = structures.InputSpec(
type=type_struct,
description=doc_dict.get(parameter.name),
default=parameter.default,
)
else:
input_spec = structures.InputSpec(
type=type_struct,
description=doc_dict.get(parameter.name),
)

inputs[io_name] = input_spec

#Analyzing the return type annotations.
Expand Down
14 changes: 13 additions & 1 deletion sdk/python/kfp/v2/components/structures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,23 @@ class InputSpec(BaseModel):
type: The type of the input.
default: Optional; the default value for the input.
description: Optional: the user description of the input.
optional: Wether the input is optional. An input is optional when it has
an explicit default value.
"""
# TODO(ji-yaqi): Add logic to cast default value into the specified type.
type: str
default: Optional[Union[str, int, float, bool, dict, list]] = None
description: Optional[str] = None
_optional: bool = pydantic.PrivateAttr()

def __init__(self, **data):
super().__init__(**data)
# An input is optional if a default value is explicitly specified.
self._optional = 'default' in data

@property
def optional(self) -> bool:
return self._optional


class OutputSpec(BaseModel):
Expand Down Expand Up @@ -578,7 +590,7 @@ def save_to_component_yaml(self, output_file: str) -> None:
"""
with open(output_file, 'a') as output_file:
json_component = self.json(
exclude_none=True, exclude_unset=True, by_alias=True)
exclude_none=False, exclude_unset=True, by_alias=True)
yaml_file = yaml.safe_dump(
json.loads(json_component),
default_flow_style=None,
Expand Down
6 changes: 4 additions & 2 deletions sdk/python/kfp/v2/components/structures_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
V2_YAML_IF_PLACEHOLDER = textwrap.dedent("""\
name: component_if
inputs:
optional_input_1: {type: String}
optional_input_1: {type: String, default: null}
implementation:
container:
image: alpine
Expand Down Expand Up @@ -75,7 +75,9 @@
'default',
]))
])),
inputs={'optional_input_1': structures.InputSpec(type='String')},
inputs={
'optional_input_1': structures.InputSpec(type='String', default=None)
},
)

V1_YAML_CONCAT_PLACEHOLDER = textwrap.dedent("""\
Expand Down