Add parameter info to argo workflow template annotations #1549
Conversation
rohanrebello
changed the title
rohanrebello
changed the title
metaflow/parameters.py
Outdated
| @@ -40,7 +40,7 @@ | |||
|
|
|||
|
|
|||
| class JSONTypeClass(click.ParamType): | |||
| name = "JSON" | |||
| name = __name__ = "JSON" | |||
There was a problem hiding this comment.
Needed to supply the type information in a standardized way across all types. The rest of the types are builtins which already have this field.
| @@ -401,6 +401,8 @@ def _process_parameters(self): | |||
| ) | |||
| seen.add(norm) | |||
|
|
|||
| param_type = str(param.kwargs.get("type").__name__) | |||
There was a problem hiding this comment.
Flatten the type information to a string instead of the type Object (which isn't serializable either). Use just the name of the type rather than the <class '...'> stuff.
| parameters[param.name] = dict( | ||
| name=param.name, | ||
| value=value, | ||
| value=default_value, |
There was a problem hiding this comment.
It would be nice to rename this in the parameter map too but I didn't want to get too ambitious, especially since it's serialized in the argo workflow template as value again. #ifItAintBroke
There was a problem hiding this comment.
Confirmed UI is getting
"metaflow/parameters": "{\"alpha\": {\"name\": \"alpha\", \"value\": \"0.01\", \"type\": \"float\", \"description\": \"Learning rate\", \"is_required\": false}, \"Booly\": {\"name\": \"Booly\", \"value\": null, \"type\": \"bool\", \"description\": null, \"is_required\": true}}",
| if param.kwargs.get("type") == JSONType or isinstance( | ||
| param.kwargs.get("type"), FilePathClass |
There was a problem hiding this comment.
@savingoyal should I use isinstance() for both comparisons? For some reason I didn't fully understand, only the FilePathClass required the use of it and it failed with the == comparison.
| @@ -574,6 +584,10 @@ def _compile_workflow_template(self): | |||
| "metaflow/user": "argo-workflows", | |||
| "metaflow/flow_name": self.flow.name, | |||
| } | |||
|
|
|||
| if self.parameters: | |||
| annotations.update({"metaflow/parameters": json.dumps(self.parameters)}) | |||
There was a problem hiding this comment.
Serialize the parameters dictionary into the metaflow/parameters annotation for consumers to deserialize.
This PR adds information on which parameters are required, to the annotations section of argo-workflow templates. This can be used to validate the well-formed-ness of workflow submissions before submitting the workflow to argo-workflows. Before this PR the only way to validate parameters was to submit the workflow and wait until metaflow runs within the argo-workflow, only to fail at parameter validation. After this change, we can fail-fast and, in the process, save on compute as well as etcd storage.
Manually tested for the following parameter types both locally and in argo-workflows: