feat(sdk): Use google.protobuf.Value in v2 for passing parameters. #6804
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,3 +11,5 @@ | |
| # 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. | ||
|
|
||
| __path__ = __import__("pkgutil").extend_path(__path__, __name__) | ||
|
There was a problem hiding this comment. I think this is probably unnecessary because pipelines/api/v2alpha1/python/setup.py Line 87 in 4360652 ref: https://packaging.python.org/guides/packaging-namespace-packages/#native-namespace-packages In comparison, to make the existing pipelines/sdk/python/kfp/__init__.py Lines 15 to 17 in 4360652 Also this change is not released, so it can't be needed for this PR, right? There was a problem hiding this comment. You're right, left over while trying to get both namespace packages to work nicely while locally installed. Removed. |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -549,37 +549,120 @@ def _resolve_condition_operands( | |
| operand2: Union[str, dsl.PipelineParam]) -> Tuple[str, str]: | ||
| """Resolves values and PipelineParams for condition operands.""" | ||
|
|
||
| for value_or_reference in [operand1, operand2]: | ||
| if not isinstance(value_or_reference, | ||
| (dsl.PipelineParam, int, float, bool, str)): | ||
| raise ValueError('Conditional requires scalar constant values' | ||
| ' for comparison. Found "{}" of type {}' | ||
| ' in pipeline definition instead.'.format( | ||
| value_or_reference, | ||
| type(value_or_reference))) | ||
|
|
||
| # Check specified type of PipelineParam is a scalar as well. | ||
| if isinstance(value_or_reference, dsl.PipelineParam): | ||
| parameter_type = type_utils.get_parameter_type( | ||
| value_or_reference.param_type) | ||
|
|
||
| if parameter_type in [ | ||
| pipeline_spec_pb2.ParameterType.STRUCT, | ||
| pipeline_spec_pb2.ParameterType.LIST, | ||
| pipeline_spec_pb2.ParameterType | ||
| .PARAMETER_TYPE_ENUM_UNSPECIFIED, | ||
| ]: | ||
| input_name = dsl_component_spec.additional_input_name_for_pipelineparam( | ||
| value_or_reference) | ||
| raise ValueError( | ||
| 'Conditional requires scalar parameter values' | ||
| ' for comparison. Found input "{}" of type {}' | ||
| ' in pipeline definition instead.'.format( | ||
| input_name, value_or_reference.param_type)) | ||
|
|
||
| parameter_types = set() | ||
| for value_or_reference in [operand1, operand2]: | ||
| if isinstance(value_or_reference, dsl.PipelineParam): | ||
| parameter_type = type_utils.get_parameter_type( | ||
| value_or_reference.param_type) | ||
| else: | ||
| parameter_type = type_utils.get_parameter_type( | ||
| type(value_or_reference).__name__) | ||
| print('{} GOT TYPE: {}, typename {}, isinstance_dict {}'.format( | ||
| value_or_reference, parameter_type, | ||
| str(type(value_or_reference)), | ||
| isinstance(value_or_reference, dict))) | ||
|
|
||
| parameter_types.add(parameter_type) | ||
|
|
||
| if len(parameter_types) == 2: | ||
| # Two different types being compared. The only possible types are | ||
| # String, Boolean, Double and Integer. We'll promote the other type | ||
| # using the following precedence: | ||
| # String > Boolean > Double > Integer | ||
|
There was a problem hiding this comment. I wasn't aware that CEL can work this way. There was a problem hiding this comment. Go for it :-) I used this as my reference btw: https://github.com/google/cel-spec/blob/master/doc/langdef.md#list-of-standard-definitions |
||
| if pipeline_spec_pb2.ParameterType.STRING in parameter_types: | ||
| canonical_parameter_type = pipeline_spec_pb2.ParameterType.STRING | ||
| elif pipeline_spec_pb2.ParameterType.BOOLEAN in parameter_types: | ||
| canonical_parameter_type = pipeline_spec_pb2.ParameterType.BOOLEAN | ||
| else: | ||
| # Must be a double and int, promote to double. | ||
| assert pipeline_spec_pb2.ParameterType.NUMBER_DOUBLE in parameter_types, 'Types: {} [{} {}]'.format( | ||
| parameter_types, operand1, operand2) | ||
| assert pipeline_spec_pb2.ParameterType.NUMBER_INTEGER in parameter_types, 'Types: {} [{} {}]'.format( | ||
| parameter_types, operand1, operand2) | ||
| canonical_parameter_type = pipeline_spec_pb2.ParameterType.NUMBER_DOUBLE | ||
| elif len(parameter_types) == 1: # Both operands are the same type. | ||
| canonical_parameter_type = parameter_types.pop() | ||
| else: | ||
| # Probably shouldn't happen. | ||
| raise ValueError('Unable to determine operand types for' | ||
| ' "{}" and "{}"'.format(operand1, operand2)) | ||
|
|
||
| operand_values = [] | ||
| for value_or_reference in [operand1, operand2]: | ||
| if isinstance(value_or_reference, dsl.PipelineParam): | ||
| input_name = dsl_component_spec.additional_input_name_for_pipelineparam( | ||
| value_or_reference) | ||
| operand_value = "inputs.parameter_values['{input_name}']".format( | ||
| input_name=input_name) | ||
| parameter_type = type_utils.get_parameter_type( | ||
| value_or_reference.param_type) | ||
| if parameter_type == pipeline_spec_pb2.ParameterType.NUMBER_INTEGER: | ||
| operand_value = 'int({})'.format(operand_value) | ||
| elif isinstance(value_or_reference, str): | ||
| operand_value = "'{}'".format(value_or_reference) | ||
| parameter_type = pipeline_spec_pb2.ParameterType.STRING | ||
| elif isinstance(value_or_reference, bool): | ||
| # Booleans need to be compared as 'true' or 'false' in CEL. | ||
| operand_value = str(value_or_reference).lower() | ||
| parameter_type = pipeline_spec_pb2.ParameterType.BOOLEAN | ||
| elif isinstance(value_or_reference, int): | ||
| operand_value = str(value_or_reference) | ||
| parameter_type = pipeline_spec_pb2.ParameterType.NUMBER_INTEGER | ||
| else: | ||
| assert isinstance(value_or_reference, float), value_or_reference | ||
| operand_value = str(value_or_reference) | ||
| parameter_type = pipeline_spec_pb2.ParameterType.NUMBER_DOUBLE | ||
|
|
||
| if parameter_type != canonical_parameter_type: | ||
| # Type-cast to so CEL does not complain. | ||
| if canonical_parameter_type == pipeline_spec_pb2.ParameterType.STRING: | ||
| assert parameter_type in [ | ||
| pipeline_spec_pb2.ParameterType.BOOLEAN, | ||
| pipeline_spec_pb2.ParameterType.NUMBER_INTEGER, | ||
| pipeline_spec_pb2.ParameterType.NUMBER_DOUBLE, | ||
| ] | ||
| operand_value = "'{}'".format(operand_value) | ||
| elif canonical_parameter_type == pipeline_spec_pb2.ParameterType.BOOLEAN: | ||
| assert parameter_type in [ | ||
| pipeline_spec_pb2.ParameterType.NUMBER_INTEGER, | ||
| pipeline_spec_pb2.ParameterType.NUMBER_DOUBLE, | ||
| ] | ||
| operand_value = 'true' if int( | ||
| operand_value) == 0 else 'false' | ||
| else: | ||
| assert canonical_parameter_type == pipeline_spec_pb2.ParameterType.NUMBER_DOUBLE | ||
| assert parameter_type == pipeline_spec_pb2.ParameterType.NUMBER_INTEGER | ||
| operand_value = 'double({})'.format(operand_value) | ||
|
|
||
| operand_values.append(operand_value) | ||
|
|
||
| return tuple(operand_values) | ||
|
|
||
|
|
@@ -822,7 +905,7 @@ def _group_to_dag_spec( | |
| _for_loop.LoopArguments.LOOP_ITEM_NAME_BASE) | ||
|
|
||
| subgroup_component_spec.input_definitions.parameters[ | ||
| loop_arguments_item].parameter_type = pipeline_spec_pb2.ParameterType.STRING | ||
| subgroup_task_spec.parameter_iterator.items.input_parameter = ( | ||
| input_parameter_name) | ||
| subgroup_task_spec.parameter_iterator.item_input = ( | ||
|
|
@@ -986,8 +1069,8 @@ def _create_pipeline_spec( | |
|
|
||
| pipeline_spec.pipeline_info.name = pipeline.name | ||
| pipeline_spec.sdk_version = 'kfp-{}'.format(kfp.__version__) | ||
| # Schema version 2.1.0 is required for kfp-pipeline-spec>0.1.3.1 | ||
| pipeline_spec.schema_version = '2.1.0' | ||
|
|
||
| dsl_component_spec.build_component_inputs_spec( | ||
| component_spec=pipeline_spec.root, | ||
|
|
@@ -1170,6 +1253,7 @@ def _create_pipeline_v2( | |
| pipeline_func) | ||
| pipeline_name = pipeline_name or pipeline_meta.name | ||
|
|
||
| print('PIPELINE META: ', pipeline_meta) | ||
| pipeline_root = getattr(pipeline_func, 'pipeline_root', None) | ||
|
|
||
| args_list = [] | ||
|
|
@@ -1204,13 +1288,22 @@ def _create_pipeline_v2( | |
| # Fill in the default values. | ||
| args_list_with_defaults = [] | ||
| if pipeline_meta.inputs: | ||
| args_list_with_defaults = [] | ||
| for input_spec in pipeline_meta.inputs: | ||
| default_value = input_spec.default | ||
|
|
||
| if input_spec.default is not None: | ||
| parameter_type = type_utils.get_parameter_type( | ||
| input_spec.type) | ||
| default_value = type_utils.deserialize_parameter_value( | ||
| value=input_spec.default, parameter_type=parameter_type) | ||
|
|
||
| print('DEFAULT: {}: {}'.format(input_spec.name, default_value)) | ||
| args_list_with_defaults.append( | ||
| dsl.PipelineParam( | ||
| sanitize_k8s_name(input_spec.name, True), | ||
| param_type=input_spec.type, | ||
| value=default_value)) | ||
|
|
||
| # Making the pipeline group name unique to prevent name clashes with templates | ||
| pipeline_group = dsl_pipeline.groups[0] | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the other fields use camel case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, but proto style guide is explicitly underscore_separated_names for fields. This is what's used everywhere else as well:
https://developers.google.com/protocol-buffers/docs/style#message_and_field_names
My suggestion is to adopt the right style here for long term health, and consider replacing the camel case ones in a future PR.