Skip to content

Commit 8d4c4a1

Browse files
thodson-usgsclaude
andcommitted
fix(waterdata): tighten ChunkPlan.max_chunks contract (n=1 passthrough, negative raises)
Two edges from the SOLID review of the parallel_chunks dial: - max_chunks=1 ("no extra fan-out") took the fan-out path: on a fitting request it materialized axes and re-rendered them through iter_sub_args before _refine(1) no-oped, instead of the verbatim passthrough. Widen the passthrough guard to `max_chunks <= 1` so 0 (off) and 1 (no fan-out) both short-circuit to the trivial plan. - A negative max_chunks silently no-oped. It can only be a caller bug (the public parallel_chunks(n) already rejects n < 1), so raise ValueError at construction rather than mask it. Docstring + Raises updated; adds a unit passthrough test for max_chunks=1 and a negative-raises test. Signed-off-by: thodson-usgs <thodson@usgs.gov> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 2d6c4b9 commit 8d4c4a1

2 files changed

Lines changed: 46 additions & 10 deletions

File tree

dataretrieval/ogc/planning.py

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -331,13 +331,15 @@ class ChunkPlan:
331331
sub-request must fit.
332332
max_chunks : int, optional
333333
Hard cap on the plan's total sub-request count (default ``0`` = off).
334-
``0`` chunks only as much as ``url_limit`` requires — the most
335-
conservative plan, fewest sub-requests. A positive cap fans the plan
336-
out to up to ``max_chunks`` sub-requests overall (the
337-
cartesian product across axes, never fewer than the byte budget
338-
already forces) — capped as a whole, not per axis, so several
339-
multi-value axes can't multiply past the cap. The plan never exceeds
340-
the cap and may land below it when no whole split lands on it exactly.
334+
``0`` (off) and ``1`` (no extra fan-out) both chunk only as much as
335+
``url_limit`` requires — the most conservative plan, fewest
336+
sub-requests — so a fitting request is a passthrough under either. A
337+
cap of ``2`` or more fans the plan out to up to ``max_chunks``
338+
sub-requests overall (the cartesian product across axes, never fewer
339+
than the byte budget already forces) — capped as a whole, not per axis,
340+
so several multi-value axes can't multiply past the cap. The plan never
341+
exceeds the cap and may land below it when no whole split lands on it
342+
exactly. A negative cap is a caller error and raises ``ValueError``.
341343
Set from the :func:`~dataretrieval.ogc.chunking.parallel_chunks` ``n``;
342344
see :meth:`_refine`.
343345
@@ -364,6 +366,8 @@ class ChunkPlan:
364366
Unchunkable
365367
If the request needs chunking but even the singleton plan
366368
doesn't fit ``url_limit``.
369+
ValueError
370+
If ``max_chunks`` is negative.
367371
"""
368372

369373
def __init__(
@@ -373,6 +377,16 @@ def __init__(
373377
url_limit: int,
374378
max_chunks: int = 0,
375379
) -> None:
380+
if max_chunks < 0:
381+
# ``0`` disables fan-out (the ambient default outside any
382+
# ``parallel_chunks`` block); a negative cap is meaningless and can
383+
# only be a caller bug, so fail loudly rather than silently no-op.
384+
# The public ``parallel_chunks(n)`` already rejects ``n < 1``; this
385+
# guards direct construction.
386+
raise ValueError(
387+
f"max_chunks must be >= 0 (0 disables fan-out); got {max_chunks!r}."
388+
)
389+
376390
self.args = args
377391
self.axes: list[_Axis] = []
378392
self.chunks: dict[str, list[list[str]]] = {}
@@ -424,9 +438,10 @@ def __init__(
424438
# A request that already fits and hasn't opted into finer chunking is
425439
# the common passthrough: leave ``axes``/``chunks`` empty so
426440
# ``total == 1`` and ``iter_sub_args`` yields the original args
427-
# verbatim. Only when ``max_chunks`` asks for extra fan-out do
428-
# we set the axes up to be refined below.
429-
if fits and max_chunks <= 0:
441+
# verbatim. ``max_chunks`` of 0 (off) or 1 (no extra fan-out) both mean
442+
# "don't split", so both take this path; only ``max_chunks >= 2`` asks
443+
# for extra fan-out and sets the axes up to be refined below.
444+
if fits and max_chunks <= 1:
430445
return
431446

432447
self.axes = axes

tests/waterdata_chunking_test.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2134,6 +2134,27 @@ def test_zero_cap_preserves_passthrough():
21342134
assert list(plan.iter_sub_args()) == [args]
21352135

21362136

2137+
def test_unit_cap_preserves_passthrough():
2138+
"""``max_chunks=1`` means "no extra fan-out", so a fitting multi-value
2139+
request stays the trivial passthrough (no axes, ``total == 1``,
2140+
``iter_sub_args`` yields the original args verbatim) — identical to the off
2141+
(0) case, not a materialized one-chunk-per-axis plan."""
2142+
args = {"monitoring_location_id": ["A", "B", "C", "D"]}
2143+
plan = ChunkPlan(args, _fake_build, url_limit=8000, max_chunks=1)
2144+
assert plan.axes == []
2145+
assert plan.total == 1
2146+
assert list(plan.iter_sub_args()) == [args]
2147+
2148+
2149+
def test_negative_cap_raises():
2150+
"""A negative ``max_chunks`` is a caller bug, not a silent no-op: it raises
2151+
``ValueError`` at construction. (The public ``parallel_chunks(n)`` already
2152+
rejects ``n < 1``; this pins the same guard on direct construction.)"""
2153+
args = {"monitoring_location_id": ["A", "B", "C", "D"]}
2154+
with pytest.raises(ValueError, match="max_chunks must be >= 0"):
2155+
ChunkPlan(args, _fake_build, url_limit=8000, max_chunks=-1)
2156+
2157+
21372158
@pytest.mark.parametrize(
21382159
("max_chunks", "expected_pieces"),
21392160
[(0, 1), (2, 2), (8, 8), (16, 10), (32, 10)],

0 commit comments

Comments
 (0)