Skip to content

Commit f1f1e69

Browse files
lmeyerovclaude
andcommitted
test(gfql): classify engine availability instead of swallowing every failure
The first version of this file's engine probe wrapped a smoke query in a bare `except Exception: return False`. Two things went wrong with that, both found by trying rather than by argument (on the sibling #1788/#1790 parametrization): * a probe that runs THE SHAPE UNDER TEST disarms its own file — reverting a production guard made the probe raise and every parameter reported SKIPPED instead of failing; * a probe that swallows EVERYTHING is worse — a transient `MemoryError: ... cudaErrorMemoryAllocation` in a fresh GPU container silently dropped cuDF from a run that otherwise passed, and a skipped GPU parameter reads as evidence of passing. So the check now CLASSIFIES: a missing module skips, a recognisable GPU-stack error skips WITH ITS TEXT QUOTED in the reason, and any other failure propagates. The smoke query stays a plain traversal, never the shape under test. GPU receipts — dgx GB10, `graphistry/test-rapids-official:26.02-gfql-polars`, `docker run --gpus all`, cuDF 26.02.01 / polars 1.35.2: 43 passed, 0 skipped (this file plus the #1793 suite), repeated. The marker list is duplicated from the copy landing in `polars_test_utils.py` alongside the #1788/#1790 parametrization; collapse to one definition once both land. Kept duplicated for now so the two PRs stay independently mergeable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
1 parent 2c72119 commit f1f1e69

2 files changed

Lines changed: 55 additions & 17 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
2424
- **Seeded typed-hop property projection keeps its fast path through DISTINCT / ORDER BY / SKIP / LIMIT**: those trailing row ops are plain frame operations, so the fast path now delegates them to the canonical chain instead of declining the whole query.
2525

