test(gfql): decide #1793's clear-vs-restore question, and pin it - #1801
Merged
Conversation
This was referenced Jul 28, 2026
… review) Review asked whether `clear_row_exec_context` should RESTORE the value the graph carried on entry rather than NULLing it — a fair question, because `attach_row_exec_context` INHERITS on the way in (a `None` argument keeps what `g` already carries), so an outer scope's value can reach an inner execution and be dropped by it. Determination: NULL is correct and restore would reintroduce #1786. Three reasons, each now carried by a test rather than by argument: 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. There is no caller state to save. That is exactly what separates this from #1786, which WAS an in-place write onto the caller's own graph — restore is the fix for a mutation, and there is no mutation. 2. The only channel restore would change is the RETURN VALUE, and putting the seed back there IS the second half of #1786 ("the result of a WITH query carries the seed, and a follow-up query on that result is answered against it"). Measured: hand-restoring the seed onto a result changes the answer of the next query on it (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 DID enter on a graph already carrying a seed (nested boundary frames) but each was handed the IDENTICAL `start_nodes` parameter, so the frame still owns what it sets. The cross-segment WITH seed travels as the explicit `start_nodes` PARAMETER (`chain_impl(..., start_nodes=)`, `_compiled_query_reentry_state`), never through the graph field, so the field's lifetime is exactly one boundary-call run. `test_exec_context_scoping.py` re-runs that ownership measurement as an assertion over a corpus of the shapes that reach every attach site, so a future path that starts relying on inheritance reopens this decision loudly instead of silently losing an outer value. MUTATION-CHECKED IN THE DECIDING DIRECTION: implementing save/restore at all three attach sites (chain, native polars chain, gfql_unified) fails 4 of the new tests on every runnable engine — while the existing #1793 suite (`test_reentry_caller_graph_immutability.py`) passes UNCHANGED. Those tests could not distinguish the two designs; this file can, which is why it exists. Engine-parametrized over pandas/polars/cuDF/polars-gpu with a fixed engine list plus a runtime probe (not `available_nonpandas_engines()`, which silently shrinks): a non-runnable engine reports SKIPPED, it does not vanish from the report. Also added to `bin/test-polars.sh` — the file has no module-level `importorskip`, so nothing would otherwise have flagged that its polars params run in no lane (#1795 class). RUNTIME DELTA: zero. No production line changed; the only non-test edits are the `clear_row_exec_context` docstring and a CHANGELOG entry, so no pyg-bench lane run is required (CB5). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB
…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
lmeyerov
force-pushed
the
audit/exec-context-restore
branch
from
July 28, 2026 03:01
f1f1e69 to
2e00a17
Compare
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
1 participant
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.
Answers the review question left open on #1793: should
clear_row_exec_contextRESTORE the value the graph carried on entry, rather than NULLing it? ("unsure how reserved & locally scoped this field is").It is a fair question —
attach_row_exec_contextINHERITS on the way in (aNoneargument means "keep whatevergalready carries"), so an outer scope's value CAN reach an inner execution, and the inner execution then drops it. That asymmetry is real and reachable from the internal API.Determination: NULL is correct. Restore would reintroduce #1786.
Three reasons, each now carried by a test rather than by argument:
clearis pure. It returnsg.bind()and never writes through to the object it was handed, so an outer scope that set the field still has it afterwards. There is no caller state to save, so there is nothing to restore for. This is exactly what separates it from GFQL: WITH re-entry mutates the caller's Plottable, silently corrupting the next query #1786, which WAS an in-placedispatch_graph._gfql_start_nodes = ...onto the caller's own graph — save/restore is the fix for a mutation, and there is no mutation here.The only channel restore would change is the RETURN VALUE — and putting the seed back there IS the second half of GFQL: WITH re-entry mutates the caller's Plottable, silently corrupting the next query #1786 ("the RESULT of a WITH query carries the seed, and a follow-up query on that result — a different graph entirely — is answered against it"). Measured, not asserted: hand-restoring the seed onto a result changes the answer of the next query run on that result (7 → 2 → 1 rows).
No execution frame ever inherits a context it did not set. Instrumenting
attach_row_exec_contextover the wholegraphistry/tests/computetree recorded 3907 calls and ZERO inheriting ones. 53 of them did enter on a graph already carrying a seed — nested boundary frames — but every one was handed the identicalstart_nodesparameter, so the frame still owns the value it sets. The cross-segmentWITHseed travels as the explicitstart_nodesparameter (chain_impl(..., start_nodes=),_compiled_query_reentry_state→start_nodes=row_start), never through the graph field, so the field's lifetime is exactly ONE boundary-call run.The test that decided it
graphistry/tests/compute/gfql/test_exec_context_scoping.py. The deciding case istest_an_outer_context_is_DROPPED_on_the_way_out: the input graph carries an outer seed, and the result must not. Restore returns the seed there; NULL returnsNone— exactly one implementation passes.test_no_execution_frame_inherits_a_context_it_did_not_setre-runs reason 3's measurement as an assertion over a corpus of the shapes that reach every attach site (generic chain boundary, native polars twin, all-callslet()body, CypherWITHre-entry, nestedWITH). If a future path starts relying on inheritance, this fails and the design question is reopened deliberately instead of an outer value being silently lost.Mutation-checked in the deciding direction
Implementing save/restore at all three attach sites (
compute/chain.py,gfql/lazy/engine/polars/chain.py,gfql_unified.py) fails 4 of the new tests on every runnable engine (pandas, polars, cuDF) — while the existing #1793 suitetest_reentry_caller_graph_immutability.pypasses unchanged. That is the point of the file: the existing tests cannot distinguish the two designs, and this one can.Engine coverage
Fixed engine list (
pandas,cudf,polars,polars-gpu) plus a runtime probe that RUNS a boundary-call query — deliberately notavailable_nonpandas_engines(), which silently shrinks (a missing engine vanishes from the report instead of showing as SKIPPED). Local receipts: 19 passed / 6 skipped with pandas + cuDF 25.10 + polars 1.42 (polars-gpu skipped,cudf_polarsabsent); GPU receipts in a comment below.Also added to
bin/test-polars.sh: the file has no module-levelimportorskip, so nothing else would have flagged that its polars params run in no lane (the #1795 class this campaign is auditing).Runtime delta: zero
No production line changed. The only non-test edits are the
clear_row_exec_contextdocstring and the CHANGELOG entry, so per CB5 no pyg-bench lane run is required — and that claim is checkable from the diff, not merely asserted.🤖 Generated with Claude Code
https://claude.ai/code/session_015YsqAZQLbqjSDrYSFz2GoB