docs+refactor(ogc): address code-review findings 1/3/5/6 on parallel_chunks

From the xhigh workflow review of the parallel_chunks branch:

- [5] Extract the duplicated positive-integer validation (numbers.Integral +
  bool-exclusion + < 1) into utils._require_positive_int; call it from both
  max_rows (engine) and parallel_chunks (chunking) so the two count knobs
  can't drift. Drops the now-unused `import numbers` from both. ChunkPlan's
  own `< 1` guard keeps its domain-specific "1 disables fan-out" message.

- [1,3] Document the consequences of fanning out a fitting request in the
  parallel_chunks docstring (new Notes section): with max_rows the result is
  drawn from the union of the sub-requests (a different, still-valid, sorted
  row set than the un-fanned call); a fanned-out call can fail partway and
  become resumable; cross-chunk dedup keys on id. These are the same caveats
  byte-forced chunking already carries — the block just extends their reach.

- [6] Collapse the triplicated cap-contract prose in planning.py: the
  ChunkPlan.max_chunks param docstring is the single authority; _refine now
  documents only its algorithm and cross-references the contract.

No behavior change. Messages preserve the substrings the tests match
("positive integer"); numpy-int caps still accepted, bool/float/str/< 1
still rejected.

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

@@ -73,7 +73,6 @@
 
 import asyncio
 import functools
-import numbers
 import os
 from collections.abc import Callable, Iterator
 from contextlib import contextmanager
@@ -84,7 +83,7 @@
 import pandas as pd
 from anyio.from_thread import start_blocking_portal
 
