Repository navigation
[Suggestion] Why is the input value field so permissive? #164
Description
The input node value field is typed to be literally anything, but my abstract understanding is that it ought to be some sort of "primitive" data. Concretely, the current implementation does not have the juice to support the arbitrary type:
import numpy as np
from python_workflow_definition import models
inp_model = models.PythonWorkflowDefinitionInputNode.model_validate(
{
"id": 0,
"type": "input",
"name": "x",
# "value": 1, # Case 1
# "value": (1, 2), # Case 2
# "value": np.arange(5) # Case 3
}
)
print(inp_model.model_dump_json(indent=2))
def cycle(model: models.PythonWorkflowDefinitionInputNode) -> bool:
reloaded = type(model).model_validate(
model.model_dump(mode="json"),
)
return model == reloaded
cycle(inp_model)- Case 1: Totally fine
- Case 2: Dumps to something pretty reasonable, but doesn't cycle
- Case 3: Won't validate because the data is not serializable
Is the typing here simply too permissive, or have I misunderstood the intent? If the latter, is there some plan for how to technically support non-primitive data?
TBH, I'm not exactly sure what I mean by primitive, but I guess something in this direction:
from typing import Union
JsonPrimitive = Union[str, int, float, bool, None]
JsonValue = Union[JsonPrimitive, list["JsonValue"], dict[str, "JsonValue"]]
Activity
jan-janssen commentedon Feb 19, 2026
I agree we currently only support data types which are JSONable and ideally would focus on primitive types. For the first publication we did not explicitly enforce this restriction. Still I agree the
pydanticscheme should be extended to enforce this. I would even be strict and go for:jan-janssen commentedon Feb 20, 2026
Maybe we could go for:
liamhuber commentedon Feb 20, 2026
(1) You will need to be extremely careful (de)serializing when supporting list -- the tuples and lists both turn to lists in json, so you'll need to catch tuples and forbid them during validation lest they be accepted by the recipe and serialized to list (which is weird, right? because tuples are more restrictive/primitive than lists yet we would be allowing lists)
(2) This lacks recursion, which is fine for our immediate needs, but is sort of a surprise. I think you want something like
JsonValue = Union[JsonPrimitive, list["JsonValue"], dict[str, "JsonValue"]](I'm not positive what the recursion syntax is here, but I believe it is doable)(3) Why restrict dict keys to strings? Why not ints, etc? This is again a sort of surprise. I'm not actually sure what would be optimal here (an argument, IMO, for not having it at all), but it would be nice to have a justification for string keys
(4) And a question: What is actually the intent of the value field on inputs? We have here recipes -- not for baking a cake, where the ideal recipe specifies exactly all the inputs because the goal is to obtain always the same optimal output, but for defining a process where I very explicitly expect people to vary inputs in order to obtain a variety of outputs. Do they then represent suggested default values? If this is the case, I would actually suggest that it is doable but unpythonic to encourage mutable defaults (
dictandlist).In this case,
tupleis actually still doable (cf. pyiron/flowrep#146). It would require some of the technical infrastructure in (1), and reworking the examples to use(3, 3, 3)instead of[3, 3, 3](similar for the range), but otherwise, IMO, strikes a decent balance between giving us a rigorous recipe definition while supporting existing examples.jan-janssen commentedon Feb 20, 2026
liamhuber commentedon Feb 20, 2026
(1-3) 👍
(4) This is the python workflow definition, and mutable defaults are a known "gotcha" in python; I don't see why should support them.
I posted a new PR that recursively supports types that are both immutable and json-primitive. The kmesh and range issue is fully resolved, since the relevant functions digest the numbers just as well with a tuple as they did with a list. The only outstanding issue is node 14 in the QE example, which sets
"value": {"Al": "Al.pbe-n-kjpaw_psl.1.0.0.UPF"}.You could add
dict[str, AllowableDefaults]to the allowable type, and similarly converttuple[AllowableDefaults, ...]tolist[AllowableDefaults]to get your vision in #166 but with recursion.In
flowrepandsemantikonthis is not artificial for me -- we parse recipes directly from python code. There, having(/allowing/encouraging) mutable defaults really hurts us, because then the python code itself becomes less good. I am disinclined to support it. I find it both clearer and more robust to draw the defaults line at immutable+serializable.jan-janssen commentedon Feb 20, 2026
I see the PWD primarily as a tool for scientists and while I agree with the challenges of mutable defaults, Python still allows it and to my experience scientists use it frequently, so I am reluctant to restrict them more than Python does. In addition, the supported workflow management systems also support both lists and dictionaries as input.
[-]Why is the input value field so permissive?[/-][+][Suggestion] Why is the input value field so permissive?[/+]XzzX commentedon Feb 23, 2026
Mutable default values in python are considered evil for good reason.
A lot of things which python allows you to do are things you should not do.
However, here we deal with default values in python recipes, they do not need to follow the same semantics as default values in python functions. If we do deep copies every time. I think they are ok.
Not talking about the question if they should be included in the recipe or not.
jan-janssen commentedon Feb 23, 2026
While the Python Workflow Definition does not enforce deep copies, I think this is anyway good practice in these graph-based workflows to make sure the workflow framework is able to track the dataflow.
liamhuber commentedon Feb 23, 2026
And I think it is not too much to ask that the WfMS developers to remember to do this, although it still presents a risk point. Per my previous comment, my bigger concern here is that one of our longstanding goals has been to write these workflows as plain python, and in this case parse the defaults from there. And in that case, these default values are coming directly from the python definitions. IMO by supporting mutable recipe defaults among our very limited selection of allowable defaults, we then not only tolerate mutable defaults but encourage their use among node developers. That might be fine when those functions get used in a WfMS that is clever enough to force-deep-copy the nodes, but makes the raw functions themselves less good.
liamhuber commentedon Mar 3, 2026
Closed by #166