Skip to content

Make execute_subdag and nodes_between traversals iterative - #1765

Open
digit50 wants to merge 1 commit into
apache:mainfrom
digit50:fix/iterative-execute-subdag
Open

digit50 wants to merge 1 commit into
apache:mainfrom
digit50:fix/iterative-execute-subdag

Conversation

@digit50

@digit50 digit50 commented Oct 15, 2026

Copy link
Copy Markdown

closes #1760

The sync Driver.execute runs the graph through graph_functions.execute_subdag, which used a nested recursive dfs_traverse that added one stack frame per dependency level. On a linear chain longer than the default recursion limit (1,200 nodes) it raised RecursionError. graph_functions.nodes_between, used by FunctionGraph.nodes_between and the task grouping code, had the same recursive shape and failed in the same way. #1483 fixed get_upstream_nodes but not these two functions.

Changes

  • execute_subdag: dfs_traverse now walks the dependencies with an explicit stack and runs each node after all its dependencies, visiting them in the same order as before. The computed and overrides short-circuits and the per-edge dependency_type (used to skip missing optional inputs) are unchanged. The part that executes a single node moved, unchanged, into a nested compute_node helper.
  • The recursive version raised RecursionError when a graph had a cycle that no override breaks, and test_function_graph_has_cycles_true relies on that. The iterative walk tracks the nodes currently in progress and raises RecursionError when it comes back to one, so that behaviour stays the same and the walk cannot loop forever.
  • nodes_between: the same explicit-stack rewrite. Each frame carries its "any dependency included" flag, so the result is the same as before.

How I tested this

  • New test tests/test_graph.py::test_execute_and_nodes_between_large_chain_no_recursion_error. It builds a 1,200-node chain (node_0 up to node_1199, each adding 1 to the previous one) with exec into a module, runs driver.Builder().with_modules(mod).build().execute(["node_1199"]) and checks the result is 1200.0. It then checks that nodes_between("node_0", "node_1199") returns all 1,200 nodes. Before the change it fails with RecursionError in dfs_traverse.
  • pytest tests/test_graph.py tests/execution tests/test_end_to_end.py tests/test_hamilton_driver.py, pytest tests/function_modifiers tests/plugins/test_polars*.py, and pytest tests/lifecycle tests/caching tests/test_*.py. The only failures in my environment also fail on main without this change: graphviz display tests (no dot binary installed), fingerprint hash constants, and the experimental feather and parquet cache tests (no writer registered).
  • I compared the old and new execute_subdag and nodes_between on 3,000 randomly generated DAGs, with optional and required edges, missing and present inputs, and overrides. Node execution order, results, raised errors and nodes_between output were identical in every case.
  • ruff check and ruff format --check on the changed files.

Notes

The RecursionError for a cyclic graph is now raised on purpose with the message "Cycle detected in the graph at node .", where before Python raised it when the stack overflowed. I kept the exception type so existing callers and tests see the same behaviour.

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

Driver.execute and FunctionGraph.nodes_between walked the graph with recursive depth first searches, so a dependency chain longer than the recursion limit raised RecursionError. Both now use an explicit stack that keeps the same visit and execution order, the computed and overrides short-circuits, and the RecursionError on a cyclic graph. A regression test runs a 1,200-node chain through Driver.execute and nodes_between.

Closes apache#1760

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync Driver.execute raises RecursionError on a 1,200-node linear chain

1 participant