Repository navigation
Conversation
Pfannkuchensack
left a comment
There was a problem hiding this comment.
I reviewed this against base 3d76b457d6 / head a3c177102f. I found no blockers: the refactor keeps the existing behavior, and the context lifetime is safe.
Checks:
- CI is green.
- Ruff 0.11.2 check/format and
git diff --checkpass. - On the PR,
tests/app/services/shared/,tests/test_graph_execution_state.pyandtests/test_node_graph.pygive 1728 passed, 0 failed. - The two import tests your description says abort with
free(): invalid pointerpassed here (Linux).
Correctness
has_iterator_ancestoris equivalent to the oldany(For/Iterate in nx.ancestors(state._iterator_graph(g), node_id)):- It uses the same
_iterator_graphfilter: Collect in-edges and ForFinaledges are dropped. - It checks strict ancestors only, so the node itself is not counted.
- A missing node raises the same
NetworkXError.
- It uses the same
- The context is scoped to one
prepare()call (or one top-level_get_if_iteration_pathscall). It is built lazily and never stored or serialized, so it needs no invalidation. Admission and materialization within oneprepare()see the sameg.
Findings (all minor)
1. Scope vs. title. The context only covers the literal-condition If ancestry fallback. The real prepare() hot path still rebuilds the filtered iterator graph on every admission check:
_get_parent_iteration_mappings(graph_materializer.py:1322)_get_if_condition_iteration_mappings- argument-less
get_node_iterators()(1441-1445,1466,1610,1821,1069)
A depth-20 nested-literal-If profile with 200 padding nodes shows 5,643 _iterator_graph builds (~7.5s of ~10.2s). Only 39 of them go through the new index. _IteratorAncestry builds the filtered graph privately and exposes only a boolean, so those consumers can't share it. A follow-up that carries the filtered iterator graph per prepare() would probably need to reshape this class.
The README describes the scope accurately; the title ("index iterator ancestry once per snapshot") reads broader than the change.
2. End-to-end effect is small. The operation-count tables use direct-query fixtures. Full next()/complete() runs (min of 3, OMP_NUM_THREADS=1):
| Graph | Base | PR |
|---|---|---|
| Nested literal Ifs, depth 20 (230 → 20 builds) | 311 ms | 292 ms (~6%) |
| Same, with 200 padding nodes | 6762 ms | 6648 ms (~1.7%) |
The other representative graphs never hit the fallback, and their build counts are identical:
- Iterate + Ifs
- 250-node chain into Ifs
- plain Iterate→Collect
- wide Add/If fan-out
Across the three suites above, the fallback fired 839 times, all but 2 of them from this PR's own unit tests.
The admitted sparse-query cost is ~1.6× per single query (247 → 388 µs at 132 nodes, 2.07 → 3.48 ms at 1,002 nodes). It is paid at most once per prepare() and only when the fallback fires, so it's not a realistic regression. Still, the net win is modest; maybe phrase the PR as groundwork rather than a performance fix.
3. Tests are coupled to internals.
test_graph_if_runtime.py:177-205andtest_graph_materializer.py:73-122count builds by monkeypatching the privateGraphExecutionState._iterator_graph.- The materializer test passes an
nx.subgraph_viewthat hides condition nodes, which production never does. - A fixture through
next()/complete()would cover the same reuse with real output expectations, e.g. a dynamic inner If feeding a chain of literal outer Ifs, liketest_nested_if_literal_condition_activates_selected_branch. test_new_context_observes_supplied_graph_mutationsadds little, since there is no retained cache it could catch.- The random-DAG equivalence test is good.
Nits
graph_if_runtime.py:16-24, 70-81: bothsource_graphanditerator_ancestry(bound to its own graph) are passed through. They could drift if a future caller passes mismatched ones. Every caller is consistent today.- README: "one compatibility
prepare()call"._GenericForPlanner.prepare(graph_for_planner.py:29) also goes through_ExecutionNodeBuilder.prepare.
Heads-up
-
Conflict with #9843: this PR conflicts with #9843 in
graph_if_runtime.py, because #9843 rewrites_get_effective_iteration_paths_for_edgeiteratively. Whichever lands second needs to threaditerator_ancestrythrough the new loop. -
Pre-existing stall, unrelated to this PR: the graph below runs nothing and stays
is_complete() == Falseon both base and this PR.next()returnsNoneimmediately.IntegerCollection([1,2]) → Iterate → Add(b=10) → If(condition=True).true_input → Collect.item- With two parallel Ifs it completes.
- Suspected cause: the collection source waits on the If's activation dependency, while the If waits for its Iterate ancestor.
- Probably worth a separate issue (near #9789).
|
I am abandoning this branch in favor of merging it into #9843 in order to avoid merge conflicts and additional review cycles. |
c51d47a to
0c78fe2
Compare
Reuse one analysis during recursive If inference and preparation. Add shared correctness and operation-count evidence.
0c78fe2 to
dd1e841
Compare
Reuse one analysis during recursive If inference and preparation. Add shared correctness and operation-count evidence.
Summary
Build the filtered iterator graph and ancestry index once per snapshot. Reuse it across recursive If inference and preparation. Build lazily when a fallback queries ancestry.
Related Issues / Discussions
Related: #9789, which has overlapping If admission work. This change preserves compatibility and does not modify that PR. No issue is claimed as fixed.
QA Instructions
free(): invalid pointer, also reproduced on unchanged main.git diff --check: passed.Results matched at fixture sizes 16, 32, 64, and 128. Counts cover graph analysis only, not all inference or scheduler work. The wide fixture shows the eager index costs more traversal work for sparse queries; these are operation counts, not wall-clock or production workload claims.
Review
Independent review found no findings or suggestions. No code changes followed the review.
Compatibility / Rollout
Checklist
What's Newcopy (if doing a release after this PR)Need help on this PR? Tag
@codesmith-botwith what you need.