[Suggestion] Why is the input value field so permissive?

[liamhuber]

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"]]

[jan-janssen]

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 pydantic scheme should be extended to enforce this. I would even be strict and go for:

JsonPrimitive = Union[str, int, float, bool, None]

[jan-janssen]

Maybe we could go for:

from typing import Union

JsonPrimitive = Union[str, int, float, bool, None]
JsonValue = Union[JsonPrimitive, list[JsonPrimitive], dict[str, JsonPrimitive]]

[liamhuber]

(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 (dict and list).
In this case, tuple is 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]

1. I guess this is an inherent risk of using JSON as serialization.
2. That is also fine for me, especially as it would prepare us for https://github.com/pythonworkflow/proposal-for-data-export
3. Again, I guess this is an inherent risk of using JSON as serialization. As far as I know dictionaries in JSON are restricted to have strings as keys. https://docs.python.org/3/library/json.html#json.dumps
4. Yes, they are intended as default values, to get a minimal working example. While I agree with the assessment of list versus tuple, that still leaves the challenge of using dictionaries. So I would prefer to use lists and dicts as they are more commonly used in the python community compared to tuples. I feel if we are too strict we artificially limit the application of the PWD.

[liamhuber]

(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 convert tuple[AllowableDefaults, ...] to list[AllowableDefaults] to get your vision in #166 but with recursion.

In flowrep and semantikon this 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]

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.