2626
### Changed
27-
- **Row execution context: `clear_row_exec_context` NULLs rather than restores, and that is now pinned rather than assumed** (tests + comments only; zero runtime delta — no production line changed). Review of #1793 asked whether the exit should RESTORE what the graph carried on entry instead of nulling, since `attach_row_exec_context` INHERITS on the way in (a `None` argument keeps what `g` already carries). Settled as NULL, for three reasons now each carried by a test: (1) `clear` is pure — it returns `g.bind()` and never writes through to the object it was handed, so an outer scope that set the field still has it afterwards and there is no caller state to save; that is what separates this from #1786, which WAS an in-place write onto the caller's own graph, and restore is the fix for a mutation. (2) The only channel restore would change is the RETURN value, and putting the seed back there IS the second half of #1786 — measured, a seed hand-restored onto a result changes the answer of the next query run on that result (7 → 2 → 1 rows). (3) No execution frame inherits a context it did not set: instrumenting `attach` over `graphistry/tests/compute` recorded 3907 calls and zero inheriting ones (53 entered on a graph already carrying a seed, but each was handed the identical `start_nodes` parameter, so the frame still owns the value), because the cross-segment `WITH` seed travels as the explicit `start_nodes` PARAMETER and never through the graph field. The new `test_exec_context_scoping.py` re-runs that ownership measurement as an assertion, so a future path that starts relying on inheritance reopens the decision loudly instead of silently losing an outer value. Mutation-checked in the deciding direction: implementing save/restore at all three attach sites fails 4 of the new tests on every runnable engine, while the existing #1793 suite passes unchanged — i.e. those tests could not distinguish the two designs and this file can.
27+
- **Row execution context: `clear_row_exec_context` NULLs rather than restores, and that is now pinned rather than assumed** (tests + comments only; zero runtime delta — no production line changed). Review of #1793 asked whether the exit should RESTORE what the graph carried on entry instead of nulling, since `attach_row_exec_context` INHERITS on the way in (a `None` argument keeps what `g` already carries). Settled as NULL, for three reasons now each carried by a test: (1) `clear` is pure — it returns `g.bind()` and never writes through to the object it was handed, so an outer scope that set the field still has it afterwards and there is no caller state to save; that is what separates this from #1786, which WAS an in-place write onto the caller's own graph, and restore is the fix for a mutation. (2) The only channel restore would change is the RETURN value, and putting the seed back there IS the second half of #1786 — measured, a seed hand-restored onto a result changes the answer of the next query run on that result (7 → 2 → 1 rows). (3) No execution frame inherits a context it did not set: instrumenting `attach` over `graphistry/tests/compute` recorded 3907 calls and zero inheriting ones (53 entered on a graph already carrying a seed, but each was handed the identical `start_nodes` parameter, so the frame still owns the value), because the cross-segment `WITH` seed travels as the explicit `start_nodes` PARAMETER and never through the graph field. The new `test_exec_context_scoping.py` re-runs that ownership measurement as an assertion, so a future path that starts relying on inheritance reopens the decision loudly instead of silently losing an outer value. Mutation-checked in the deciding direction: implementing save/restore at all three attach sites fails 4 of the new tests on every runnable engine, while the existing #1793 suite passes unchanged — i.e. those tests could not distinguish the two designs and this file can. Engine-parametrized over pandas/cuDF/polars/polars-gpu from a fixed list plus a classified availability check (a missing module skips, a recognisable GPU-stack error skips with its text quoted, and any other failure propagates — a silently skipped GPU parameter reads as evidence of passing); GPU receipts on dgx GB10 / cuDF 26.02.01 / `docker run --gpus all`: 43 passed, 0 skipped.
2828
- **`is_lazy` is a type predicate, so the polars engine's eager/lazy split is checked instead of asserted**: `dtypes.is_lazy` decides WHICH member of the two-member `PolarsFrame` union a frame is, but it was declared `-> bool`, so that answer was discarded at the call boundary — an eager-only attribute (`.height`, `.columns`, `.schema`) reached after a lazy guard was unprovable, and the native polars chain closed the gap with `cast("pl.DataFrame", frame)`. A cast is not a type: it re-asserts, unchecked, the exact fact the predicate had just established, once per call site, and a wrong one surfaces as a production `AttributeError` rather than a CI error. `is_lazy` is now declared `-> TypeIs["pl.LazyFrame"]` (PEP 742), which narrows the *negative* branch as well as the positive — the eager side is the one that needed it, so `TypeGuard` would not have done — and the cast is gone. Four previously untyped chain-combine helpers (`_semi`, `_combine_edges`, `_apply_node_names`, and the `_known_empty` guard) now carry real signatures: `_semi`'s two frames share the `PolarsT` TypeVar because polars joins do not mix eagerness, and the two combine helpers take the `_LazyShim` collect-once duck-type they are actually called with. Typing `_apply_node_names` also retired a `getattr(next_step, "edges_empty", None)` — the shim declares that tri-state in `__slots__`, so a typo is now a checker error instead of a silent `None` that would have re-armed the very cardinality gate the guard disarms. Internal only: no runtime behaviour, no public API, and the `TypeIs` import is `TYPE_CHECKING`-only, so no new runtime dependency floor.
2929
- **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.
3030

graphistry/tests/compute/gfql/test_exec_context_scoping.py

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -38,15 +38,18 @@
3838
plumbing, but the shapes below are exercised on all four engines because the two attach/clear
3939
pairs are duplicated across the generic chain and the native polars chain. No CI lane runs cuDF
4040
or polars-gpu (``ci-gpu.yml`` is hard-disabled and does not install the ``polars`` extra), so
41-
those two parameters report SKIPPED on CI — visibly, via a runtime probe, never as a silent
42-
pass. They are exercised out of band on the dgx GPU box against
41+
those two parameters report SKIPPED on CI — visibly, with the reason quoted, never as a silent
42+
pass. The availability check CLASSIFIES rather than swallows: a missing module skips, a
43+
recognisable GPU-stack error skips with its text quoted, and any other failure propagates,
44+
because a silently skipped GPU parameter reads as evidence of passing. They are exercised out
45+
of band on the dgx GPU box against
4346
``graphistry/test-rapids-official:26.02-gfql-polars`` (``docker run --gpus all`` — omitting
4447
``--gpus all`` FABRICATES failures rather than skipping).
4548
"""
4649
from __future__ import annotations
4750

4851
from functools import lru_cache
49-
from typing import Any, List, Tuple
52+
from typing import Any, List, Optional, Tuple
5053

5154
import pandas as pd
5255
import pytest
@@ -101,26 +104,61 @@ def _seed(engine: str) -> Any:
101104
return _frame(engine, pd.DataFrame({"id": [0]}))
102105

103106

104-
@lru_cache(maxsize=None)
105-
def _engine_runnable(engine: str) -> bool:
106-
"""Probe by RUNNING the smallest version of what these tests do.
107+
#: Substrings that identify a broken/absent GPU STACK rather than a product defect. Matched
108+
#: against ``type(ex).__name__: str(ex)``, lowercased. Deliberately narrow — anything not
109+
#: listed is treated as a REAL failure, because guessing wrong in that direction produces a
110+
#: silently green GPU lane. (Duplicated from the copy landing in `polars_test_utils.py`
111+
#: alongside the #1788/#1790 engine parametrization; collapse to one definition once both land.)
112+
_GPU_ENVIRONMENT_MARKERS = (
113+
"libnvrtc", "libcuda", "libcudart", "nvrtc", "cuinit",
114+
"no cuda-capable device", "cuda driver", "cuda runtime",
115+
"cuda_error", "cudaerror", "driver version",
116+
"gpu engine requested", # polars' own message when cudf_polars is absent
117+
)
118+
107119

