Repository navigation
Output nodes, global parameter names, and default port handling in PWD format #88
Description
In its current state, the PWD also contains the inputs with concrete values, e.g., for the simple arithmetic workflow
{
"nodes": [
{"id": 0, "function": "workflow.get_prod_and_div"},
{"id": 1, "function": "workflow.get_sum"},
{"id": 2, "value": 1},
{"id": 3, "value": 2}
],
"edges": [
{"target": 0, "targetPort": "x", "source": 2, "sourcePort": null},
{"target": 0, "targetPort": "y", "source": 3, "sourcePort": null},
{"target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod"},
{"target": 1, "targetPort": "y", "source": 0, "sourcePort": "div"}
]
}We see this as problematic for a few reasons: On one hand, it is somewhat inconsistent, as it means that data nodes should explicitly be part of the workflow definition. However, neither the result output is part of the PWD graph representation as data nodes nor are the intermediate values, e.g., prod and div. Instead, in the current representation, the prod and div outputs are only represented by the edges. If some data objects are part of the PWD, then all of them should be.
Further, providing concrete input values means that the definition contained in the JSON PWD corresponds to a concrete workflow "instance" rather than just the general workflow logic. I recall that this was also brought up in one of the comments on the paper draft, which mentioned that we should showcase that it’s possible to modify the input values after loading a workflow into a framework from the PWD.
Therefore, we propose to remove the data nodes from the "nodes" section of the PWD JSON and instead, to still keep the relevant information, to add it to the "edges" section. This would give the following PWD:
{
"nodes": [
{"id": 0, "function": "workflow.get_prod_and_div"},
{"id": 1, "function": "workflow.get_sum"}
],
"edges": [
{"target": 0, "targetPort": "x", "source": null, "sourcePort": null},
{"target": 0, "targetPort": "y", "source": null, "sourcePort": null},
{"target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod"},
{"target": 1, "targetPort": "y", "source": 0, "sourcePort": "div"},
{"target": null, "targetPort": null, "source": 1, "sourcePort": "result"}
]
}That means that the "global" inputs and outputs of the workflow are represented by "dangling" edges of the workflow graph. While it's not ideal, we think this is fine for now to keep the modifications minimal. Thus, with this modification, any unused return values of intermediate functions would automatically become global outputs of the workflow. One could instead add additional keys to the edges such as workflow_input_name and workflow_output_name to explicitly expose those, e.g.:
{
"nodes": [
{"id": 0, "function": "workflow.get_prod_and_div"},
{"id": 1, "function": "workflow.get_sum"}
],
"edges": [
{"target": 0, "targetPort": "x", "source": null, "sourcePort": null, "workflowInputName": "a"},
{"target": 0, "targetPort": "y", "source": null, "sourcePort": null, "workflowInputName": "b"},
{"target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod"},
{"target": 1, "targetPort": "y", "source": 0, "sourcePort": "div"},
{"target": null, "targetPort": null, "source": 1, "sourcePort": "result", "workflowOutputName": "c"}
]
}Or add a global_ports section to the PWD. To be thought about in the future.
Ping @mbercx and @giovannipizzi.
Activity
jan-janssen commentedon May 2, 2025
I consider the value nodes primarily default inputs. The default inputs of the workflow can be different from the default inputs of the individual functions, so I would like to include them in the Python Workflow Definition. In terms of the edges I aimed to follow the ELK format so I am reluctant to store the output names on the workflow. Still I agree that it makes sense to include the final output in the Python Workflow Definition. My recommendation would again be to have a special node for this final output.
GeigerJ2 commentedon May 2, 2025
Hi @jan-janssen, thanks for your quick response! Overall, I'm fine if we keep the default values in the definition. The issues are more the different treatment of the input and output data nodes, as well as being more explicit in the PWD.
To achieve this, we just thought about this format:
{ "nodes": [ {"id": 0, "type": "function", "value": "workflow.get_prod_and_div"}, {"id": 1, "type": "function", "value": "workflow.get_sum"}, {"id": 2, "type": "input", "name": "a", "defaultValue": 1}, {"id": 3, "type": "input", "name": "b", "defaultValue": 2}, {"id": 4, "type": "output", "name": "c"} ], "edges": [ {"target": 0, "targetPort": "x", "source": 2, "sourcePort": null}, {"target": 0, "targetPort": "y", "source": 3, "sourcePort": null}, {"target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod"}, {"target": 1, "targetPort": "y", "source": 0, "sourcePort": "div"}, {"target": 4, "targetPort": null, "source": 1, "sourcePort": "result"} ] }This would be more in line with the original PWD, and would avoid the "dangling" edges. Apart from the one additional node and edge for the output, the
namekey would allow for a clear assignment on the workflow level (e.g., what if the workflow has two functions and both take an input value "x", so we cannot expose "x" directly), as well as the type of the node, and thedefaultValue.Or, an alternative approach that would contain the same relevant information would be to add explicit inputs and outputs sections:
{ "nodes": [ { "id": 0, "function": "workflow.get_prod_and_div" }, { "id": 1, "function": "workflow.get_sum" } ], "edges": [ { "target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod" }, { "target": 1, "targetPort": "y", "source": 0, "sourcePort": "div" } ], "inputs": [ { "name": "a", "target": 0, "targetPort": "x", "defaultValue": 1 }, { "name": "b", "target": 0, "targetPort": "y", "defaultValue": 2 } ], "outputs": [{ "name": "c", "source": 1, "sourcePort": "result" }] }However, as this is quite different from the PWD in its current state, I'd probably go with the first option.
jan-janssen commentedon May 2, 2025
I really like this first suggestion and it should be quick to implement.
GeigerJ2 commentedon May 2, 2025
Great, yeah, I agree. I'll see to make a PR to fix that in the AiiDA code. Just a note that we could use "value" for the function module path rather than "function" again, as we have that already as the type. Just forgot to modify that before posting in my comment here.
jan-janssen commentedon May 3, 2025
Maybe one small suggestion to simplify the parsing - replace
"defaultValue"also with"value":{ "nodes": [ {"id": 0, "type": "function", "value": "workflow.get_prod_and_div"}, {"id": 1, "type": "function", "value": "workflow.get_sum"}, {"id": 2, "type": "input", "name": "a", "value": 1}, {"id": 3, "type": "input", "name": "b", "value": 2}, {"id": 4, "type": "output", "name": "c"} ], "edges": [ {"target": 0, "targetPort": "x", "source": 2, "sourcePort": null}, {"target": 0, "targetPort": "y", "source": 3, "sourcePort": null}, {"target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod"}, {"target": 1, "targetPort": "y", "source": 0, "sourcePort": "div"}, {"target": 4, "targetPort": null, "source": 1, "sourcePort": "result"} ] }[-]Remove explicit data nodes from the PWD and represent by edges[/-][+]Output nodes, global parameter names, and default port handling in PWD format[/+]jan-janssen commentedon May 11, 2025
I updated the functionality in #95 and #96 . It is now implemented as:
{ "nodes": [ {"id": 0, "type": "function", "value": "workflow.get_prod_and_div"}, {"id": 1, "type": "function", "value": "workflow.get_sum"}, {"id": 2, "type": "input", "value": 1, "name": "x"}, {"id": 3, "type": "input", "value": 2, "name": "y"}, {"id": 4, "type": "output", "name": "result"} ], "edges": [ {"target": 0, "targetPort": "x", "source": 2, "sourcePort": null}, {"target": 0, "targetPort": "y", "source": 3, "sourcePort": null}, {"target": 1, "targetPort": "x", "source": 0, "sourcePort": "prod"}, {"target": 1, "targetPort": "y", "source": 0, "sourcePort": "div"}, {"target": 4, "targetPort": null, "source": 1, "sourcePort": null} ] }jan-janssen commentedon May 15, 2025
I close this issue, as I have the feeling the recent pull requests mentioned in the comment above fulfill this requirement.