fix(gfql): gfql_clear_caches() cleared the wrong name and silently did nothing - #1836
Merged
Conversation
…nothing The Cypher AST memo is an lru_cache on _parse_cypher_cached; parse_cypher itself carries no cache. gfql_clear_caches() looked up cache_clear with getattr(obj, ..., None) and skipped a None result, so naming parse_cypher made the call a silent no-op and every 'cold-process' number measured through it was wrong. - parser.py gains clear_cypher_parser_caches(), mirroring clear_expr_parser_caches - every clear in gfql_clear_caches is now UNCONDITIONAL: a wrong name raises - the 3 Lark LALR parser lru_caches stay uncleared on purpose (grammar, not input) - new test enumerates every @lru_cache in the GFQL tree by AST and fails unless it is either cleared or exempted with a written reason Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx
…tyle caches Audit of every GFQL-tree file for process-lifetime caches (2026-08-01): - 11 @lru_cache functions: all already classified (2 cleared, 9 exempt singletons). - ONE unaccounted cache: _SINGLE_ALIAS_CACHE in the polars row pipeline, an OrderedDict keyed by (predicate string, alias, schema). Keyed by caller input, so it belongs to the class gfql_clear_caches promises to empty; it sat in the guard's blind spot because the AST scan only saw decorators. - Config registries (_COST_GATE_FRAC_OVERRIDES, lazy-mode overrides) and per-invocation locals are correctly out of scope: clearing them would change semantics, not cost. gfql_clear_caches now empties the single-alias lowering memo unconditionally (module import is safe on polars-free installs; its polars imports are all TYPE_CHECKING-guarded). The guard test gains a second AST scan for module-level dict/OrderedDict/set bindings named cache/memo, so the next hand-rolled memo fails the lock instead of hiding, and a functional pin proves the new clear works. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
The new functional pin for _SINGLE_ALIAS_CACHE needs polars installed, and polars exists only in the test-polars lane's explicit file list. The lane completeness meta-test caught the omission. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
…n documented Owner direction: replace the central list-of-names approach with structured registration. Each cache setup site now registers, adjacent to its own definition, either a clearable handle (bound cache_clear / dict.clear / lock-guarded callable) or a process-singleton exemption with its written reason. gfql_clear_caches() imports the clearable hosts and runs cache_registry.clear_all(), which raises on an empty registry. The name-lookup failure mode that shipped a silent no-op cannot recur: registration hands over the bound clear at definition time, and a new pin proves the clear survives a module-attribute swap. The coverage lock now enforces registration: its AST sweep (decorators plus dict-style memos) imports each discovered module and fails on any cache absent from the registry. Exemption reasons moved from test data into the source, validated at registration. Convention documented in ai/docs/gfql/caches.md and the review skill's GFQL section (BLOCKER on unregistered memos). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
…tubs Protocols carry only cache_clear (mypy's _lru_cache_wrapper stub has no __name__), names resolve via getattr with a str() wrap, and the thin-reason probe in the lock test is itself an lru_cache function. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
lmeyerov
commented
Aug 2, 2026
lmeyerov
commented
Aug 2, 2026
Owner direction: the review skill is generic and already routes reviewers to nearby .md guidance and the always-inspect set (DEVELOP.md among them); component rules do not belong inline there. ai/docs/ is effectively deprecated. The registry module docstring stays the spec; DEVELOP.md gets the compact pointer next to the other CI guards. No new files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
…-during-read The hit path read sat outside the KeyError guard, so a registry clear_all() or cross-thread eviction landing between move_to_end and the lookup raised KeyError to the caller instead of recomputing. The read now rides inside the same try. Deterministic interleave test simulates the race; registry docstring records the threading contract (import-lock registration, snapshot clear, reload raises on purpose). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
…ched too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
Five workers hammer the expr/cypher/single-alias hot paths while a sixth loops the clear; assertions are value-correctness against single-threaded oracles plus no escaped exceptions, so the test cannot flake on timing. Complements the deterministic clear-during-read interleave pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
lmeyerov
added a commit
that referenced
this pull request
Aug 2, 2026
Resolve CHANGELOG.md conflict by keeping both Fixed entries: this PR's show_indexes engine-usability entry and master's gfql_clear_caches registry entry (#1836). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
lmeyerov
added a commit
that referenced
this pull request
Aug 2, 2026
…ded ns unit The new named-fast-path dtype test hardcoded 'datetime64[ns]', but pandas 3.x (pandas==3.0.5 in the failing CI lanes) infers datetime64[us] from pd.to_datetime on date strings, so the INPUT itself is [us] and both the served lane and the policy-forced full path preserve it exactly. Assert what the test actually means: the served lane's dtype equals the input's and the full path's (and is a datetime64), pandas-version-agnostic. Verified green under both pandas 3.0.5 and pandas 2.2.3. Fixes red test-pandas-compat-gfql (latest, py3.14), test-core-python (3.13), and test-gfql-core (3.14) on the merge result with master (#1836/#1837). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011AB4RZpph3uSFUpzKnZJcr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
gfql_clear_caches()did not clear the Cypher AST memo, and could not have. Thelru_cachesits on_parse_cypher_cached;parse_cypheris a thin validating wrapper that carries no cache. The clear loop looked its target up dynamically and skipped a miss in silence:hasattr(parse_cypher, "cache_clear")isFalse, so the loop body never ran. Nothing raised, nothing warned.Why it mattered beyond hygiene
A cache-clearing function that cannot fail is indistinguishable from one that does nothing, and this one was load-bearing for measurement. A
cold-processbenchmark arm — defined as "clear every process-lifetime cache, then run" — was used to publish a per-query compile cost of 2.3–10.2 ms, i.e. 22–65% of a whole 20k-row call. That number was wrong: the AST memo was never emptied, so the delta being attributed to compilation was measuring something else. Directly profiled, compilation is 0.94–2.61 ms (parse 0.64–1.44 + lower 0.30–1.17). LALR(1) parsing was never the problem it was reported to be.The same silence is a correctness hazard: a stale process-lifetime memo makes results order-dependent, so a test can pass alone and fail in a suite.
The fix
cypher/parser.pygainsclear_cypher_parser_caches(), mirroring the existingclear_expr_parser_caches(), and it clears_parse_cypher_cached.gfql_clear_caches()is now unconditional. A wrong or renamed target raisesAttributeErrorinstead of quietly doing less than the docstring claims.@lru_cache(maxsize=1)Lark parser objects stay uncleared, on purpose: they hold the LALR(1) parse tables, which are a function of the grammar rather than of any query, are built once per process, and cost more to rebuild than any parse they serve. Clearing them would put grammar construction inside every "cold" measurement — a cost no caller can ever pay twice.The test
graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py— static + functional.It walks the AST of every file under
graphistry/compute/gfql/plusgfql_unified.py, collects every@lru_cache/@cachedecorated function, and fails unless each one is either inCLEAREDor inEXEMPTwith a written reason. Adding a memo and saying nothing breaks the build; so does leaving a stale name in the lists after the cache is gone. Two memos are cleared (_parse_cypher_cached,_parse_expr_cached); nine singletons are exempted, each with its justification recorded next to it.The functional half pins the specific defect:
parse_cyphermust still have nocache_clear(so the indirection this guards is unchanged), the memo must be non-empty after a parse and empty after the clear, the LALR tables must be the same object after a clear, and replacing a clear target with one lackingcache_clearmust raise rather than skip.Verification
Static, local:
./bin/lint.shclean;MYPY_CMD="uvx mypy==2.3.0" ./bin/mypy.sh→ no issues in 327 source files.Tests on dgx-spark in
graphistry/test-rapids-official:26.02-gfql-polars,--gpus all --network none, polars 1.35.2 / pandas 2.3.3:test_clear_caches_covers_every_cache.pygraphistry/tests/compute/gfql/Follow-on, not in this PR
The published compile figure still needs correcting in the benchmark repo, and the
cold-processarm needs re-measuring now that the clear actually works — that will move the boards it fed. Filed as follow-up work; this PR is only the fix and its guard.🤖 Generated with Claude Code
https://claude.ai/code/session_01YYZRXegrALuXd3NHH5evqx