Skip to content

Commit 84ce18d

Browse files
authored
Merge pull request #1836 from graphistry/fix/gfql-clear-caches-real-targets
fix(gfql): gfql_clear_caches() cleared the wrong name and silently did nothing
2 parents 6ab6d64 + 1c7dee9 commit 84ce18d

12 files changed

Lines changed: 552 additions & 17 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
4242
- **GFQL execution context is declared rather than attached at runtime**: the private `_gfql_*` per-execution fields (index policy and registry, row-pipeline base graph, carried seed nodes, edge aliases, shortest-path backend, and the indexed-bindings handoff) are now declared on `Plottable` with defaults on `PlotterBase`, instead of being set with `setattr` and read back with `getattr(..., default)`. No public API or behaviour change; internal call sites are typed, and hand-rolled `Plottable` stand-ins must now construct the full context.
4343

4444
### Fixed
45+
- **`gfql_clear_caches()` cleared the wrong name and silently did nothing**: the clear targeted `parse_cypher` while the memo lives on the `_parse_cypher_cached` body, so every "cold-process" measurement taken through it was warm. Every process-lifetime GFQL cache now self-registers in `graphistry/compute/gfql/cache_registry.py` at its own definition site -- a bound clear handle for caller-input-keyed memos (including the previously unaccounted polars single-alias lowering `OrderedDict`, found by audit), or a written exemption for maxsize=1 function-of-the-code singletons. A coverage lock fails CI on any unregistered memo (decorator AND dict-style scans), clears are recompute-safe against concurrent reads, and a thread-stress test pins value correctness under concurrent clearing. Convention: DEVELOP.md "GFQL Cache Registry".
4546
- **The compiled-query cache key omitted the engine, which a shared cache would have turned into a wrong answer.** The key relied on `node_dtypes` to capture engine-dependent compilation, but polars and polars-gpu report *identical* node dtypes and compile differently. While the cache was partitioned per `Plottable` this was masked rather than avoided; sharing plans process-wide exposed it immediately as 16 failures in `test_const_fold_engine_parity.py` on the polars-gpu arm. The key now carries the **resolved** engine -- resolved, because `auto` resolves per graph, so keying on the requested engine would let two graphs that resolve differently share a plan.
4647

4748
- **`master` is green again: the type-hygiene ratchet tripped on merge, and the fix is fewer `cast()`s rather than a raised cap**: `bin/ci_type_hygiene_baseline.json` is a per-file SNAPSHOT, so it goes stale whenever a PR *other than the one that owns the baseline* touches a baselined file. #1830 captured its baseline on a branch whose merge-base is `b6181d355` — before #1800, #1799 and #1816 landed — and each of those three added `cast()` calls to a file #1830 had already pinned. All four were green on their own bases; the combination was not, and nothing in any of the four merges signalled it: `explicit-cast` came out 7 over baseline across `gfql_fast_paths.py` (+5), `gfql_unified.py` (+1) and the polars `row_pipeline.py` (+1), failing `python-lint-types` on 3.11/3.13/3.14. The baseline is **unchanged** here; the findings were removed instead. Three of the seven were never `typing.cast` at all — the guard matches any call whose name is `cast`, including the attribute form, so `pl.Expr.cast`, a polars RUNTIME dtype conversion, counts as a typing finding; those three carry the documented `# hygiene-ok: explicit-cast` escape hatch with the reason. The other four were real, and are gone by DECLARATION rather than by call-site assertion, which is the idiom the guard is asking for: `_two_hop_cached_equal_domain_degree_counts` declares `counts: Tuple[DataFrameT, DataFrameT]` once and both arms assign to it, so four casts collapse to one localized `# type: ignore[assignment]` on the polars arm (`DataFrameT` is pinned to pandas at checking time); `_apply_connected_optional_match` declares `seed_ids: SeriesT` / `node_ids: SeriesT` instead of casting, because selecting one column off a frame is a Series on every engine. (The neighbouring `cast(DataFrameT, df_to_engine(...))` pair is deliberately left alone: `df_to_engine` carries no return annotation, so removing those would mean annotating a helper with 115 call sites, and the polars arm of that branch is not reached by any CI lane — rewriting it would have dragged a pre-existing coverage blind spot into the changed-line gate for no typing gain.) All three files now sit AT or BELOW baseline (18/18, 132/133, 41/42). Typing-only: `typing.cast` is the identity function at runtime, so every removal is provably value-preserving, and the one restructured line binds an unchanged `pl.Expr` list to a name so the per-line suppression fits the 127-column limit. **The hazard is latent, not spent** — any future PR adding a finding to a baselined file reds `master` the same way, and the guard cannot see it from inside either PR.

