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.

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

Support list as pipeline parameter expression #147

Merged
merged 4 commits into from
May 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions examples/pipeline-with-parameters-example.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@
type: string
- name: param0-flag
type: flag
- name: listings
default: [all_harbors, all_uploads, all_uploads2]
type: string
multiple: separate
multiple-separator: ","
optional: false

- pipeline:
name: Example Pipeline with Parameters
Expand Down Expand Up @@ -45,6 +51,11 @@
targets:
- train.parameter.param0-flag
default: false
- name: listings
targets:
- train.parameter.listings
- train_parallel.parameter.listings
default: [abc, def, ghi]
nodes:
- name: train
step: train_step
Expand Down
4 changes: 2 additions & 2 deletions tests/__snapshots__/test_validation.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,10 @@
# name: test_bad_examples_cli[step-missing-required-properties.yaml]
'''
>>> step-missing-required-properties.yaml
error: Oneof validation on step.command.oneOf: 8 is not valid under any of the given schemas (1.step.command)
------------------------------------------------------------
error: Type validation on step.image.type: True is not of type 'string' (0.step.image)
------------------------------------------------------------
error: Oneof validation on step.command.oneOf: 8 is not valid under any of the given schemas (1.step.command)
------------------------------------------------------------
error: Required validation on step.required: 'command' is a required property (0.step)
------------------------------------------------------------
error: Required validation on step.required: 'name' is a required property (0.step)
Expand Down
37 changes: 35 additions & 2 deletions tests/test_pipeline_conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,38 @@ def test_pipeline_parameter_conversion(pipeline_with_parameters_config):
# When pipeline parameter has no default value, the expression should be empty
parameter_config = next(param for param in pipe.parameters if param.name == parameter_name)
expression_value = parameter_config.default if parameter_config.default is not None else ""
assert parameter["expression"] == expression_value
assert type(parameter["expression"]) == type(expression_value)
# When pipeline parameter is list it should be converted to a variant parameter
if isinstance(expression_value, list):
assert parameter["expression"] == {
"style": "single",
"rules": {"value": expression_value},
}
else:
assert parameter["expression"] == expression_value
assert type(parameter["expression"]) is type(expression_value)


def test_pipeline_parameter_conversion_with_args(pipeline_with_parameters_config):
pipeline = pipeline_with_parameters_config.pipelines["Example Pipeline with Parameters"]
args = {
"param0-float": 1.5,
"param0-int": 3,
"param0-string": "iceberg",
"param0-flag": True,
"listings": ["msg", "pfa"],
}
Comment on lines +89 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this come from in practice?

Copy link
Contributor

@hylje hylje May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added pipeline argument parsing when converting for a future CLI enhancement. This PR lets you send lists as pipeline parameters if you make the values you want default in YAML config, but CLI needs some extra stuff to let you specify what you want to send and override the defaults. That extra stuff should also get converted right away, so we don't need to reimplement the conversion on CLI end.

result = PipelineConverter(
config=pipeline_with_parameters_config,
commit_identifier="latest",
parameter_arguments=args,
).convert_pipeline(pipeline)
params = result["parameters"]

assert params["param0-float"]["expression"] == 1.5
assert params["param0-int"]["expression"] == 3
assert params["param0-string"]["expression"] == "iceberg"
assert params["param0-flag"]["expression"]
assert params["listings"]["expression"] == {
"style": "single",
"rules": {"value": ["msg", "pfa"]},
}
2 changes: 1 addition & 1 deletion valohai_yaml/objs/pipelines/pipeline_parameter.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def __init__(
name: str,
targets: Optional[Union[List[str], str]] = None,
value: Optional[str] = None,
default: Optional[str] = None,
default: Optional[Union[List[str], str]] = None,
category: Optional[str] = None,
description: Optional[str] = None,
) -> None:
Expand Down
32 changes: 30 additions & 2 deletions valohai_yaml/pipelines/conversion.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Any, Dict, List, TypedDict, Union
from typing import Any, Dict, List, Optional, TypedDict, Union

from valohai_yaml.objs import (
Config,
Expand All @@ -14,6 +14,16 @@
ConvertedObject = Dict[str, Any]


class VariantExpression(TypedDict):
"""Variant expression template."""

style: str
rules: Dict[str, Any]


ExpressionValue = Union[str, int, bool, float, VariantExpression]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would this VariantExpression dictionary affect they way roi and/or valohai-utils parsing the value now? Btw, should we call it ExpressionValueVariant?

Copy link
Contributor

@hylje hylje May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend accepts variant expressions as parameter values, and it is the only way it accepts list parameters. The format works for all parameter value types though.

Valohai-utils is not involved in this part of the process at all. We pass a list in structured format to backend, and backend receives a list in structured format and keeps it as a list.

When the execution is prepared by backend, if you pass a list parameter as a command line parameter and parse command line parameters in a Python script using valohai-utils, it must use comma separated values as that is the only format valohai-utils accepts. Other scripts or programs can accept different parameter passing styles, which is why we support custom multiple-separators.

You can also use file based configuration to read the structured list parameter as JSON (valohai/config/parameters.json), which skips the whole parsing step and you get the original list. This is what valohai-utils automatically does for parameters that haven't been overridden as command line parameters.


class ConvertedPipeline(TypedDict):
"""TypedDict for converted Pipeline object."""

Expand All @@ -30,9 +40,11 @@ def __init__(
*,
config: Config,
commit_identifier: str,
parameter_arguments: Optional[Dict[str, Union[str, list]]] = None,
) -> None:
self.config = config
self.commit_identifier = commit_identifier
self.parameter_arguments = parameter_arguments or {}

def convert_pipeline(self, pipeline: Pipeline) -> ConvertedPipeline:
return {
Expand All @@ -43,9 +55,17 @@ def convert_pipeline(self, pipeline: Pipeline) -> ConvertedPipeline:

def convert_parameter(self, parameter: PipelineParameter) -> ConvertedObject:
"""Convert a pipeline parameter to a config-expression payload."""
param_value: Union[ExpressionValue, List[str]]
if parameter.name in self.parameter_arguments:
param_value = self.parameter_arguments[parameter.name]
elif parameter.default is not None:
param_value = parameter.default
else:
param_value = ""

return {
"config": {**parameter.serialize()},
"expression": parameter.default if parameter.default is not None else "",
"expression": self.convert_expression(param_value),
}

def convert_node(self, node: Node) -> ConvertedObject:
Expand Down Expand Up @@ -102,3 +122,11 @@ def convert_executionlike_node(
}

return node_data

def convert_expression(self, expression: Union[ExpressionValue, list]) -> ExpressionValue:
if isinstance(expression, list):
return VariantExpression(
style="single",
rules={"value": expression},
)
return expression
Loading