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

Conversation

memona008
Copy link
Contributor

@memona008 memona008 commented Mar 6, 2024

Resolves 300

Changes in this PR:

  • Added convert expression function to convert YAML lists as list expressions, which behaves the same as pipelines on the web application
  • Add pipeline parameters support for PipelineConverter

@memona008 memona008 marked this pull request as ready for review March 6, 2024 13:10
@memona008 memona008 requested a review from akx March 6, 2024 13:10
@memona008 memona008 self-assigned this Mar 6, 2024
@memona008 memona008 requested a review from ruksi March 11, 2024 09:23
@ruksi ruksi removed their request for review April 12, 2024 07:40
@hylje hylje assigned hylje and unassigned memona008 May 16, 2024
@hylje
Copy link
Contributor

hylje commented May 16, 2024

Adopting this branch and PR with a slightly different approach

@hylje hylje force-pushed the memona/support-list-as-pipeline-parameter branch 3 times, most recently from 2f585ca to aab8353 Compare May 16, 2024 13:33
@hylje
Copy link
Contributor

hylje commented May 16, 2024

With these changes, you can pass YAML pipeline config parameter default lists into the API using valohai-cli. It is not yet possible to pass custom list parameters on the vh pipeline run command line.

@hylje hylje requested review from akx and ruksi May 16, 2024 13:36
@hylje hylje force-pushed the memona/support-list-as-pipeline-parameter branch 2 times, most recently from 29e324b to 108f642 Compare May 23, 2024 14:23
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.

Comment on lines +89 to +95
args = {
"param0-float": 1.5,
"param0-int": 3,
"param0-string": "iceberg",
"param0-flag": True,
"listings": ["msg", "pfa"],
}
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.

@dangquangdon dangquangdon merged commit 4e154b4 into master May 31, 2024
6 checks passed
@dangquangdon dangquangdon deleted the memona/support-list-as-pipeline-parameter branch May 31, 2024 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow using lists for pipeline parameters form the CLI
4 participants