Skip to content

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

Merged
lmeyerov merged 10 commits into
masterfrom
fix/gfql-clear-caches-real-targets
Aug 2, 2026
Merged

fix(gfql): gfql_clear_caches() cleared the wrong name and silently did nothing#1836
lmeyerov merged 10 commits 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
lmeyerov and others added 4 commits August 1, 2026 17:33
…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
Comment thread agents/skills/review/SKILL.md Outdated
Comment thread ai/docs/gfql/caches.md Outdated
lmeyerov and others added 5 commits August 2, 2026 06:43
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
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
@lmeyerov
lmeyerov merged commit 84ce18d into master Aug 2, 2026
77 checks passed
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
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