-from dataretrieval.utils import HTTPX_DEFAULTS, Ambient
+from dataretrieval.utils import HTTPX_DEFAULTS, Ambient, _require_positive_int
 
 from . import progress as _progress
 from .interruptions import (
@@ -250,6 +249,29 @@ def parallel_chunks(n: int) -> Iterator[None]:
         any request is issued, so a bad value fails loudly rather than silently
         doing nothing.
 
+    Notes
+    -----
+    Fanning out carries the same consequences as the byte-limit chunking the
+    getters already do for oversized requests; opting in just brings them to a
+    request that would otherwise be a single call:
+
+    - ``max_rows``: each sub-request paginates up to ``max_rows`` rows
+      independently, then the combined result is sorted and truncated to
+      ``max_rows``. So a call with ``max_rows`` set returns a *different*
+      (though still valid and deterministically sorted) row set inside a
+      ``parallel_chunks`` block than without one — the cap is drawn from the
+      union of the sub-requests, not a single stream. Don't pair a tight
+      ``max_rows`` preview with ``parallel_chunks`` if you need exactly the
+      rows the un-fanned call would return.
+    - Resumability: a single request either fully succeeds or fully fails,
+      but a fanned-out call can fail partway (e.g. a mid-call rate-limit) and
+      raise a resumable :class:`~dataretrieval.exceptions.ChunkInterrupted`
+      (or ``QuotaExhausted``) carrying the completed sub-requests, which you
+      finish with ``exc.call.resume()``.
+    - Cross-sub-request de-duplication keys on the feature ``id``; features
+      with no ``id`` can't be deduped, so overlapping filter clauses split
+      across chunks may yield duplicate rows.
+
     Examples
     --------
     >>> from dataretrieval import waterdata
@@ -262,13 +284,9 @@ def parallel_chunks(n: int) -> Iterator[None]:
     --------
     ChunkPlan._refine : the planning-side effect of ``n``.
     """
-    # Accept any integer type (incl. numpy ints, mirroring ``max_rows``); ``bool``
-    # is an ``Integral`` subclass but nonsensical here, so reject it explicitly.
-    if not isinstance(n, numbers.Integral) or isinstance(n, bool) or n < 1:
-        raise ValueError(
-            f"parallel_chunks(n): n must be a positive integer (e.g. 2, 8, 32); "
-            f"got {n!r}."
-        )
+    # Fail loudly on a bad ``n`` at ``with`` entry, before any request. Shared
+    # rules with ``max_rows`` via the helper (accepts numpy ints, rejects bool).
+    _require_positive_int(n, "parallel_chunks(n)", examples="2, 8, 32")
     with _parallel_chunks(n):
         yield
 

dataretrieval/ogc/engine.py

@@ -27,7 +27,6 @@
 import functools
 import json
 import logging
-import numbers
 import re
 from collections.abc import (
     AsyncIterator,
@@ -59,6 +58,7 @@
     _default_headers,
     _get,
     _network_error,
+    _require_positive_int,
 )
 
 # Set up logger for this module
@@ -876,18 +876,12 @@ def get_ogc_data(
     - Handles optional arguments such as `convert_type`.
     - Applies column cleanup and reordering based on service and properties.
     """
-    # Enforce a genuine positive integer: a float (even ``10.0``) or ``bool``
-    # would pass a bare ``< 1`` check and then crash deep in
+    # Enforce a genuine positive integer up front: a float (even ``10.0``) or
+    # ``bool`` would pass a bare ``< 1`` check and then crash deep in
     # ``pd.DataFrame.head`` with an opaque ``TypeError`` after HTTP I/O has
-    # already fired. ``numbers.Integral`` (not ``int``) so numpy integers —
-    # e.g. ``max_rows`` derived from a numpy/pandas computation — are accepted;
-    # ``bool`` is an ``Integral`` subtype, so exclude it explicitly.
-    if max_rows is not None and (
-        not isinstance(max_rows, numbers.Integral)
-        or isinstance(max_rows, bool)
-        or max_rows < 1
-    ):
-        raise ValueError(f"max_rows must be a positive integer (got {max_rows!r}).")
+    # already fired. Shared with ``parallel_chunks(n)`` via the helper.
+    if max_rows is not None:
+        _require_positive_int(max_rows, "max_rows")
 
     if dialect is None:
         dialect = _DEFAULT_DIALECT

dataretrieval/ogc/planning.py

@@ -512,36 +512,30 @@ def _plan(
     def _refine(self, max_chunks: int) -> None:
         """
         Fan the plan out more finely than the byte budget alone requires —
-        the parallel_chunks dial (see
+        the ``parallel_chunks`` dial (see
         :func:`~dataretrieval.ogc.chunking.parallel_chunks` for why a caller
-        would want this).
-
-        Caps the plan's *total* sub-request count (:attr:`total`, the
-        cartesian product across all axes) at ``max_chunks``, not
-        each axis independently — with several multi-value axes, a cap of 32
-        still means at most 32 sub-requests overall, not ``32 ** n_axes``.
-        The cap is a hard ceiling: every split multiplies the plan by
-        ``(k+1)/k`` for the chosen axis (adding ``total // k`` sub-requests,
-        not one), so a split is taken only when it keeps ``total`` within the
-        cap. When no in-budget split remains the plan lands *below* the cap
-        rather than overshooting it — a multi-axis plan may not divide evenly
-        onto ``max_chunks`` (e.g. two even axes can reach 4 but not 5, so a
-        cap of 5 yields 4). Each split picks the single largest splittable
-        chunk among the in-budget axes (ties broken by axis-extraction order,
-        then lowest index), so growth is distributed round-robin rather than
-        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 == 1``.
+        would want this, and :class:`ChunkPlan`'s ``max_chunks`` parameter for
+        the cap's contract: total-not-per-axis, a hard ceiling that may land
+        below the cap).
+
+        Implementation. Each split multiplies the plan by ``(k+1)/k`` for the
+        chosen axis (adding ``total // k`` sub-requests, not one), so a split
+        is taken only when it keeps :attr:`total` within the cap; when no
+        in-budget split remains the plan stops *below* the cap rather than
+        overshooting (two even axes can reach 4 but not 5, so a cap of 5 yields
+        4). Each split picks the single largest splittable chunk among the
+        in-budget axes (ties broken by axis-extraction order, then lowest
+        index), so growth is distributed round-robin rather than 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 == 1``.
 
         Parameters
         ----------
         max_chunks : int
-            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.
+            The ``parallel_chunks(n)`` value; see :class:`ChunkPlan`'s
+            ``max_chunks`` parameter for the full contract.
         """
         if max_chunks <= 1:
             return

dataretrieval/utils.py

@@ -4,6 +4,7 @@
 
 from __future__ import annotations
 
+import numbers
 import os
 import warnings
 from collections.abc import Callable, Iterable, Iterator
@@ -66,6 +67,33 @@ def __call__(self, value: _T) -> Iterator[None]:
             self._var.reset(token)
 
 
+def _require_positive_int(
+    value: int, name: str, *, examples: str | None = None
+) -> None:
+    """Validate that ``value`` is a positive integer, else raise ``ValueError``.
+
+    Accepts any :class:`numbers.Integral` (so a numpy/pandas integer passes,
+    not only ``int``) but rejects ``bool`` — an ``Integral`` subtype that is
+    nonsensical as a count. A non-integer (float, str, ``None``) or a value
+    ``< 1`` raises before any I/O, rather than crashing later (e.g. deep in
+    ``pd.DataFrame.head``). Shared by the count-like knobs (``max_rows`` and
+    ``parallel_chunks(n)``) so they validate by identical rules and can't drift.
+
+    Parameters
+    ----------
+    value : int
+        The value to check. Typed ``int`` for callers, but validated at
+        runtime because the real value may be anything the user passed.
+    name : str
+        Parameter name, used as the subject of the error message.
+    examples : str, optional
+        Illustrative values appended to the message (e.g. ``"2, 8, 32"``).
+    """
+    if not isinstance(value, numbers.Integral) or isinstance(value, bool) or value < 1:
+        eg = f", e.g. {examples}" if examples else ""
+        raise ValueError(f"{name} must be a positive integer{eg} (got {value!r}).")
+
+
 def _default_headers() -> dict[str, str]:
     """Build the default HTTP headers for a USGS web-API request.