refactor(waterdata): make max_chunks a positive-integer count (1 = off, reject 0)

Follow-up to the parallel_chunks contract cleanup. max_chunks is a
sub-request *count*, so its valid domain is the positive integers. Move the
"off" sentinel from 0 to 1 (which already behaves identically — no extra
fan-out, per the earlier n=1-passthrough change) and reject anything below 1:

- ChunkPlan.max_chunks default 0 -> 1; guard `< 0` -> `< 1`, so 0 and
  negatives now raise ValueError instead of silently no-oping.
- _parallel_chunks ambient default 0 -> 1; read-site + comments updated.
- _refine off-guard `<= 0` -> `<= 1` (clearer intent; the loop already
  no-oped at 1).
- Docstrings (class param, Raises, _refine, ambient) updated.

Tests: zero-cap passthrough -> default/unit passthrough; negative-raises ->
invalid-raises parametrized over {0, -1}; ramp param (0,1) -> (1,1) with the
coverage guard keyed on expected_pieces; the two byte-only baselines and the
ambient-default assertions move 0 -> 1. The public parallel_chunks(n) already
rejected n < 1, so no user-facing behavior changes.

Signed-off-by: thodson-usgs <thodson@usgs.gov>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: thodson-usgs

dataretrieval/ogc/chunking.py

@@ -184,9 +184,9 @@ def get_active_client() -> httpx.AsyncClient | None:
 # limit alone requires. Scoped to a ``with parallel_chunks(...):`` block (a
 # ContextVar), deliberately NOT an env var (see :func:`parallel_chunks` for
 # why). The ambient holds ``n`` — the requested cap on the plan's total
-# sub-request count; ``0`` (the default, outside any block) means "chunk only as
-# much as the byte limit needs".
-_parallel_chunks: Ambient[int] = Ambient("ogc_parallel_chunks", 0)
+# sub-request count; ``1`` (the default, outside any block) means "off — chunk
+# only as much as the byte limit needs, no extra fan-out".
+_parallel_chunks: Ambient[int] = Ambient("ogc_parallel_chunks", 1)
 
 
 @contextmanager
@@ -739,7 +739,7 @@ def wrapper(
         ) -> tuple[pd.DataFrame, Any]:
             limit = _OGC_URL_BYTE_LIMIT if url_limit is None else url_limit
             # Read the parallel_chunks dial ``n`` from the ambient set by
-            # ``parallel_chunks`` (0 = off outside any such block; otherwise the
+            # ``parallel_chunks`` (1 = off outside any such block; otherwise the
             # requested total sub-request cap). It only affects *planning*, done
             # here up front, so a later resume — which re-issues the
             # already-planned sub-requests — needs no snapshot.

dataretrieval/ogc/planning.py

@@ -330,18 +330,19 @@ class ChunkPlan:
         Byte budget for the request (URL + body) — a hard ceiling every
         sub-request must fit.
     max_chunks : int, optional
-        Hard cap on the plan's total sub-request count (default ``0`` = off).
-        ``0`` (off) and ``1`` (no extra fan-out) both chunk only as much as
-        ``url_limit`` requires — the most conservative plan, fewest
-        sub-requests — so a fitting request is a passthrough under either. A
-        cap of ``2`` or more fans the plan out to up to ``max_chunks``
-        sub-requests overall (the cartesian product across axes, never fewer
-        than the byte budget already forces) — capped as a whole, not per axis,
-        so several multi-value axes can't multiply past the cap. The plan never
-        exceeds the cap and may land below it when no whole split lands on it
-        exactly. A negative cap is a caller error and raises ``ValueError``.
-        Set from the :func:`~dataretrieval.ogc.chunking.parallel_chunks` ``n``;
-        see :meth:`_refine`.
+        Hard cap on the plan's total sub-request count (default ``1`` = off).
+        ``1`` chunks only as much as ``url_limit`` requires — the most
+        conservative plan, fewest sub-requests — so a fitting request is a
+        passthrough. A cap of ``2`` or more fans the plan out to up to
+        ``max_chunks`` sub-requests overall (the cartesian product across axes,
+        never fewer than the byte budget already forces) — capped as a whole,
+        not per axis, so several multi-value axes can't multiply past the cap.
+        The plan never exceeds the cap and may land below it when no whole
+        split lands on it exactly. ``max_chunks`` is a sub-request count, so a
+        value below ``1`` (``0`` or negative) is a caller error and raises
+        ``ValueError``. Set from the
+        :func:`~dataretrieval.ogc.chunking.parallel_chunks` ``n``; see
+        :meth:`_refine`.
 
     Attributes
     ----------
@@ -367,24 +368,25 @@ class ChunkPlan:
         If the request needs chunking but even the singleton plan
         doesn't fit ``url_limit``.
     ValueError
