Skip to content

Commit 8cfac54

Browse files
thodson-usgsclaude
andcommitted
refactor(ogc): keep snapshot semantics for ChunkInterrupted partial state
Revert the live-view change (item 3 of #346): restore exc.partial_frame / exc.partial_response as raise-time snapshots rather than properties that delegate to exc.call. Snapshot is the better contract for an exception — a record of the failure moment, not a live handle: - In a resume loop each interruption stays a faithful record of what it saw. Live-view aliased every exception to the one shared call, so after a second failure exc1.partial_frame and exc2.partial_frame returned the same (later) state. - After a successful resume, live-view's .partial_frame returned the COMPLETE result — a field named "partial" that no longer was. - Exception payloads shouldn't mutate after you catch them (cf. CalledProcessError.output). Removing the delegation also drops the properties and the _pickled_* / __getstate__ snapshot machinery it required: partial_frame/partial_response are plain instance attributes again, pickled by the base __getstate__ (net -21 lines in interruptions.py). No behavior change versus released main — this PR is unmerged, so live-view never shipped. #346 is now a true no-behavioral-change refactor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent fda9170 commit 8cfac54

2 files changed

Lines changed: 54 additions & 69 deletions

File tree

dataretrieval/ogc/interruptions.py

Lines changed: 26 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,14 @@ class ChunkInterrupted(DataRetrievalError):
5555
total_chunks : int
5656
Total sub-requests in the plan.
5757
partial_frame : pandas.DataFrame
58-
Live view of work completed so far — delegates to
59-
``call.partial_frame``. Advances as ``call.resume()`` makes
60-
further progress. Returns an empty ``DataFrame`` when
61-
``call`` is ``None`` (degraded/unpickled state).
58+
Combined frame of work completed by the moment this exception
59+
was raised. Snapshot at raise time — does NOT advance on a
60+
later ``call.resume()`` (use ``exc.call.partial_frame`` for
61+
the live view).
6262
partial_response : httpx.Response or None
63-
Live view of the raw aggregate response — delegates to
64-
``call.partial_response``. ``None`` when ``call`` is ``None``
65-
or nothing has completed yet. (Raw, not finalized — use
63+
Raw aggregate response covering the completed sub-requests at
64+
raise time; ``None`` if nothing had completed yet. Same snapshot
65+
semantics as ``partial_frame``. (Raw, not finalized — use
6666
``exc.call.resume()`` for the finalized ``(df, metadata)`` result.)
6767
6868
Examples
@@ -118,51 +118,30 @@ def __init__(
118118
self.total_chunks = total_chunks
119119
self.call = call
120120
self.retry_after = retry_after
121-
122-
@property
123-
def partial_frame(self) -> pd.DataFrame:
124-
"""Live view of work completed so far — delegates to ``call.partial_frame``.
125-
126-
Returns an empty ``DataFrame`` when ``call`` is ``None`` (degraded /
127-
unpickled state), unless a pickled snapshot was restored during
128-
unpickling.
129-
"""
130-
if self.call is not None:
131-
return self.call.partial_frame
132-
# Degraded / unpickled: return the snapshot captured at pickle time,
133-
# or an empty frame if no snapshot exists (hand-constructed exc).
134-
return getattr(self, "_pickled_partial_frame", pd.DataFrame())
135-
136-
@property
137-
def partial_response(self) -> httpx.Response | None:
138-
"""Live view of the raw aggregate response.
139-
140-
Delegates to ``call.partial_response``.
141-
Returns ``None`` when ``call`` is ``None`` (degraded / unpickled state)
142-
and no pickled snapshot exists.
143-
"""
144-
if self.call is not None:
145-
return self.call.partial_response
146-
return getattr(self, "_pickled_partial_response", None)
121+
# Snapshot partial state at raise time so the exception stays a stable
122+
# record of the failure moment: ``exc.partial_frame`` /
123+
# ``.partial_response`` do NOT advance on a later ``call.resume()``
124+
# (that live view is on ``call.partial_frame`` / ``.partial_response``).
125+
# This keeps each interruption in a resume loop a faithful record of
126+
# what it saw, rather than every exception aliasing the shared call's
127+
# advancing state. ``.copy()`` guards the single-chunk fast path, where
128+
# the combined frame may be returned verbatim.
129+
if call is None:
130+
self.partial_frame: pd.DataFrame = pd.DataFrame()
131+
self.partial_response: httpx.Response | None = None
132+
else:
133+
self.partial_frame = call.partial_frame.copy()
134+
self.partial_response = call.partial_response
147135

148136
def __getstate__(self) -> dict[str, Any]:
149137
# Drop the live ChunkedCall before pickling: its ``.fetch`` is an
150138
# undecorated module function pickle can't reference by name, so the
151139
# interruption can't cross a process boundary with ``.call`` attached.
152-
# The degraded ``call=None`` form keeps the counts, retry hint, and
153-
# partial frame / response; only ``.resume()`` is lost (cross-process
154-
# resume was never possible anyway).
155-
#
156-
# Since ``partial_frame`` and ``partial_response`` are now live
157-
# properties (delegating to ``self.call``), snapshot them here into
158-
# private ``_pickled_*`` keys. The base ``__setstate__`` restores
159-
# those keys via ``self.__dict__.update``, and the properties fall
160-
# back to them (through ``getattr``) once ``call`` is ``None`` — so
161-
# no ``__setstate__`` override is needed.
162-
state = {**super().__getstate__(), "call": None}
163-
state["_pickled_partial_frame"] = self.partial_frame.copy()
164-
state["_pickled_partial_response"] = self.partial_response
165-
return state
140+
# The degraded ``call=None`` form keeps the counts, retry hint, and the
141+
# snapshotted partial frame / response — plain instance attributes the
142+
# base ``__getstate__`` already pickles; only ``.resume()`` is lost
143+
# (cross-process resume was never possible anyway).
144+
return {**super().__getstate__(), "call": None}
166145

167146

168147
class QuotaExhausted(ChunkInterrupted):

tests/waterdata_chunking_test.py

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -956,11 +956,13 @@ async def fetch(args):
956956
assert err.partial_response is not None
957957

958958

959-
def test_partial_frame_is_live_view_across_resume():
960-
"""``exc.partial_frame`` is now a live view delegating to
961-
``exc.call.partial_frame``. Calling ``exc.call.resume()`` advances the
962-
underlying ``ChunkedCall`` and ``exc.partial_frame`` reflects that — it
963-
is no longer a frozen snapshot."""
959+
def test_partial_frame_snapshot_stable_across_resume():
960+
"""``exc.partial_frame`` / ``exc.partial_response`` snapshot the
961+
state at raise time. Calling ``exc.call.resume()`` advances the
962+
underlying ``ChunkedCall`` but must NOT mutate the snapshot on
963+
the exception — otherwise a diagnostic that reads
964+
``exc.partial_frame`` after a resume sees post-resume state under
965+
a name that promises pre-resume state."""
964966
state = {"i": 0, "blow_up": True}
965967

966968
async def fetch(args):
@@ -977,24 +979,27 @@ async def fetch(args):
977979
with pytest.raises(QuotaExhausted) as excinfo:
978980
decorated({"sites": ["S1" * 10, "S2" * 10, "S3" * 10, "S4" * 10, "S5" * 10]})
979981
err = excinfo.value
980-
pre_resume_rows = len(err.partial_frame)
981-
assert pre_resume_rows > 0 # two chunks worth of data captured
982+
snapshot_rows = len(err.partial_frame)
983+
assert snapshot_rows > 0 # two chunks worth of data captured
982984

983-
# Resume; the live view on .call grows — and so does exc.partial_frame.
985+
# Resume; the live view on .call grows.
984986
state["blow_up"] = False
985987
err.call.resume()
986-
assert len(err.call.partial_frame) > pre_resume_rows
988+
assert len(err.call.partial_frame) > snapshot_rows
987989

988-
# The exception's partial_frame IS the live view — it advances too.
989-
assert len(err.partial_frame) == len(err.call.partial_frame)
990+
# The exception's snapshot must NOT advance.
991+
assert len(err.partial_frame) == snapshot_rows
990992

991993

992-
def test_partial_frame_is_live_view_when_single_chunk():
993-
"""``partial_frame`` is a live view delegating to ``call.partial_frame``.
994-
When a single chunk completes, ``partial_frame`` still reflects the
995-
current state of ``call.partial_frame`` (which recomputes on each
996-
access). This verifies the live-view semantics even on the single-
997-
chunk fast path."""
994+
def test_partial_frame_snapshot_is_a_copy_when_single_chunk():
995+
"""``_combine_chunk_frames`` returns ``non_empty[0]`` verbatim on
996+
its single-frame fast path. ``ChunkInterrupted.__init__`` must
997+
therefore defensively ``.copy()`` so an in-place mutation of the
998+
underlying chunk frame (e.g. user diagnostic code adding a
999+
column on the live view) doesn't leak through the snapshot.
1000+
Companion to ``test_partial_frame_snapshot_stable_across_resume``,
1001+
which uses ≥2 completed chunks and so goes through
1002+
``pd.concat`` (which already produces a fresh frame)."""
9981003
state = {"i": 0, "blow_up": True}
9991004

10001005
async def fetch(args):
@@ -1017,11 +1022,12 @@ async def fetch(args):
10171022
err = excinfo.value
10181023
assert err.completed_chunks == 1
10191024

1020-
# The live view delegates to call.partial_frame; verify it's
1021-
# accessible and reflects the call's current state.
1022-
assert not err.partial_frame.empty
1023-
assert list(err.partial_frame.columns) == ["sites"]
1024-
assert err.partial_frame.equals(err.call.partial_frame)
1025+
snapshot_cols = list(err.partial_frame.columns)
1026+
# Mutate the underlying chunk in place — the snapshot must NOT
1027+
# reflect the mutation.
1028+
err.call._chunks[0][0]["extra"] = 0
1029+
assert list(err.partial_frame.columns) == snapshot_cols
1030+
assert "extra" not in err.partial_frame.columns
10251031

10261032

10271033
def test_combine_chunk_responses_returns_independent_headers():

0 commit comments

Comments
 (0)