Repository navigation
Fix graph execution edge cases - #9843
Conversation
Reuse one analysis during recursive If inference and preparation. Add shared correctness and operation-count evidence.
Bind root-frame branch inputs to prepared If activation frames so selected values become schedulable inside For bodies. Defer branch edge materialization until the If branch is selected. Add mixed, all-true, and all-false execution regressions.
Finalize empty nested For frames alongside non-empty contexts, filter sentinel nodes from iteration-scoped body inputs, and collect final outputs at the enclosing frame. Cover empty inner collections under both scheduler paths.
Use iterative path traversal and refresh materializer caches after graph edits. Add deep-chain and cache-lifecycle regressions.
Pfannkuchensack
left a comment
There was a problem hiding this comment.
Thanks for tackling these. I reviewed the PR against base 3d76b457d6 / head fd6b65d00b. Repro results below were checked on both trees. Ruff check/format and git diff --check pass. CI is red, though, and I found a few new wrong-result cases, so this needs another round.
Blockers
1. 9 existing tests fail (also in CI on linux/macos/windows: 9 failed, 13286 passed).
All of them are in tests/test_graph_execution_state.py. For example, test_graph_for_output_collection_is_scoped_to_parent_iterator_context expected [[['alpha']], [['beta']]] but got [[['alpha'], ['beta']]].
- The cause is the new
parent_path[: group_depth - 1]group key for For-final outputs intoCollect.item(graph_materializer.py:1062-1066). It merges For outputs across parent Iterate contexts. - The actual #9819 fix, the
NodeInputError, comes from the_prepared_node_has_edge_outputfilter. With only the group-key hunk reverted, all 9 tests pass again and #9819 stays fixed. The result is then[[[11, 12]], [[]]](per parent). - The expectation in #9819 (
[[11,12],[]]) contradicts the existing tested contract. Either drop the hunk and adjust the new test, or change the contract deliberately and update those 9 tests and the README.
2. #9820 depends on node insertion order and emits partial results.
- Take the exact issue graph and shuffle
add_nodeorder. 11 of 20 runs givecollect == ['fallback', [22]]instead of['fallback', [12, 22]], withis_complete() == False. rightexpands both frames, butbodyruns only for one, andbranch_collect/collectstill execute with the partial data. Base stalls on this graph; head now produces wrong data.- Suspected cause:
iterator_nodescomes fromnx.ancestorsset order but is paired by index (prepared_iterators[index],iterator_nodes.index(...)) in_matches_sibling_if_collect_context(graph_materializer.py:1341-1381) and in the sibling expansion (2052-2077). - The new test pins a single insertion order, so it doesn't catch this.
3. Per-item If with a branch from a closed axis completes with frames silently dropped.
- Graph:
Iterate([0,1,2]) → cond → If.condition,For([5,6]).output_collection → If.true_input, rootfallback → If.false_input,If.value → Collect. - Head:
is_complete() == True, output[['fb']]. Only the If for frame(0,)is ever created. Expected['fb', [5,6], [5,6]]. Base stalls. - In production this queue item would complete successfully with wrong outputs.
- Likely cause: the disjoint-axes check (
graph_if_runtime.py:120-135, same pattern atgraph_materializer.py:1600-1613) usesnx.ancestorson the flat source graph. That graph still contains Collect in-edges and For-final edges, so closed axes count as open.
Major
4. Hot-path regression, including graphs without any If.
_matches_sibling_if_collect_contextruns for every product mapping (graph_materializer.py:1445). Each call does the uncached_get_sibling_iterate_if_collect_branch(1665), which scans all Collects and callsget_node_iterators()→iterator_graph(), a full graph copy.- Measurements:
Range(300) → Iterate → 30-node chain → 6 Collects: 8.4s vs 1.9s on base (~4.4×). About 87% of the time is in this path (54kiterator_graphcopies).test_loop_scheduler_mapping_visits_scale_linearly[iterate-True-True]: 24.7s vs 16.6s.
- The new
_sibling_iterate_if_collect_idscache doesn't cover this lookup. - Suggestion: compute an
iterate_id → (if_id, field) | Nonemap once per materializer (same lifetime and invalidation) and skip the check entirely when it is empty. prepare()also gains an extra fulltopological_sortper call (~1915), even when no sibling shape exists.
5. "Sibling" detection also matches nested Iterates.
_get_sibling_iterate_if_collect_branchonly checkslen(iterator_ids) == 2, not that the Iterates are actually siblings.outer Iterate → Range(stop=outer.item) → inner Iterate → Add → Collect → If.true_inputwith a per-outer condition and two or more true frames raisesValueError: stream buffer is closedorduplicate stream sequence has conflicting data. Base hangs there.
6. prepare() returning None starves other ready nodes.
- At
graph_materializer.py:2078-2079, when the chosen node is a deferred sibling Iterate whose If hasn't resolved,prepare()returns None, even though other nodes are ready, including the If's own condition chain. - Adding one extra consumer of
rightor ofbranch_collectto the #9820 graph deadlocks it afterleft, left. On base those consumers still ran (['r10', 'r20']).
7. Visible errors become silent stalls.
- The unrelated-open-frame guard (
graph_if_runtime.py:117-137continue,graph_materializer.py:1606-1618iter([])) turns base'sKeyError(mixed selection[True, False]) into a stall with no error. _run_session_loopnever completes or fails such a session, so the queue item stays in progress forever.test_if_does_not_combine_unrelated_open_iterator_framesasserts that stall (not is_complete(),not errors) as expected behavior. An explicitUnsupported…error (as in #9789) would be a better contract. At minimum, drop the completion/no-error assertions.
8. #9821: the traversal still enumerates every path, and the perf numbers compare against the candidate, not base.
_get_effective_iteration_paths_for_edgekeeps per-path visited sets without memoization, so diamond ladders are exponential: 22 diamonds (76 nodes) take 29s. The source graph is a validated DAG, so per-node memoization gives O(V+E).- Recursion through
_get_if_iteration_pathsfor chained Ifs remains. - The "42% lower" is relative to an earlier candidate. Against base, an If-branch chain is ~2× slower (L=300: 11.6s vs 5.9s), and both are roughly n^2.5.
9. Architecture. The "For/Iterate axes feeding X" computation is inlined about 7 times:
graph_if_runtime.py:90-96,122-129graph_materializer.py:1359-1370,1401-1414,1566-1577,1604-1613,1684-1696
The copies mix the flat source graph and iterator_graph, and that inconsistency is the root of blocker 3. get_node_iterators / _get_iteration_axes already own "open axes of a node". Together with the post-hoc filtering of the full Cartesian product and the general "Collect fed by an If → restrict to condition axes" rule in _get_parent_iteration_mappings, this adds more shape-specific patches to the compatibility materializer. A single frame-alignment rule would be easier to keep correct.
Tests
- Base vs head: the #9818, #9819, #9820, cache and 1000-node #9821 tests fail on base and pass on head. The two #9695 tests, the unrelated-frame guard and the 400-node e2e test already pass on base.
- #9821 coverage:
- The 400-node e2e test (
test_execution_engine_differential.py:8647) passes on base, so it doesn't guard #9821, but it costs ~15s in the default lane. - The real regression test (
:8722) calls a private method with a fabricatedsource_prepared_mapping("prepared_iterate"doesn't exist). - A ~150-node e2e test through
GraphExecutionStatewithsys.setrecursionlimit(len(inspect.stack()) + 250)(restored infinally) fails on base (0.17s) and passes on head (1.45s). It could replace both.
- The 400-node e2e test (
- The 1000-node e2e run quoted in the description isn't in the suite.
- Cache test (
:8675): it primes the cache through private methods. A public-interface variant also kills the "remove the invalidation" mutation:- Build without
right → body. - Run one
next()/complete(). state.add_edge(right → body).- Run to completion.
- Build without
test_graph_update_node_renames_float_source…(test_node_graph.py:1475) duplicatestest_graph_collector_edges_are_independent_of_add_order(:1398-1429). No transiently invalid state ever exists in it, so I'd drop it. Thevalidate_selftest at:1452is fine.- Gaps worth pinning:
- #9818 on the compatibility scheduler.
- The shapes this PR newly makes work, e.g. nested Iterate with outer-frame condition and inner-frame branches, and an iterate-derived true branch with a root false branch.
- More inputs for the
all → anychange in_mark_empty_for_complete([[], [1]],[[], []]). - A dump/load round-trip for the new shapes. I checked this at every step for #9818/#9819/#9820 on both schedulers, and it works.
- Nit:
_run_to_completion(:8495) duplicates the existing_run_graph(:1335), which already takesforce_compatibility_scheduler.
Looks good
- Cache invalidation: complete for the supported edit paths (
graph.py:732, rollback, rehydration,model_copy(update={"graph": ...})). It also fixes the previously stale_iteration_axes_by_source. The cache is a private attribute, never persisted, and bounded. - #9818: correct for the reported shape and for root chains (
[F,F,T],[T,F,T],[F,T,F],[T],[F]). Unselected root sources never run. - #9821: fixes the RecursionError (a 600-node e2e chain completes; base raises).
- Checkpoint resume: works for all new shapes.
Order complete If condition dependencies before unrelated iterator axes. Invalidate the cached order when source graph edits change dependencies. Cover chained and direct Collect scopes, renamed condition and producer IDs, and graph edits after cache warmup.
Reuse a lazy ancestry index across recursive If execution while preserving execution fixes.
Pass one ancestry context through recursive If traversal and materialization. Clarify the prepare scope and update the direct helper test caller.
Admit selected literal branches before If activation; cover Iterate and controller gating.
Pfannkuchensack
left a comment
There was a problem hiding this comment.
Follow-up review at head 401c92a2ae (base 723d600742, previous review at fd6b65d00b). Thanks for the quick turnaround. Several of the earlier blockers are properly fixed, and the new frame-alignment tests are strong. A few silent-wrong-result and silent-stall cases remain, though, and one of them is new.
Checks.
- Full suite on head: 13,486 passed. The 85 local failures all fail on base too: my venv has diffusers 0.37 and lacks a few packages (ltx2/ernie/image_index/umap).
-m slow: green.- Every changed test file passes alone.
- Ruff 0.11.2 check/format and
git diff --checkare clean. - CI linux-cpu is green now.
Fixed since the last review
- 9 failing
test_graph_execution_state.pytests: the For-final group-key hunk is reverted, and #9819 now yields the per-parent[[[11, 12]], [[]]]. - #9820 order dependence: 0 bad results across 120 shuffled insertion orders × 4 hash seeds, plus 96 randomized-id variants, on both schedulers.
- Nested Iterates misdetected as siblings: the original repro is correct now, with no stream-buffer errors.
- Hot-path cost on If-free and non-sibling graphs: gone, because the sibling branches are now cached and the extra topo pass is gated.
- Long chain into an If branch: much faster than base (L=300: 1.2s vs 6.0s).
- Literal-If stall from #9838 (
0bb4557bf0): correct for every shape I tried, including dump/load at every step. Covered shapes:- true and false conditions;
- literal inside a wired If and wired inside a literal;
- inside For and inside nested Iterate;
- chained literals.
Blockers
A. New: a per-item If whose branch is a Collect over an unrelated root Iterate completes with truncated data.
Graph: xs → Iterate x → cond → If.condition, ys → Iterate y → yv → Collect → If.true_input, root fallback → If.false_input, If.value → Collect out.
| xs / ys | head | expected | base |
|---|---|---|---|
[1,1,0] / [10,20,30] |
is_complete() == True, [[10,20],[10,20],'fb'] |
[[10,20,30],[10,20,30],'fb'] |
stalls |
[1,0] / [10,20] |
[[10],'fb'] |
[[10,20],'fb'] |
stalls |
[0,1] / [10,20] |
ValueError: stream event is out of order from complete() |
['fb',[10,20]] |
stalls |
fd6b65d00balso stalls here. Bisecting puts the change at the #9838 merge (8fb6e656e0).- How it goes wrong:
- The branch-compatibility check now uses the iterator graph, where
Collect.itemedges are cut, so the If gets materialized. _get_source_activation_dependencies_for_preparation(graph_if_runtime.py:200-221) then prefix-matches y's iteration path against x's If frames (iteration_path[: len(frame)] == frame).- As a result,
create_execution_node(graph_materializer.py:1528) drops every y item whose index equals a false or nonexistent x frame.
- The branch-compatibility check now uses the iterator graph, where
- In production this completes the queue item with wrong data.
B. Still present: a closed For axis feeding a per-item branch drops frames.
Graph: Iterate([0,1,2]) → cond → If.condition, For([5,6]).output_collection → If.true_input, root fallback, If → Collect.
- Head returns
is_complete() == Truewith[['fb']]; expected['fb',[5,6],[5,6]]. With xs[1,1], the Collect runs with[]and then the graph stalls. Base stalls in both cases. - Likely cause:
_IteratorAncestry.get_iterator_ancestorsadds the For itself (graph_iterator_ancestry.py:49-50).graph_if_runtime.py:131-142therefore treats a closed For output as an open axis that is disjoint from the condition axis, and filters the branch out.
Major
C. Errors still become silent stalls, and more shapes now stall with partial output.
The session processor only completes a queue item when is_complete() (session_processor_default.py:538), so each of these leaves an item stuck "in progress" with no message.
| Shape | base | head |
|---|---|---|
#9820 + one extra consumer of right.item or branch_collect.collection |
runs the extra consumer (['r10','r20']) |
deadlocks after left_source, right_source, left, left |
| unrelated open branch axis, mixed selection | KeyError |
stall, no error |
| nested Iterate, outer-frame condition | KeyError 'B' |
stall; one collect frame emitted ['f1'] |
| deep branch, root false | KeyError 'inner' |
stall; one of two Collect executions emitted ['F'] |
literal If(condition=True) feeding a per-item If's branch (#9818 variant) |
stall, no output | correct output, but is_complete() stays False: the unselected root source is never run or marked inactive (graph_if_activation.py:149-175 checks the literal If at frame (1,), where only () exists) |
nested inner-Iterate Collect branch, all outer frames false ([1,1]) |
stall | stall, introduced by 10c74c6df8 |
same, with an extra consumer of inner |
stall, no output | per-outer root Collect, one emits ['F'], then stall |
- Deadlock cause:
prepare()returns None atgraph_materializer.py:2187-2188while other nodes are ready. - The guard test pins the stall:
test_if_does_not_combine_unrelated_open_iterator_framesstill asserts the stall as expected behavior. Please assert an explicitUnsupported…error instead, or at least drop the completion and no-error assertions.
D. Mirror of the literal-If fix is still uncovered.
Iterate → Add → If(condition=True).false_inputwith rootR → If.true_input(and its mirrorcondition=False) stalls after running onlyR. Same on base andfd6b65d00b.Iterate → Add → If(condition=False).true_inputwithfalse_inputunwired stalls with zero nodes executed. The same shape at root completes with[None].test_single_if_in_iterate_body_completescovers onlycondition=Truewith true wired, and only on the generic scheduler.
E. Per-item If scheduling got 2-3× more expensive.
test_loop_scheduler_mapping_visits_scale_linearly, quiet machine, serial:
| Variant | base | head |
|---|---|---|
[iterate-True-True] |
17.0s | 23.3s |
[for-True-True] |
12.8s | 17.4s |
[iterate-True-False] |
9.5s | 16.7s |
[for-True-False] |
11.6s | 15.3s |
Profiling iterate/1200/include_if (25.9s → 47.0s under cProfile):
enqueue_if_readycalls went from 0.73M to 2.17M, and_discard_rejected_activation_nodefrom 2.89M to 5.77M.- Both run in the existing full scans (
_enqueue_activation_ready_nodes/_discard_rejected_activation_nodes) after each of the 1,200 If resolutions. Since Ifs are now prepared earlier, each scan covers more exec nodes. - The tests count mapping visits, so they still pass, but the default lane grows by ~25s and real per-item If workflows pay the same cost.
- Scoping those scans to the exec nodes that depend on the resolved If would fix this together with the pre-existing O(n²).
F. #9821 traversal is still exponential on diamonds.
_get_effective_iteration_paths_for_edgekeeps a per-path active set and no memo. A diamond ladder into an If branch takes 45 ms / 0.6s / 34s for D = 10 / 16 / 22, inside oneprepare()call that cannot be cancelled. Base stalls quickly on this shape, so head turns a stall into an effective hang.- Per-node memoization over the validated DAG gives O(V+E).
- The sibling shape also still filters the full Cartesian product per mapping with uncached work: 10×10 / 20×20 / 30×30 take 68 / 382 / 1267 ms, about 75% of it in
_matches_sibling_if_collect_context.
G. Iterator order has three authorities.
_get_iterator_source_order(condition ancestry first, then by id)._iteration_relationship_graph(lexicographic)._get_parent_iteration_mappings(nx.ancestorsset order).
Effect, on an If-free graph with two sibling Iterates (zz=[10,20], aa=[1,2,3]) → Add → Collect:
- Head gives
[[11,21],[12,22],[13,23]]whenzzis added first, and the base result[[], [11,12,13], [21,22,23]]whenaais added first. - Base is already odd here, but output now depends on insertion order, which is the same class of problem as the old #9820 blocker.
The "For/Iterate ancestry feeding X" filter also grew from about 5 inline copies to about 20: the condition-iterator block at graph_materializer.py:1385, 1427, 1594, 1752 and variants at 64-69, 1633-1643 and graph_execution_runtime.py:53-60. The copies still mix the flat graph with the iterator graph, which is where A and B come from. One owner for "open axes of a node" would make these cases much easier to keep correct.
Minor
- Engine-frame cache:
_engine_frame_foris a process-globallru_cache(maxsize=4096). Frames of finished sessions stay alive, and a session with more than 4096 frames thrashes the LRU. A per-state dict on_GraphExecutionStateRuntimekeyed by(state.id, call depth, path)avoids both, and I'm happy to send it. The README also doesn't mention the new caches yet: flat iterator graph, node iterators, If memberships, frames. - README gaps: besides the caches, it doesn't cover:
- the iterator graph now keeps
Collect.collectionedges; - If readiness now uses only the condition ancestry (
_get_if_readiness_ancestors); - the condition-first ordering;
- For empty-completion changed from
alltoany.
- the iterator graph now keeps
_IteratorAncestryduplicates the builder caches:has_iterator_ancestor(n) == bool(get_node_iterators(n)), andget_iterator_ancestorsis that plus self._iterator_source_orderalso builds its own uncached iterator graph fromgraph.nx_graph()(twice), not from_get_source_graph_flat().- Dead code:
graph_if_runtime._get_if_iteration_paths(old signature) is now only called from tests.graph_materializer.py:1088-1089(parent_path = …; return parent_path) is a leftover of the reverted hunk.
Tests
Still open from last time:
- The 400-node e2e test (
:8925) passes on base, so it doesn't guard #9821. - The only real #9821 guard (
:9000) still uses a fabricated"prepared_iterate"mapping plus a private_IteratorAncestry. test_graph_update_node_renames_float_source…(test_node_graph.py:1475) still duplicates:1398.- No #9818 compatibility-scheduler variant. It works on head and stalls on base, so it would be a valid regression test.
- No
all→anyinputs beyond[[1,2],[]].[[],[1]]raisesNodeInputErroron base and gives[[[]],[[11]]]on head. - No rehydration test for the new shapes. I checked #9820 at all 16 checkpoints on both schedulers, and it's fine.
New:
- The private
_get_sibling_iterate_if_collect_branchassertions and priming calls (:8673, 8724, 8780, 8860) make base fail withAttributeErrorbefore the behavioral assertions run. With them removed, the behavior assertions still fail on base, so the private ones add no regression value. - The
_iterator_graphcall-count tests (test_graph_if_runtime.py:175-242,test_graph_materializer.py:73-122) pin call structure rather than behavior; 3 of 4 IDs pass on base.
Worth adding: regression tests for A–D above, ideally asserting an explicit error where a shape is unsupported.
Reject divergent If axes and shared Collect branch contexts before execution. Add regression coverage and document supported branch shapes.
|
@Pfannkuchensack, I addressed the review follow-ups since 401c92a. I added regressions for divergent inner iterator axes and shared Collect outputs. Divergent axes now raise UnsupportedIfIterationAxesError, and a Collect feeding multiple If branch contexts raises UnsupportedSharedIfCollectBranchesError before invocation. I updated the graph execution README and PR body. I reran the full execution-engine inventory with PYTHONOPTIMIZE=0: 1,943 passed, 21 skipped, 2 xfailed, and 10 subprocess tests aborted at shutdown with free(): invalid pointer. No graph behavior tests failed. The slow lane passed 4 tests; Ruff and diff checks passed. |
Pfannkuchensack
left a comment
There was a problem hiding this comment.
Third-round review at head 6bb686f0e1 (base 723d600742, previous round at 401c92a2ae). Thanks, this is a big step forward: most of the round-2 list is fixed, the new tests are behavioural, and per-item If scheduling is now faster than base. The new axis-ordering change does introduce two regressions against 401c92a2ae, though, so I'd hold off on merging.
Checks.
- Full suite on head: 13,545 passed. The 85 local failures are environment-only (they fail on base too, from my stale venv).
- Every changed test file passes alone, and
-m slowis green. - Ruff 0.11.2 check/format and
git diff --checkare clean, and CI is green on all platforms. test_for_scheduler_performance.pygained tests only. Existing sizes, budgets and assertions are unchanged.
Fixed since round 2
- A (root-Iterate Collect branch truncated): correct, including
[0,1], emptyysand the false-branch mirror; resume is clean at every step. - B (closed For axis):
[0,1,2]now gives['fb',[5,6],[5,6]], and the empty and short For variants are correct. - C, deadlock: #9820 with an extra consumer of
right.item/branch_collectnow completes, and the consumers run. - C, literal If feeding a per-item branch: completes.
- C, nested inner-Iterate Collect with all frames false: completes.
- C, the guard test: it now asserts
UnsupportedIfIterationAxesErrorinstead of pinning a stall. - E, per-item If cost: fixed and now faster than base.
test_loop_scheduler_mapping_visits_scale_linearly[iterate-True-False]is 10.0s on base, 17.3s on401c92a2aeand 5.4s on head.enqueue_if_readycalls went from 2.17M to 4.8k. I also re-ran the old full scans as an oracle after every If resolution, across ~1,900 tests: no missed discards, only later (equivalent) enqueues. - F, diamond: D=10/16/22 is 80 / 170 / 298 ms (was 60 / 782 / 31,385 ms).
- G, order dependence: sibling Iterate order no longer depends on insertion order or hash seed, and the duplicated ancestry filters mostly moved into
_IteratorAncestry. - Minors:
- Engine frames are now a per-state dict, and the test is behavioural.
- The dead wrapper and the no-op lines are gone.
_iterator_source_orderno longer builds its own graph.- The private sibling-branch assertions and the
_iterator_graphcall-count tests are gone. - #9818 is now covered on both schedulers.
Blockers
1. Regression vs 401c92a2ae: silent wrong data when the #9820 condition comes from the body.b side.
Graph: #9820, but right.item → minus_one → condition.
| left / right | 401c92a2ae |
head (is_complete() == True) |
|---|---|---|
[5,6,7,8] / [10] |
[[15,16,17,18]] |
[[15]] |
[1,2,3] / [10,20] |
[[11,12,13],[21,22,23]] |
[[11,12],[21,22]] |
- Base stalls on this graph.
- Every
cond_side=rightcase with the longerleftfails, on both schedulers, and onlybody (0,0)is materialized. - Likely cause:
_get_ordered_iterator_sources(nowgraph_iterator_ancestry.py:70-104) returns destination-port order, sobody's axes are(left, right). The collapsed sibling path still uses condition-first axes(right, left)(graph_materializer.py:65-76). The removed condition-first_get_iterator_source_orderwas what kept these two consistent.
2. Regression vs 401c92a2ae: #9820 with an empty right collection stalls.
| left / right | 401c92a2ae |
head |
|---|---|---|
[1,2] / [] |
['fallback', []] |
is_complete() == False, no error |
[2,2] / [] |
[[], []] |
same |
[2] / [] |
[[]] |
same |
- Head stops after
right_source(or after the If);body/branch_collectare never prepared or retired. - The If-free product with an empty
rightstill works.
3. Regression vs base: one Collect into both fields of the same If is rejected.
branch_collect → if.true_inputandbranch_collect → if.false_input(#9820 shape) now raisesUnsupportedSharedIfCollectBranchesErrorbefore any node runs. Base completes it with[[11,21],[12,22]].- The check (
graph_materializer.py~1777-1784) compares(if_id, field). Both fields of one If share the same condition frame, so this case shouldn't conflict. - The shape is contrived, but it worked before.
Major
4. Stall became silent wrong data: nested inner-Iterate Collect branch plus an extra consumer of inner.item (or body).
- The If gets materialized per inner frame, which duplicates results:
src=[2]gives[[[100,101],[100,101]]]; expected[[[100,101]]].[3,1,2]gives[[100,101,102]]×3, 'F', [[100,101]]×2.
- Likely cause: once
inneris prepared before the If,_get_effective_iteration_paths_for_edge(graph_if_runtime.py:20-85) walks through the unprepared Collect'sitemedge without collapsing the collected axis. - Without the extra consumer, all 80 shuffled insertion orders are correct.
5. The new errors are raised late or depend on the data.
UnsupportedIfIterationAxesError is raised during path inference, and only for the branch that actually gets selected.
| Unrelated open branch axis | head |
|---|---|
xs=[1,0] |
raises only after xs, both x, both upstream nodes and both cond have run (in a real workflow that could be expensive generations) |
xs=[0,0] |
still a silent stall, no error |
- The check is purely topological, so it could run statically over both branches at the first
prepare(), or better, in graph validation at enqueue. UnsupportedSharedIfCollectBranchesErroralready fires on the firstprepare()with nothing executed, which is the right timing.- Both errors reach the processor as
ValueError, so the queue item fails with the class name and a clear message. That's much better than a stall.
6. More shapes that still stall without an error. The table shows head behaviour, then base and 401c92a2ae.
| Shape | head | base / 401c92a2ae |
|---|---|---|
Condition per left, true branch is the uncollected left×right product (s_per_left_if_product_branch) |
stall after one Collect frame emits ['fallback'] |
401c92a2ae: ValueError |
Nested Iterate, condition from the outer A, true branch from inner B, false branch from A (nested_cond_outer) |
stall, frame (0,) emitted ['f1'] |
base: KeyError 'B' |
| Deep branch, root false | stall, one Collect emitted ['F'] |
— |
- In the first two, the axis check accepts a branch that carries more axes than the condition (
graph_if_runtime.py:142-155), but the runtime can't complete it when only one branch is deeper. - Those should either work or raise the new error.
7. D, the literal-If mirror shapes, still stalls and is untested. Both schedulers.
| Shape | head |
|---|---|
Iterate → Add → If(condition=True).false_input, root R → true_input |
runs only R, then stalls |
same with condition=False and the inputs mirrored |
runs only R, then stalls |
Iterate → Add → If(condition=False).true_input, false_input unwired |
stalls with zero nodes run |
The same shapes at root complete.
8. Performance: prepare() now sweeps every unprepared source on every call.
- Where:
graph_materializer.py:2028-2037, with_is_source_inactiveatgraph_if_runtime.py:206-246, 284-310. - The sweep used to be gated on sibling-Collect branches and is now gated on "graph has any If". Each
_get_source_activation_dependencies_for_preparationcall also rescans all of the owner's frames. - ifchain into an If branch:
| L | base | 401c92a2ae |
head |
|---|---|---|---|
| 150 | 1.2s | 270 ms | 786 ms |
| 300 | 6.9s | 933 ms | 2,964 ms |
So head is about 3.2× slower than 401c92a2ae, though still faster than base, and outputs are identical. 93.6k _is_source_inactive calls account for 6.7 of 8.8s.
- Per-item If with a root fallback (n=1000) is +27%.
- Re-evaluating only sources with activation dependencies when an If resolves, plus looking up owner frames by axis key, should bring this back.
Tests
- #9821 has no effective guard now.
test_deep_if_branch_chain_completes_through_execution_state(150 nodes) and the 400-node e2e test both pass on base.- Base completes 150 and 400 and raises
RecursionErroronly around 1,000, so a regression of the fix would go unnoticed. - Bumping
test_deep_any_chain_inside_iterate_if_completesto ~1,000 (the issue's repro) fixes that, and the 400-node test could be dropped (5.8s).
- Coverage gaps:
- No tests for blockers 1–3, item 4, item 6 or D.
test_literal_if_inside_iterate_uses_selected_branch_framewires the per-item source into the selected branch; D is the opposite wiring.- The
[[],[1]]input that actually fails on base belongs intest_iterate_for_collect_preserves_empty_inner_collection. The new inputs were added to a test that already passes on base.
- Coupled test:
test_repeated_if_resolution_indexes_activation_dependencies_oncecounts iterations inside one private method, so it would not catch a reintroduced full scan elsewhere. The slow "legacy comparison" only resets an index flag rather than reproducing the old scan (its own output shows 12.29s vs 11.60s). - Dead assertion:
tests/test_graph_execution_state.py:5309-5310repeats line 5307 verbatim sincefalse_valuenow appears inexecuted_history. - Duplicate:
test_node_graph.py:1475still duplicates:1398.
Minor
- Behaviour changes to document in the README:
- Unselected branch sources are now "retired empty": empty
source_prepared_mappingentry, and they appear inexecuted_historywithout being invoked. The README covers the mapping (:670) but notexecuted_history(:333). - Sibling Iterates into one node now group by destination-port order, also for If-free graphs.
UnsupportedIfIterationAxesErrorand when it fires.
- Unselected branch sources are now "retired empty": empty
- Remaining sibling-shape cost: about 69% of the 30×30 time is uncached
_get_selected_if_frames_for_preparationcalls in_matches_sibling_if_collect_context. _get_iteration_axes(graph_materializer.py:61-90):- It still orders axes via the lexicographic relationship graph.
- Its inline condition-axis block doesn't exclude closed For final outputs, unlike
get_iterator_axes_for_edge. That's a suspected inconsistency; I haven't demonstrated an effect.
- Unused code in
_IteratorAncestry:get_iterator_ancestorshas no callers.get_iterator_ancestors_for_edgeis a pass-through.has_iterator_ancestoris called only for its side effect.- There are two instances (runtime and builder) over the same iterator graph.
_get_effective_iteration_paths_for_edge:visited_source_idsis effectively dead; every caller passesset().- The memo key ignores it, so a future caller passing a non-empty set would poison the cache.
- Leftover O(V) checks:
_has_admitted_source_mappingand_record_activation_dependenciesstill recomputeany(isinstance(n, IfInvocation) …)per call, even though the builder has_has_if_nodes. - Pre-existing:
- An empty condition Iterate (
xs=[]) gives the right output but never reportsis_complete(). zip_collect_then_if_root_condstill stalls._attach_pending_if_inputsis O(N²) and dominates the per-item If perf test.
- An empty condition Iterate (
Pin expected outputs for sibling-Collect axis order, empty right collections, shared Collect branches and nested Collect branches with extra consumers. Require early rejection or completion for unsupported If axes, cover literal Ifs selecting a root branch, and guard invoke-ai#9821 cheaply with a lowered recursion limit.
Correct If and iterator branch execution while retaining the incoming regression tests and documenting materializer lifecycle behavior.
Journal planner preparation mutations so failed materialization can be retried safely. Add regression coverage for cache isolation, frame propagation, and rollback across both schedulers.
|
@Pfannkuchensack I addressed the findings since your test commit at ab23f58. I fixed lost or misordered sibling Iterate/Collect frames, empty-stream stalls, Collect ownership and nested-consumer cases, unsupported open-axis stalls, literal-root If handling, and deep traversal. I added bounded support for a root-conditioned If backed by a completed direct Collect, including ordinary descendants and a downstream Collect. I made planner materialization rollback atomic: failures restore execution nodes, mappings, frames, activation tokens, and gates, then rebuild ready queues so preparation can retry. I documented the supported behavior and rollback contract in the shared execution-engine README. I reran the execution-engine inventory with PYTHONOPTIMIZE=0 in the parent environment: 2,015 passed, 21 skipped, 2 xfailed, and 10 subprocess import aborts. All 10 reproduce serially with |
Pfannkuchensack
left a comment
There was a problem hiding this comment.
Fourth-round review at head 0407c73adf (base 358257bca6, previous round at ab23f5817e). This is close.
- Every round-3 blocker and major is fixed: blockers 1–3, items 4–6, D and the zip-collect shape.
- Our regression tests pass unchanged; the two you tightened are stricter than what we proposed, which is better.
- The new rollback held up under heavy fault injection.
What's left: one shape that this PR's own previous state ran correctly is now rejected, and a per-prepare cost on ordinary If-free graphs.
Checks.
- Full suite on head: 13,734 passed. The 85 local failures are environment-only, all fail on base too, and none are in engine or graph tests.
-m slowis green, and each changed engine test file passes alone.- Ruff 0.11.2 check/format and
git diff --checkare clean. test_for_scheduler_performance.py: no budget, size or assertion was weakened relative to main.
Verified fixed
All of the following were checked on both schedulers.
| Shape | head |
|---|---|
| condition from the other sibling axis | correct, all sizes |
#9820 with empty right (7 inputs × both fields) |
correct |
| one Collect into both fields of one If | correct |
| nested Collect branch + extra consumer | correct; 80 shuffled insertion orders, 0 bad |
| unrelated / deeper open axes | UnsupportedIfIterationAxesError before any node runs, for every condition value ([1,0], [0,0], [1,1], []) |
| literal-If mirror shapes (D) | correct |
zip_collect_then_if_root_cond |
correct, (0,) → [11, 21], (1,) → [12, 22] |
- Insertion order and seeds: 0 bad across 20–40 seeds on the order-sensitive scripts.
- Root-If over a completed direct Collect: correct with dump/load at every step, across consumer chains, a downstream Collect, Iterate and For consumers, two root Ifs sharing the Collect, the false-branch variant, and
left=[1],right=[],left=[]. - Rollback (
d2eb673394): ~42.7k fault-injection runs over 19 shapes × 2 schedulers.- Exceptions were injected before and after the N-th call of 15+ materialization functions:
create_execution_node,_initialize_execution_node,_attach_execution_edges,set_iteration_path,_record_activation_dependencies,_activation_gate,_engine_frame,_mark_source_executed,_record_empty_iterate_stream,_materialize_root_if_for_completed_collect_branch, and others. Each failure was followed by a direct retry, or a dump/load then retry. - No wrong result, no stall, and no unexpected exception. Post-rollback state matched the pre-transaction state apart from two benign artifacts (below).
- The failure surfaces through the processor as a failed queue item with the error type and message.
- Exceptions were injected before and after the N-th call of 15+ materialization functions:
- Performance: the late/data-dependent error and the sibling-shape uncached work are fixed (sibling 30×30: 0.58s, down from 1.0s), and the diamond stays linear.
- ifchain L=300, measured interleaved: base 4.85s,
ab23f5817e3.0s, head 2.0s. [iterate-True-False]: 3.4s.
- ifchain L=300, measured interleaved: base 4.85s,
Major
1. Regression vs ab23f5817e: both branches deeper than the condition are now rejected.
Graph: src=[1,2,3] → Iterate A, A.item → Range(stop) → Iterate B, A.item → Add(-1) → condition → If.condition, B.item → t → If.true_input, B.item → f → If.false_input, If → Collect.
| result | |
|---|---|
| main | KeyError 'B' mid-run |
ab23f5817e |
correct on both schedulers, dump/load clean at all 34 steps: [((0,), ['f0']), ((1,), ['t0','t1']), ((2,), ['t0','t1','t2'])] |
| head | UnsupportedIfIterationAxesError … deeper open iterator axes without a Collect boundary: condition ['A'], branch ['A', 'B'] before any node runs |
- Same for
s_deep_both_branches(both branches per inner item, deeper than the condition), which gave[[100,101,102],[200,201]]onab23f5817e. - This isn't a regression against main, which crashed. But it is a working broadcast shape that the PR loses.
- The check at
graph_if_runtime.py:42(condition_iterators < branch_iteratorswithout a Collect) rejects every deeper non-Collect branch. The shapes that actually failed before are narrower:- only one branch deeper (
nested_cond_outer), which stalled; - a deeper branch that mixes axes (
A.item + B.iteminto one branch), which gave silent wrong data[None]; - the zip product.
- only one branch deeper (
- Narrowing the rule to those would keep the both-branches-deeper broadcast. The tightened
nested-true-branch/product-branch/nested-branch-root-falsetests stay valid under a narrower rule.
2. Every prepare() now runs in a transaction; If-free graphs pay about 23%.
iffree(300 items, 30-node chain, 6 Collects), interleaved, min of 3, outputs identical:
| iffree | |
|---|---|
| main | 2.0s |
ab23f5817e |
1.9s |
| head | 2.45s |
-
The mechanism is the right one: it reuses the
_ApplyTransactionundo journal, takes no snapshot, and costs O(mutations). -
Most of the cost is GC. The journal's closures live long enough to trigger extra gen2 collections:
with / without transaction iffree, GC on 2.90 / 2.30s iffree, GC off 1.75 / 1.65s root If over Iterate(3000) +20% product 30×30 +9% -
If-free graphs are the common production case. The only multi-store rewrite that needs this rollback is
_materialize_root_if_for_completed_collect_branch, so scoping the transaction to that path (or to prepares that touch an If) would remove the overhead. -
Related:
_run_materialization_transactionis now the third copy of open → rollback → reset → rehydrate, next to_prepare_direct_iterate_collect(graph_iterate_planner.py:465-485) andapply(), each ordered slightly differently. One shared helper would avoid drift.
3. The root-If Collect expansion scales with groups × branch size.
Zip-collect with a root condition, scaled (main and ab23f5817e stall on this shape):
| size | head | If-free product |
|---|---|---|
| 100×10 | 1.7s | — |
| 200×10 | 6.3s | 1.08s |
| 400×10 | 19.6s | 2.6s |
| 60×60 | 8.5s | — |
- About 70% of the time is in
_materialize_root_if_for_completed_collect_branch. Each If copy goes through_try_resolve_if_nodeinto_get_activation_dependents_for_if(graph.py:689-713), which pulls in every frame-()dependent (the whole branch) and runsnx.topological_sort(execution_graph.subgraph(...))per copy. - The same path makes 483k
_discard_rejected_activation_nodecalls. - An index from the If to its branch dependents (built once per expansion), or at least a README note on this limit, would help.
Minor
Validation coverage. README line ~680 says unsupported axes raise before invocation, but these still stall silently. All are pre-existing (main and ab23f5817e stall too).
| Shape | Why the static check misses it | head |
|---|---|---|
Condition derived through a Collect: Fixed(Collect over B per A), branches from B |
ancestry sees no axes on the condition, but at runtime it is per A |
stalls after 15 nodes |
Branch is a Collect of C per (A,B), condition from A |
— | stalls after 22 nodes |
Empty condition Iterate with a node between the Iterate and If.condition |
the new empty-condition handling only registers a direct Iterate source (graph_materializer.py:283) |
never is_complete() |
Also still stalling, as on main and ab23f5817e:
If.value → If2.true_inputchained off the zip Collect;- literal
If(condition=False)with the zip Collect ontrue_input.
Tests.
- Deleted main test:
test_prepared_execution_count_scans_each_source_oncewas deleted inc6c8d22bfb. It still passes against head code, and it's the only test that catches disabling the_count_unexecuted_preparedcache: with that cache disabled, all other 1,969 engine tests pass. Please restore it. test_pending_if_attachment_sweep_scalingcounts deterministically but only runs in the slow lane, so CI never guards the O(N²) attachment fix. A count-only n=300 check takes ~0.8s, and base already fails it (90,000 > 9,600).test_if_chain_retirement_candidate_work_scales_linearlycosts 3–5s in the default lane and bounds only the private candidate function. End-to-end_is_source_inactivework is still quadratic (46k calls at L=300), now almost entirely from_get_completed_source_ids_cache(pre-existing).- Deferred-consumer rollback test (
:10126): it only checksafterids and edges. Dropping the gate-undo_tx_recordin_materialize_root_if_for_completed_collect_branchstill passes it; a full state snapshot like the If-init test would catch that. - Still open:
test_node_graph.py:1475duplicates:1398. The fourtest_zip_collect_then_after_*no-If controls also pass on main (nit).
Rollback details.
- The README says "each planner preparation expansion is applied as a state transaction", but
_attach_pending_if_inputs()(graph.py:3461) andprepare_nested_iterate_sequencerun outside it. Both are idempotent and recovered fine in fault injection, so only the wording needs fixing. - Rollback resets
_execution_scheduler, so a forced compatibility scheduler (tests only) reverts to the default after a failure. - In
nested_for_inner_emptyon the compatibility scheduler, rollback leaves anindegree=0entry for an already executed inner-For exec node. It's cosmetic; outputs are unaffected.
Code.
- Axis order: now reconciled in three places.
_get_iteration_axesstill sorts lexicographically (graph_materializer.py:1897), and its inline condition-axis block (68-97) doesn't exclude closed For final outputs.graph_if_runtime.py:276-283adds another reconciliation and calls the materializer-private_get_ordered_input_iterator_axes, which also re-sorts input edges on every call (46k calls at ifchain L=300). _IteratorAncestry:get_iterator_ancestors_for_edgeis a pass-through.has_iterator_ancestoris only called for its side effect.- There are now three instances (runtime, builder, validator) over the same graph.
- Dead or unused:
visited_source_idsin_get_effective_iteration_paths_for_edgeis effectively dead and not part of the memo key.GraphExecutionState._prepare(graph.py:4219) has no callers.
- O(V) per call:
_record_activation_dependencies(graph_if_runtime.py:260) still does anany(isinstance(…, IfInvocation))scan per exec node (~0.12s in iffree). - Pre-existing: per-item If with a root fallback is still O(frames²) (162k
_is_activation_dependency_satisfiedcalls at n=400). - Size: for context,
graph_materializer.pyis +38% vs main,graph_if_runtime.py+62%, there are 12_can_use_*shape predicates, and_can_use_fresh_literal_if_iterate_collectmatches one exact 5–6 node topology. Not a blocker for this PR, but the next round of fixes in this area would benefit from consolidating axis ownership first.
From my side, items 1 and 2 are what I'd want before merge. Items 3 and the minors could be follow-ups if you prefer.
Restore supported deeper-Iterate If broadcasts and remove If-free prepare transaction overhead. Index active If dependents and document the transaction boundary.
|
@Pfannkuchensack Since my last update, I restored the supported deeper-Iterate If broadcast, removed If-free materialization transaction overhead, and indexed active If dependents for root-If/Collect planning. I added regression and scaling coverage, narrowed the README rollback guarantee, and reran the execution-engine inventory: 2,038 passed, 21 skipped, 2 xfailed. The earlier subprocess aborts did not recur in the final run; their cause remains unknown. |
Keep cached completion coherent across prepared execution registration and source discard.\nDocument the cache lifecycle and cover scheduler bounds and rollback state.
|
I added default-lane bounds for pending-If scans and end-to-end If-chain inactivity work on both scheduler routes. I also fixed completed-source cache invalidation across prepared-execution registration and source discard, strengthened rollback snapshots, retained direct no-If controls, and updated the graph execution README. The final inventory had 2,035 passes and 10 subprocess native aborts; each abort reproduces with the published f6d3fdb graph implementation. |
|
@Pfannkuchensack I updated the PR description with the cache lifecycle details from e89f1af. I verified the final behavior with regressions for duplicate registration and source discard; the focused lifecycle and nested-scheduler set passed 17 cases. The full inventory had 2,035 passes and 10 subprocess aborts; each abort reproduces with published f6d3fdb graph code. Astra's final review found no material issues. |
…ches/1435503-1792098680/a554ef133a8a3cdc6b153c03cf984757781d21e2
…ches/1446250-1792103668/a554ef133a8a3cdc6b153c03cf984757781d21e2
…ches/1466549-1792117301/a554ef133a8a3cdc6b153c03cf984757781d21e2
…ches/1469768-1792119056/a554ef133a8a3cdc6b153c03cf984757781d21e2
…ches/1521496-1792160713/a554ef133a8a3cdc6b153c03cf984757781d21e2
…ches/1594733-1792171819/a554ef133a8a3cdc6b153c03cf984757781d21e2
…ches/1678173-1792197515/a554ef133a8a3cdc6b153c03cf984757781d21e2
Summary
Fix graph execution result loss and stalls across iterator/Collect frame alignment, lazy If branches, empty streams, and deep dependency traversal. Preserve destination-port ordering, reject unsupported open-axis relationships explicitly, support the bounded root-conditioned If/direct-Collect case, and roll back failed planner materialization atomically. Pending-If attachment indexes prepared source/path pairs once per sweep.
The execution engine also preserves the supported case where both If branches use the same deeper Iterate axes than the condition, skips materialization transaction overhead on If-free graphs, and limits root-If/Collect activation resolution to active dependents while preserving topological order. A process-local completed-source cache avoids repeated source-inactivity scans for serial If chains and remains coherent across new prepared executions, duplicate registration, source discard, graph edits, rollback, and restore.
Related Issues / Discussions
Closes #9695
QA Instructions
PYTHONOPTIMIZE=0,OMP_NUM_THREADS=1, and 4 pytest workers: 2,035 passed, 21 skipped, 2 xfailed. Ten subprocess tests aborted withfree(): invalid pointer; all ten also abort individually with the publishedf6d3fdbgraph implementation, so they are not candidate-specific. The native abort cause remains unknown.git diff --checkpassed.Review
The reviewed compatibility and performance follow-ups are covered by regressions, and the README documents transaction boundaries and completed-source cache lifecycle. Independent final review found no material issues. Ten subprocess tests in the final inventory aborted with
free(): invalid pointer; each reproduces individually with the publishedf6d3fdbgraph implementation. Their cause remains unknown.Compatibility / Rollout
No public API or persisted-state format changed. A root-conditioned If with a completed direct Collect branch runs once per preserved Collect frame; ordinary descendants and a downstream Collect preserve those groups. This bounded support does not extend through further nested controls or loops. One Collect may feed both branch ports of the same If; sharing it across distinct If owners raises
UnsupportedSharedIfCollectBranchesError. Unsupported open-axis condition/branch relationships raiseUnsupportedIfIterationAxesErrorbefore invocation. Empty Iterate streams feeding If conditions retire branch sources without invocation and complete the downstream empty collection; retirement can appear inexecuted_history, which is not an invocation trace.Materialization transactions cover
_GenericForPlanner.prepare()and If-bearing_ExecutionMaterializer.prepare()calls. If-free materializer preparation runs without the wrapper; root-If completed-Collect expansion retains its own transaction. Pending-If attachment,prepare_nested_iterate_sequence(), and direct Iterate/Collect preparation are outside that wrapper.Checklist
What's Newcopy (if doing a release after this PR)Need help on this PR? Tag
@codesmith-botwith what you need.