Repository navigation
perf(serve): build the MCP traversal view and shortest-path graphs once per graph - #4192
rohit-jsfreaky wants to merge 1 commit into
Conversation
…ce per graph query_graph copied every edge of the loaded graph into an undirected traversal view on every call, and shortest_path rebuilt a sorted DiGraph (or Graph) of every edge on every call. The structures are the same for a given graph object, so build each once and cache it on G.graph, exactly like the trigram index: a hot reload swaps in a new G, so a cached structure never outlives its graph. Sharing is safe because nothing writes to them: _filter_graph_by_context builds a new graph when a filter applies, the traversals and the renderer only read the view, and only nx.shortest_path reads the path graphs. The path graph is still built from sorted node and edge lists, so Graphify-Labs#2074's deterministic route is unchanged. The traversal-view docstring that chose a fresh copy per query is updated. graphify's own graph (18,865 nodes / 38,458 edges), the same calls run interleaved with the cache cleared vs kept, 240 outputs identical: query_graph 524 -> 316 ms median (filtered "what calls X": 508 -> 360), shortest_path 211 -> 0.8 ms directed, 109 -> 0.7 ms undirected. Memory: +8 MB for the view, +24 MB for both path graphs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @rohit-jsfreaky. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. PR-changed functions: 0/2 verified (0 proven, 0 may-equivalent, 0 distinguished) · 2 not verified (2 vacuous).
Not verified on this run: \_shortest\_path\_text (vacuous: never exercised), \_traversal\_view (vacuous: never exercised).
Graphify review — findings
Caches the undirected traversal view and the sorted shortest-path search graphs on G.graph, like the trigram index, so warm query_graph and shortest_path calls no longer copy every edge of the graph per request. The path graph is cached per direction mode via the new _path_search_graph helper. A hot reload swaps in a fresh G, so neither cache can outlive the graph it was built from.
No blocking issues surfaced. 8 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 559 functions depend on the 151 functions this change touches.
Health — this change adds coupling hotspots:
- new:
main()— 102 callers, 3 callees - new:
_query_graph_text()— 30 callers, 10 callees - new:
dispatch_command()— 2 callers, 128 callees - new:
_score_query()— 15 callers, 7 callees - new:
_load_graph()— 23 callers, 3 callees - new:
_query_terms()— 20 callers, 3 callees - new:
run_benchmark()— 16 callers, 3 callees - new:
_run_cli()— 6 callers, 7 callees - …and 15 more — each is listed as a finding
Verification — 559 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 375 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
11 of 338 test file(s) selected (3%) via static blast radius.
tests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_file_label_disambiguation.py— impacttests/test_mcp_graph_view_cache.py— impact, changed-testtests/test_path_cli.py— impacttests/test_query_induced_edges.py— impacttests/test_query_mcp_direction.py— impacttests/test_query_names_its_graph.py— impacttests/test_serve.py— impacttests/test_serve_http.py— impacttests/test_terraform.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_shortest\_path\_text.
The verifier did not have enough to check \_shortest\_path\_text, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 35 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly KeyError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_traversal\_view.
The verifier did not have enough to check \_traversal\_view, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: not verifiable: all 5 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous
· 2 grounded finding(s) anchored inline below; 21 more finding(s) on lines outside this diff (see the check run).
| G.graph["_traversal_view"] = H | ||
| return H | ||
|
|
||
| def _query_graph_text( |
There was a problem hiding this comment.
_query_graph_text()
fans out to 10 callees (efferent coupling); 30 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
| return H | ||
|
|
||
|
|
||
| def _shortest_path_text(G: nx.Graph, arguments: dict) -> str: |
There was a problem hiding this comment.
_shortest_path_text()
12 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
This branch has not been deployed
1 participant
What does this PR do?
Fixes #4191.
_traversal_view(G)builds the undirected view once per graph object and caches it onG.graph["_traversal_view"]._path_search_graph(G, undirected)holds the sorted DiGraph / Graph_shortest_path_textused to rebuild per call, cached per direction onG.graph["_path_search_graphs"]. Its body is the old inline code, comments included, so graphify path: non-deterministic shortest-path route + calls label not backed by any stored edge #2074's deterministic route and path: no route through a contains edge — file-to-file dependency via a shared symbol reports no path #3878's reversecontainshops are unchanged.Same pattern as the trigram index: a hot reload swaps in a new
G, so a cached structure never outlives its graph.About the design note this changes.
_traversal_view's docstring said: "A fresh copy per query rather than a cached one:_filter_graph_by_contextalready copies per query when a filter applies, and the copy shares node data dicts withG, so only the edge dicts are duplicated." That was a memory/simplicity trade-off. The numbers below show the per-call copy is most of a warm query and the cache costs ~27 MB, so this PR flips it and updates the docstring. Happy to drop either half if you prefer to keep the old trade-off.Why sharing is safe. Nothing writes to the cached structures (checked every function on the path with an AST scan for add/remove/update/setdefault/pop/item assignment):
_filter_graph_by_contextbuilds a new graph when a filter applies and never touches its input;_bfs,_dfs,_complete_induced_edges,_subgraph_to_textonly read (their writes go to local sets/dicts), and none reads.graph;nx.shortest_pathreads the path graphs.Each structure has a single call site.
Type of change
(performance; no behaviour change)
Verification & Invariants
Invariant: every
query_graph/shortest_pathanswer is byte-identical to a freshly built structure, in any order of calls, and a reloaded graph never sees the previous graph's structures.graphify's own graph (18,865 nodes / 38,458 edges). The same calls run interleaved in one process with the cache cleared (= v8 behaviour) vs kept, so thermal drift hits both sides; outputs asserted equal on every call (240 calls):
query_graph, no filter (n=120)query_graph, "what calls X" heuristic filter (n=30)shortest_path, directed (n=45)shortest_path,undirected=true(n=45)Separate processes (v8 vs PR, alternated, 2 runs each; 80 queries + 30 paths; all outputs identical across the 4 processes):
query_graphmedianshortest_pathmedianquery_graphcall (pays the one build)(An earlier run on a hotter machine gave the same ratios: 524 -> 316 ms, 211 -> 0.8 ms.)
How was this tested?
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. (n/a, no skill sources touched)🤖 Generated with Claude Code