Repository navigation
Conversation
aff22cf to
d62878b
Compare
lstein
left a comment
There was a problem hiding this comment.
Requesting changes for two new stalls, one silent wrong-value change and one over-rejection. Each was reproduced against main (6af2057) and the PR head (11ba2a7). The default and compatibility schedulers behave the same in every case.
Repros use tests.test_nodes helpers (create_edge, create_loop_linkage, AnyTypeTestInvocation, MarkedAnyTypeTestInvocation, FixedCollectionTestInvocation), plus CollectionConcatInvocation and BooleanInvocation. Edges are written as (src, field, dst, field). "Stall" means next() returns None while is_complete() is False.
Blockers
1. Stall with a wired root condition and both branches from For outputs under two nested Iterates (regression)
src(first=[[["a"]], [["b","c"]]]) -> it0 ; it0.item -> cc0.first ; cc0.collection -> it1.collection
for X in (T, F):
it1.item -> forX.collection ; forX.item -> bodyX.value (Marked marker=X) ; bodyX.value -> retX.output ; loop_linkage(forX, retX)
forT.output_collection -> if.true_input ; forF.output_collection -> if.false_input
BooleanInvocation(value=True|False).value -> if.condition ; if.value -> collect.item
- main:
{(0,): [['Ta']], (1,): [['Tb','Tc']]}, and theFalsecase gives the matchingFvalues. - PR: stalls for both values. Nothing runs after
it1. - Variant: one For whose output feeds
true_inputdirectly andfalse_inputthrough a passthrough also stalls. In that variant the downstream Collect has already emitted{(0,): [], (1,): []}. - Unaffected: the literal-condition and one-Iterate versions complete.
- Likely cause: the new wired-condition gating of a direct For combines with the new
continuewhile the For is unfinalized (graph_materializer.py, selected-edge attachment). The If cannot get frames until the For finalizes, and the For waits for the If's verdict. That is the deadlock the removed comment ingraph_if_activation.pydescribed.
2. A shape this PR newly accepts stalls when its inner levels are empty
Use this PR's fixture _outer_condition_with_two_collects_graph(share_branch_axes=True) and change only nodes["source"].first to [[]], [[], []] or [[[]], [[]]].
- PR: no If executions are created, which is correct, and
collectemits{(0,): [], (1,): []}. Buttrue_copy/false_copyare never retired, sois_complete()staysFalse. - Effect: in the session processor this leaves a queue item that never finishes.
- main: an equivalent graph completes.
maincannot load the fixture itself.
Should fix
3. Duplicate If executions at path (), and literal vs computed conditions disagree
src(first=[["c"],["d"],["e"]]) -> it0 ; it0.item -> for.collection ; for body Marked("F") -> ret ; loop_linkage(for, ret)
AnyType(value="R").value -> if.true_input ; for.output_collection -> if.false_input
BooleanInvocation(value=True).value -> if.condition ; if.value -> collect.item
- PR: three If executions, all with iteration path
().collectgets['R','R','R']. A non-Collect sink downstream runs once, so three executions collapse onto one frame. - main, and the PR with a literal
True: one If, giving['R']. - Empty Iterate: the selected root branch disappears.
maingives a value; the PR gives[].
The same split shows up in the existing shape Iterate A([["a","b"],["c"]]) -> For -> mid -> if.true_input, with a root fallback on false_input:
| condition | main | PR |
|---|---|---|
literal False |
['fb'] |
['fb'] |
computed False |
['fb'] |
['fb','fb'] |
computed False, src=[] |
['fb'] |
[] |
- The PR updates
test_root_condition_if_with_for_output_branch_under_iterate_completesto expect["fb","fb"]. The new literal test expects["fb"]for the same graph. - On #9789 we agreed to keep
['fb']for this shape. - The README says the If broadcasts over parent frames through a selected For final output. Here it broadcasts while the For branch is unselected.
- Whichever cardinality is chosen, literal and computed conditions with the same value should agree, and the If executions should carry distinct frame paths.
4. Validation rejects graphs that main runs correctly, and the rule depends on unrelated Collects
src(first=[[[["a"],["b","c"]]], [[["d"]],[[]]], [[["e"]]]]) -> it0 -> cc0 -> it1 -> cc1 -> it2
it2.item -> tm(Marked "t") -> tcol.item ; tcol.collection -> tp.first (Concat) ; tp.collection -> if.true_input
it2.item -> fm(Marked "f") -> fcol.item ; fcol.collection -> fp.first (Concat) ; fp.collection -> if.false_input
it0.item -> cond(FixedCollectionTestInvocation).value ; cond.condition -> if.condition ; if.value -> collect.item
- main: runs it with correct per-frame values.
- PR:
UnsupportedIfIterationAxesError: ... deeper open iterator axes kept by a Collect: condition ['it0'], branch ['it0', 'it1']. - Both branches keep the same
[it0, it1]axes with identical frames. - Unrelated Collect lifts the rejection: add
AnyType("r") -> R(Collect), withR.collectionfeeding bothtp.secondandfp.second, and the PR accepts and runs the graph._validate_collect_branch_axescollects Collects fromnx.ancestorsover the whole source graph, so any shared Collect satisfies it, not "a shared Collect in that chain" as the README states. - The other main-accepts/PR-rejects cases I found have asymmetric axes, and rejecting those looks like legitimate tightening.
5. RecursionError on long passthrough chains (low)
_get_edge_frame_axes now recurses through ordinary nodes.
- Graph: a per-item wired If (
src(["x",""]) -> it -> FixedCollection cond) whosetrue_inputis a chain of 1000AnyTypenodes. - PR: raises
RecursionErroron the firstnext(). 800 nodes still work. - main: fine at 1200 nodes.
- The Collect branch of the same function already uses an explicit stack; the passthrough path could do the same.
Checked and fine
- Tests:
tests/test_graph_execution_state.py,tests/app/services/sharedandtest_if_processor_sqlite.pypass: 1972 passed, 2 xfailed. - Benchmarks: the slow benchmarks in
test_graph_execution_performance.pyandtest_for_scheduler_performance.pymatchmainwithin about 1%. - Shape harness: I ran the 237-shape If/Iterate/For harness from #9789.
- 229 shapes are unchanged.
- 2 shapes that stall on
mainnow complete. - 2 values change; these are item 3.
- 4 differ only in random node IDs in error messages.
- Direct For under a per-item condition: rejected frames skip the For, its body and ForReturn in every order tried. That includes all-true, all-false, an empty input, a For also consumed outside the If, and a For feeding both branches.
- Other checks: error paths (
set_node_erroron the body, ret, For, condition or fallback), nested Ifs around a gated direct For, theA or B and C or Dprecedence in the activation gate, and the extra empty outer groups kept by the_get_collect_source_iterator_idschange are all fine. - Fuzz: a 3000-seed For/Iterate/Collect/If fuzz across the default and compatibility schedulers, plus a save/reload round-trip mid-run, showed no scheduler or round-trip divergence. It found 26 graphs that stall on
mainand complete on this PR.
|
@lstein Thanks for the review. The reported If-frame cases are resolved. I also cleaned up the axis traversal and Collect path inference to avoid repeated work. |
e086358 to
06c04a2
Compare
Remove unused compatibility activation queries and consolidate Collect-axis and input-edge ordering logic. Preserve scheduler-specific comparisons and For-final ancestry semantics.
Handle nested For-final and empty Collect frame scheduling with correct root selection cardinality. Validate retained Collect axes, reject asymmetric branches, and keep passthrough traversal linear.
Reuse branch destination membership and retained Collect axes during frame inference. Cover empty-path fallback with an edge-axis lookup regression.
06c04a2 to
7968947
Compare
lstein
left a comment
There was a problem hiding this comment.
Thanks for the fixes. I re-reviewed 7968947 against its merge base with main (8359490). I also ran the cases against the PR as it was before your last two commits (685c027, called "pre-fix" below), to separate new regressions from older behavior.
Earlier findings: F1, F2, F4 and F5 are fixed. F3 is not.
New problems: four stalls that main doesn't have. All four come from f2732f1 / 7968947; both main and the pre-fix head complete every case below. Requesting changes for F3 and these four.
Repros use tests.test_nodes helpers. Edges are written as (src, field, dst, field). "Stall" means next() returns None while is_complete() is False. The default and compatibility schedulers behave the same unless noted.
F3 (still open): duplicate If executions at path ()
src(first=[["c"],["d"],["e"]]) -> it0 ; it0.item -> for.collection ; for body Marked("F") -> ret ; loop_linkage(for, ret)
AnyType(value="R").value -> if.true_input ; for.output_collection -> if.false_input
BooleanInvocation(value=True).value -> if.condition ; if.value -> collect.item
- PR: 3 If executions, all with iteration path
(), socollectgets['R','R','R']. main, and the PR with a literalTrue: one execution,['R'].
The README now says "literal and computed root conditions have the same result cardinality", but they still differ on the shape from the first review, src([["a","b"],["c"]]) -> Iterate A -> For -> mid -> if.true_input with a root fallback on false_input:
| condition | main | PR |
|---|---|---|
literal False |
['fb'] |
['fb'] |
computed False |
['fb'] |
['fb','fb'] |
Please make computed and literal conditions agree, and give each If execution its own frame path. Alternatively, narrow the README.
New regressions
R1 (blocker): a literal If whose unselected branch is a For's final output stalls
For(id="for", collection=["a","b"]) ; for.item -> body(Marked "b:") -> ret ; loop_linkage(for, ret)
for.output_collection -> if.true_input ; AnyType(value="fb").value -> if.false_input
If(condition=False) ; if.value -> sink(AnyType).value # or a Collect.item sink
- PR: only
fallbackruns, then the run stalls. mainand pre-fix:'fb'.- Mirror case: the same stall happens with the For on
false_inputandIf(condition=True). - Not affected: computed conditions, and the selected-For cases.
- Coverage: this is the most basic For-in-If shape, and the 2023-test suite doesn't cover it.
Cause:
- The new gate at
graph_if_activation.py:64(branch_field == _get_source_level_selected_field(if)) now makes the unselected direct For wait for the If to reject its branch. - But a literal If still has to pass
_has_unmaterializable_for_final_inputin the materializer's readiness check, which is skipped only for wired conditions. That check waits for every direct For-final input to finish, including the gated one. - The literal early return added to
_is_source_inactivedoesn't break the cycle at root level, because nothing queues the For for retirement. - Likely fix: for a literal If, check only the selected field's inputs, as
_get_if_readiness_ancestorsalready does. The If can then resolve, and_queue_rejected_activation_branchretires the For.
R2: R1 nested under a wired outer If
src(first=[["a","c"],["c","b"]]) -> it0
it0.item -> forT.collection ; forT.item -> bT(Marked "T") -> rT ; loop_linkage(forT, rT)
it0.item -> forF.collection ; forF.item -> bF(Marked "F") -> rF ; loop_linkage(forF, rF)
forT.output_collection -> iif.true_input ; forF.output_collection -> iif.false_input ; If(id="iif", condition=False|True)
iif.value -> p(Marked "p:").value ; p.value -> if.true_input ; AnyType("R").value -> if.false_input
BooleanInvocation(value=True).value -> if.condition ; if.value -> out(Collect).item
- PR: stalls for either literal value on
iif. mainand pre-fix:["p:['Fa','Fc']", "p:['Fc','Fb']"].- Not affected: the same graph without the outer If, or with the outer condition
False.
R3: unselected direct For under an Iterate stalls after a mid-run save/reload
src(first=[["a","b"],["c"],["d"]]) -> it ; it.item -> for.collection ; for body Marked("F") -> ret ; loop_linkage(for, ret)
for.output_collection -> if.true_input ; AnyType("R").value -> if.false_input ; If(condition=False) ; if.value -> out(Collect).item
- Trigger:
dump_execution_statethenload_execution_state, after the lastitexecution and before the If (step 5 here). - PR: stalls. After the load,
for,bodyandretreport inactive but are never retired. - Without the reload, and at every other step: the PR gives
['R']. mainand pre-fix:['R']at every step.- Fuzz: a reload sweep over 3000 seeds found 11 such cases at steps 4–5, all with this shape.
R4: a wired root condition selecting a Collect group over a For under an empty Iterate stalls
src(first=[[], [[[]], [["c","a","c"]]]]) -> it0 -> cc0(Concat) -> it1 -> cc1(Concat) -> it2
es(Concat, first=[]) -> eit(Iterate) ; eit.item -> for.collection ; for body Marked("F") -> ret ; loop_linkage(for, ret)
for.output_collection -> if.true_input
it1.item -> cm(Marked "x") -> col(Collect).item ; col.collection -> p(Marked "F:") -> if.false_input
BooleanInvocation(value=False).value -> if.condition ; if.value -> out(Collect).item
- PR: stalls after the Iterate chain;
cm,col,p,ifandoutnever run. main:['F:[]', "F:['x[[]]', \"x[['c', 'a', 'c']]\"]"].- pre-fix: completes, but with
[].
The README's new "axis reached only through an unselected For final output" exception describes exactly this shape. Two more fuzz seeds give the same stall.
Performance
A per-item wired If whose true_input is a chain of N passthrough nodes:
| N | main | pre-fix | PR |
|---|---|---|---|
| 200 | 0.30s | 0.40s | 0.93s |
| 400 | 1.10s | 1.36s | 3.49s |
| 800 | 3.68s | 4.89s | 13.3s |
- Where the time goes:
cProfileat N=400 puts 6.9 of 10.0s in the new_has_empty_if_frame._is_source_inactivecalls it for every node on the branch chain, and each call:- runs
_get_if_branch_frame_axeswith a fresh{}cache; - runs its own walk over the branch looking for For final outputs.
- runs
- Cheap fix: return early unless some frame Iterate has executed with no items, or cache the axes per If.
- Wide graphs are unaffected: 25, 50 or 100 Ifs on one Iterate run the same as
main.
Checked and fine
- Tests:
tests/test_graph_execution_state.py,tests/app/services/sharedandtest_if_processor_sqlite.py: 2023 passed, 2 xfailed. - Shape harness: the 237-shape harness from #9789 matches
mainon 233 shapes and fixes 2 ofmain's stalls. It differs on two: R1, and the computed-Falseresult count under F3. - Fuzz: 3000 seeds over For/Iterate/Collect/If graphs, plus another 3000 that add nested Ifs, root-level Fors and Fors under empty Iterates. Run on the default and compatibility schedulers, with save/reload at several steps.
- No regressions beyond the ones above.
- It fixes 38 graphs that stall on
main.
- Mechanisms that held up: the iterative
_get_edge_frame_axes,_has_unresolved_if_frame_axes(it never deferred forever in any probed case), the new fallbacks in_get_effective_iteration_paths_for_edge, and the empty-iterator mapping fallback.
Summary
Fix two If/For scheduling gaps:
Examples:
Related Issues / Discussions
Also closes #9918 and closes #9922.
QA Instructions
uv run --no-sync pytest -n 4 tests/test_graph_execution_state.py tests/app/services/shared/test_execution_engine_differential.py tests/app/services/shared/test_graph_if_activation.py tests/app/services/shared/test_graph_if_dependencies.py tests/app/services/shared/test_graph_if_runtime.py tests/app/services/shared/test_graph_iterator_ancestry.py tests/app/services/shared/test_graph_materializer.py- 1,345 passed, 2 xfailed.uv run --no-sync pytest -q tests/app/services/session_processor/test_if_processor_sqlite.py- 8 passed.git diff --checkpassed.Review
The final review found no remaining blockers. An over-broad root-axis rejection found earlier was removed; root-value broadcast and nested cases remain covered.
Compatibility / Rollout
Checklist
What's Newcopy (if doing a release after this PR)Need help on this PR? Tag
@codesmith-botwith what you need.