-        If ``max_chunks`` is negative.
+        If ``max_chunks`` is less than 1 (0 or negative).
     """
 
     def __init__(
         self,
         args: dict[str, Any],
         build_request: Callable[..., httpx.Request],
         url_limit: int,
-        max_chunks: int = 0,
+        max_chunks: int = 1,
     ) -> None:
-        if max_chunks < 0:
-            # ``0`` disables fan-out (the ambient default outside any
-            # ``parallel_chunks`` block); a negative cap is meaningless and can
-            # only be a caller bug, so fail loudly rather than silently no-op.
-            # The public ``parallel_chunks(n)`` already rejects ``n < 1``; this
-            # guards direct construction.
+        if max_chunks < 1:
+            # ``max_chunks`` is a sub-request *count*: the minimum is ``1``
+            # (the ambient default outside any ``parallel_chunks`` block),
+            # which means "off — no extra fan-out". ``0`` or negative is a
+            # meaningless count and can only be a caller bug, so fail loudly
+            # rather than silently no-op. The public ``parallel_chunks(n)``
+            # already rejects ``n < 1``; this guards direct construction.
             raise ValueError(
-                f"max_chunks must be >= 0 (0 disables fan-out); got {max_chunks!r}."
+                f"max_chunks must be >= 1 (1 disables fan-out); got {max_chunks!r}."
             )
 
         self.args = args
@@ -438,8 +440,8 @@ def __init__(
         # A request that already fits and hasn't opted into finer chunking is
         # the common passthrough: leave ``axes``/``chunks`` empty so
         # ``total == 1`` and ``iter_sub_args`` yields the original args
-        # verbatim. ``max_chunks`` of 0 (off) or 1 (no extra fan-out) both mean
-        # "don't split", so both take this path; only ``max_chunks >= 2`` asks
+        # verbatim. ``max_chunks == 1`` (off / no extra fan-out) means
+        # "don't split", so it takes this path; only ``max_chunks >= 2`` asks
         # for extra fan-out and sets the axes up to be refined below.
         if fits and max_chunks <= 1:
             return
@@ -452,7 +454,7 @@ def __init__(
             self._plan(build_request, url_limit)
         # Soft pass: optionally split further than the byte budget requires.
         # Purely additive — never re-raises, and the byte budget stays
-        # satisfied; a no-op at ``max_chunks <= 0``.
+        # satisfied; a no-op at ``max_chunks == 1``.
         self._refine(max_chunks)
 
         if self.canonical_url is None:
@@ -530,18 +532,18 @@ def _refine(self, max_chunks: int) -> None:
         one axis saturating before another is touched. Purely additive — only
         ever *splits* existing chunks, so the byte pass's work and the
         ``url_limit`` invariant are both preserved, and it never raises. A
-        no-op at ``max_chunks <= 0``.
+        no-op at ``max_chunks == 1``.
 
         Parameters
         ----------
         max_chunks : int
-            Hard cap on the plan's total sub-request count (``0`` = off) — the
+            Hard cap on the plan's total sub-request count (``1`` = off) — the
             ``parallel_chunks(n)`` value. The cap is on the whole plan (the
             cartesian product across axes), not per axis, so several multi-value
             axes can't multiply past it; the plan never exceeds it and may fall
             short when no whole split lands on it exactly.
         """
-        if max_chunks <= 0:
+        if max_chunks <= 1:
             return
         while True:
             total = self.total

tests/waterdata_chunking_test.py

@@ -2122,13 +2122,13 @@ async def fetch(args):
 # ---------------------------------------------------------------------------
 
 
-def test_zero_cap_preserves_passthrough():
-    """``max_chunks=0`` (the default) must not perturb the existing
+def test_default_preserves_passthrough():
+    """The default ``max_chunks`` (1 = off) must not perturb the existing
     plan: a multi-value request that fits the byte limit is still the trivial
     passthrough (no axes, ``total == 1``), byte-for-byte the pre-feature
     behavior."""
     args = {"monitoring_location_id": ["A", "B", "C", "D"]}
-    plan = ChunkPlan(args, _fake_build, url_limit=8000, max_chunks=0)
+    plan = ChunkPlan(args, _fake_build, url_limit=8000)  # default max_chunks=1
     assert plan.axes == []
     assert plan.total == 1
     assert list(plan.iter_sub_args()) == [args]
@@ -2137,34 +2137,36 @@ def test_zero_cap_preserves_passthrough():
 def test_unit_cap_preserves_passthrough():
     """``max_chunks=1`` means "no extra fan-out", so a fitting multi-value
     request stays the trivial passthrough (no axes, ``total == 1``,
-    ``iter_sub_args`` yields the original args verbatim) — identical to the off
-    (0) case, not a materialized one-chunk-per-axis plan."""
+    ``iter_sub_args`` yields the original args verbatim) — identical to the
+    default (off), not a materialized one-chunk-per-axis plan."""
     args = {"monitoring_location_id": ["A", "B", "C", "D"]}
     plan = ChunkPlan(args, _fake_build, url_limit=8000, max_chunks=1)
     assert plan.axes == []
     assert plan.total == 1
     assert list(plan.iter_sub_args()) == [args]
 
 
