perf(serve): persist the trigram index and add an exact-id fast path - #3271
perf(serve): persist the trigram index and add an exact-id fast path#3271mallyskies wants to merge 1 commit into
Conversation
…de ids A cold CLI call spent ~6.7s of ~10.3s rebuilding the trigram index. `_get_trigram_index` already memoizes, but on `G.graph` — right for a long-lived server, useless to a CLI where every invocation is a fresh process. The index is small and pickles in ~0.05s, so persist it. - `_find_node_tiers` returns immediately when the query is already a node id. Callers holding one — notably the second call after a `find_node_ambiguity` error, which hands the caller an id — were paying a full index build to rediscover a node the graph looks up in constant time. A single-entry tier also reports no ambiguity, which is correct: an id names exactly one node. - `_get_trigram_index` reads and writes a pickle keyed on the graph file's path, mtime and size. Written via temp-file-plus-rename so a torn write is never read back; every failure path falls through to a rebuild rather than raising, so a corrupt or unwritable cache costs speed, never correctness. It lives under the user cache dir (LOCALAPPDATA on Windows, XDG_CACHE_HOME or ~/.cache elsewhere), not beside graph.json — an 80 MB derived binary inside the corpus lands inside whatever VCS tracks the graph, and is not something you want synced between machines. GRAPHIFY_TRIGRAM_CACHE_DISABLE=1 opts out; GRAPHIFY_TRIGRAM_CACHE_DIR relocates it. The cache key needs the graph's path. `serve._load_graph` stashes it, but the CLI builds its own graph in `cli.py` and never calls that function, so the two load sites `explain` and `path` use stash it too. Measured on a 209k-node, 221 MB graph: 10.2s -> 3.4s once warm, and a two-call ambiguity resolution ~20.5s -> ~6.8s. Verified byte-identical output with and without the cache, that touching graph.json forces a rebuild, and that a corrupted or unwritable cache degrades rather than raising.
![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.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 4 advisory finding(s) below merit a look before merge.
Formal verification. 2 change(s) tested, no difference found (not proven).
Graphify review — findings
Adds an on-disk trigram-index cache so cold CLI queries stop rebuilding the index every invocation: _get_trigram_index now loads a pickled index keyed on the graph file's path/mtime/size and stores it after a build, with the cache living under XDG_CACHE_HOME/LOCALAPPDATA (overridable via GRAPHIFY_TRIGRAM_CACHE_DIR, disableable via GRAPHIFY_TRIGRAM_CACHE_DISABLE) rather than beside graph.json where VCS would pick it up. To make the cache reachable from the CLI—which bypasses _load_graph—both graph-loading paths in dispatch_command stamp _graph_path onto the graph, and _load_graph sets it too. Also short-circuits _find_node_tiers when label is an existing node id, returning it directly so exact-id lookups (including the ambiguity two-step's second call) skip the index build entirely.
Worth a look
- Untrusted trigram cache is deserialized with pickle —
graphify/serve.py:429· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Non-atomic temp-file rename race with concurrent writers using shared suffix —
graphify/serve.py· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Concurrent trigram cache writers share one temp file —
graphify/serve.py:444· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Cache path errors can now escape _get_trigram_index —
graphify/serve.py:446· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 587 functions depend on the 182 functions this change touches.
Health — this change adds coupling hotspots:
- new:
dispatch_command()— 2 callers, 123 callees - new:
_query_graph_text()— 20 callers, 9 callees - new:
_score_query()— 15 callers, 5 callees - new:
_query_terms()— 20 callers, 3 callees - new:
run_benchmark()— 16 callers, 3 callees - new:
_stale_graph_sources()— 7 callers, 6 callees - new:
_load_graph()— 9 callers, 4 callees - new:
_build_server()— 2 callers, 17 callees - …and 11 more — each is listed as a finding
Verification — 587 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: 533 function(s) in the blast radius were not formally verified this run
Formal verification
Could not verify: Could not verify dispatch\_command.
The verifier did not have enough to check dispatch\_command, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
No difference found (not proven): No behavior difference found in \_find\_node\_tiers (not a proof).
The verifier ran both versions of \_find\_node\_tiers on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
No difference found (not proven): No behavior difference found in \_get\_trigram\_index (not a proof).
The verifier ran both versions of \_get\_trigram\_index on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Could not verify: Could not verify \_load\_graph.
The verifier did not have enough to check \_load\_graph, 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: not verifiable: all 23 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly SystemExit — names the real obstacle, not a sampling gap)
· 1 grounded finding(s) anchored inline below; 18 more finding(s) on lines outside this diff (see the check run).
| pass | ||
|
|
||
|
|
||
| def _get_trigram_index(G: nx.Graph) -> dict: |
There was a problem hiding this comment.
_get_trigram_index()
6 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
1 participant
A cold CLI call spends most of its time rebuilding the trigram index.
_get_trigram_indexmemoizes onG.graph, which is right for a long-lived server but does nothing for a CLI where every invocation is a fresh process. The index is small and pickles in a fraction of a second, so it can be persisted to disk instead.Two changes.
An on-disk trigram cache, keyed on the graph file's path, mtime and size. Writes go to a temporary file that is renamed into place, so a partially-written file is never read back. Nothing in the cache path raises. If the file is missing, stale or unreadable, the index is rebuilt exactly as it is today.
The cache lives under
XDG_CACHE_HOMEorLOCALAPPDATArather than besidegraph.jsonbecause a large cache file written into the corpus could mistakenly end up part of the corpus itself.GRAPHIFY_TRIGRAM_CACHE_DIRoverrides the location andGRAPHIFY_TRIGRAM_CACHE_DISABLE=1turns the cache off.An exact-id fast path.
_find_node_tiersnow returns immediately when the query is already a node id. Callers that hold one were paying a full index build to rediscover a node the graph looks up in constant time. The clearest case is the second call after afind_node_ambiguityerror, which hands the caller an id and invites them to call again with it. A single-entry tier also reports no ambiguity, which is correct, since an id names exactly one node.NOTE: These numbers come from a corpus roughly 12x your largest published benchmark, ERPNext at 22,620 nodes and 48,710 edges. On our 285k-node, 445k-edge graph that was about 6.7s of a 10.3s call. The saving is proportional rather than absolute, so another graph will benefit more or less than ours. Fortunately, the mechanism costs virtually nothing when the cache misses.
🤖 Generated with Claude Code