✨ Store a modelled socket through its own model - #814
Draft
elinscott wants to merge 31 commits into
Draft
Conversation
node-graph's socket spec records structured_type extras for enum-typed sockets so coerce_inputs_from_spec can rebuild the member before a task body runs - the declared contract is that the serialized form is the bare value. The AiiDA adapter never honoured it: raw Enum instances reached aiida-pythonjob's general_serializer, which has no serializer for them and failed the whole submission. Flatten enums (recursively, including dict keys/values and list items) before serialize_ports. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
node-graph's socket spec records structured_type extras for enum-typed sockets so coerce_inputs_from_spec can rebuild the member before a task body runs - the declared contract is that the serialized form is the bare value. The AiiDA adapter never honoured it: raw Enum instances reached aiida-pythonjob's general_serializer, which has no serializer for them and failed the whole submission. Flatten enums (recursively, including dict keys/values and list items) before serialize_ports. Add unit tests in tests/test_serializer.py covering _flatten_enums (bare/IntEnum/str-Enum members, dict keys and values, list/tuple, mixed nesting, enum-free passthrough, wrapt-proxied members) plus one end-to-end serialize_ports check that an enum-valued entry serializes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ation The read side does not round-trip Enums: node-graph's coerce_inputs_from_spec records structured_type extras only for dataclass/pydantic/TypedDict, never for Enum sockets, so a task body declaring a plain Enum input receives the bare value (isinstance False, x == Color.RED False), not the member. Correct the _flatten_enums docstring (was claiming a round-trip that does not happen) and add test_body_receives_bare_value_not_member, which drives a real wg.run() and asserts the body sees the bare value - it flips loudly if node-graph later adds enum reconstruction. set/frozenset are not descended into: a set fails in general_serializer regardless of contents (not JSON-serializable, no registered serializer), so flattening enums inside one would not help. Document this and pin it with test_flatten_leaves_sets_untouched. Guard dict-key flattening: two distinct keys collapsing to the same flattened value (an Enum member and its bare value, or two members sharing a .value) now raises instead of silently dropping an entry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test_body_receives_bare_value_not_member asserted that a task body declaring a plain Enum input receives the bare value. That outcome is not this package's to fix: it depends on whether the installed node-graph reconstructs Enum sockets, so the test reports red on a dependency swap rather than on a defect, and its premise was wrong for @task.graph bodies, which receive the stored orm.Str, not a bare value. - Assert the invariant instead: whatever the boundary delivers, Color(c) rebuilds the member that was passed, in both a function task's body and a @task.graph body. - Keep the difference pinned with a second test asserting that what arrives agrees with what the installed node-graph advertises, so a reconstruction that silently stops working still fails here. - Reword the _flatten_enums docstring, which stated the bare value as fact, to state the rule and the portable idiom. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_enum_input_rebuilds_to_the_member_that_was_passed failed in the @task.graph body when the installed node-graph reconstructs the Enum member before the body runs. - Rebuild via Color(getattr(c, 'value', c)) in both observer bodies, so the assertion holds for every form the boundary delivers. - Point the _flatten_enums docstring and the test docstring at that idiom, which they previously gave as Color(c). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_flatten_enums rebuilt every dict/list/tuple unconditionally, so an enum-free namedtuple came back as a plain tuple and an OrderedDict or defaultdict came back as a plain dict, even though nothing needed flattening. Return the original object when no element changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two tests reconstructed an Enum member by hand inside a task body (Color(getattr(c, 'value', c))) to work around the two node-graph arrival forms, contradicting the settled contract that a task body only ever uses the annotation. - observe_enum reports type_name, is_member, equals_member, equals_value; no constructor call on the received value. - observe_enum_in_graph uses the member directly (c.name), matching what a @task.graph body actually receives. - test_enum_arrival_follows_the_node_graph_capability asserts the is_member/equals_* combination for both arrival forms. - test_enum_input_arrives_as_the_member_in_a_graph_body replaces the old rebuild-parity test, skipping (with a stated reason) when the installed node_graph does not reconstruct Enum sockets. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`@task(input_model=M)` / `output_model=M` and `@task.graph(input_model=M)` now reach node-graph's contract machinery from this package's decorators. - Specs are built in aiida-workgraph's socket vocabulary, so a modelled `int` field is a `workgraph.int` socket rather than node-graph's. - `spec_from_model` is re-exported from `socket_spec` bound to that vocabulary, for a task that wants the model's sockets without its rules. - A wrapped executor replaces the spec's, leaving the spec itself inferred from the undecorated function. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A `Decimal` field could not be stored at all: the socket serializes one leaf at a time and the generic serializer has no entry point for the type, so the submission failed at submit time. - When a task's input model owns a socket, the model renders that socket's stored value, so a `field_serializer` decides the stored form. - The walk crosses nested models and typed mappings, so a leaf renders through the model that declares it however deep it sits. - A socket the walk cannot place still falls through to the generic serializer, which fails as loudly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- A model's `Enum` and `Decimal` fields are stored as the values it renders and arrive in the body as the objects it declares; each has a control task with the same sockets and no model. - A rule the socket layer cannot see fails the process that broke it, with a message naming the task and the model. - A graph task's contract is held where the engine expands it, including a subgraph whose bound another task produced. - `design-input-model.md` records the three checkpoints, the content-invariance rule and what was considered and left out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A `@task.graph` under a model with a `str` field failed at expansion: the body is handed storage nodes, so the contract was checked against an `orm.Str` rather than against the string it holds. - The adapter answers node-graph's `to_python` with what each node holds, rendering through aiida-pythonjob's deserializers. - A node no deserializer can render is left as it is, so a contract that names an AiiDA type still sees one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- A graph whose model declares a `str` field runs under the engine; with the reading monkeypatched back to the identity, the same graph fails at expansion and never becomes a process. - The design note and the pull request description say where the reading happens and what it leaves alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A `@task.graph` whose model declared `label: str` refused its own input at expansion: the body was handed the `orm.Str` the write path had stored. - Take the storage node off a socket whose model says the body receives Python, and leave it on for one declaring an AiiDA type or `Any`. - Key that on the model's own declaration rather than the socket identifier, which maps `int` and `orm.Int` to the same `workgraph.int` and so cannot tell the two apart. - Put the tag back on the unwrapped value, so a graph body still draws a link from it instead of holding a copy. - Drop `to_python`, whose reading the deserialization edge now does. - Read the control back through a `Decimal` field, which is `workgraph.annotated`: a control on a `str` field would stop discriminating under an adapter that unwraps by socket identifier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`add_task` now runs the model's check, so `Field(le=100)` given 500 is refused at that line rather than at the run edge. - Assert the refusal at `add_task`, and keep the run-edge behaviour as the control, reached with the write check stubbed out. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pull request description and the design note were tracked, so both appeared in the diff a reviewer reads and would have landed upstream. - Remove them from the repository; both stay in the worktree, untracked. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A plain `str` or `int` behind a tag came back untagged, and every unwrapped value came back under a new uuid, so one value read as two. - Retag whether or not anything came off the value, carrying the tag's uuid over with it. - Replace the tag test, which asserted only a zero exit status, with a check that the subtask reads the very node the graph was given, and a control that unwraps without retagging and loses it. - Add unit checks that a value needing no unwrapping keeps its tag, and that an unwrapped one keeps its uuid. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A task refusing a value with a `@field_validator`, wired inside a `@task.graph` that accepts it, was refused only once the subgraph had been expanded and the task submitted: the workgraph carried a `WorkGraph<eps>` process for work that could never run. - Pin the moment: the graph task fails with no called descendants, so neither the subgraph nor the task it would have held becomes a process. - Pair it with the value the rule admits, which expands, runs, and reaches the body as the enum member. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`body_receives` says a field declaring `orm.StructureData` reaches the body as the node. It does not: a leaf runs as an aiida-pythonjob process, whose read edge deserializes every input before it loads the spec carrying that mark, so the body is handed an `ase.Atoms` and the model refuses the value it declared. Under `Any` the same edge takes the node off in silence. - Assert the mark both sockets carry, then what each body is actually handed, with the same socket declared by annotation as the control. - Record the limit and where it lives in the design description. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A `@task.graph(input_model=M)` body was handed `'collinear'` where a body declaring the same field by annotation was handed `Spin.COLLINEAR`, so `spin == Spin.COLLINEAR` and `spin in (...)` answered False under a model and True without one. - Assert what a modelled body can tell about its value -- the class, the comparison, the membership, and the tag its link is drawn from -- against the annotated body reporting the same string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A task decorated under one name and bound to another -- `leaf = task(input_model=M)(_body)` -- was accepted by `add_task` with a value its model refuses, and `wg.run()` finished carrying that value. The spelling that leaves the decorated name bound to the handle refused it. - Pin both spellings at the write and at the run edge, where a cross-field rule the write could not hold fails the submitted process. - Keep the bounds the model admits beside them as the control, and pin the process label, which the name a task is stored under must not change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A leaf task whose input model declares `orm.StructureData` was handed the `ase.Atoms` the node deserializes to, so the model refused the value it had declared; under `Any` the node was taken off in silence. - Read the `body_receives` mark off the task's own inputs spec and name those paths to `aiida-pythonjob` as inputs to keep as nodes. - Name them only when there are any, so a task of plain data is prepared exactly as before. Needs an `aiida-pythonjob` that takes `keep_as_node`; against one that does not, a task declaring such an input fails with an unexpected-key error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Pin that a leaf's body is handed the one member its caller wrote, out of this engine's storage, with the top-level field it left out arriving at the model's default. - Read the depth test's nested body as a mapping, which is what a nested-model field now reaches a body as. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Against an aiida-pythonjob without `keep_as_node` the declaration was taken for an input of the function, so every task with a node-typed or `Any` model field failed with an unexpected-key error; the `**kwargs` call site and the monitor task never sent it at all. - Send the declaration only when the installed aiida-pythonjob accepts the keyword, so an older one behaves as it did before. - Send it from the var_kwargs call site and from MonitorFunctionTask, which prepared their processes without it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The graph-body tests read `type(value).__name__` and compared it to `TaggedValue`, which names one of the tagging layer's classes rather than the property the tests are about; against a tree where a scalar is tagged by a subclass they read as failures with nothing wrong. - Ask whether the body's value is a tagged value, and keep the class name in the report when it is not. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Pin that a leaf's body reads back the one member written into one key, out of this engine's storage. - Read the bodies that consumed a mapping of models by attribute as mappings, which is what an item now reaches a body as. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit bf64e5b.
This reverts commit 0bc517a.
A model declaring an AiiDA type on a task declared with `@task` asked for something that route cannot deliver: a PyFunction reads its inputs out of their nodes before the body runs, so the body was handed an `ase.Atoms` where the field said `StructureData` and the model refused the value it had asked for, at run time, inside the process. - Refuse such a model at decoration instead, naming every field that declares a node and what to do about it. - Take `input_model`/`output_model` on `@task.calcfunction`, whose body is handed the nodes: the models are enforced inside the process. - Pin both routes: the refusal for a `StructureData` field and for a field under `Any`, and a calcfunction body reading each of them back as the node. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A namespace-shaped model on `@task.calcfunction` was accepted where it was declared and excepted at run: a process function's parameter is one port, so AiiDA turned the mapping into a single `orm.Dict`, the members lost the nodes they were, and the model refused a `Dict` it never declared -- leaving the task with no process to read a reason from. - Refuse a nested-model or `dict[str, T]` field at decoration, naming the fields and the route that does take a namespace. - Pin what the shape a calcfunction does carry does: a rule it admits runs, and a rule that waits for the run edge excepts the process with the model's report on the node. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pin on a broken rule inside a calcfunction asserted that the task records no process handle, which holds when the call raises out of the engine and not when it does not: the same test passed alone and failed in the whole suite. - Ask the profile for the excepted node by state, and assert what a calcfunction actually carries: no exit status, and the model's report on the node. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea
scinode/node-graph#182 lets a task declare its inputs and outputs as Pydantic models:
@task(input_model=PhInputs, output_model=PhOutputs). The model declares the sockets, and node-graph runs the model's validators. That PR is the baseline for this PR; read it first.What that PR does not implement is serialization. node-graph never stores a socket value: it stays a Python object from the moment it is written to the moment the body runs.
aiida-workgraph cares about serialization. Every socket value becomes an AiiDA node on the way into a process and is read back from a node on the way out. That path has its own type table —
general_serializer,_flatten_enums,builtins_type_mapping— and knows nothing about Pydantic models.If we just take the new input model implementation as-is, we run into three errors
Decimalinput cannot be stored at allEnuminput works only through a package-specific workaroundThis PR makes the Pydantic model the authority on serialization, too.
The checks node-graph defines are hooked into the two places this engine runs them — graph expansion inside a submitted process, and a task's own process for
output_model.What it looks like
amountis stored as'0.10', the string the model's serializer rendered, and the body is handed aDecimalback — sostr(amount * 2)is exactly'0.20', never a float's'0.2000000000000000041...'.Changes
On the way in, storage goes through the model.
AiidaSerializationAdapter.serializeasks node-graph which input model owns a socket and, if one does, renders the value withmodel_dump(mode='json')instead of_flatten_enums. The lookup follows the socket's full path through nested models and typed mappings, so aDecimalatitems['a'].inner.valueis rendered by the model that declares it. AnEnumis stored as its bare value and rebuilt bymodel_validate; no enum-specific code remains.On the way out, the read edge unwraps what the model declares as Python.
deserializetakes the AiiDA node off a socket whose model says the body receives Python, and leaves it on for an AiiDA type orAny. The model must decide this, not the socket identifier:intandorm.Intboth map toworkgraph.int. The value keeps its tag and the node's uuid either way, so a graph body draws a link from it rather than holding a copy. A leaf body therefore gets the model's validated values (Enumas the member,Decimalas aDecimal). A graph body gets them too: the expansion check hands back the values the model made, re-tagged leaf by leaf with the original tag and uuid, sospin == Spin.COLLINEARholds inside a graph body and the link still draws.A model that declares an AiiDA type makes the task a calcfunction. A PyFunction body is handed the value a node carries, never the node, so
@task(input_model=M)with a field declaringorm.StructureData(orAny, at any depth, mapping items included) is refused at decoration:@task.calcfunction(input_model=M)takes the model, AiiDA'scalcfunctionwraps the validated callable, and the body receives the nodes. A namespace-shaped model — a nested model or adict[str, T]— is refused on a calcfunction, because aiida-core gives a process function one port carrying one node per named parameter; a rule broken inside a calcfunction body excepts the node, with the model's report onnode.exception.The checks run at this engine's two process boundaries. node-graph's graph-expansion check sits in
materialize_graph, whichGraphTask.executecalls, so a@task.graph(input_model=M)inside a submitted process is checked when its inputs are known — including a subgraph whose bound an upstream task produced. A leaf task runs as an AiiDA process rather than through node-graph's executor wrapper, so the run-edge check is hooked into the process runner: a return valueoutput_modelrejects fails that process with exit status 323 and the model's report as the exit message (verdi process show).Nothing changes for a task without a model. A socket no model owns takes the base adapter, which is also what lets #799's identifier-keyed unwrap coexist with this one.
Testing
tests/test_input_model.py, 64 tests against a live profile; the contract's own tests are node-graph's. All reproduced on this tip.str, theDecimalcontrol cannot be stored.str/int/float/bool/dict/listas Python,Anyandorm.Intas nodes, with a spec assertion thatintandorm.Intshare an identifier. Control: the basedeserializemakes the same graph fail at expansion.collinearexpands, runs, and hands the body the enum member.Spin.COLLINEARunder its tag, the same as an annotated graph body; before this branch it reported'collinear'. (reproduced)StructureDatafield and a field underAnyare each refused on@taskwith the message above, and the same two models on@task.calcfunctionrun with the bodies reading backStructureDataandDict; a namespace-shaped model is refused on a calcfunction at decoration; a rule broken in a calcfunction body leaves the node excepted with the report on it. Control: the unmodelled@taskstill loses the node in silence. (reproduced)system={'nbnd': 20}out of a five-member namespace reads back['nbnd']from storage, and a mapping's items likewise. (reproduced)AttributeError: 'TaskHandle' object has no attribute 'build'— the knownTaskHandle/GraphTaskHandlepairing noise, untouched here. Paired with bothpatchedbranches, which carry the fix for that noise: 373 passed, 6 skipped, 0 failed.Open decision — how a graph body gets the form its field declares
The read edge is one commit and can be dropped on its own. Measured on one harness against #799's identifier-keyed unwrap (what the body receives per field kind; whether a graph input's node is still the subtask's node by uuid):
deserializealoneDecimal,Enum,dict,list,Anyreach the model as nodes and it refuses themstr/int/float/bool/dict/listand the renderedDecimal/Enumas Python;Anyand AiiDA-typed as nodesThe link loss is #799's
return value.value, which drops theTaggedValuea graph body turns into a link; keeping the tag is what this row does differently. Measured against #799 at the revision the prototype carries;patchedalso unwrapsworkgraph.dict/workgraph.list, which changes nothing above.Not yet covered
A leaf taking a namespace of nodes —
pseudos: dict[str, UpfData], one node per element — has no model that can declare it: aiida-core delivers that shape through**kwargsand nothing else (a mapping written into a named parameter meetsto_aiida_typeand collapses into oneorm.Dict), and no model field can name**kwargs. Under a model it lives on a@task.graph, whose own socket may bedict[str, Node], or each node takes a field of its own. Giving the contract a spelling for it is separate work.Note for the maintainers
The
mypypre-commit hook islanguage: system, so it runs whatevermypyis first onPATH— an interpreter withoutaiidaornode_graph— and reports 16 errors in files this branch does not touch, identically on the unmodified tip. Against the project's interpreter (mypy --config-file=pyproject.toml --python-executable .venv/bin/python src) the tree is clean, so these commits skipped the hook. Fix is yours to pick:args: [--python-executable, .venv/bin/python], orlanguage: pythonwithadditional_dependencies: [aiida-core, node-graph].