To what extent input connection validation?

[liamhuber]

In #69, #76, #85 and #86, we work fairly hard to validate that the recipes are internally consistent. This includes ensuring that all requested edges can be made given the available nodes and their available IO ports.

One thing that isn't covered is the validation that inputs are sufficiently connected to data. I.e., some internal input of a workflow or other flow control recipe may be left unconnected.

In the case that such a node is an AtomicNode, this might be ok as those can verifiably get default values. We don't ever want to log those actual values here (this is a recipe format and data free), but it may behoove us to at least indicate for which input ports defaults are available.

For the other recipe formats, I had envisioned them being viable alone -- i.e. not parsed from a python function (which might have defaults) but being usable purely from their data format, e.g. read from a JSON file, in which case defaults are never available for any ports.

I'm not sure how I feel about this, perhaps demanding that the recipes be usable without default value specification is too annoying? Or perhaps we want to remain ambivalent and simply not check one way or the other that input is sufficiently provided to all nodes? I am also not completely against the verbose but very reliable validation that all non-atomic nodes are fully input-connected, and that atomic nodes are explicit about which ports can be left safely alone.

[samwaseda]

You mean something like this?

def my_workflow(x):
    result = add(x, y) # y not defined
    return result

My AST parser already checks it - but I guess that's not what you are talking about.

If you want to make sure that the workflow starts with global inputs and ends with global outputs, you can use flowrep.workflow.get_workflow_graph:

wf_dict = my_workflow.serialize_workflow()
G = fwf.get_workflow_graph(wf_dict)

for key, value in G.in_degree:
    if value == 0:
        assert key.startswith("inputs")

for key, value in G.out_degree:
    if value == 0:
        assert key.startswith("outputs")

[liamhuber]

You mean something like this? ...

That is a python example of the situation I'm talking about, yes.

My AST parser already checks it - but I guess that's not what you are talking about.

Indeed, what I'm talking about needs to cover the case of running around with a bunch of JSON files and never looking into the actual .py implementations.

If you want to make sure that the workflow starts with global inputs and ends with global outputs

That's not what I need to ensure. I guess this is already ensured by the fact that all models.NodeModel have explicit inputs and outputs fields they specify. So we're guaranteed this by construction.

I'll post these responses now, and then build a more concrete example of what I mean.

[liamhuber]

Consider a simple nested recipe built around this atomic node:

from flowrep import model, parser

@parser.atomic
def add(x, y=1):
    z = x + y
    return z


add.flowrep_recipe.model_dump(mode="json")
>>> {'type': 'atomic',
...  'inputs': ['x', 'y'],
...  'outputs': ['z'],
...  'fully_qualified_name': '__main__.add',
...  'unpack_mode': 'tuple'}

Where we have a sub-workflow and a parent workflow:

sub_wf = model.WorkflowNode.model_validate(
    {
        "type": "workflow",
        "inputs": ["x", "y"],
        "outputs": ["z"],
        "nodes": {"add_0": add.flowrep_recipe},
        "input_edges": {"add_0.x": "x", "add_0.y": "y"},
        "edges": {},
        "output_edges": {"z": "add_0.z"},
    }
)

wf = model.WorkflowNode.model_validate(
    {
        "type": "workflow",
        "inputs": ["x", "y"],
        "outputs": ["atomic_z", "macro_z"],
        "nodes": {
            "add_0": add.flowrep_recipe,
            "macro": sub_wf,
        },
        "input_edges": {
            "add_0.x": "x",
            # "add_0.y": "y",
            "macro.x": "x",
            # "macro.y": "y",
        },
        "edges": {},
        "output_edges": {
            "atomic_z": "add_0.z",
            "macro_z": "macro.z",
        }
    }
)

Currently these models both validate fine, and my question is whether or not we should accept that.

The missing edge definition # "add_0.y": "y", is OK because add has a default value for y -- but we don't know that. If we stored which inputs do(n't) have default values in the fields of AtomicNode, we could know this conclusively.

The missing edge definition # "macro.y": "y", is not OK because we explicitly declare the y input to the macro, and explicitly connect the y-value of the child add to it, but never have the opportunity to provide it with data. Thus, the inner add instance is explicitly going to receive non-data at runtime.

I see two ways out, neither of which I like:

1. Include data in the recipes. IMO this is a non-starter. A major point of this level of recipe is that it is data-free and thus serializes in a trivial way.
2. Demand that the ground truth for workflows be a python function definition, and always store that reference in the recipe, at which point we can extract default values from there just as for atomic nodes. This...is a choice we could make. It is very much not my favourite.

So my question then is, should we even validate for this? There are times where we'd be able to catch certainly-invalid recipes, e.g. failing to pass input data to an atomic node that has no default value. On the other hand, it would exclude the possibility of ever relying on default data at the workflow level, which might validate just fine in a more sophisticated recipe style like semantikon where python object type hints and defaults can be available.

[samwaseda]

I would add data to the recipes. To be honest, I think the recipes should have the format that would allow us to enrich it with more metadata, so that the semantikon representation is basically an enriched flowrep representation.

[liamhuber]

I absolutely agree with adding data at the semantikon level. I expect this to be much more useable. However, I am extremely wary of adding it here. First, I do see the merit of having a representation that is effectively 100% references to python operations and graph topology. Second, as soon as we allow data serialization is a huge headache. With an eye on pulling models from here into python_workflow_definition, I think that dodging actual data massively improves both cross-manager interoperability and long term reproducibility goals.

[liamhuber]

I'd like to come back to this. My original concern is resolved, so if you're content @samwaseda I think we could close it and carry any further discussion in a separate issue (probably on the semantikon repo?)

Regarding my original concern, I'm content the the pydantic validation ensures nothing is explicitly wrong, but does not guarantee that the recipe has sufficient edges to run. I.e. the WfMS might load a recipe, try to run it, and get a complaint that a node doesn't have enough data. This is fine, this is just not the responsibility of the pydantic flowrep recipe model layer.

Regarding data, the concept of having data in the base recipe keeps coming up, but I am still completely unconvinced of this. We immediately have two choices: (1) only accept special "privileged" data that can be cleanly JSONified, or (2) always couple the recipes to some sort of other serialization from which we can deserialize arbitrary python objects. I have yet to hear convincing arguments that either of these will be doable in a way that is pleasant for users and supports our goal of a simple and highly interoperable recipe format.

Regarding semantikon -- here I 100% see the need for data, and am totally fine taking route (2) where needed, etc. For semantikon to operate we just need the real python objects: type hints, ontological entities, retrospective graphs with actual data to produce rad knowledge graphs, etc. This all comes at a cost of interoperability -- we're going to require you to be more bought into our ecosystem. Instead of a lightweight flowrep package that you in principle don't even explicitly need because you can interact entirely via JSON, we ask you to accept flowrep, semantikon, and whatever attacks we use for real data (de)serialization (bagofholding?). So I 100% want these things, I just want to separate responsibilities of power vs interoperability between flowrep and semantikon.

I also want semantikon to enrich the recipes here, but what I don't currently have strong feelings on is whether

1. that's via direct inheritance so the semantikon version still has inputs: list[str] and something like ports: dict[str, PortType] where PortType has fields like value, default, annotation¹, or
2. via extension, where semantikon has inputs: dict[str, PortType], and separately stashes away something like flowrep_recipe: flowrep.models.nodes.union.NodeType

Footnotes

1. And whatever else I'm not thinking of now. My point is just that once we care about data we need much richer data ↩