DEVELOP.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,17 @@ Do **not** raise a cap with `--update-baseline` to make a new finding go away.
168168
Lowering caps after fixing debt is the intended use; commit the code change and
169169
the baseline update together.
170170

171+
### GFQL Cache Registry
172+
173+
Every process-lifetime cache in `graphistry/compute/gfql/**` registers itself in
174+
`graphistry/compute/gfql/cache_registry.py`, at its own definition site, as either
175+
clearable (keyed by caller input; `gfql_clear_caches()` empties it) or an exempt
176+
process singleton with a written reason. The module docstring is the spec;
177+
`graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py` fails CI on
178+
any unregistered memo. Never clear a cache by name lookup -- registration hands
179+
over the bound clear handle precisely because a name-based clear once shipped a
180+
silent no-op and a wrong published benchmark number.
181+
171182
#### Conventions behind the checks
172183

173184
- **Never write to a caller's `Plottable`.** Caching by `setattr` keyed on

bin/test-polars.sh

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ python -m pytest --version
2222
# fails if any module-level polars-gated test file is absent from it (or listed but gone).
2323
POLARS_TEST_FILES=(
2424
graphistry/tests/compute/test_polars.py
25+
# cache-coverage lock: its static scans run everywhere, but the functional pin for the
26+
# polars single-alias lowering memo can only execute where polars is installed
27+
graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py
2528
graphistry/tests/compute/gfql/test_engine_polars_hop.py
2629
graphistry/tests/compute/gfql/test_engine_polars_chain.py
2730
graphistry/tests/compute/gfql/test_engine_polars_row_pipeline.py
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
"""Central ledger of every process-lifetime GFQL cache.
2+
3+
Each cache SETUP SITE registers itself here, adjacent to its own definition, as
4+
either CLEARABLE (keyed by caller input; ``gfql_clear_caches`` empties it) or a
5+
PROCESS SINGLETON (a function of the code, exempt, with the written reason).
6+
7+
Why a registry instead of a central list of names: the bug that motivated this
8+
file was ``gfql_clear_caches`` looking a clear target up BY NAME and silently
9+
doing nothing when the memo actually lived on a different object
10+
(``parse_cypher`` vs ``_parse_cypher_cached``). A registration hands over the
11+
bound ``cache_clear``/``dict.clear`` of the real object at definition time, so
12+
there is no later lookup to get wrong. Classification and exemption reasons
13+
live next to the cache they describe, and the coverage lock in
14+
``graphistry/tests/compute/gfql/test_clear_caches_covers_every_cache.py``
15+
statically discovers every cache in the tree and fails when one is not
16+
registered -- registration is enforced, not optional.
17+
18+
A module that is never imported registers nothing, and that is correct: its
19+
cache object does not exist, so there is nothing to clear.
20+
21+
Threading: registration runs at import time under the import lock plus this
22+
module's own lock; ``clear_all`` snapshots under the lock and clears outside it.
23+
``lru_cache.cache_clear`` is internally locked; dict-style memos must make their
24+
hit paths recompute-safe against a clear racing a read (see the single-alias
25+
memo). ``importlib.reload`` of a cache host raises the registered-twice error on
26+
purpose: the reloaded module would leave a zombie cache this registry can no
27+
longer reach, and that deserves a loud failure, not a silent overwrite.
28+
"""
29+
30+
from __future__ import annotations
31+
32+
import threading
33+
from typing import Callable, Dict, NamedTuple, Optional, Protocol
34+
35+
36+
class _SupportsCacheClear(Protocol):
37+
"""An ``@lru_cache``-decorated function (mypy's wrapper stub carries no __name__,
38+
so the name is read with getattr at runtime, where functools always sets it)."""
39+
40+
def cache_clear(self) -> None: ...
41+
42+
43+
class _SupportsClear(Protocol):
44+
"""A dict-like memo: anything with an argument-free ``clear``."""
45+
46+
def clear(self) -> None: ...
47+
48+
49+
class CacheEntry(NamedTuple):
50+
name: str
51+
clear: Optional[Callable[[], None]] # None => exempt process singleton
52+
reason: Optional[str] # None => clearable
53+
54+
55+
_REGISTRY: Dict[str, CacheEntry] = {}
56+
_LOCK = threading.Lock()
57+
58+
59+
def _register(entry: CacheEntry) -> None:
60+
with _LOCK:
61+
existing = _REGISTRY.get(entry.name)
62+
if existing is not None and existing != entry:
63+
raise ValueError(f"cache {entry.name!r} registered twice with different handles")
64+
_REGISTRY[entry.name] = entry
65+
66+
67+
def register_clearable(fn: _SupportsCacheClear, name: Optional[str] = None) -> None:
68+
"""Register an ``@lru_cache`` function whose memo gfql_clear_caches must empty."""
69+
clear = getattr(fn, "cache_clear", None) # runtime check: typing cannot stop a bare fn
70+
if clear is None:
71+
raise TypeError(f"{fn!r} has no cache_clear; register the decorated function itself")
72+
resolved = name if name is not None else str(getattr(fn, "__name__", repr(fn)))
73+
_register(CacheEntry(resolved, clear, None))
74+
75+
76+
def register_clearable_dict(name: str, mapping: _SupportsClear) -> None:
77+
"""Register a hand-rolled dict/OrderedDict memo for clearing."""
78+
_register(CacheEntry(name, mapping.clear, None))
79+
80+
81+
def register_clearable_callable(name: str, clear: Callable[[], None]) -> None:
82+
"""Register a clear callable for a cache that needs its own locking discipline."""
83+
_register(CacheEntry(name, clear, None))
84+
85+
86+
def register_process_singleton(fn: _SupportsCacheClear, reason: str) -> None:
87+
"""Exempt a maxsize=1, function-of-the-code cache, with the reason in writing."""
88+
fn_name = str(getattr(fn, "__name__", repr(fn)))
89+
if len(reason.split()) < 6:
90+
raise ValueError(f"{fn_name}: exemption reason is too thin to be a reason")
91+
_register(CacheEntry(fn_name, None, reason))
92+
93+
94+
def clear_all() -> None:
95+
"""Empty every registered clearable cache. Raises if nothing is registered."""
96+
with _LOCK:
97+
entries = list(_REGISTRY.values())
98+
if not any(entry.clear is not None for entry in entries):
99+
raise RuntimeError(
100+
"no clearable GFQL cache is registered; the registry import wiring is broken"
101+
)
102+
for entry in entries:
103+
if entry.clear is not None:
104+
entry.clear()
105+
106+
107+
def entries() -> Dict[str, CacheEntry]:
108+
"""Snapshot for the coverage lock test."""
109+
with _LOCK:
110+
return dict(_REGISTRY)

graphistry/compute/gfql/call/validation.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@
5353
ring_continuous_axis_payload_error,
5454
)
5555
from graphistry.compute.gfql.row.prefilter import is_alias_prefilters
56+
from graphistry.compute.gfql.cache_registry import register_process_singleton
5657
from graphistry.compute.gfql.row.order_expr import (
5758
is_order_aggregate_alias_ast,
5859
order_expr_ast_static_supported,
@@ -92,6 +93,9 @@ def _where_rows_expr_parser_fn() -> Optional[WhereRowsParserBundle]:
9293
return None
9394

9495

96+
register_process_singleton(_where_rows_expr_parser_fn, "binds imported callables and returns None when lark is absent; a resolved import is not stale state, and re-resolving it cannot change the answer")
97+
98+
9599
def _where_rows_expr_parse(expr: str) -> Optional[WhereRowsParsedExpr]:
96100
parser_bundle = _where_rows_expr_parser_fn()
97101
if parser_bundle is None:

graphistry/compute/gfql/cypher/parser.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
from __future__ import annotations
22

3+
from graphistry.compute.gfql.cache_registry import (
4+
register_clearable,
5+
register_process_singleton,
6+
)
7+
38
import ast as pyast
49
from dataclasses import dataclass, replace
510
from functools import lru_cache
@@ -627,6 +632,9 @@ def _parser_lalr() -> _ParserLike:
627632
return cast(_ParserLike, parser)
628633

629634

635+
register_process_singleton(_parser_lalr, "Lark LALR(1) tables for the whole-query grammar; function of the grammar, built once per process, and far dearer than any single parse it serves")
636+
637+
630638
@lru_cache(maxsize=1)
631639
def _pattern_parser() -> _ParserLike:
632640
Lark, _, _, _ = _lark_imports()
@@ -642,6 +650,9 @@ def _pattern_parser() -> _ParserLike:
642650
return cast(_ParserLike, parser)
643651

644652

653+
register_process_singleton(_pattern_parser, "Lark LALR(1) tables for the pattern-fragment start rule; same reasoning as the whole-query tables")
654+
655+
645656
@lru_cache(maxsize=1)
646657
def _where_predicate_chain_parser() -> _ParserLike:
647658
Lark, _, _, _ = _lark_imports()
@@ -659,6 +670,9 @@ def _where_predicate_chain_parser() -> _ParserLike:
659670
return cast(_ParserLike, parser)
660671

661672

673+
register_process_singleton(_where_predicate_chain_parser, "Lark LALR(1) tables for the flat WHERE-chain start rule; same reasoning as the whole-query tables")
674+
675+
662676
def _retarget_span(s: SourceSpan, base: SourceSpan) -> SourceSpan:
663677
"""Shift an atom-relative span (from re-parsing an isolated atom substring) into
664678
absolute query coordinates, given the atom's absolute ``base`` span. Exact for
@@ -2192,6 +2206,26 @@ def _parse_cypher_cached(query: str) -> Union[CypherQuery, CypherUnionQuery, Cyp
21922206
return node
21932207

21942208

2209+
register_clearable(_parse_cypher_cached)
2210+
2211+
2212+
def clear_cypher_parser_caches() -> None:
2213+
"""Empty the Cypher query->AST memo. Called by ``gfql_clear_caches``.
2214+
2215+
Clears ``_parse_cypher_cached`` ONLY. The three ``@lru_cache(maxsize=1)`` Lark parser
2216+
objects (``_parser_lalr``, ``_pattern_parser``, ``_where_predicate_chain_parser``) are
2217+
deliberately left alone: they hold the LALR(1) parse tables, which are a pure function
2218+
of the grammar rather than of any query, are built once per process, and cost far more
2219+
to rebuild than any query parse. Emptying them would make a "cold" measurement include
2220+
grammar construction -- a cost no caller can ever pay twice.
2221+
2222+
Note ``parse_cypher`` itself carries NO cache and therefore no ``cache_clear``; the memo
2223+
lives on the ``_parse_cypher_cached`` body it delegates to. Clearing the wrong name here
2224+
silently did nothing for as long as it went unnoticed.
2225+
"""
2226+
_parse_cypher_cached.cache_clear()
2227+
2228+
21952229
def _validate_graph_bindings(node: Union[CypherQuery, CypherGraphQuery]) -> None:
21962230
"""Validate graph binding names: no duplicates, no forward/circular refs."""
21972231
bindings = node.graph_bindings

graphistry/compute/gfql/expr_parser.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
from __future__ import annotations
22

3+
from graphistry.compute.gfql.cache_registry import (
4+
register_clearable,
5+
register_process_singleton,
6+
)
7+
38
from dataclasses import dataclass
49
from functools import lru_cache
510
from typing import Any, Callable, Dict, FrozenSet, Iterable, List, Optional, Protocol, Sequence, Set, Tuple, Type, Union, cast
@@ -311,6 +316,9 @@ def _parser() -> _ParserLike:
311316
return cast(_ParserLike, parser)
312317

313318

319+
register_process_singleton(_parser, "Lark LALR(1) tables for the row-expression grammar; function of the grammar, built once per process")
320+
321+
314322
def _parse_string_token(token: str) -> str:
315323
try:
316324
value = parse_cypher_string_token(token)
@@ -808,6 +816,9 @@ def _parse_expr_cached(expr: str) -> ExprNode:
808816
return cast(ExprNode, node)
809817

810818

819+
register_clearable(_parse_expr_cached)
820+
821+
811822
def is_expr_node(node: object) -> bool:
812823
return isinstance(node, _EXPR_NODE_TYPES)
813824

graphistry/compute/gfql/index/cypher_ddl.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
"""
1313
from __future__ import annotations
1414

15+
from graphistry.compute.gfql.cache_registry import register_process_singleton
16+
1517
from functools import lru_cache
1618
import re
1719
from typing import Optional, Pattern, Tuple, cast
@@ -41,6 +43,9 @@ def _ddl_prefix_re() -> Pattern[str]:
4143
return re.compile(_DDL_PREFIX_PATTERN, re.IGNORECASE)
4244

4345

46+
register_process_singleton(_ddl_prefix_re, "a compiled regex over a module-level pattern constant; function of the code alone")
47+
48+
4449
@lru_cache(maxsize=1)
4550
def _ddl_res() -> Tuple[Pattern[str], Pattern[str], Pattern[str], Pattern[str]]:
4651
return (
@@ -51,6 +56,9 @@ def _ddl_res() -> Tuple[Pattern[str], Pattern[str], Pattern[str], Pattern[str]]:
5156
)
5257

5358

59+
register_process_singleton(_ddl_res, "compiled regexes over module-level pattern constants; function of the code alone")
60+
61+
5462
def looks_like_index_ddl(query: str) -> bool:
5563
return bool(isinstance(query, str) and _ddl_prefix_re().match(query))
5664

graphistry/compute/gfql/lazy/engine/polars/row_pipeline.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
PolarsFrameT = TypeVar("PolarsFrameT", "pl.DataFrame", "pl.LazyFrame")
3131

3232
from graphistry.Plottable import Plottable
33+
from graphistry.compute.gfql.cache_registry import register_clearable_dict
3334
from graphistry.utils.json import JSONVal
3435
# Engine-neutral wire-format payload types (ASTCall.params). Shapes are safelist-validated
3536
# (gfql/call/validation.py) before reaching these helpers, so the runtime isinstance/len
@@ -724,6 +725,9 @@ def _bare_column_ast(node: ExprNode, alias: str) -> Optional[ExprNode]:
724725
_SINGLE_ALIAS_CACHE_MAX = 512
725726

726727

728+
register_clearable_dict("_SINGLE_ALIAS_CACHE", _SINGLE_ALIAS_CACHE)
729+
730+
727731
def _single_alias_cache_key(
728732
expr: str, alias: str, schema: Mapping[str, "pl.DataType"], columns_nan_free: bool
729733
) -> _SingleAliasKey:
@@ -790,10 +794,12 @@ def lower_single_alias_predicate(
790794
if key in _SINGLE_ALIAS_CACHE:
791795
try:
792796
_SINGLE_ALIAS_CACHE.move_to_end(key)
793-
except KeyError: # pragma: no cover - concurrent eviction; recompute-safe
794-
pass
795-
else:
797+
# The read stays INSIDE the try: a concurrent eviction or a registry
798+
# clear_all() landing between move_to_end and this lookup must degrade
799+
# to a recompute, never surface KeyError to the caller.
796800
return _SINGLE_ALIAS_CACHE[key]
801+
except KeyError: # concurrent eviction/clear; recompute-safe
802+
pass
797803
lowered = _lower_single_alias_predicate_uncached(expr, alias, schema, columns_nan_free)
798804
_SINGLE_ALIAS_CACHE[key] = lowered
799805
# Eviction races can only DROP an entry (a redundant recompute), never fabricate a hit.

graphistry/compute/gfql/row/pipeline.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@
7070
is_entity_text_scalar,
7171
)
7272
from graphistry.compute.gfql.same_path_types import NODE_IDENTITY_COLUMN
73+
from graphistry.compute.gfql.cache_registry import register_process_singleton
7374
from graphistry.compute.gfql.series_str_compat import is_non_textual_scalar_dtype, series_sequence_len, series_str_match
7475
from graphistry.compute.gfql.row.ordering import (
7576
build_list_sort_columns,
@@ -116,6 +117,9 @@ def _gfql_expr_runtime_parser_bundle() -> Optional[GFQLRuntimeParserBundle]:
116117
return None
117118

118119

120+
register_process_singleton(_gfql_expr_runtime_parser_bundle, "an import-resolution bundle, None on a minimal install; re-resolving cannot change the answer")
121+
122+
119123
@lru_cache(maxsize=1)
120124
def _gfql_cudf_list_sort_requires_host_bridge() -> bool:
121125
"""cuDF 25.02 can segfault in list-sort pivot internals; bridge to pandas there."""
@@ -132,6 +136,9 @@ def _gfql_cudf_list_sort_requires_host_bridge() -> bool:
132136
return (major, minor) <= (25, 2)
133137

134138

139+
register_process_singleton(_gfql_cudf_list_sort_requires_host_bridge, "probes the installed cuDF version for a segfaulting list-sort; the installed version cannot change inside a process")
140+
141+
135142
def _gfql_bridge_cudf_df_to_pandas(work_df: Any) -> Any:
136143
"""Prefer Arrow bridge for cuDF host conversion on older RAPIDS releases."""
137144
if hasattr(work_df, "to_arrow"):

0 commit comments

Comments
 (0)