Simplify Locs, remove -1s; symlink Input nodes in start_graph, and graph in each map-elem/loop-iter#280
Simplify Locs, remove -1s; symlink Input nodes in start_graph, and graph in each map-elem/loop-iter#280
Conversation
| ins: dict[PortID, OutputLoc], | ||
| ) -> None: | ||
| # We have to write something here to mark the node/graph as started. | ||
| # TODO ALAN - pass NodeDef? But can't really make valid/correct inputs. |
There was a problem hiding this comment.
This is a bit dubious, as per PR description
acl-cqc
force-pushed
the
acl/start_graph
branch
from
151da05 to
d0f0bc0
Compare
| executor: ControllerExecutor, | ||
| loc: Loc, | ||
| graph_input: OutputLoc, | ||
| ins: dict[PortID, OutputLoc], |
There was a problem hiding this comment.
I could add an enable_logging here, to match start; there'd be no uses of it, but there are none in start either....
johnchildren
left a comment
johnchildren
left a comment
There was a problem hiding this comment.
I didn't get into the meat of the PR yet, but I had a couple of comments about minor things
This reverts commit d35bd23.
acl-cqc
changed the title
|
|
||
| def get_eval_node( | ||
| storage: ControllerStorage, node_location: Loc, errored_nodes: list[Loc] | ||
| def render_graph( |
There was a problem hiding this comment.
thus, it feels like this should be graph.py and what's currently graph.py should be something else....I mean I can move this into the current graph.py and put just get_eval_node here but the latter is pretty trivial and hardly justifies its own file
| if parent is None: | ||
| raise TierkreisError("Eval node must have parent.") | ||
|
|
||
| thunk = storage.read_output(parent.N(node.graph[0]), node.graph[1]) |
There was a problem hiding this comment.
It's a shame to have to do this detailed inspection here....the alternative would be to store both graph_def and node_def in different files, and for Eval (only), write both files. Then storage.is_node_started would have to check whether either file existed.
Seems more work and perhaps complexity but more principled and better separation; thoughts?
| ["node_location_str", "graph", "port", "target"], | ||
| [ | ||
| ("-.N0", simple_eval(), "value", b"null"), | ||
| ("-.N4.M0", simple_map(), "0", b"null"), |
There was a problem hiding this comment.
Not sure about this "0". Previous code did have something to explicitly add a "0" to the node outputs if there was a port "*", which I have removed, because post-PR there wasn't a "*" either....
| ["node_location_str", "graph", "target"], | ||
| [ | ||
| ("-.N0", simple_eval(), ["value"]), | ||
| ("-.N4.M0", simple_map(), ["0"]), |
| raise TierkreisError( | ||
| f"No output named {output_name} in node {node_location}" | ||
| ) | ||
| if node is not None and node.type == "const" and output_name == Labels.VALUE: |
There was a problem hiding this comment.
This is a bit different to the old _build_node_outputs, shout if you don't think it makes (more) sense....
There was a problem hiding this comment.
I think in a recent pr for some visualizer changes, I implemented similar logic, yours seems more elegant.
Related to the tests in this:
("-.N0", simple_eval(), "value", b"null"), ("-.N4.M0", simple_map(), "0", b"null"),
A lot of the behavior in GraphData is retrofitted to behave like the original FileStorage implementation for the visualizer to work properly. This means for some ports we basically had to emulate a value being there so we can get a node to show up in the map to view its graph.
| import os | ||
| from pathlib import Path | ||
|
|
||
| BODY_PORT = "body" |
There was a problem hiding this comment.
Hmmm, perhaps I should use this everywhere (and move to labels.py) rather than delete it!?
|
|
||
| def start_nodes( | ||
| @dataclass | ||
| class LoopIterTask: |
There was a problem hiding this comment.
This is actually StartGraphTask just that we only use it for loop iters...
3 participants
start_graphthat takesdict[string, OutputLoc]and symlinks to those for all Input nodes in the graph (silently skipping any missing inputs)write_node_def; insteadwrite_graph_def(symlink).read_node_defchecks the Loc ends in N;read_graph_defthe converse (returning a GraphData).NodeRunDatatoRunNodeTaskand defineTask = RunNodeTask | LoopIterTask(returned in WalkResult) where the latter, used forwalk_loop, callsstart_graphbodyinput containing the graph, allowing recursive graphsN(-1)s and "fake"Eval((-1,..)...)s, and hence the need forExteriorRefin feat: Rust internals #266, which this is intended to precedeTODOs:
body? E.g.__tkr_body