-def test_negative_cap_raises():
-    """A negative ``max_chunks`` is a caller bug, not a silent no-op: it raises
-    ``ValueError`` at construction. (The public ``parallel_chunks(n)`` already
-    rejects ``n < 1``; this pins the same guard on direct construction.)"""
+@pytest.mark.parametrize("bad", [0, -1])
+def test_invalid_cap_raises(bad):
+    """``max_chunks`` is a sub-request count, so a value below 1 (``0`` or
+    negative) is a caller bug, not a silent no-op: it raises ``ValueError`` at
+    construction. (The public ``parallel_chunks(n)`` already rejects ``n < 1``;
+    this pins the same guard on direct construction.)"""
     args = {"monitoring_location_id": ["A", "B", "C", "D"]}
-    with pytest.raises(ValueError, match="max_chunks must be >= 0"):
-        ChunkPlan(args, _fake_build, url_limit=8000, max_chunks=-1)
+    with pytest.raises(ValueError, match="max_chunks must be >= 1"):
+        ChunkPlan(args, _fake_build, url_limit=8000, max_chunks=bad)
 
 
 @pytest.mark.parametrize(
     ("max_chunks", "expected_pieces"),
-    [(0, 1), (2, 2), (8, 8), (16, 10), (32, 10)],
+    [(1, 1), (2, 2), (8, 8), (16, 10), (32, 10)],
 )
 def test_cap_ramps_then_saturates(max_chunks, expected_pieces):
     """A single 10-atom axis that fits the byte limit splits into
     ``min(10, cap)`` pieces: 1 (off), 2, 8, then saturating at 10 (one atom per
     chunk) once the cap overshoots the atom count. Monotonic and bounded, and
     whenever it splits the partition is a cover — every atom exactly once. (The
-    cap-0 passthrough has no axis to cover; see the passthrough test.)"""
+    cap-1 passthrough has no axis to cover; see the passthrough test.)"""
     atoms = [f"S{i:02d}" for i in range(10)]
     plan = ChunkPlan(
         {"monitoring_location_id": atoms},
@@ -2173,7 +2175,7 @@ def test_cap_ramps_then_saturates(max_chunks, expected_pieces):
         max_chunks=max_chunks,
     )
     assert plan.total == expected_pieces
-    if max_chunks:
+    if expected_pieces > 1:
         flattened = [
             a for chunk in plan.chunks["monitoring_location_id"] for a in chunk
         ]
@@ -2205,7 +2207,7 @@ def test_cap_below_byte_split_does_not_reduce_fan_out():
     # Heavy axis of four 30-char atoms; a limit tight enough that the byte pass
     # must drive every atom into its own sub-request (4 pieces > the cap of 2).
     args = {"monitoring_location_id": ["X" * 30, "Y" * 30, "Z" * 30, "W" * 30]}
-    baseline = ChunkPlan(args, _fake_build, url_limit=250, max_chunks=0)
+    baseline = ChunkPlan(args, _fake_build, url_limit=250, max_chunks=1)
     assert baseline.total > 2  # byte pass alone already fanned out past 2
     refined = ChunkPlan(args, _fake_build, url_limit=250, max_chunks=2)
     # cap 2 < baseline pieces → refine is a no-op here.
@@ -2218,7 +2220,7 @@ def test_cap_never_exceeds_the_byte_budget():
     a chunk), and the fan-out is at least what the byte pass required."""
     args = {"monitoring_location_id": ["X" * 30, "Y" * 30, "Z" * 30, "W" * 30]}
     limit = 310
-    byte_only = ChunkPlan(args, _fake_build, url_limit=limit, max_chunks=0)
+    byte_only = ChunkPlan(args, _fake_build, url_limit=limit, max_chunks=1)
     plan = ChunkPlan(args, _fake_build, url_limit=limit, max_chunks=32)
     assert plan.total >= byte_only.total
     for sub in plan.iter_sub_args():
@@ -2318,13 +2320,13 @@ def test_cap_does_not_mask_unchunkable():
 def test_parallel_chunks_publishes_n_on_the_ambient():
     """The context manager publishes ``n`` on the ambient for the block and
     restores the previous value on exit — including proper nesting."""
-    assert _parallel_chunks.get() == 0
+    assert _parallel_chunks.get() == 1  # default (off, = no extra fan-out)
     with parallel_chunks(32):
         assert _parallel_chunks.get() == 32
         with parallel_chunks(2):
             assert _parallel_chunks.get() == 2
         assert _parallel_chunks.get() == 32  # outer restored
-    assert _parallel_chunks.get() == 0  # default (off) outside any block
+    assert _parallel_chunks.get() == 1  # default (off) outside any block
 
 
 @pytest.mark.parametrize(
@@ -2348,7 +2350,7 @@ def test_parallel_chunks_rejects_non_positive_int(bad):
     with pytest.raises(ValueError, match="must be a positive integer"):
         with parallel_chunks(bad):
             pass
-    assert _parallel_chunks.get() == 0
+    assert _parallel_chunks.get() == 1  # default (off) — unchanged by a rejected call
 
 
 def test_parallel_chunks_drives_end_to_end_fan_out():