fix(sdk): add default value for inputs #7405
Conversation
|
Skipping CI for Draft Pull Request. |
google-oss-prow
bot
added
size/XL
and removed
do-not-merge/work-in-progress
size/L
labels
|
/test kubeflow-pipelines-samples-v2 |
| @@ -20,13 +20,9 @@ | |||
| from collections import OrderedDict | |||
| from typing import Callable, Dict, List, Optional | |||
|
|
|||
| from deprecated.sphinx import deprecated | |||
There was a problem hiding this comment.
Looks like shutil, kfp.deprecated.components, and kfp.deprecated.dsl are also unused imports. Thoughts on removing those as well?
| @@ -378,6 +374,8 @@ def build_python_component( | |||
| add_files=add_files) | |||
|
|
|||
| logging.info('Building and pushing container image.') | |||
|
|
|||
| from kfp.deprecated.containers._container_builder import ContainerBuilder | |||
There was a problem hiding this comment.
Does moving this import to the top level per pylint c0415 import-outside-toplevel cause any issues?
| return task_factory_function | ||
|
|
||
|
|
||
| @deprecated( |
There was a problem hiding this comment.
Does removing something like this require changes to our release notes/changelog?
There was a problem hiding this comment.
Good call, adding to the release notes. Thanks
| "format": { | ||
| "defaultValue": "csv", | ||
| "parameterType": "STRING" | ||
| }, |
There was a problem hiding this comment.
This is a behavior change. Previously we "hide" this to workaround Vertex Pipelines backend's lack of support for optional inputs. Can we test submitting this and see if that limitation is gone?
There was a problem hiding this comment.
Tried and worked on Optional inputs and xgboost examples.
| continue | ||
|
|
||
| if type_utils.is_parameter_type(input_spec.type): | ||
| component_spec.input_definitions.parameters[ | ||
| input_name].parameter_type = type_utils.get_parameter_type( | ||
| input_spec.type) | ||
| if input_spec.default is not None: |
There was a problem hiding this comment.
Not an issue with this PR, but I do realize this is not a universal solution for all default values. Because default value could be None:
e.g.:
@component
def my_component(some_input: Optional[str] = None):
print(some_input)
The way we support this is to not passing the parameter at all (not showing up in component spec input definitions), so that it pick up the default value from the function.
There was a problem hiding this comment.
I see, does unset behave the same as setting it to None? If they are the same, then probably it's okay here?
| input_dict_parameter: | ||
| parameterType: STRUCT | ||
| message: |
There was a problem hiding this comment.
I'm curious why do we see such diffs? Does that mean the order is nondeterministic? Or the order depends on the specific version of a protobuf library?
/cc @connor-mccarthy
There was a problem hiding this comment.
It appears only dsl.Condition and dsl.ParallelFor compilation steps produce non-deterministically ordered YAML.
I feel reasonably confident that this is not attributed to the actual "write yaml" code, but rather to how these conditions/loops are represented in the in-memory IR. I see we're using a defaultdict(set) here to store this data and I think the use of set could explain the non-determinism.
This probably warrants another PR to investigate: swapping out the defaultdict(set) for defaultdict(list) (and updating downstream methods from .add to .append) to see if this resolves the issue.
There was a problem hiding this comment.
I think we might need to bring back sort_keys=True when exporting json/yaml. Not blocking this PR though.
|
/test kubeflow-pipelines-samples-v2 |
|
/retest-required |
|
/lgtm Thanks, Yaqi! |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test kubeflow-pipelines-samples-v2 |
* fix(sdk): add default value for inputs * merge conflict * release * fix sample
Description of your changes:
Add default value for inputs, if it's specified.
Removed some already-broken deprecated tests for unit tests to pass.