Skip to content

fix(gfql): gfql_clear_caches() cleared the wrong name and silently did nothing - #1836

Open
lmeyerov wants to merge 1 commit into
masterfrom
fix/gfql-clear-caches-real-targets
Open

fix(gfql): gfql_clear_caches() cleared the wrong name and silently did nothing#1836
lmeyerov wants to merge 1 commit into
masterfrom
fix/gfql-clear-caches-real-targets

Conversation

@lmeyerov

Copy link
Copy Markdown
Contributor

The bug

gfql_clear_caches() did not clear the Cypher AST memo, and could not have. The lru_cache sits on _parse_cypher_cached; parse_cypher is a thin validating wrapper that carries no cache. The clear loop looked its target up dynamically and skipped a miss in silence:

for cached in (parse_cypher,):
    clear = getattr(cached, "cache_clear", None)
    if clear is not None:      # never true
        clear()

hasattr(parse_cypher, "cache_clear") is False, 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-process benchmark 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.py gains clear_cypher_parser_caches(), mirroring the existing clear_expr_parser_caches(), and it clears _parse_cypher_cached.
  • Every clear in gfql_clear_caches() is now unconditional. A wrong or renamed target raises AttributeError instead of quietly doing less than the docstring claims.
  • The three @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/ plus gfql_unified.py, collects every @lru_cache/@cache decorated function, and fails unless each one is either in CLEARED or in EXEMPT with 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_cypher must still have no cache_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 lacking cache_clear must raise rather than skip.

Verification

Static, local: ./bin/lint.sh clean; 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:

suite result
test_clear_caches_covers_every_cache.py 6 passed
graphistry/tests/compute/gfql/ 8164 passed, 53 skipped, 19 xfailed

Follow-on, not in this PR

The published compile figure still needs correcting in the benchmark repo, and the cold-process arm 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

…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
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.

1 participant