108-
Cheaper probes do not discriminate on a box with cudf/cudf_polars importable but no working
109-
CUDA runtime — frame construction and simple ops all succeed there and the suite then dies
110-
inside the first real kernel. So the probe is an actual boundary-call run.
120+
@lru_cache(maxsize=None)
121+
def _engine_skip_reason(engine: str) -> Optional[str]:
122+
"""``None`` => this engine MUST run here; a string => a stated, checkable skip reason.
123+
124+
Two traps, both established by trying rather than by argument:
125+
126+
* An IMPORT-only check does not discriminate on a box where cudf / cudf_polars import
127+
against an incomplete CUDA runtime — construction and simple ops succeed and the suite
128+
then dies inside the first real kernel, which reads as a product failure. So a smoke
129+
QUERY runs.
130+
* A check that SWALLOWS every exception is worse. A transient `cudaErrorMemoryAllocation`
131+
in a fresh GPU container silently dropped cuDF from an otherwise-green run, and a
132+
skipped GPU parameter reads as evidence of passing. So: a missing module skips, a
133+
recognisable GPU-stack error skips WITH ITS TEXT QUOTED, and anything else propagates.
134+
135+
The smoke query is a plain traversal, never the shape under test — a check that ran the
136+
asserted shape would let a regression disarm its own test.
111137
"""
138+
import importlib.util
139+
140+
for module in {"polars": ("polars",), "polars-gpu": ("polars", "cudf_polars"),
141+
"cudf": ("cudf",)}.get(engine, ()):
142+
if importlib.util.find_spec(module) is None:
143+
return f"{module} is not installed"
112144
try:
113-
_graph(engine).gfql(list(BOUNDARY_OPS), engine=engine)
114-
return True
115-
except Exception: # noqa: BLE001 — any failure means "cannot run", never "test fails"
116-
return False
145+
_graph(engine).gfql([n(), e_forward(), n()], engine=engine)
146+
except BaseException as ex: # noqa: BLE001 — classified below, never blanket-swallowed
147+
if isinstance(ex, ImportError):
148+
return f"{type(ex).__name__}: {ex}"
149+
text = f"{type(ex).__name__}: {ex}".lower()
150+
if any(marker in text for marker in _GPU_ENVIRONMENT_MARKERS):
151+
return f"{type(ex).__name__}: {ex}"
152+
raise
153+
return None
117154

118155

119156
def _require(engine: str) -> None:
120-
if not _engine_runnable(engine):
157+
reason = _engine_skip_reason(engine)
158+
if reason is not None:
121159
pytest.skip(
122-
f"engine {engine!r} is not runnable in this environment — NOT evidence that it "
123-
"passes; see the COVERAGE BOUNDARY note in this module's docstring"
160+
f"engine {engine!r} unavailable here ({reason}) — NOT evidence that it passes; "
161+
"see the COVERAGE BOUNDARY note in this module's docstring"
124162
)
125163

126164

0 commit comments

Comments
 (0)