[gabortar]

Summary

Fixes #4874.

Graph.toposort() orders local functions with a nested helper, get_used_funcs(nodes). The helper looped over self.subgraphs(), which are the subgraphs of the graph being sorted, instead of the subgraphs of the nodes it was given. self is the same on every recursive call, which caused two bugs:

• Infinite recursion: if the graph has a local function and any top-level node with a subgraph (If/Loop/Scan), sorting functions fails with RecursionError. This happens with mode="full" (the default) and mode="functions", from both Graph.toposort() and Function.toposort().
• Missed dependencies: otherwise, function bodies are never searched below their top level. A function that calls another function only from inside a subgraph is sorted as if it had no dependency, so it can come before the function it calls.

Fix

Recurse into each node's own subgraphs:

 def get_used_funcs(nodes):
     inputs = {}
-    for subgraph in self.subgraphs():
-        inputs.update(get_used_funcs(subgraph.nodes))
     for node in nodes:
+        for subgraph in node.subgraphs():
+            inputs.update(get_used_funcs(subgraph.nodes))
         func_id = (node.domain, node.op)

Whenever the old loop ran at all, it never terminated, so every graph that sorts successfully today skipped it. For valid graphs, the result only changes when a function uses another function from inside a subgraph.

Behavior change

A cycle between function definitions that goes through a subgraph (for example, func2 calls func1 in an If branch and func1 calls func2) now raises OnnxGraphSurgeonException: Cycle detected in function definitions!, the same as direct cycles already did. Previously it was missed and the functions were sorted silently. ONNX doesn't allow these models anyway: the ModelProto.functions comment in onnx.proto says "recursive reference is not allowed".

Tests

New cases in tests/ir/test_graph.py::TestToposort:

Test	Before the fix	After
test_function_order_with_subgraph_in_graph[full], [functions]	RecursionError	pass
test_function_toposort_with_subgraph_in_function	RecursionError	pass
test_function_order_dependency_in_subgraph[full], [functions] (dependency only in the else-branch)	wrong order	pass
test_function_order_dependency_in_nested_subgraph	wrong order	pass
test_function_circular_dep_through_subgraph	no exception	pass

I also checked that the tests reject incomplete fixes. Removing the loop without the per-node recursion, searching only one nesting level, or searching only a node's first subgraph each make at least one new test fail.

Checked manually as well, not added as tests:

• Loop and Scan bodies
• an If nested 300 levels deep
• functions with the same name in different domains
• diamond-shaped dependencies (no false cycle)
• all recurse_subgraphs/recurse_functions combinations
• export → onnx.checker.check_model(full_check=True) → onnxruntime inference on a model whose function calls another only inside an If

Testing

Run in python:3.12-slim with tests/requirements.txt and docs/requirements.txt installed (onnx 1.20.0, onnxruntime 1.19.2):

• PYTHONPATH=. python3 -m pytest tests/: 299 passed, 1 skipped before the change; 306 passed, 1 skipped after.
• sphinx-build docs build/docs -j auto -W (the docs step of make test): passes.
• black --check on the changed files and git diff --check: clean.

I added a CHANGELOG entry under vNext → Fixed.

[gabortar]

Hi! Just following up on this PR. It's been open for a couple of weeks, and I'd appreciate any feedback when someone has a chance to review it.

Happy to make adjustments if needed. Thanks!