From 3839b8869b908208adf005339c1b11e82f0c32d4 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Sun, 6 Sep 2026 16:55:01 -0500 Subject: [PATCH 1/2] docs: copy edits across the ADRs, guides, and glossary Copy edits only: say each thing in plain terms throughout the architecture records, user guides, CONTEXT.md, AGENTS.md, CONTRIBUTING.md, README, and NEWS. No decision, glossary term, or code changes. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015Vb9Y5QQdoHTac4cgvTexc --- AGENTS.md | 63 +++++----- CONTEXT.md | 83 +++++++------- CONTRIBUTING.md | 44 +++---- NEWS.md | 28 ++--- README.md | 4 +- .../decisions/0000-documenting-decisions.rst | 50 ++++---- .../decisions/0001-modular-monolith.rst | 4 +- .../0002-sync-api-async-internals.rst | 4 +- .../decisions/0003-dependency-direction.rst | 2 +- .../decisions/0004-error-retry-resume.rst | 33 +++--- .../0006-service-neutral-transport.rst | 79 ++++++------- .../decisions/0007-adapter-facades.rst | 22 ++-- .../decisions/0008-fan-out-execution.rst | 51 +++++---- .../decisions/0009-layered-configuration.rst | 65 +++++------ .../0010-adapter-scoped-settings.rst | 78 +++++++------ .../decisions/0011-configuration-profiles.rst | 56 ++++----- .../decisions/0012-deprecation-horizons.rst | 30 ++--- .../decisions/0013-core-and-domain-terms.rst | 26 ++--- docs/source/architecture/index.rst | 43 +++---- docs/source/meta/contributing.rst | 2 +- docs/source/reference/exceptions.rst | 4 +- docs/source/userguide/configuration.rst | 108 +++++++++--------- docs/source/userguide/errors.rst | 14 +-- docs/source/userguide/timeconventions.rst | 2 +- 24 files changed, 453 insertions(+), 442 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b0c9006f3..9b7d42b76 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,43 +4,44 @@ - **`CONTEXT.md` is the shared vocabulary** — getter, query, chunk, plan, fan-out, page, adapter, facade, leaf, transport, collection, profile, effective configuration, and the legacy names that are deliberately not renamed. Read it - before writing code, docstrings, or commit messages. Two kinds of term live - there and they bind differently (ADR 0013). A **core** term is ours and takes - one spelling everywhere *including identifiers*, so a name in the code that - conflicts with it is a defect. A **domain** term -- *monitoring location*, - *collection* -- is fixed for prose only: each adapter keeps its own service's - spelling in its parameters, so `nwis.get_record(service="dv")` and WQP's - `Station` are that service's language rather than drift. Each entry states - which kind it is. + before writing code, docstrings, or commit messages. Two kinds of term are + defined there and they bind differently (ADR 0013). A **core** term is ours + and takes one spelling everywhere *including identifiers*, so a name in the + code that conflicts with it is a defect. A **domain** term -- *monitoring + location*, *collection* -- is fixed for prose only: each adapter keeps its + own service's spelling in its parameters, so `nwis.get_record(service="dv")` + and WQP's `Station` are that service's vocabulary rather than drift. Each + entry states which kind it is. - Architectural decisions and their rationale: `docs/source/architecture/decisions/` (ADRs, referenced by number throughout the code and by `.importlinter`). - Contributor workflow, style, and the quality gates in detail: `CONTRIBUTING.md`. ## How the tree is organized Use `ls`/`grep` for the file list; what follows is the placement logic, so you -can predict where a thing lives. +can predict where a thing is defined. - `dataretrieval/` — the public surface is one *adapter* module per service, named for the service (`nldi`, `nwdc`, `ngwmn`, `streamstats`, `wqp`, and legacy `nwis`); each owns that service's URLs, parameters, and response quirks. `waterdata/` is the one adapter large enough to be a package, split by collection family; its `api.py` is a compatibility facade holding no logic. - Everything else in the package is shared machinery the adapters sit on top of + Everything else in the package is shared code the adapters depend on — configuration, credentials, progress, exceptions, code tables, response - formats. Shared machinery below the adapter layer must not know about any + formats. Shared code below the adapter layer must not reference any particular service. -- `dataretrieval/ogc/` — the OGC API protocol machinery (chunk planning, +- `dataretrieval/ogc/` — the OGC API protocol code (chunk planning, filters, request building, response shaping). Shared by the two OGC services only; `.importlinter` refuses any other importer. -- `dataretrieval/transport/` — service-neutral request machinery (HTTP, retry, +- `dataretrieval/transport/` — service-neutral request code (HTTP, retry, pagination, fan-out). It names no service and no protocol, and is not public API. - Leading-underscore top-level modules are private; the dependency-free *leaves* - sit at the floor of the stack so anything may use them without pulling in the + are at the bottom of the stack so anything may use them without pulling in the rest of the package. Check for an existing leaf before writing a small helper. -- **`.importlinter` is the map.** Its `layers` contract lists every top-level - module in dependency order and is `exhaustive = True`, so it is both the - authoritative statement of where a module sits and the thing that fails when a - new module has no home. Read it before adding a module or an import. +- **`.importlinter` records where every module belongs.** Its `layers` contract + lists every top-level module in dependency order and is `exhaustive = True`, + so it is both the authoritative statement of where a module belongs and the + thing that fails when a new module has not been placed. Read it before adding + a module or an import. - `tests/` — flat, one `*_test.py` per module or concern, organized into four dependency-oriented layers (public contract, adapter contract, component, cross-component) that `tests/contracts/README.md` defines and assigns files to. @@ -74,10 +75,10 @@ can predict where a thing lives. - Tests: `coverage run -m pytest tests/ && coverage report`, or focused like `pytest tests/waterdata_test.py::test_mock_get_samples`. `coverage report` is a merge gate: branch coverage with a `fail_under` ratchet in - `[tool.coverage.report]`. Chase the uncovered *branch*, not the number -- a - test written to colour a line green catches nothing and costs a maintenance - slot. If a path is genuinely unreachable, add it to `exclude_also` with a - reason, or leave the ratchet alone. + `[tool.coverage.report]`. Cover the uncovered *branch*, not the number -- a + test written only to mark a line as covered catches nothing and adds + maintenance. If a path is genuinely unreachable, add it to `exclude_also` + with a reason, or leave the ratchet alone. - Types: `mypy` (`strict = true` in `pyproject.toml`; CI runs it over the PR-merged-into-main, so bare `dict`/`list` annotations fail there even if they pass on your branch). @@ -88,7 +89,7 @@ can predict where a thing lives. ## Testing gotchas - The suite is offline by default: `addopts = "-m 'not live'"`. Tests marked - `@pytest.mark.live` hit real USGS services and run on a schedule + `@pytest.mark.live` call real USGS services and run on a schedule (`.github/workflows/live-api.yml`); run them locally with `pytest tests/ -m live`. - HTTP is mocked with `pytest-httpx`'s `httpx_mock` fixture plus fixtures under `tests/data/`; keep new API tests offline. @@ -101,7 +102,7 @@ can predict where a thing lives. ## Error messages Most callers here are programs — a script, a pipeline stage, an agent — so a message is the only channel through which a caller can correct itself. Every -raise states the problem and then the move that fixes it, in that order. +raise states the problem and then the action that fixes it, in that order. - Name the remedy, not just the fault. `"Service not recognized"` gives a caller nothing to try next; listing the services it does accept does. For a transport @@ -111,11 +112,11 @@ raise states the problem and then the move that fixes it, in that order. owns the wording for the shared shapes — bad value in a closed vocabulary (`require_one_of`), missing argument (`require_argument`), incomplete group (`require_together`), no filter at all (`require_any_of`), and conflicting - arguments (`require_exactly_one`, `reject_together`). Reach for one before + arguments (`require_exactly_one`, `reject_together`). Use one before hand-writing a message. A service-specific pointer is not a reason to - hand-write: every check takes a `remedy=` for the move it cannot derive. Every - check raises `ValueError` -- one class for a bad argument value, so a caller - catches by shape rather than by which module rejected it. + hand-write: every check takes a `remedy=` for the action it cannot derive. + Every check raises `ValueError` -- one class for a bad argument value, so a + caller catches by shape rather than by which module rejected it. - `require_argument` returns the narrowed value and `require_exactly_one` the winning `(name, value)` pair, so use their results rather than re-testing for `None` to satisfy mypy — a second, unreachable message beside the first is @@ -127,9 +128,9 @@ raise states the problem and then the move that fixes it, in that order. private local no getter accepts, `configure(Configuration(...))` was a silent no-op because `configure` is a context manager, `pip install dataretrieval[nldi]` globs in zsh, and a navigation missing its `data_source` - spelled `None` into the URL and returned an empty frame. Run the corrected + wrote `None` into the URL and returned an empty frame. Run the corrected call against the real service; wording review does not catch these. -- Shared checks take the caller's spelling. `_validate_data_source`, +- Shared checks take the caller's argument name. `_validate_data_source`, `_format_api_dates`, and `require_one_of` all accept a `name=` so the subject of the message is the argument that was actually passed. A helper that hard-codes one noun reports the wrong parameter the moment a second call site reuses it. @@ -151,7 +152,7 @@ raise states the problem and then the move that fixes it, in that order. - The `API_USGS_PAT` credential is owned by the `credentials` leaf and applied as the `X-Api-Key` header by `transport.http.default_headers()`, which sends it only to the host it belongs to. Never hard-code tokens in examples or tests. -- Water Data request builders translate Python kwargs to API spellings +- Water Data request builders translate Python kwargs to API names (`skip_geometry` -> `skipGeometry`, `filter_lang` -> `filter-lang`); tests assert exact URLs and query params. - Multi-value OGC params are comma-joined GETs, except `monitoring-locations` diff --git a/CONTEXT.md b/CONTEXT.md index 0ada45e0a..0eaa10901 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -2,26 +2,27 @@ The shared vocabulary for `dataretrieval`. This is a glossary, not a specification: it fixes what words mean so that code, docstrings, ADRs, and -conversation use them the same way. Architectural decisions live in +conversation use them the same way. Architectural decisions are recorded in `docs/source/architecture/decisions/`. -Two kinds of term live here, and they carry different obligations (ADR 0013). +Two kinds of term are defined here, and they impose different obligations +(ADR 0013). **Core terms** are ours. The package invented them and no service has a claim on them — *chunk*, *page*, *fan-out*, *source*, *dialect*, *leaf*. One spelling, everywhere it appears: prose, identifiers, tests. Where a core term conflicts -with a name in the code the term wins and the name is legacy, listed at the end. -A second spelling is a defect, not a variation. +with a name in the code the term is authoritative and the name is legacy, +listed at the end. A second spelling is a defect, not a variation. **Domain terms** belong to the services, which name the same thing differently and will not be reconciled. For these the glossary fixes one word for *prose*, -so that documents about the package read consistently. It does not fix the wire -or an adapter's public surface: each adapter keeps its own service's spelling in -its parameters. Such an entry names the per-service spellings itself; those are -not legacy names. +so that documents about the package read consistently. It does not fix the +names used in requests or an adapter's public surface: each adapter keeps its +own service's spelling in its parameters. Such an entry names the per-service +spellings itself; those are not legacy names. -A word carrying a package-wide meaning has an entry here, and one entry may -name another; what no entry may do is lean on a word this document leaves +A word with a package-wide meaning has an entry here, and one entry may +name another; what no entry may do is rely on a word this document leaves undefined. Naming a word only to say what an ADR calls it is a cross-reference, not a use. @@ -48,7 +49,7 @@ the reason is not part of the term. **Plan** — An enumeration of a query's chunks: how many there are, and what each one is. A plan says how a query divides; it does not execute. Computing a plan is protocol-specific — a byte budget, a per-location rule — while executing one -is not, which is why the two live apart. +is not, which is why the two are kept in separate modules. **Fan-out** — Executing a query's chunks concurrently. Chunking is how the work divides; fan-out is how it is distributed. The two are independent, and only @@ -60,12 +61,12 @@ A chunk of a large query commonly spans many pages. ## Failure and resumption -**Transient failure** — A failure a later attempt could survive: a rate limit, a -service error, a timeout. Distinguished from a **deterministic failure**, which -would fail identically every time — an unresolvable hostname, an unsupported -scheme, a malformed request. Only transient failures are retried, and only -transient failures produce a resumable interruption. Both answers follow from -one judgement about what a failure means, and must agree. +**Transient failure** — A failure that might not recur on a later attempt: a +rate limit, a service error, a timeout. Distinguished from a **deterministic +failure**, which would fail identically every time — an unresolvable hostname, +an unsupported scheme, a malformed request. Only transient failures are +retried, and only transient failures produce a resumable interruption. Both +answers follow from one judgement about what a failure means, and must agree. **Stall timeout** — How long a call may receive nothing at all before retrying stops, measured from when data last arrived rather than from the call's start. @@ -115,7 +116,7 @@ for compatibility and is not where new work goes. `monitoring-locations`, `time-series-metadata`. The unit a getter targets. A collection is not a service. Water Data is a service; `daily` is one of its -collections. The distinction matters because the OGC machinery is shared: the +collections. The distinction matters because the OGC code is shared: the same code path retrieves a Water Data collection and an NGWMN one, and only the service differs. @@ -126,7 +127,7 @@ and keep their spelling. Prose says *collection*, including prose about the adapters that spell it otherwise — with one exception: deprecated NWIS keeps `service` in its docstrings as well as its parameters. Describing a parameter in a term its own module never uses helps nobody, and a module being retired is -not where new vocabulary should land. +not where new vocabulary should be introduced. **Collection family** — A group of collections sharing a shape and therefore a getter signature. Their getters deliberately resemble one another; the @@ -173,13 +174,13 @@ vocabulary. A public keyword is not automatically a setting. `ssl_check` is a getter argument on four adapters and resolves through no chain at all; the settings are -the roster the configuration system knows. +the list the configuration system recognizes. **Scope** — How much of the package a setting's value applies to: the whole package, or one adapter. Orthogonal to source: the scope says who a value is for, the source says where it came from, and precedence orders sources first, scopes within them. ADR 0010's word for a scope level is *tier* — the top-level -tier that survives, the host or gateway tier it defers. +tier that remains, the host or gateway tier it defers. **Package-wide setting** — A setting that applies to every adapter: the retry count, the progress line, the stall timeout. Set once, honored everywhere. @@ -216,7 +217,7 @@ selected. What `show_configuration()` prints beside each value, and what a parser names when it rejects one. A source is the category; an origin label is the instance within it. -*Core terms.* The configuration chain is shared machinery, so one spelling binds +*Core terms.* The configuration chain is shared code, so one spelling binds its identifiers as well as its prose. The code uses both names: `_resolve` returns `(raw, label, source)`, and the parsers take the `label` as the subject of any error message they raise. @@ -229,17 +230,17 @@ one source can span several rungs — so prose that means a whole category says **Selection** — Naming which profile an adapter should use. Done in code; a profile is never selected by the environment or implied by the file, so the -set of profiles in a file is inert until something asks for one. +set of profiles in a file is inert until something selects one. **Built-in default** — The value a setting takes when no source supplies one. Package-wide. **Adapter default** — The value a *particular adapter* prefers when no source -supplies one, because that adapter warrants a different figure — NWDC asks for -4 concurrent requests where the OGC getters take 32. Supplied by the adapter in -code, not by the user. It replaces the built-in default for calls through that -adapter and nothing else. A value from any source outranks it: otherwise an -adapter could discard a value the caller set explicitly. +supplies one, because that adapter warrants a different figure — NWDC defaults +to 4 concurrent requests where the OGC getters default to 32. Supplied by the +adapter in code, not by the user. It replaces the built-in default for calls +through that adapter and nothing else. A value from any source outranks it: +otherwise an adapter could discard a value the caller set explicitly. Distinct from an **adapter-scoped setting**, which is the *user* naming a value for one adapter. Both narrow to a single adapter; only one of them is something @@ -252,13 +253,13 @@ value. Where the distinction matters — reporting what a call will actually use ## Boundaries **Adapter** — A module owning one service's conventions: its URLs, parameters, -error shapes, and response quirks. Adapters may use shared machinery; shared -machinery may not know about adapters. +error shapes, and response quirks. Adapters may use shared code; shared code +may not reference adapters. -**Dialect** — The per-API quirks the shared OGC machinery needs in order to +**Dialect** — The per-API quirks the shared OGC code needs in order to serve two services from one code path: which collections must be POSTed as CQL2, which render dates date-only, which columns to coerce and sort by. An -adapter supplies one and the machinery reads it, which is how protocol code +adapter supplies one and the shared code reads it, which is how protocol code stays free of service names. **Single-shot adapter** — An adapter whose query is always exactly one request: @@ -280,7 +281,7 @@ holding one general mechanism so that anything may use it without acquiring the rest of the package. Before writing a small helper, check whether a leaf already generalizes it. -**Transport** — The service-neutral machinery for issuing requests: timeouts, +**Transport** — The service-neutral code for issuing requests: timeouts, retry, pagination, fan-out, aggregation. It names no service and no protocol, and is not public API. @@ -288,25 +289,25 @@ and is not public API. Core-term spellings recorded so they are not mistaken for drift, and not re-litigated: frozen misnamings, permanent aliases, and names that agree with -this glossary by more than luck. A domain term at an adapter's surface is not a -legacy name and is not listed here; it belongs with that term's own entry -(ADR 0013). +this glossary by more than coincidence. A domain term at an adapter's surface +is not a legacy name and is not listed here; it belongs with that term's own +entry (ADR 0013). - `completed_chunks` / `total_chunks` on interruptions, and `set_chunks()` / `start_chunk()` on the progress reporter, count chunks as defined above and are consistent with this glossary. They predate it; the agreement is real rather than coincidental. - `ChunkInterrupted` is a permanent alias of `FanOutInterrupted` — the same - class object under the name it was first published as. Both spellings are + class object under the name it was first published as. Both names are correct; neither is scheduled for removal. - *No-progress budget* is ADR 0006's name for the **stall timeout**. The record keeps its wording; prose outside it says *stall timeout*, and the setting is `stall_timeout`. - `ChunkedCall` is a permanent alias of `FanOut`, published on the OGC - compatibility path. Like `ChunkInterrupted`, both spellings are correct. + compatibility path. Like `ChunkInterrupted`, both names are correct. - `utils.query` is one *request*, not a query as defined above. It is a frozen public path (`dataretrieval.utils.query`) and predates this glossary. -- `service` named a collection throughout the OGC machinery. Resolved: the +- `service` named a collection throughout the OGC code. Resolved: the OGC internals, the Water Data wrappers, and all eleven typed getters now say `collection`; `waterdata.get_cql` takes `collection`; and the type alias is `WATERDATA_COLLECTIONS`. `service=` on `get_cql` and the `WATERDATA_SERVICES` @@ -324,8 +325,8 @@ legacy name and is not listed here; it belongs with that term's own entry query rather than five sets of data, and the OGC definition of *collection* is scoped to "access mechanisms defined by OGC API standard(s)", which Samples does not implement. Kept as-is by decision: renaming a public - keyword costs a deprecation cycle, and no better-evidenced replacement is in - reach. + keyword costs a deprecation cycle, and no better-evidenced replacement is + available. - `waterdata.get_codes(code_service=)` is correct and stays. The Samples documentation calls it a "code service" in prose and serves it from `/codeservice/`, so this reproduces the service's own vocabulary, like diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7c2b7cbf6..e20922bba 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -106,7 +106,7 @@ Run them locally with: pytest tests/ -m live ``` -New tests should be offline. Reach for `live` only when the assertion is a claim +New tests should be offline. Use `live` only when the assertion is a claim about the upstream service rather than about this package. ### Coding Standards and Style @@ -120,9 +120,9 @@ it, and it is the only module that reads the environment for a setting), been re-implemented at least once by someone who did not know it was there, and the copies drift: the same question gets a different cycle guard, a different error message, a different edge case. None of the automated checks catch it, -because two eight-line helpers are below the clone detector's floor and neither -one couples nor complicates anything. A grep for the mechanism you are about to -write is the only thing that does. +because two eight-line helpers are below the clone detector's minimum size and +neither one couples nor complicates anything. A grep for the mechanism you are +about to write is the only thing that does. The continuous integration and pre-commit configurations enforce formatting, linting, and strict type checking. Run the relevant checks before opening a PR: @@ -141,17 +141,17 @@ lint-imports The last three come from `pip install -e '.[metrics]'`, and each has a pre-commit hook running the identical check, so a clean pre-commit run means CI agrees. -`coverage report` is a ratchet too. The threshold lives in -`[tool.coverage.report]` in `pyproject.toml` and sits at the measured value, so -it fails on regression rather than demanding new tests of a change that added -none. Raise it when coverage rises; lower it only deliberately, and say why in -the commit. +`coverage report` is a ratchet too. The threshold is in +`[tool.coverage.report]` in `pyproject.toml` and is set to the measured value, +so it fails on regression rather than demanding new tests of a change that +added none. Raise it when coverage rises; lower it only deliberately, and say +why in the commit. Coverage is measured with branches on, because most of what this package gets wrong is a branch rather than a line -- a dispatch arm routing to the wrong -getter, an error path that never fires, a fallback that quietly becomes the -norm. Chase the *uncovered branch*, not the percentage: a test written only to -turn a line green adds maintenance and catches nothing. If a path cannot be +getter, an error path that never executes, a fallback that quietly becomes the +norm. Cover the *uncovered branch*, not the percentage: a test written only to +mark a line as covered adds maintenance and catches nothing. If a path cannot be reached without contorting the code, exclude it in `[tool.coverage.report] exclude_also` with a reason, or leave the ratchet where it is. Either costs less than a test that adds maintenance and catches nothing. @@ -163,7 +163,7 @@ Windows run genuinely measures a smaller suite. For the same reason, the threshold assumes the whole suite: on Windows, or without the `nldi` extra installed, some tests skip and the local number comes in under the gate through no fault of your change. Run -`coverage report --fail-under=0` in that situation and let CI grade the +`coverage report --fail-under=0` in that situation and let CI evaluate the ratchet. `xenon` and `complexipy` are complexity ratchets: the thresholds are the @@ -182,8 +182,8 @@ quarantine, and collection-family independence. **That file is the only place dependency direction is enforced.** These rules were once asserted a second time in `tests/architecture_test.py` by hand-parsing -the AST; that duplication is gone, and re-adding it would mean one rule with two -homes that drift apart. What the tests still own is everything an import graph +the AST; that duplication is gone, and re-adding it would mean one rule in two +places that drift apart. What the tests still own is everything an import graph cannot see -- which *symbols* cross a seam, declared `__all__` surfaces, the AST shape of a facade, boundaries that must be asserted positively (`lint-imports` can forbid an edge, never require one), and package-wide cycle detection (see @@ -197,7 +197,7 @@ history: ```bash wily build dataretrieval --max-revisions 50 # index recent commits (slow, once) -wily report dataretrieval # how the package moved over time +wily report dataretrieval # how metrics changed over time wily diff dataretrieval --revision main # what your branch changed wily rank dataretrieval maintainability.mi # worst-maintained files today ``` @@ -205,7 +205,7 @@ wily rank dataretrieval maintainability.mi # worst-maintained files today `wily` is advisory and is never a merge gate -- rising complexity in a file that gained a complex feature is information, not a failure. -#### The periodic deep sweep +#### The periodic whole-package analysis Duplication, coupling, cohesion, dependency depth, and dead code are tracked by [`pyscn`](https://github.com/ludo-technologies/pyscn) on a weekly schedule @@ -215,7 +215,7 @@ measures move over months rather than commits. You do not need it to contribute. It answers "what should we clean up next?" -- including for an agent working on this repo, which gets a whole-package -structural picture from one command: +structural overview from one command: ```bash pip install -e '.[health]' # wheels: macOS ARM64, Linux x86-64, Windows x86-64 @@ -223,10 +223,10 @@ pip install -e '.[health]' # wheels: macOS ARM64, Linux x86-64, Windows x86-64 pyscn analyze dataretrieval # HTML report, or --json for the numbers ``` -Read its findings as leads, not verdicts. Its clone detector flags this +Read its findings as suggestions, not conclusions. Its clone detector flags this package's per-collection getters -- thin, heavily documented wrappers whose bodies are necessarily similar -- and collapsing them into one parameterized -function would trade the documented public surface for a metric. Its +function would sacrifice the documented public surface for a metric. Its dependency-injection heuristics expect a class-oriented design this package deliberately does not have. @@ -328,7 +328,7 @@ link checking. The package version is derived automatically from Git tags by `setuptools_scm` (see `[tool.setuptools_scm]` in `pyproject.toml`), so there is -no version string to edit by hand. To cut a release, tag the commit (for +no version string to edit by hand. To make a release, tag the commit (for example, `git tag v1.2.3`) and push the tag; both the installed package version and the documentation's `version` and `release` values follow from it. @@ -348,7 +348,7 @@ locally, and describe what they add or fix. ### Adding Examples to the Documentation -The documentation includes examples as Jupyter notebooks, all of which live in +The documentation includes examples as Jupyter notebooks, all of which are in the `demos/` subdirectory. To add one that the documentation runs and renders, do the following in a separate branch of the repository: diff --git a/NEWS.md b/NEWS.md index 5aa293f45..9a6e21e8a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,26 +1,26 @@ **09/01/2026:** **Announcement:** We at USGS Water Data for the Nation want your feedback! Tell us how we're doing by taking our quick [survey](https://usgswaterresources.gov1.qualtrics.com/jfe/form/SV_07gX8G1DeOtVrH8), available through September 2026. -**08/27/2026:** **Bug fix:** `nwis.get_discharge_peaks` and `nwis.get_record(service='peaks')` silently discarded every peak whose date is only partly known. NWIS zero-fills the unknown part of a historical peak's date -- `YYYY-MM-00` when the day is not known, `YYYY-00-00` when the month is not either (the `Bd` and `Bm` `peak_cd` qualifiers) -- and neither parses as a date, so `preformat_peaks_response` coerced both to `NaT` and then dropped the row along with its discharge value. These are real peaks, and disproportionately a site's largest: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Such a peak is now kept, with `datetime` left as `NaT`. The date is not completed into one NWIS does not have: a `datetime64` column cannot hold a partial date, so any value there would assert a day the record does not claim. **Behavior change:** peaks queries return more rows than before, and `datetime` may now be `NaT` -- a caller selecting on the datetime index will not see those peaks and should filter on `peak_dt` instead. **Behavior change:** `peak_dt` is no longer removed from the returned frame. It is the only column carrying a censored peak's year, since the peaks response has no `water_yr`, and the only dependable way to tell an unknown day from a known one -- `peak_cd` does not always carry the qualifier (22 of 24 censored dates across six sites tested). For peaks with a resolved timestamp alongside explicit `year`/`month`/`day` and a `qualifier` field, use `waterdata.get_peaks()`. +**08/27/2026:** **Bug fix:** `nwis.get_discharge_peaks` and `nwis.get_record(service='peaks')` silently discarded every peak whose date is only partly known. NWIS zero-fills the unknown part of a historical peak's date -- `YYYY-MM-00` when the day is not known, `YYYY-00-00` when the month is not either (the `Bd` and `Bm` `peak_cd` qualifiers) -- and neither parses as a date, so `preformat_peaks_response` coerced both to `NaT` and then dropped the row along with its discharge value. These are real peaks, and disproportionately a site's largest: site 14105700 lost 20 of its 167 peaks, among them an 1859 flood of 847,000 ft3/s, and 06934500 lost its 1844 peak of 700,000 ft3/s. Such a peak is now kept, with `datetime` left as `NaT`. The date is not completed into one NWIS does not have: a `datetime64` column cannot hold a partial date, so any value there would assert a day the record does not claim. **Behavior change:** peaks queries return more rows than before, and `datetime` may now be `NaT` -- a caller selecting on the datetime index will not see those peaks and should filter on `peak_dt` instead. **Behavior change:** `peak_dt` is no longer removed from the returned frame. It is the only column that holds a censored peak's year, since the peaks response has no `water_yr`, and the only dependable way to tell an unknown day from a known one -- `peak_cd` does not always include the qualifier (22 of 24 censored dates across six sites tested). For peaks with a resolved timestamp alongside explicit `year`/`month`/`day` and a `qualifier` field, use `waterdata.get_peaks()`. -**08/26/2026:** **Bug fix:** `nwis.format_response(df, service='peaks')` and `nwis.preformat_peaks_response` raised `KeyError('peak_dt')` on an empty peaks response instead of returning an empty frame. Both are public, and every other service already treated an empty result as a legitimate empty frame rather than an error (issue #171); the peaks arm slipped through because it reformats the datetime column before the empty-frame check. Callers can now check `df.empty` rather than catching an exception. A *non-empty* frame with no `peak_dt` column is malformed rather than empty, and still raises. +**08/26/2026:** **Bug fix:** `nwis.format_response(df, service='peaks')` and `nwis.preformat_peaks_response` raised `KeyError('peak_dt')` on an empty peaks response instead of returning an empty frame. Both are public, and every other service already treated an empty result as a legitimate empty frame rather than an error (issue #171); the peaks branch was missed because it reformats the datetime column before the empty-frame check. Callers can now check `df.empty` rather than catching an exception. A *non-empty* frame with no `peak_dt` column is malformed rather than empty, and still raises. **08/25/2026:** Removed `dataretrieval.ogc.retry`, which only re-exported private helpers. Deprecated `dataretrieval.ogc.interruptions`; import exceptions from `dataretrieval` or `dataretrieval.interruptions` instead. The old path will be removed in a future major release, no earlier than 2027-08-25. -**08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast. +**08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services have the data (NGWMN returns 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast. -**08/20/2026:** Argument checks now share one vocabulary, and every rejection explains how to correct the call. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own spelling of the parameter and a remedy for the move it cannot derive, and every check raises `ValueError` -- one class for a bad argument value. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. **Behavior change:** the deprecated `nwis` query entry points (`query_waterdata`, `query_waterservices`, `get_record`) now answer a missing major filter, an incomplete bounding box, or an unknown service with `ValueError` rather than their historic `TypeError` -- `TypeError` remains for a genuinely mistyped argument, such as a non-string `sites`. Code catching `TypeError` there, or matching on the old strings, must update. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns. +**08/20/2026:** Argument checks now share one vocabulary, and every rejection explains how to correct the call. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own name for the parameter and a remedy for the action it cannot derive, and every check raises `ValueError` -- one class for a bad argument value. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box rejections in `query_waterdata` / `query_waterservices` are rendered in the shared form. **Behavior change:** the deprecated `nwis` query entry points (`query_waterdata`, `query_waterservices`, `get_record`) now respond to a missing major filter, an incomplete bounding box, or an unknown service with `ValueError` rather than their historic `TypeError` -- `TypeError` remains for a genuinely mistyped argument, such as a non-string `sites`. Code catching `TypeError` there, or matching on the old strings, must update. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` responded to `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` wrote `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and responded with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns. -**08/13/2026:** Warning categories now say what they mean. Two advisories about *upstream data* were emitted as `DeprecationWarning` and as an uncategorized `warnings.warn` respectively; both are now `DataCurrencyWarning` (a `UserWarning` subclass, exported as `dataretrieval.DataCurrencyWarning`). **Behavior change:** WQP's legacy-WQX notice moves from `DeprecationWarning` to `DataCurrencyWarning`. Because `legacy=True` is the default on every WQP getter and the notice is unconditional, a downstream project running `-W error::DeprecationWarning` previously could not call any WQP getter with default arguments; it now can. The flip side is visibility — `DeprecationWarning` is silent by default outside `__main__`, so this notice will now print to stderr for library and notebook callers who never saw it. Silence it with `warnings.filterwarnings("ignore", category=dataretrieval.DataCurrencyWarning)`, or set `legacy=False` where a WQX3.0 profile exists. The NWIS qw-endpoint retirement notice gains the same category (it previously had none, arriving as a bare `UserWarning` that deprecation filters ignored). `DeprecationWarning` now means only that a name in this package is going away, always with a replacement and, where published, a removal date; those horizons are declared once in `dataretrieval._deprecation.REMOVALS` rather than spelled at each call site. +**08/13/2026:** Warning categories now say what they mean. Two advisories about *upstream data* were emitted as `DeprecationWarning` and as an uncategorized `warnings.warn` respectively; both are now `DataCurrencyWarning` (a `UserWarning` subclass, exported as `dataretrieval.DataCurrencyWarning`). **Behavior change:** WQP's legacy-WQX notice moves from `DeprecationWarning` to `DataCurrencyWarning`. Because `legacy=True` is the default on every WQP getter and the notice is unconditional, a downstream project running `-W error::DeprecationWarning` previously could not call any WQP getter with default arguments; it now can. The cost is visibility — `DeprecationWarning` is silent by default outside `__main__`, so this notice will now print to stderr for library and notebook callers who never saw it. Suppress it with `warnings.filterwarnings("ignore", category=dataretrieval.DataCurrencyWarning)`, or set `legacy=False` where a WQX3.0 profile exists. The NWIS qw-endpoint retirement notice gains the same category (it previously had none, arriving as a bare `UserWarning` that deprecation filters ignored). `DeprecationWarning` now means only that a name in this package is being removed, always with a replacement and, where published, a removal date; those horizons are declared once in `dataretrieval._deprecation.REMOVALS` rather than declared at each call site. -**08/11/2026:** Settings resolve through a layered chain instead of the environment alone, and a *configuration profile* is a named set of settings for **one adapter**. The new `dataretrieval.configuration` module resolves every setting in one order, highest first: a configuration passed to an active `dataretrieval.configure(...)` block, a profile that block selected, the setting's `API_USGS_*` environment variable, the adapter's `[]` table in `~/.dataretrieval/config.toml` (or `DATARETRIEVAL_CONFIG`), the file's top-level keys, the adapter's own built-in preference, then the package default. Precedence applies **per setting**, so a file that sets only `concurrency` leaves an environment `API_USGS_PAT` in effect, and a `[ngwmn]` table still inherits every top-level key it does not name. `configure()` takes configuration objects positionally, at most one per adapter and nothing else: `configure(Configuration(api_key=vault.read("usgs/pat")), WaterdataConfiguration.load("bulk"), NgwmnConfiguration(concurrency=4))`. The adapter a configuration targets is a property of its class, so a caller never restates it, and each adapter owns its class in the module that *reads* those settings (`waterdata.WaterdataConfiguration`, `ngwmn.NgwmnConfiguration`, `nwdc.NwdcConfiguration`, `wqp.WqpConfiguration`, `nldi.NldiConfiguration`, `streamstats.StreamstatsConfiguration`) — an adapter accepts only the settings it reads, so `[streamstats] parallel_chunks = 8` is an error rather than a line that quietly does nothing. The block is delivered through a `ContextVar`, so a credential set inside it cannot leak across threads or asyncio tasks, which is what makes it safe for a server or notebook handling several users' keys and is the thing assigning to `os.environ` could never do (issue #352). The file gains named profiles beside each adapter's default profile: `[waterdata]` is always in effect, `[waterdata.bulk]` only when a caller selects it with `WaterdataConfiguration.load("bulk")`, and a selected profile still inherits the default profile and the package-wide keys per setting. A profile named in code outranks the setting's environment variable — the one place the ladder inverts the environment-above-file rule, because losing a deliberate selection to a stale shell export is what a caller would file a bug about. An adapter's configuration may also carry a `base_url`, which redirects that adapter's requests for the duration of the block — a staging instance, a mirror, a recording proxy — and no other adapter's; for Water Data one value moves the OGC collections, the Samples database, the statistics service and the STAC catalog together. It is settable in a `configure()` block only: a `base_url` key in the file and an exported `API_USGS_BASE_URL` each raise rather than being read, since a redirect a config file or a shell profile can set is one no reader of the script can see. The API key does not follow a redirect — it is scoped to the single host that honors it — and is deliberately not per-adapter: it authenticates to the gateway fronting a host, and Water Data and NGWMN share that host, one key, and one hourly quota. `dataretrieval.show_configuration()` reports each setting's effective value and where it came from, naming the profile behind each value (`configure() block [waterdata.bulk]` rather than a bare block), listing the profiles a file defines whether or not this run selected any, and naming any adapter this process has not imported rather than omitting it — without ever printing the key. One parser per setting owns its grammar, so a value means the same thing whichever source wrote it. **Breaking change:** `RetryPolicy.from_env()` is now `RetryPolicy.from_configuration()` and resolves through the whole chain rather than the environment alone. **Behavior change:** a credential-shaped keyword passed to a getter's `**kwargs` query passthrough — Water Data's `**queryables` and every WQP getter's search filters — now raises `TypeError` naming `configure(Configuration(api_key=...))` instead of putting a secret in a URL that clients, proxies and logs retain. The names refused are `api_key=`, `token=`, `x_api_key=`, `password=`, `auth=`, `pat=` and similar spellings; a filter the server actually defines is unaffected. **Bug fix:** `API_USGS_STALL_TIMEOUT` was read straight from `os.environ`, so it could not be set by a `configure()` block or the config file and never appeared in `show_configuration()`; it now resolves through the chain like every other setting. Rationale in ADRs 0009, 0010 and 0011; terms in `CONTEXT.md`. +**08/11/2026:** Settings resolve through a layered chain instead of the environment alone, and a *configuration profile* is a named set of settings for **one adapter**. The new `dataretrieval.configuration` module resolves every setting in one order, highest first: a configuration passed to an active `dataretrieval.configure(...)` block, a profile that block selected, the setting's `API_USGS_*` environment variable, the adapter's `[]` table in `~/.dataretrieval/config.toml` (or `DATARETRIEVAL_CONFIG`), the file's top-level keys, the adapter's own built-in preference, then the package default. Precedence applies **per setting**, so a file that sets only `concurrency` leaves an environment `API_USGS_PAT` in effect, and a `[ngwmn]` table still inherits every top-level key it does not name. `configure()` takes configuration objects positionally, at most one per adapter and nothing else: `configure(Configuration(api_key=vault.read("usgs/pat")), WaterdataConfiguration.load("bulk"), NgwmnConfiguration(concurrency=4))`. The adapter a configuration targets is a property of its class, so a caller never restates it, and each adapter owns its class in the module that *reads* those settings (`waterdata.WaterdataConfiguration`, `ngwmn.NgwmnConfiguration`, `nwdc.NwdcConfiguration`, `wqp.WqpConfiguration`, `nldi.NldiConfiguration`, `streamstats.StreamstatsConfiguration`) — an adapter accepts only the settings it reads, so `[streamstats] parallel_chunks = 8` is an error rather than a line that quietly does nothing. The block is delivered through a `ContextVar`, so a credential set inside it cannot leak across threads or asyncio tasks, which is what makes it safe for a server or notebook handling several users' keys and is the thing assigning to `os.environ` could never do (issue #352). The file gains named profiles beside each adapter's default profile: `[waterdata]` is always in effect, `[waterdata.bulk]` only when a caller selects it with `WaterdataConfiguration.load("bulk")`, and a selected profile still inherits the default profile and the package-wide keys per setting. A profile named in code outranks the setting's environment variable — the one place the ladder inverts the environment-above-file rule, because a stale shell export overriding a deliberate selection would look like a bug. An adapter's configuration may also include a `base_url`, which redirects that adapter's requests for the duration of the block — a staging instance, a mirror, a recording proxy — and no other adapter's; for Water Data one value moves the OGC collections, the Samples database, the statistics service and the STAC catalog together. It is settable in a `configure()` block only: a `base_url` key in the file and an exported `API_USGS_BASE_URL` each raise rather than being read, since a redirect a config file or a shell profile can set is one no reader of the script can see. The API key does not follow a redirect — it is scoped to the single host that honors it — and is deliberately not per-adapter: it authenticates to the gateway fronting a host, and Water Data and NGWMN share that host, one key, and one hourly quota. `dataretrieval.show_configuration()` reports each setting's effective value and where it came from, naming the profile behind each value (`configure() block [waterdata.bulk]` rather than a bare block), listing the profiles a file defines whether or not this run selected any, and naming any adapter this process has not imported rather than omitting it — without ever printing the key. One parser per setting owns its grammar, so a value means the same thing whichever source wrote it. **Breaking change:** `RetryPolicy.from_env()` is now `RetryPolicy.from_configuration()` and resolves through the whole chain rather than the environment alone. **Behavior change:** a credential-shaped keyword passed to a getter's `**kwargs` query passthrough — Water Data's `**queryables` and every WQP getter's search filters — now raises `TypeError` naming `configure(Configuration(api_key=...))` instead of putting a secret in a URL that clients, proxies and logs retain. The names refused are `api_key=`, `token=`, `x_api_key=`, `password=`, `auth=`, `pat=` and similar names; a filter the server actually defines is unaffected. **Bug fix:** `API_USGS_STALL_TIMEOUT` was read straight from `os.environ`, so it could not be set by a `configure()` block or the config file and never appeared in `show_configuration()`; it now resolves through the chain like every other setting. Rationale in ADRs 0009, 0010 and 0011; terms in `CONTEXT.md`. -**08/11/2026:** `dataretrieval.wateruse` is now `dataretrieval.nwdc`. Every other adapter is named for the service it retrieves from — `ngwmn`, `nldi`, `wqp`, `streamstats`, `nwis` — and this one was named for one subset of what its service offers. The National Water Availability Assessment Data Companion serves ten modeled datasets; the water-use models are five of them, the rest being hydrologic, atmospheric-forcing, and assessment outputs (`GET https://api.water.usgs.gov/nwaa-data/models`). **Deprecation:** `dataretrieval.wateruse` still works and re-exports `dataretrieval.nwdc` unchanged, emitting a `DeprecationWarning` on import; it will be removed on or after 2027-08-11. The alias forwards rather than copies, so `wateruse.get_wateruse is nwdc.get_wateruse` — monkeypatching or identity comparison through either spelling behaves the same. `import dataretrieval` stays silent: the package imports `nwdc` directly, so only code naming `wateruse` itself sees the warning. Function and constant names are unchanged (`get_wateruse`, `MODELS`, `WATERUSE_URL`, `DEFAULT_CONCURRENT_REQUESTS`). Terms are defined in `CONTEXT.md`. +**08/11/2026:** `dataretrieval.wateruse` is now `dataretrieval.nwdc`. Every other adapter is named for the service it retrieves from — `ngwmn`, `nldi`, `wqp`, `streamstats`, `nwis` — and this one was named for one subset of what its service offers. The National Water Availability Assessment Data Companion serves ten modeled datasets; the water-use models are five of them, the rest being hydrologic, atmospheric-forcing, and assessment outputs (`GET https://api.water.usgs.gov/nwaa-data/models`). **Deprecation:** `dataretrieval.wateruse` still works and re-exports `dataretrieval.nwdc` unchanged, emitting a `DeprecationWarning` on import; it will be removed on or after 2027-08-11. The alias forwards rather than copies, so `wateruse.get_wateruse is nwdc.get_wateruse` — monkeypatching or identity comparison through either name behaves the same. `import dataretrieval` stays silent: the package imports `nwdc` directly, so only code naming `wateruse` itself sees the warning. Function and constant names are unchanged (`get_wateruse`, `MODELS`, `WATERUSE_URL`, `DEFAULT_CONCURRENT_REQUESTS`). Terms are defined in `CONTEXT.md`. **08/09/2026:** `waterdata.get_cql` takes `collection` rather than `service`. OGC API - Features (17-069r4) normatively names this value the `collectionId`: Requirement 20 fixes the path template `/collections/{collectionId}/items`, and Requirement 18 defines `collectionId` as each `id` in the collections response -- which is literally how the package builds the URL, and what the live API returns. *Service* names the API itself (Water Data, NGWMN). **Deprecation:** `service=` still works and resolves to `collection`, with a `DeprecationWarning`; it will be removed on or after 2027-08-09. Positional callers (`get_cql("daily", cql)`) are unaffected. The `WATERDATA_SERVICES` type alias is now `WATERDATA_COLLECTIONS`, with `WATERDATA_SERVICES` retained as a permanent alias for the same object. Terms are defined in `CONTEXT.md`. -**08/09/2026:** Every retrieval path now runs through one executor. `waterdata.get_cql` (via the OGC `fetch_ogc_request`) and `waterdata.get_stats_por` / `get_stats_date_range` (via the Statistics page walk) previously bypassed `dataretrieval.transport.fanout.FanOut` through a private sync bridge, which meant they were the only getters in the package with **no retry**: a mid-page-walk 429 or 503 failed the whole call while every typed getter and Water Use rode it out. Both now run as a one-item fan-out and the 25-line `transport/sync.py` is gone. **Behavior change:** those three getters now retry transient failures (`API_USGS_RETRIES`, default 4) and, when the retries are exhausted, raise the resumable `ServiceInterrupted` / `QuotaExhausted` rather than `ServiceUnavailable` / `RateLimited` / `NetworkError` — all remain `DataRetrievalError`, so broad handlers are unaffected, but narrow handlers around those calls must widen, and `.call.resume()` is now available on the interruption. A failure that retrying cannot fix (bad scheme, a hostname that does not resolve) still surfaces as `NetworkError` immediately. The progress line moved with it: `FanOut.resume()` opens the reporter it ticks into, so a driver can no longer run the shared executor and silently print nothing, and a `.call.resume()` fired long after the interruption now reports progress instead of running mute. Internal tidying with no public effect: the WQX3 / legacy-WQP CSV datetime shaping moved out of `dataretrieval.utils` (whose docstring reserves it for non-service-specific shaping) into the `dataretrieval._wqx` leaf; the five Water Data endpoint URLs are declared once in `dataretrieval.waterdata.endpoints` instead of being derived in three modules; the OGC queryables document is parsed by `dataretrieval.ogc.schema` so every OGC adapter can offer the table, with `waterdata.get_queryables` unchanged as its documented wrapper; and `ogc/engine.py` imports each symbol from the module that defines it. +**08/09/2026:** Every retrieval path now runs through one executor. `waterdata.get_cql` (via the OGC `fetch_ogc_request`) and `waterdata.get_stats_por` / `get_stats_date_range` (via the Statistics page walk) previously bypassed `dataretrieval.transport.fanout.FanOut` through a private sync bridge, which meant they were the only getters in the package with **no retry**: a mid-page-walk 429 or 503 failed the whole call while every typed getter and Water Use retried through it. Both now run as a one-item fan-out and the 25-line `transport/sync.py` is gone. **Behavior change:** those three getters now retry transient failures (`API_USGS_RETRIES`, default 4) and, when the retries are exhausted, raise the resumable `ServiceInterrupted` / `QuotaExhausted` rather than `ServiceUnavailable` / `RateLimited` / `NetworkError` — all remain `DataRetrievalError`, so broad handlers are unaffected, but narrow handlers around those calls must widen, and `.call.resume()` is now available on the interruption. A failure that retrying cannot fix (bad scheme, a hostname that does not resolve) is still raised as `NetworkError` immediately. The progress line moved with it: `FanOut.resume()` opens the reporter it ticks into, so a driver can no longer run the shared executor and silently print nothing, and a `.call.resume()` invoked long after the interruption now reports progress instead of printing nothing. Internal tidying with no public effect: the WQX3 / legacy-WQP CSV datetime shaping moved out of `dataretrieval.utils` (whose docstring reserves it for non-service-specific shaping) into the `dataretrieval._wqx` leaf; the five Water Data endpoint URLs are declared once in `dataretrieval.waterdata.endpoints` instead of being derived in three modules; the OGC queryables document is parsed by `dataretrieval.ogc.schema` so every OGC adapter can offer the table, with `waterdata.get_queryables` unchanged as its documented wrapper; and `ogc/engine.py` imports each symbol from the module that defines it. -**08/09/2026:** Internal structure cleanup, no public API change. Validating a server-supplied next-page link is now one policy in `dataretrieval.transport.links` instead of three divergent copies (the OGC engine, the ratings STAC walk, and Water Use). Two of those copies were fixed by the merge: the OGC page walk now resolves a *relative* `next` href against the page it came from (it previously handed the unresolved reference back as the pagination cursor) and refuses an unparseable one rather than following it unchecked. Cross-host refusal, credential stripping, and Water Use's host-alias rewrite are unchanged, as is the error type each walk raises. `parse_retry_after` moved to `dataretrieval.exceptions`, next to the `DataRetrievalError.retry_after` field it exists to produce. The one-shot HTTP query path (`query`, `to_str`, and their helpers) moved out of `dataretrieval.utils` into the private `dataretrieval._querying`; `dataretrieval.utils.query` and `dataretrieval.utils.to_str` remain the documented public paths, as `Ambient` and `BaseMetadata` already do. `waterdata` profile validation moved next to the tables it validates in `waterdata.types`, and `nwis.get_dv`/`get_iv` now share one body. +**08/09/2026:** Internal structure cleanup, no public API change. Validating a server-supplied next-page link is now one policy in `dataretrieval.transport.links` instead of three divergent copies (the OGC engine, the ratings STAC walk, and Water Use). Two of those copies were fixed by the merge: the OGC page walk now resolves a *relative* `next` href against the page it came from (it previously returned the unresolved reference as the pagination cursor) and refuses an unparseable one rather than following it unchecked. Cross-host refusal, credential stripping, and Water Use's host-alias rewrite are unchanged, as is the error type each walk raises. `parse_retry_after` moved to `dataretrieval.exceptions`, next to the `DataRetrievalError.retry_after` field it exists to produce. The one-shot HTTP query path (`query`, `to_str`, and their helpers) moved out of `dataretrieval.utils` into the private `dataretrieval._querying`; `dataretrieval.utils.query` and `dataretrieval.utils.to_str` remain the documented public paths, as `Ambient` and `BaseMetadata` already do. `waterdata` profile validation moved next to the tables it validates in `waterdata.types`, and `nwis.get_dv`/`get_iv` now share one body. **08/06/2026:** Fan-out execution is now shared across services. Chunking is how a query is divided structurally (a Water Data/NGWMN URL over the byte limit); fan-out is how the pieces are distributed operationally. Only the first is protocol-specific, so the executor moved to `dataretrieval.transport.fanout` (`FanOut`, over a three-member `FanOutPlan` protocol) while chunk planning stays in `dataretrieval.ogc`. Water Use no longer re-implements the fan-out gather and inherits resume, progress reporting, and `API_USGS_CONCURRENT`: a multi-location pull interrupted by a rate limit now raises a resumable interruption whose `.call.resume()` re-issues only the locations that did not finish, instead of discarding every completed one. The interruption taxonomy moved to the `dataretrieval.interruptions` leaf and its base class is now `FanOutInterrupted`; **`ChunkInterrupted` is a permanent alias of the same class**, so `except ChunkInterrupted` keeps working. **Breaking change:** a Water Use fan-out interrupted by a 5xx, 429, or recoverable connection failure now raises `ServiceInterrupted`/`QuotaExhausted` rather than `ServiceUnavailable`/`RateLimited`/`NetworkError` — all remain `DataRetrievalError`, so broad handlers are unaffected, but narrow handlers around a Water Use call must widen. **Breaking change:** `wateruse.MAX_CONCURRENT_REQUESTS` is removed; set `API_USGS_CONCURRENT` (which now outranks any service default) or read `wateruse.DEFAULT_CONCURRENT_REQUESTS`. @@ -30,17 +30,17 @@ **08/02/2026:** Phase 1 OGC boundary stabilization: the `dataretrieval.ogc` package now exposes a deliberate, small facade (`OgcDialect`, `prepare_request_args`, `get_ogc_data`, `fetch_ogc_request`) that service adapters (NGWMN, Water Data's generic wrapper) import. Internal request-construction helpers moved to a new `ogc.requests` module; the dialect type and endpoint constants live in the leaf `ogc.policy` module. `ogc.shaping` no longer depends on `ogc.engine` at all, and the complete runtime OGC import graph is now acyclic. `_default_headers` now accepts a target URL and adds `X-Api-Key` only for `api.waterdata.usgs.gov`; shared sync and async clients also strip the key before following any cross-host redirect, including external rating-asset downloads. `waterdata.utils` no longer bulk-re-exports private OGC symbols, and consumers import implementation helpers from their canonical modules. No public API, return-value, or deprecation changes. -**08/02/2026:** Fixed source-distribution and wheel package discovery so the `dataretrieval.ogc` and `dataretrieval.waterdata` subpackages are included in installed artifacts. CI now builds and installs the wheel outside the source checkout before importing the core service modules. Added an architecture baseline, initial decision records, and executable dependency-direction guardrails for the existing modular-monolith boundaries. +**08/02/2026:** Fixed source-distribution and wheel package discovery so the `dataretrieval.ogc` and `dataretrieval.waterdata` subpackages are included in installed artifacts. CI now builds and installs the wheel outside the source checkout before importing the core service modules. Added an architecture baseline, initial decision records, and executable dependency-direction checks for the existing modular-monolith boundaries. **06/23/2026:** **Breaking change (1.2.0):** the minimum supported Python is now **3.10** (`requires-python = ">=3.10"`). 3.9 support was already effectively broken — the `waterdata` module's dependencies (`anyio`, the test stack) require 3.10+, and the `waterdata` test modules already skipped on <3.10. `anyio` is now declared as a direct dependency (it is imported directly by `waterdata`), and the CI/ruff/mypy targets move to 3.10. Also fully removed the deprecated `variable_info` metadata property: the `NWIS_Metadata` override only warned and returned `None` (it relied on the defunct `get_pmcodes`), and the `BaseMetadata` abstract is gone too since nothing implemented it — accessing `.variable_info` now raises `AttributeError`. `site_info` is unaffected. **06/23/2026:** **Breaking change (1.2.0):** removed the `nadp` module and the deprecated `samples` module ahead of the 1.2.0 release. `nadp` was deprecated on 05/01/2026 — NADP is not a USGS data source, so retrieve NADP data directly from https://nadp.slh.wisc.edu/. The `samples.get_usgs_samples` shim (a deprecated forward to the modern getter) is gone; use `waterdata.get_samples()` instead. `import dataretrieval.nadp` / `import dataretrieval.samples` now raise `ModuleNotFoundError`. -**06/03/2026:** The request-error hierarchy is now unified. Every module (`nwis`, `wqp`, `nldi`, `waterdata`, `nadp`, `streamstats`) raises a subclass of `dataretrieval.DataRetrievalError` on a failed request, so a single `except dataretrieval.DataRetrievalError` spans them all. An HTTP error status surfaces as an `HTTPError` carrying `.status_code` (inspect it to branch on a specific code); the retryable 429/5xx subset is `TransientError` (`RateLimited` / `ServiceUnavailable`, carrying `.retry_after`); and a request too large to satisfy is a `RequestTooLarge` (`URLTooLong` for an over-long single request, `Unchunkable` when the Water Data chunker cannot split a call small enough). Connection-level failures (timeouts, DNS, refused connections) are wrapped as a `NetworkError`, with the underlying `httpx` exception on `__cause__`. Every `DataRetrievalError` also exposes `.status_code` (`None` when there is no HTTP status), `.retry_after`, and `.retryable`, so a single `except dataretrieval.DataRetrievalError as e` clause can branch on the status or retry transient failures without knowing the concrete subclass. **Breaking change:** these exceptions no longer multiply-inherit a built-in — code that caught request failures with `except ValueError` or `except RuntimeError` should switch to `except dataretrieval.DataRetrievalError` (or a specific subclass). A no-data result is **not** an error: the modern getters (`waterdata`, `wqp`, `nldi`) return an empty DataFrame when nothing matches. Only the deprecated `nwis` (waterservices) path still raises `NoSitesError` on no data. +**06/03/2026:** The request-error hierarchy is now unified. Every module (`nwis`, `wqp`, `nldi`, `waterdata`, `nadp`, `streamstats`) raises a subclass of `dataretrieval.DataRetrievalError` on a failed request, so a single `except dataretrieval.DataRetrievalError` spans them all. An HTTP error status is raised as an `HTTPError` with `.status_code` (inspect it to branch on a specific code); the retryable 429/5xx subset is `TransientError` (`RateLimited` / `ServiceUnavailable`, with `.retry_after`); and a request too large to satisfy is a `RequestTooLarge` (`URLTooLong` for an over-long single request, `Unchunkable` when the Water Data chunker cannot split a call small enough). Connection-level failures (timeouts, DNS, refused connections) are wrapped as a `NetworkError`, with the underlying `httpx` exception on `__cause__`. Every `DataRetrievalError` also exposes `.status_code` (`None` when there is no HTTP status), `.retry_after`, and `.retryable`, so a single `except dataretrieval.DataRetrievalError as e` clause can branch on the status or retry transient failures without knowing the concrete subclass. **Breaking change:** these exceptions no longer multiply-inherit a built-in — code that caught request failures with `except ValueError` or `except RuntimeError` should switch to `except dataretrieval.DataRetrievalError` (or a specific subclass). A no-data result is **not** an error: the modern getters (`waterdata`, `wqp`, `nldi`) return an empty DataFrame when nothing matches. Only the deprecated `nwis` (waterservices) path still raises `NoSitesError` on no data. **05/17/2026:** The OGC `waterdata` getters (`get_daily`, `get_continuous`, `get_field_measurements`, and the rest of the multi-value-capable functions) now transparently chunk requests whose URLs would otherwise exceed the server's ~8 KB byte limit. -**05/16/2026:** Fixed silent truncation in the paginated `waterdata` request loops (`_walk_pages` and `get_stats_data`). Mid-pagination failures (HTTP 429, 5xx, network error) were previously swallowed — pagination would quietly stop and the function would return whatever rows it had collected, leaving callers with truncated DataFrames they had no way to detect. The loops now status-check every page like the initial request and raise `RuntimeError` on any failure, with the upstream exception chained as `__cause__` and a short menu of recovery actions (wait and retry, reduce the request, or obtain an API token) in the message. **Behavior change**: callers that previously consumed partial DataFrames on transient upstream blips will now see an exception; retry the call (possibly with a smaller `limit` or narrower query). +**05/16/2026:** Fixed silent truncation in the paginated `waterdata` request loops (`_walk_pages` and `get_stats_data`). Mid-pagination failures (HTTP 429, 5xx, network error) were previously caught and ignored — pagination would quietly stop and the function would return whatever rows it had collected, leaving callers with truncated DataFrames they had no way to detect. The loops now status-check every page like the initial request and raise `RuntimeError` on any failure, with the upstream exception chained as `__cause__` and a short list of recovery actions (wait and retry, reduce the request, or obtain an API token) in the message. **Behavior change**: callers that previously consumed partial DataFrames on transient upstream failures will now see an exception; retry the call (possibly with a smaller `limit` or narrower query). **05/07/2026:** Bumped the declared minimum Python version from **3.8** to **3.9** (`pyproject.toml`'s `requires-python` and the ruff target). This brings the manifest in line with what was already being tested — CI's matrix has long covered only 3.9, 3.13, and 3.14, the `waterdata` test module already skipped itself on Python < 3.10, and several modules already use 3.9-only stdlib (e.g. `zoneinfo`). Users on 3.8 will no longer be able to install the package; please upgrade. @@ -52,7 +52,7 @@ **05/06/2026:** Added `waterdata.get_field_measurements_metadata(...)` — wraps the OGC `field-measurements-metadata` collection. Returns one row per (location, parameter) field-measurement series describing its period of record, units, etc., without the underlying observations. Discrete-measurement analogue to `get_time_series_metadata`. Mirrors R's `read_waterdata_field_meta`. -**05/06/2026:** Added `waterdata.get_peaks(...)` — wraps the new OGC `peaks` collection, returning the annual peak streamflow / stage record for a monitoring location (one row per water year, per parameter). Standard input to flood-frequency analysis. Supports calendar/water-year filters and the usual location/parameter/CQL knobs shared with the other OGC getters. +**05/06/2026:** Added `waterdata.get_peaks(...)` — wraps the new OGC `peaks` collection, returning the annual peak streamflow / stage record for a monitoring location (one row per water year, per parameter). Standard input to flood-frequency analysis. Supports calendar/water-year filters and the usual location/parameter/CQL options shared with the other OGC getters. **05/05/2026:** Added `waterdata.get_combined_metadata(...)` — wraps the Water Data API's `combined-metadata` collection, which joins the monitoring-locations catalog with the time-series-metadata catalog and returns one row per (location, parameter, statistic) inventory entry. This is the most flexible "what data is available" endpoint in the API: any location attribute (state, HUC, site type, drainage area, well-construction depth, …) can be combined with any time-series attribute (parameter code, statistic, data type, period of record, …) in a single query. Mirrors R's `read_waterdata_combined_meta`. @@ -72,7 +72,7 @@ **12/04/2025:** The `get_continuous()` function was added to the `waterdata` module, which provides access to measurements collected via automated sensors at a high frequency (often 15 minute intervals) at a monitoring location. This is an early version of the continuous endpoint and should be used with caution as the API team improves its performance. In the future, we anticipate the addition of an endpoint(s) specifically for handling large data requests, so it may make sense for power users to hold off on heavy development using the new continuous endpoint. -**11/24/2025:** `dataretrieval` is pleased to offer a new module, `waterdata`, which gives users access USGS's modernized [Water Data APIs](https://api.waterdata.usgs.gov/). The Water Data API endpoints include daily values, instantaneous values, field measurements (modernized groundwater levels service), time series metadata, and discrete water quality data from the Samples database. Though there will be a period of overlap, the functions within `waterdata` will eventually replace the `nwis` module, which currently provides access to the legacy [NWIS Water Services](https://waterservices.usgs.gov/). More example workflows and functions coming soon. Check `help(waterdata)` for more information. +**11/24/2025:** `dataretrieval` has a new module, `waterdata`, which gives users access to USGS's modernized [Water Data APIs](https://api.waterdata.usgs.gov/). The Water Data API endpoints include daily values, instantaneous values, field measurements (modernized groundwater levels service), time series metadata, and discrete water quality data from the Samples database. Though there will be a period of overlap, the functions within `waterdata` will eventually replace the `nwis` module, which currently provides access to the legacy [NWIS Water Services](https://waterservices.usgs.gov/). More example workflows and functions coming soon. Check `help(waterdata)` for more information. **09/03/2024:** The groundwater levels service has switched endpoints, and `dataretrieval` was updated accordingly in [`v1.0.10`](https://github.com/DOI-USGS/dataretrieval-python/releases/tag/v1.0.10). Older versions using the discontinued endpoint will return 503 errors for `nwis.get_gwlevels` or the `service='gwlevels'` argument. Visit [Water Data For the Nation](https://waterdata.usgs.gov/blog/wdfn-waterservices-2024/) for more information. diff --git a/README.md b/README.md index b45eb5e10..e639981d1 100644 --- a/README.md +++ b/README.md @@ -156,7 +156,7 @@ pull, that default is needlessly conservative: every sub-request pages through its own results, so dividing the query into more, smaller sub-requests lets those pages be fetched **in parallel**. `parallel_chunks(n)` opts a single call into that finer split, fanning it out into `n` sub-requests. The finer split -pays off only when the result is large enough to span many pages *and* the query +helps only when the result is large enough to span many pages *and* the query has a multi-value argument to divide, such as a list of monitoring locations. On a small query — or one with nothing to split — it only adds requests, so `parallel_chunks` is a deliberate, scoped `with` block, never the default. @@ -165,7 +165,7 @@ a small query — or one with nothing to split — it only adds requests, so from dataretrieval import waterdata # All stream gages in Ohio, then 20 years of their daily discharge — large -# enough to span many pages, so it profits from a finer split. +# enough to span many pages, so it benefits from a finer split. sites, _ = waterdata.get_monitoring_locations(state="Ohio", site_type_code="ST") with waterdata.parallel_chunks(32): # request up to 32 optional chunks diff --git a/docs/source/architecture/decisions/0000-documenting-decisions.rst b/docs/source/architecture/decisions/0000-documenting-decisions.rst index 34089e609..55fb52b32 100644 --- a/docs/source/architecture/decisions/0000-documenting-decisions.rst +++ b/docs/source/architecture/decisions/0000-documenting-decisions.rst @@ -14,13 +14,13 @@ Context This package documents itself heavily and deliberately. Its public getters are thin wrappers whose numpydoc parameter tables *are* the deliverable: 55% of all -docstring lines in ``dataretrieval/`` sit in the service adapters, at a ratio of +docstring lines in ``dataretrieval/`` are in the service adapters, at a ratio of 2.5 prose lines per line of code. CONTRIBUTING already requires those tables. The problem is in the internal modules behind them. Rationale -- the argument for why a rule holds -- accumulated in module and function docstrings alongside the ADRs that already owned it, because a paragraph can be written where the -reader is standing while a citation sends them to a record they have to open. +reader already is, while a citation sends them to a record they have to open. Those modules hold 82% of the package's comment lines, and an audit of that prose found roughly 500 lines restating decisions already recorded in ADRs 0003 through 0011: ``configuration.py`` re-derives the layered-resolution design in @@ -49,16 +49,16 @@ their full parameter tables, however long. A private helper's docstring says what it does and what its callers may rely on. **Inline comments own the local constraint.** Why *these* lines are written this -way, when a name cannot carry it -- an ordering that matters, an upstream quirk, -a bail-out that looks removable. One or two lines, adjacent to the code they -explain. A comment that outgrows that is describing something wider than the -lines beneath it, and belongs in one of the venues below. +way, when a name cannot express it -- an ordering that matters, an upstream +quirk, an early return that looks removable. One or two lines, adjacent to the +code they explain. A comment longer than that is describing something wider +than the lines beneath it, and belongs in one of the venues below. **Commit messages own the history.** Benchmark numbers, the symptom that prompted a change, what the code used to do, what was tried and rejected. This is the venue with a date and a diff attached. It is the one place where "was once optional" or "measured 1.6x slower" stays true forever without maintenance. -Source files carry the current state, not the route to it. +Source files describe the current state, not how it was reached. **ADRs own the cross-cutting decision.** A choice that constrains code outside the file stating it, or that a future contributor could plausibly undo from @@ -68,7 +68,7 @@ number a new one sequentially and follow :doc:`template`. **The glossary owns the vocabulary.** ``CONTEXT.md`` defines terms with package-wide meaning. Documents use those terms rather than redefining them, and -where a term and the code disagree, the term wins. +where a term and the code disagree, the term is authoritative. Three rules follow: @@ -76,13 +76,13 @@ Three rules follow: replaced by a reference to that ADR's number. A pointer that does not resolve is visible; a paraphrase that has drifted is not. - **Write from the reader's position.** A citation replaces an argument only if - the sentence left behind stands on its own. Prose that assumes the reader has - the cited record already open, or leans on a term the glossary does not - define, has moved the cost of the duplication rather than removed it. + the sentence left behind is understandable on its own. Prose that assumes the + reader has the cited record already open, or relies on a term the glossary + does not define, has moved the cost of the duplication rather than removed it. - **An accepted ADR is not edited to reverse its meaning.** A later decision supersedes it and links back, as the decisions index already requires. Additive clauses and corrections are recorded in the amended record's - ``Notes``, and its ``Status`` says the record was amended, so a reader meets + ``Notes``, and its ``Status`` says the record was amended, so a reader sees that fact before the Decision text rather than after it. Consequences @@ -94,15 +94,15 @@ Consequences opening an ADR. That cost is accepted -- the reader who needs the argument is rarer than the reader who needs the contract, and the ADR is the version that is maintained. -- Rationale is not deleted when it moves. Prose that leaves a docstring lands in - an ADR clause or in the commit message that removes it. The commit message is - where a reviewer looks for what a documentation change discarded. +- Rationale is not deleted when it moves. Prose that leaves a docstring is moved + to an ADR clause or to the commit message that removes it. The commit message + is where a reviewer looks for what a documentation change discarded. - Docstring volume in the service adapters is expected to stay high and is not a metric to optimize. A ratio measured over a public adapter says nothing about whether it is over-documented. - The policy applies going forward. Existing prose is migrated when a module is - being changed for another reason, rather than in a sweep that would touch - every file at once. + being changed for another reason, rather than in a single pass that would + touch every file at once. Compliance ---------- @@ -111,9 +111,9 @@ Reviewers apply two questions to added prose. First: *does this explain the lines beneath it, or does it argue for a rule that binds another file?* The second belongs in an ADR, cited by number. Then: *could a reader who has not opened the cited record follow this sentence?* If not, the citation has hidden -the explanation rather than relocated it. The repair is to restore the reader's -footing -- name the term, resolve the pronoun, say which venue owns the rest -- -not to restate the argument the citation replaced. +the explanation rather than relocated it. The repair is to give the reader what +they need -- name the term, resolve the pronoun, say which venue owns the +rest -- not to restate the argument the citation replaced. The mechanical part is checkable, and it is the part that goes stale: a docstring or comment that names an ADR must name one that exists. @@ -123,18 +123,18 @@ docstring or comment that names an ADR must name one that exists. the suite rather than leaving a dangling pointer. Whether a given paragraph should have been a citation remains a review judgement. No test is proposed: a proxy metric here would push contributors to delete parameter documentation to -move a number. +improve a number. Notes ----- The reader's-position rule and the second review question were added after acceptance. Review of the pull request that introduced this record found prose -this record had put in the right venue and left unreadable from outside the -author's head: undefined jargon, a pronoun with no antecedent, and a mapping -between two numbering schemes that needed a second document open. One instance +this record had put in the right venue and that only the author could follow: +undefined jargon, a pronoun with no antecedent, and a mapping between two +numbering schemes that needed a second document open. One instance broke this record's own history rule. The venue rules say where an explanation -goes; none of them asked who it reads for. +goes; none of them considered who would read it. ``Context`` and the measurements below are this package's. ``Decision``, ``Consequences``, and the review questions in ``Compliance`` are written to hold diff --git a/docs/source/architecture/decisions/0001-modular-monolith.rst b/docs/source/architecture/decisions/0001-modular-monolith.rst index a179b0ba1..68b22e7ce 100644 --- a/docs/source/architecture/decisions/0001-modular-monolith.rst +++ b/docs/source/architecture/decisions/0001-modular-monolith.rst @@ -37,8 +37,8 @@ Consequences therefore require import checks and review discipline. - Shared infrastructure must remain small enough that it does not become a god module. -- A new service should begin as its own adapter and earn shared abstractions - through demonstrated duplication rather than up-front generalization. +- A new service should begin as its own adapter and acquire shared abstractions + only after duplication has been demonstrated, not by up-front generalization. Compliance ---------- diff --git a/docs/source/architecture/decisions/0002-sync-api-async-internals.rst b/docs/source/architecture/decisions/0002-sync-api-async-internals.rst index 1d9d06039..6219dd504 100644 --- a/docs/source/architecture/decisions/0002-sync-api-async-internals.rst +++ b/docs/source/architecture/decisions/0002-sync-api-async-internals.rst @@ -28,7 +28,7 @@ details, not a second public API promise. Ambient per-call policy (the progress reporter) must propagate into the worker context. A resumable OGC call binds the state needed to rebuild its remaining requests -- base URL, dialect, row cap -- into its fetch closures, so a resume -fired after the original getter has returned rebuilds against the values the +invoked after the original getter has returned rebuilds against the values the call was created with. Consequences @@ -36,7 +36,7 @@ Consequences - Existing scripts and notebooks retain simple blocking call sites. - Concurrent network waits improve large paginated downloads. -- Each top-level async-backed call pays worker-thread and portal startup cost. +- Each top-level async-backed call incurs worker-thread and portal startup cost. - Cancellation, context propagation, and client ownership need explicit tests. - A future public async API, if justified, should be additive and share the same lower-level contracts rather than duplicate behavior. diff --git a/docs/source/architecture/decisions/0003-dependency-direction.rst b/docs/source/architecture/decisions/0003-dependency-direction.rst index 60251917a..41eb9090f 100644 --- a/docs/source/architecture/decisions/0003-dependency-direction.rst +++ b/docs/source/architecture/decisions/0003-dependency-direction.rst @@ -25,7 +25,7 @@ infrastructure. In particular: - ``dataretrieval.exceptions`` is a runtime-dependency-light leaf. - ``dataretrieval.ogc`` must not import Water Data, NGWMN, Water Use, or NWIS. - ``dataretrieval.ogc`` must not depend on the mixed legacy ``utils`` module; - shared scoped state lives in a dependency-free leaf instead. + shared scoped state is kept in a dependency-free leaf instead. - Modern modules must not import deprecated NWIS. - Service-neutral transport must not import OGC modules or service adapters. - Non-OGC services must obtain generic execution behavior from transport, not diff --git a/docs/source/architecture/decisions/0004-error-retry-resume.rst b/docs/source/architecture/decisions/0004-error-retry-resume.rst index c5c159de1..c99461649 100644 --- a/docs/source/architecture/decisions/0004-error-retry-resume.rst +++ b/docs/source/architecture/decisions/0004-error-retry-resume.rst @@ -6,7 +6,7 @@ Status Accepted. The clause assigning resumable partial state to OGC is superseded by :doc:`0008-fan-out-execution`, which moves fan-out *execution* into transport -and gives every fanned-out service resume. The rest stands. +and gives every fanned-out service resume. The rest remains in effect. Amended after acceptance under :doc:`0000-documenting-decisions`; the ``Notes`` section records every clause added or corrected. @@ -25,20 +25,20 @@ Decision All request failures exposed by public service modules derive from ``DataRetrievalError`` and provide uniform ``status_code``, ``retry_after``, and -``retryable`` attributes. Status mapping lives in one policy function. +``retryable`` attributes. Status mapping is done in one policy function. Where automatic recovery is supported, retries are bounded, use exponential backoff with full jitter, honor only bounded ``Retry-After`` delays, and preserve cancellation. OGC fan-out retains completed chunks and raises a typed ``ChunkInterrupted`` with a handle that resumes only missing work. Fatal or -unknown failures are not disguised as resumable transients. +unknown failures are not classified as resumable transients. The shared transport layer supplies bounded retry and callback-driven cursor pagination, but each adapter opts in only where its requests are idempotent and its protocol exposes a cursor. Chunk planning and resumable partial state remain OGC-specific capabilities rather than assumptions imposed on every service. -**Warnings carry the advisories that are not failures.** The taxonomy above +**Warnings convey the advisories that are not failures.** The taxonomy above covers what stops a call; two things that do not stop one are decided here as well, because getting either wrong turns a condition that should not stop a call into one that does. @@ -54,9 +54,10 @@ never to the pages of one query. An advisory about *upstream data* -- a dataset the service has stopped updating -- is a ``UserWarning``, never a ``DeprecationWarning``. Downstream projects run -their suites under ``-W error::DeprecationWarning``, and spelling "this data is -stale" as a deprecation makes their build fail over something no code change of -theirs can fix. ``DeprecationWarning`` means *this package's* API is going away. +their suites under ``-W error::DeprecationWarning``, and expressing "this data +is stale" as a deprecation makes their build fail over something no code change +of theirs can fix. ``DeprecationWarning`` means *this package's* API is being +removed. **A ``Retry-After`` hint is honored as information even when it is not actionable.** A hint too long to wait for, or expressed as a date, is parsed and @@ -65,13 +66,13 @@ service asked for; the retry policy separately declines to wait beyond its cap. A date already in the past yields no hint at all rather than a zero-second one, which would read as "retry immediately" -- the opposite of what the header said. -**Every error must survive a process boundary.** Reconstruction goes through -``__new__`` plus ``__getstate__``/``__setstate__`` rather than ``cls(*args)``, -because these errors carry fields whose values are not the constructor's -arguments. A subclass holding an unpicklable handle -- a client, a task -- must -shed it in ``__getstate__``. Without this a failure raised inside a worker -process is replaced by a pickling error on the way out, losing the diagnosis -exactly when it is hardest to reproduce. +**Every error must be reconstructible across a process boundary.** +Reconstruction goes through ``__new__`` plus ``__getstate__``/``__setstate__`` +rather than ``cls(*args)``, because these errors have fields whose values are +not the constructor's arguments. A subclass holding an unpicklable handle -- a +client, a task -- must remove it in ``__getstate__``. Without this a failure +raised inside a worker process is replaced by a pickling error as it is +returned, losing the diagnosis exactly when it is hardest to reproduce. Consequences ------------ @@ -103,8 +104,8 @@ Notes ----- The warning, hint-parsing, and pickling clauses were added after the original -decision. They record, under ADR 0000, rules the code was carrying in prose. The -skip clause records a carve-out that previously read as contradicting this +decision. They record, under ADR 0000, rules the code was stating in prose. The +skip clause records an exception that previously read as contradicting this record and :doc:`0006-service-neutral-transport`; 0006 now points here for it. The ``Status`` line was also annotated retroactively: this record assigned diff --git a/docs/source/architecture/decisions/0006-service-neutral-transport.rst b/docs/source/architecture/decisions/0006-service-neutral-transport.rst index b8d0f5c2a..53238fa99 100644 --- a/docs/source/architecture/decisions/0006-service-neutral-transport.rst +++ b/docs/source/architecture/decisions/0006-service-neutral-transport.rst @@ -6,7 +6,7 @@ Status Accepted. The clause assigning resumable ``ChunkedCall`` state to OGC is superseded by :doc:`0008-fan-out-execution`, which moves fan-out *execution* -into transport and leaves chunk *planning* in OGC. The rest stands. +into transport and leaves chunk *planning* in OGC. The rest remains in effect. Amended after acceptance under :doc:`0000-documenting-decisions`; the ``Notes`` section records every clause added or corrected. @@ -22,10 +22,10 @@ details -- Water Use previously imported its page walker and sync bridge from ``ogc.engine``. Duplicating them would allow authentication, timeout, retry, and failure behavior to drift. -"Neutral" here means neutral across the USGS services this package talks to, not -across HTTP APIs in general. The layer knows the ``API_USGS_*`` environment +"Neutral" here means neutral across the USGS services this package calls, not +across HTTP APIs in general. The layer reads the ``API_USGS_*`` environment variables and the quota header USGS returns. Claiming broader neutrality than -that invites generality no caller needs. +that would add generality no caller needs. Decision -------- @@ -64,23 +64,23 @@ interruption handles. Thin imports at previous private OGC and utility paths preserve compatibility where a consumer still uses them. A path no consumer imports is deleted rather than kept as a module that exists to satisfy its own test. Tunables are never re-exported by value: a caller can patch a copy taken -at import time without reaching the policy that reads it, so +at import time without affecting the policy that reads it, so ``transport.retry`` is the single place they are read from. Automatic retry is enabled only on active, idempotent request paths, and only -for failures a later attempt could survive -- rate limiting, server errors, and -transport failures that are not deterministic. Which server errors qualify is -per adapter: a fanned-out call re-sends any 5xx, riding out a transient -upstream failure, while a single-shot adapter re-sends only the gateway -statuses, because its service answers a *rejected query* with a 500, and -re-sending that would spend a caller's quota on a request that cannot succeed. -Both sets are narrower than ``DataRetrievalError.retryable``, deliberately: -that field tells a caller re-issuing might work, where spending someone's quota -unasked needs a stricter bar. Deprecated NWIS calls retain their compatibility -behavior. A failed pagination or fan-out operation raises rather than returning -successful siblings as an apparently complete result -- with one narrow -carve-out for a fan-out over independent items, recorded in -:doc:`0004-error-retry-resume`. +for failures that may not recur on a later attempt -- rate limiting, server +errors, and transport failures that are not deterministic. Which server errors +qualify is per adapter: a fanned-out call re-sends any 5xx, to wait out a +transient upstream failure, while a single-shot adapter re-sends only the +gateway statuses, because its service responds to a *rejected query* with a +500, and re-sending that would spend a caller's quota on a request that +cannot succeed. Both sets are narrower than ``DataRetrievalError.retryable``, +deliberately: that field tells a caller re-issuing might work, where spending +someone's quota unasked needs a stricter criterion. Deprecated NWIS calls +retain their compatibility behavior. A failed pagination or fan-out operation +raises rather than returning successful siblings as an apparently complete +result -- with one narrow exception for a fan-out over independent items, +recorded in :doc:`0004-error-retry-resume`. Two independent bounds limit retry: an attempt count and a no-progress budget measured in seconds since data last arrived. Attempts alone leave elapsed time @@ -88,17 +88,18 @@ unbounded, since each attempt may itself block until its timeout; the budget alone would cut short a slow but productive download. Receiving a page restarts the budget, and an attempt already in flight is never interrupted. -**Waiting is not the same as silence.** The budget bounds time the *service* -left the caller with nothing, so time the package chose to spend is credited -back by the measured amount: a wait the server named in ``Retry-After``, and -time a chunk spent queued behind the concurrency gate. The first retry is -exempt outright. Without these exemptions a policy that honors a server's hint -would spend its own budget obeying it, and a call would lose retries for being -throttled by settings the caller chose. Credit is never stamped into the -future -- a timestamp ahead of now would make elapsed silence negative and -silently disable the bound. Because half of that accounting is the retry -driver's, the concurrency gate is acquired *per attempt* inside the retry -driver rather than held by the caller across one. +**Time spent waiting is not time without progress.** The budget bounds time +the *service* left the caller with nothing, so time the package chose to spend +is credited back by the measured amount: a wait the server named in +``Retry-After``, and time a chunk spent waiting for the concurrency semaphore. +The first retry is exempt outright. Without these exemptions a policy that +honors a server's hint would spend its own budget obeying it, and a call would +lose retries for being throttled by settings the caller chose. A credit never +sets the reference time later than now: a timestamp ahead of now would make +the elapsed no-progress time negative and silently disable the bound. Because +half of that accounting is the retry driver's, the concurrency semaphore is +acquired *per attempt* inside the retry driver rather than held by the caller +across one. **A server-supplied next-page link is untrusted response data.** One shared policy parses it, resolves it against the request, refuses a host the caller @@ -109,12 +110,12 @@ follow such links -- OGC ``links``, the ratings STAC search, and Water Use's three, so a fourth parse outside that policy is a defect rather than a variation. -**Refusing credential-shaped keywords is the credentials leaf's job.** ADR 0009 -owns the rule that a wide ``**kwargs`` or ``**queryables`` passthrough refuses -such names; what belongs here is where the predicate lives. It is the fourth -question that leaf answers, alongside which host honors the key, whether a -destination qualifies, and how the key is withheld -- one definition, so ten -getters cannot drift into ten spellings of the same check. +**Refusing credential-shaped keywords belongs to the credentials leaf.** ADR +0009 owns the rule that a general ``**kwargs`` or ``**queryables`` passthrough +refuses such names; what belongs here is where the predicate is defined. It is +the fourth question that leaf answers, alongside which host honors the key, +whether a destination qualifies, and how the key is withheld -- one definition, +so ten getters cannot drift into ten versions of the same check. Consequences ------------ @@ -125,7 +126,7 @@ Consequences - Service-specific request and result contracts remain explicit instead of being forced into a universal adapter abstraction. - Retry can increase latency and quota consumption, so attempt counts, waits, - and total silent time remain bounded, and cancellation signals are never + and total no-progress time remain bounded, and cancellation signals are never wrapped. - Guidance the progress reporter prints is gated on the host it applies to, so a service that cannot use an API key is not told to obtain one. @@ -154,9 +155,9 @@ embedded userinfo. Notes ----- -The waiting-is-not-silence, next-page-link, and credentials-leaf clauses were -added after the original decision, consolidating under ADR 0000 the rules the -code was carrying in prose -- the budget exemptions were argued in five places +The waiting-time, next-page-link, and credentials-leaf clauses were added +after the original decision, consolidating under ADR 0000 the rules the code +was stating in prose -- the budget exemptions were argued in five places across ``transport/retry.py``, ``transport/liveness.py``, and ``transport/fanout.py``. diff --git a/docs/source/architecture/decisions/0007-adapter-facades.rst b/docs/source/architecture/decisions/0007-adapter-facades.rst index de740d011..a8b35aa6e 100644 --- a/docs/source/architecture/decisions/0007-adapter-facades.rst +++ b/docs/source/architecture/decisions/0007-adapter-facades.rst @@ -48,19 +48,19 @@ legacy ``dataretrieval.utils`` names are split across private modules by dependency and *do* report the documented path, because there the alternative is a public, documented import location pointing at a private module. -**Typed getters are the surface; exactly one generic escape hatch sits beside +**Typed getters are the surface; exactly one generic query path accompanies them.** ``cql`` is the only untyped member of the collection families, and deliberately so. The alternative in one direction -- a single generic query function replacing the typed getters -- gives up the parameter documentation and validation that are most of these getters' value. The alternative in the -other -- a ``cql=`` passthrough on every family -- multiplies the escape hatch +other -- a ``cql=`` passthrough on every family -- multiplies the generic path by the number of collections while making each family's surface partly untyped. -One hatch, named as such, keeps both properties. +One generic path, named as such, keeps both properties. **Identifier columns are parsed as text.** This one clause applies -package-wide, legacy NWIS included: it is about what an adapter hands back, not +package-wide, legacy NWIS included: it is about what an adapter returns, not how it is organized. HUCs, parameter codes, FIPS codes, and monitoring-location -identifiers (``site_no`` in NWIS) carry significant leading zeros, and a bare +identifiers (``site_no`` in NWIS) have significant leading zeros, and a bare ``read_csv`` infers them as integers and drops those zeros -- ``"00060"`` becomes ``60``, so the value is silently wrong rather than missing. Every adapter reading a USGS tabular response names its @@ -76,7 +76,7 @@ contracts. Consequences ------------ -- Collection changes touch fewer implementation and test files. +- Collection changes affect fewer implementation and test files. - Existing package and ``waterdata.api`` import paths remain stable. - Explicit exports make accidental public-surface growth reviewable. - More modules mean a facade to maintain, plus executable signature and export @@ -91,17 +91,17 @@ Compliance facade identity, and compatibility names. ``tests/architecture_test.py`` requires a logic-free facade, exact active-service exports, and separate OGC request construction and schema execution. ``.importlinter`` keeps the -collection families independent of each other, holds the facade-only consumers -(NGWMN and ``waterdata.cql``) to the OGC facade, and prevents one adapter from -importing another. The identifier-column rule is covered by +collection families independent of each other, restricts the facade-only +consumers (NGWMN and ``waterdata.cql``) to the OGC facade, and prevents one +adapter from importing another. The identifier-column rule is covered by ``tests/nwdc_test.py::test_huc12_id_kept_as_string_with_leading_zero`` and the equivalent leading-zero assertions in the WQP and NWIS adapter tests. Notes ----- -The ``__module__`` scoping note and the escape-hatch and identifier-column +The ``__module__`` scoping note and the generic-path and identifier-column clauses were added after the original decision; the rest of the record is -unchanged. They consolidate under ADR 0000 the rules the code was carrying in +unchanged. They consolidate under ADR 0000 the rules the code was stating in prose. The scoping note in particular records why ``_querying.py`` reassigning ``__module__`` is not a violation of this record, a question an audit raised. diff --git a/docs/source/architecture/decisions/0008-fan-out-execution.rst b/docs/source/architecture/decisions/0008-fan-out-execution.rst index 82da7044a..2ff3dc38b 100644 --- a/docs/source/architecture/decisions/0008-fan-out-execution.rst +++ b/docs/source/architecture/decisions/0008-fan-out-execution.rst @@ -6,7 +6,7 @@ Status Accepted. Supersedes the clause of :doc:`0006-service-neutral-transport` assigning "resumable ``ChunkedCall`` state" to OGC's protocol concerns; the rest -of ADR 0006 stands. +of ADR 0006 remains in effect. Amended after acceptance under :doc:`0000-documenting-decisions`; the ``Notes`` section records every clause added or corrected. @@ -26,11 +26,11 @@ The two are independent, and only the first is protocol knowledge: dividing a query needs the byte budget, the CQL2 grammar, and which parameters are list-valued, while distributing the pieces needs none of it. -The package had not drawn that line. ``ChunkPlan`` (division) and -``ChunkedCall`` (distribution) sat side by side in ``dataretrieval.ogc`` as +The package had not made that distinction. ``ChunkPlan`` (division) and +``ChunkedCall`` (distribution) were both defined in ``dataretrieval.ogc`` as siblings, and ADR 0006 grouped them together deliberately. That grouping was correct while a byte plan was the only thing anyone fanned out over. It stopped -being correct once Water Use fanned out too: unable to reach an OGC-internal +being correct once Water Use fanned out too: unable to import an OGC-internal executor, ``wateruse._fan_out`` re-implemented the semaphore, the ``asyncio.gather``, and the cancellation-beats-HTTP-error failure precedence, with a comment naming ``ChunkedCall._run`` as the original. One rule, two @@ -38,7 +38,7 @@ copies, kept in agreement by that comment. The duplicate was not merely redundant. It lacked resume, so a rate limit partway through discarded every location that had already succeeded -- against -an hourly quota, on fan-outs that reach into the hundreds. It reported no +an hourly quota, on fan-outs of hundreds of locations. It reported no progress. And it read its own module-global concurrency cap, so a user setting ``API_USGS_CONCURRENT`` to lower the request rate found one adapter ignoring them. @@ -56,20 +56,21 @@ optionally a ``finalize`` hook applied to the combined frame. injected into the executor rather than applied at the call site because a resume re-enters the *executor*, never the getter that started the call. Shaping done after the getter returns would run on the first attempt and be skipped on the -resumed one, so the same query would answer differently depending on whether it -was interrupted. Anything that must be true of the returned frame belongs in -``finalize``. +resumed one, so the same query would return different results depending on +whether it was interrupted. Anything that must be true of the returned frame +belongs in ``finalize``. **The concurrency bound is an ``asyncio.Semaphore``, not the connection pool.** The pool is sized to match the semaphore rather than used as the throttle: a -pool smaller than the fan-out would queue chunks inside ``httpx`` and surface as -``PoolTimeout``, which the taxonomy reads as a transient failure and reports as -a resumable interruption -- a spurious one, caused entirely by the package's own -settings rather than by the service. One throttle, and the pool follows it. +pool smaller than the fan-out would queue chunks inside ``httpx`` and appear as +``PoolTimeout``, which the taxonomy classifies as a transient failure and +reports as a resumable interruption -- a spurious one, caused entirely by the +package's own settings rather than by the service. There is one throttle, and +the pool is sized to it. **A CQL2-JSON filter is passed through, never divided.** The planner does not chunk a ``cql-json`` filter and does not size-check its body; an over-budget -body is the server's judgement to render. Splitting a filter expression means +body is for the server to accept or reject. Splitting a filter expression means understanding its semantics well enough to guarantee the union of the parts equals the whole, which is a different undertaking from splitting a list of identifiers along a comma. @@ -77,14 +78,14 @@ identifiers along a comma. ``FanOutPlan`` is a ``Protocol`` of ``__len__`` and ``__iter__``, generic in the item type -- a sized, iterable collection of chunk descriptions, and nothing more. The executor passes each item to the adapter's own ``fetch`` -without inspecting it, so the item type is the adapter's business: the OGC +without inspecting it, so the item type is the adapter's concern: the OGC getters yield kwargs dicts, Water Use yields ready ``httpx.Request`` objects. The standard protocols, rather than custom members, are a deliberate choice. A plan declaring ``total`` and ``iter_chunk_args()`` would be stating ``len`` twice under a private name: the two could then report different counts, and a test would have to assert they agree. Every adapter whose chunks are -already a list would also need a wrapper class whose only job is renaming +already a list would also need a wrapper class whose only purpose is renaming ``len``. With the standard names a plain ``list`` is a plan, which is exactly what Water @@ -98,13 +99,13 @@ chunks from a byte budget over multi-value axes, a list of requests derives nothing, and so there is no shared implementation an abstract base could hold. The identity of the query as a whole is *not* part of the plan. ``canonical_url`` -is a value stamped on the combined response, not a property of how the work +is a value set on the combined response, not a property of how the work divides, so it is an argument to ``FanOut``. ``ChunkPlan`` computes one while planning and the OGC call site passes it through; Water Use passes its first location's URL, since the service has no request expressing "all of these". ``dataretrieval.ogc`` keeps chunk planning: the byte budget, the axis -partitioning, the CQL2 filter split, the ``parallel_chunks`` dial. Those are +partitioning, the CQL2 filter split, the ``parallel_chunks`` setting. Those are division, and division is protocol-specific. The interruption taxonomy moves to ``dataretrieval.interruptions``, a top-level @@ -124,14 +125,14 @@ Concurrency is one general setting with per-adapter defaults. a different default for when it is unset. The precedence is deliberate: an explicitly set environment variable outranks an adapter default, never the reverse. An adapter that could override the general setting would make -``API_USGS_CONCURRENT=1`` a lie. An adapter default applies only when the -variable is unset; it never displaces a value the caller set. +``API_USGS_CONCURRENT=1`` untrue. An adapter default applies only when the +variable is unset; it never overrides a value the caller set. Consequences ------------ - Water Use gains resume, progress reporting, and the shared concurrency - setting, and sheds roughly 75 lines of duplicated orchestration. + setting, and removes roughly 75 lines of duplicated orchestration. - One implementation of failure precedence, so cancellation-beats-error and deterministic failure ordering cannot drift between services. - **Breaking:** a Water Use fan-out interrupted by a 5xx, 429, or recoverable @@ -146,11 +147,11 @@ Consequences - Resume re-issues a failed location's entire page walk, so pages fetched before the failure are fetched again. This already applied to OGC -- a partial walk never enters the completion map -- and is a cost, not a correctness problem. -- Water Use frames carry ``huc12_id``, not ``id``, so ``_combine_chunk_frames`` +- Water Use frames have ``huc12_id``, not ``id``, so ``_combine_chunk_frames`` concatenates them without deduplicating. Correct, because locations partition - by construction, but the executor's dedup safety net does not apply there. -- ``transport`` is no longer purely leaf-shaped: ``fanout`` is a composite that - drives retry, publishes the client pagination borrows, and calls + by construction, but the executor's deduplication does not apply there. +- ``transport`` is no longer a pure leaf: ``fanout`` is a composite that + runs the retry loop, provides the client that pagination uses, and calls ``combining``. It remains HTTP execution policy, which is the test the package applies. @@ -178,7 +179,7 @@ Notes ----- The ``finalize``, semaphore, and CQL2-JSON clauses were added after the original -decision, consolidating under ADR 0000 rules the code was carrying in prose -- +decision, consolidating under ADR 0000 rules the code was stating in prose -- the semaphore rule was stated four times in ``transport/fanout.py`` alone, and that file now states it once and cites this record. diff --git a/docs/source/architecture/decisions/0009-layered-configuration.rst b/docs/source/architecture/decisions/0009-layered-configuration.rst index e5bce3902..eb466c108 100644 --- a/docs/source/architecture/decisions/0009-layered-configuration.rst +++ b/docs/source/architecture/decisions/0009-layered-configuration.rst @@ -17,7 +17,7 @@ every service accepts the same settings -- is false. put the setting in one. A profile is now named under the adapter it configures (``[.]``); the global table and ``DATARETRIEVAL_PROFILE`` are retired, since a table that switched every - service at once could not carry per-service detail. + service at once could not express per-service detail. - **"The environment ranks above the file"**, inverted for -- and only for -- a profile selected in code. Everything the caller did not name in code still follows the rule as written here. @@ -29,7 +29,7 @@ every service accepts the same settings -- is false. had already narrowed the objection to a payload-shape preference. The chain itself, the ``ContextVar`` delivery, host-scoped credentials, and the -leaf constraint stand. +leaf constraint remain in effect. Amended after acceptance under :doc:`0000-documenting-decisions`; the ``Notes`` section records every clause added or corrected. @@ -53,8 +53,8 @@ normally go, and it is unsafe here. Every Water Data getter ends in ``_get_args(locals())`` with a ``**queryables`` catch-all that forwards unrecognized keywords to the API as query parameters. A credential parameter missed in one of ~20 signatures would be serialized into a URL. The maintainers -object to an ``api_key=`` parameter on a second ground: it invites keys pasted -into shared scripts. +object to an ``api_key=`` parameter on a second ground: it encourages keys +pasted into shared scripts. Decision -------- @@ -89,18 +89,18 @@ Supporting decisions: ``configure()`` argument inherits from lower sources. Explicit ``None`` is a scoped reset to built-in behavior, so a server can guarantee an anonymous call rather than accidentally falling through to its process credential. -- **No public getter grows a credential parameter.** ``configure`` is the only +- **No public getter gains a credential parameter.** ``configure`` is the only programmatic path, and a fitness function asserts no getter accepts ``api_key`` / ``session`` / ``token``. The generic ``**queryables`` path also refuses credential-shaped names before request construction so they cannot - enter a URL. That refusal covers names carrying a secret; ``session`` is + enter a URL. That refusal covers names that hold a secret; ``session`` is deliberately not among them (see Notes). - **The module owns each setting's parser.** ``unbounded``, bounds, and - rejection messages live in one place. ``tomllib`` returns typed scalars, so - the file and Python API validate source-level types before normalized values - pass through the shared parsers. Legacy environment-only forms, including a - blank numeric value and an arbitrary non-empty progress value, remain - compatible without making the new surfaces equally permissive. + rejection messages are defined in one place. ``tomllib`` returns typed + scalars, so the file and Python API validate source-level types before + normalized values pass through the shared parsers. Legacy environment-only + forms, including a blank numeric value and an arbitrary non-empty progress + value, remain compatible without making the new surfaces equally permissive. - **Each setting's policy is a row in a named table, never a branch in shared code.** Type, bounds, and parser are declared as data, guarded at import time for completeness, so adding a setting cannot silently inherit whatever the @@ -113,21 +113,21 @@ Supporting decisions: key the version *does* recognize, placed in a table that cannot use it, raises: that is a mistake the caller can fix, and ignoring it silently would leave the user believing a setting is in effect when it is not. -- **Credential-shaped keyword refusal is a usability guardrail, not a security +- **Credential-shaped keyword refusal is a usability check, not a security control.** Names are matched as substrings after separators are stripped, and the check errs toward rejecting. It never inspects values, so it stops a caller who mistyped a credential into a query filter -- it does not stop - anyone determined to send one. Naming it a security control would invite - reliance it cannot carry. -- **The key travels only over https, to the one authorized host.** The scheme is + anyone determined to send one. Naming it a security control would encourage + reliance it does not justify. +- **The key is sent only over https, to the one authorized host.** The scheme is matched as well as the host, because redirects and server-supplied next-page - links are attacker-influenced data and a downgrade to http would put the - credential on the wire in clear text. Userinfo on a handed-in URL is stripped - before the request is built, so ``httpx`` cannot build an ``Authorization`` - header nobody configured. This states the predicate ADR 0006 defers to the - credentials leaf. + links are attacker-influenced data and a downgrade to http would send the + credential in clear text. Userinfo on a caller-supplied URL is + stripped before the request is built, so ``httpx`` cannot build an + ``Authorization`` header nobody configured. This states the predicate ADR + 0006 defers to the credentials leaf. - **TOML, read with** ``tomllib``. Stdlib from Python 3.11; the ``tomli`` - backport is declared under an environment marker and disappears when + backport is declared under an environment marker and is dropped when ``requires-python`` moves to ``>=3.11``. YAML was rejected because PyYAML is a dependency at every Python version and the settings are flat. - **Not every setting gets an environment variable.** ``parallel_chunks`` @@ -140,8 +140,9 @@ Supporting decisions: setting from leaking into unrelated calls. - **Names distinguish execution capacity from planning granularity.** ``concurrency`` names the maximum chunks in flight and maps to the existing - ``API_USGS_CONCURRENT`` variable. ``parallel_chunks`` asks the planner for - optional extra chunks; it does not promise that many execute simultaneously. + ``API_USGS_CONCURRENT`` variable. ``parallel_chunks`` requests optional + extra chunks from the planner; it does not promise that many execute + simultaneously. The name is retained because the context manager is already public. ``parallelism`` and ``chunk_parallelism`` were rejected because they would conflate this planning hint with ``concurrency``. @@ -177,7 +178,7 @@ Supporting decisions: ``RetryPolicy.from_configuration()`` because WQP and StreamStats report a rejected query as a 500. A value resolved from the chain always outranks a caller default -- a service able to override an explicit setting would make - ``concurrency=1`` a lie. + ``concurrency=1`` untrue. - **Per-service overrides are deferred, not refused.** One ``configure()`` block cannot currently set one value for Water Use and another for Water Data. @@ -186,8 +187,8 @@ Supporting decisions: inside this chain -- a ``[wateruse]`` table beside the top-level keys, read as ``configuration.concurrency(default, service=...)``. It costs a second dimension in resolution, which ``show_configuration()`` must then render as a - matrix rather than a list, and that cost should buy a requirement before it is - paid. + matrix rather than a list, and that cost should be paid only when a + requirement exists. - **A configuration object would have no way to reach the call.** The public surface is free functions -- ``waterdata.get_daily(...)``, not a client with @@ -202,7 +203,7 @@ Supporting decisions: Consequences ------------ -- A credential can be supplied per thread or per task without touching +- A credential can be supplied per thread or per task without modifying ``os.environ``, which is what issue #352 asked for. - Host scoping is unchanged and unconditional: a key from any source is sent only to ``api.waterdata.usgs.gov`` and is stripped on cross-host redirects. @@ -230,17 +231,17 @@ thread and asyncio isolation, host scoping for file-sourced keys, redaction in Notes ----- -The setting-table, forward-compatibility, guardrail-scoping, and credential- -egress clauses were added after the original decision, consolidating under ADR -0000 rules that the code was carrying in prose. None changes behavior. +The setting-table, forward-compatibility, refusal-scoping, and +credential-egress clauses were added after the original decision, consolidating +under ADR 0000 rules that the code was stating in prose. None changes behavior. The "Not every setting gets an environment variable" bullet was also extended in place: it deferred its argument to a ``parallel_chunks`` docstring, and that -argument now sits in the bullet itself, because the docstring it pointed at was +argument is now in the bullet itself, because the docstring it pointed at was the prose being consolidated. The ``**queryables`` clause above originally named ``session`` among the -rejected spellings. It was corrected after the fact: ``session`` carries no +rejected names. It was corrected after the fact: ``session`` holds no secret, so refusing it with a credentials message told callers the wrong thing, and as a substring it claimed part of a namespace the *server* owns -- any future query parameter containing it would have been unreachable behind that diff --git a/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst b/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst index 8f1df66b6..af9fc1b18 100644 --- a/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst +++ b/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst @@ -6,17 +6,18 @@ Status Accepted, except for two clauses. Supersedes the "One flat set of setting names" and "Per-service overrides are deferred" clauses of -:doc:`0009-layered-configuration`; the rest of ADR 0009 stands, subject to what -ADR 0011 supersedes there. +:doc:`0009-layered-configuration`; the rest of ADR 0009 remains in effect, +subject to what ADR 0011 supersedes there. :doc:`0011-configuration-profiles` supersedes decisions 5 and 8 below -- adapter schemas held centrally as ``TypedDict``, and each adapter a named keyword on ``configure()``. Each adapter now declares a ``BaseConfiguration`` subclass in the module that *reads* those settings, and ``configure()`` takes instances of them positionally, which is what removes the adapter roster from -the call site. The spelling shown in decision 1 goes with decision 8. Decisions -2, 3, 4, 6 and 7 stand: the tiers, source-major precedence, package-wide -environment variables, the host-scoped key, and the adapter names. +the call site. The ``configure()`` form shown in decision 1 is superseded with +decision 8. Decisions 2, 3, 4, 6 and 7 remain in effect: the tiers, +source-major precedence, package-wide environment variables, the host-scoped +key, and the adapter names. Context ------- @@ -75,7 +76,7 @@ single-shot adapters -- there is nothing to fan out. ``ssl_check`` applies to four adapters (``waterdata``, ``nwdc``, ``nwis``, ``wqp``) and is currently a per-call keyword outside the chain entirely; it reaches ``httpx``'s ``verify``, verified by spying on the client. A flat namespace accepts -``configure(streamstats={"parallel_chunks": 8})`` without complaint, which is +``configure(streamstats={"parallel_chunks": 8})`` without error, which is the typo class ADR 0009 exists to catch. The credential is a separate axis, and measurement settled it. Probing the live @@ -117,17 +118,17 @@ Settings are scoped to the **adapter**, not the service, and not the host. .. note:: - The file table stands; the ``configure()`` spelling above is superseded - by :doc:`0011-configuration-profiles` along with decision 8. One block - still configures several adapters at once, now as + The file table remains in effect; the ``configure()`` form above is + superseded by :doc:`0011-configuration-profiles` along with decision 8. + One block still configures several adapters at once, now as ``configure(NgwmnConfiguration(concurrency=4), WqpConfiguration(retries=2))``. -2. **The top-level tier survives.** An adapter table *overrides* it per key; it - does not replace it. Every setting still has a package-wide spelling, and +2. **The top-level tier remains.** An adapter table *overrides* it per key; it + does not replace it. Every setting still has a package-wide form, and the shipped ``API_USGS_*`` variables are package-wide by construction. ``retries`` and ``stall_timeout`` are additionally adapter-scopable, because - a service that answers slowly or refuses often warrants its own budget + a service that responds slowly or refuses often warrants its own budget without changing anyone else's. ``progress`` is not: it describes the caller's terminal, and there is one progress line per call, so scoping it per adapter could only produce a contradiction. @@ -135,13 +136,14 @@ Settings are scoped to the **adapter**, not the service, and not the host. 3. **Precedence stays source-major.** Resolution walks block, then environment, then file, as ADR 0009 defines; *within* each source an adapter-scoped value outranks a top-level one. The environment therefore still outranks the file, - so a stale adapter table cannot quietly beat a variable exported for one run. + so a stale adapter table cannot quietly override a variable exported for one + run. 4. **Adapter-scoped settings get no environment variables.** Every entry in ``ENV_VARS`` stays package-wide, for the reason ``parallel_chunks`` already has none: an exported variable is inherited by every subprocess and invisible at the call site. Six adapters times four settings would be a - namespace nobody could hold in mind. + namespace nobody could remember. 5. **Each adapter's schema is a** ``TypedDict``. Its ``__annotations__`` *are* the schema -- there is no second table to maintain, ``mypy --strict`` checks @@ -151,9 +153,9 @@ Settings are scoped to the **adapter**, not the service, and not the host. *Superseded by* :doc:`0011-configuration-profiles`. The schema is now a frozen dataclass owned by the adapter, for the same "the annotations are the - schema" reason -- what changed is where it lives. A ``TypedDict`` had to be - declared centrally to annotate a central keyword, which put a Water Data - setting's definition in a module that knows nothing about Water Data. + schema" reason -- what changed is where it is defined. A ``TypedDict`` had + to be declared centrally to annotate a central keyword, which put a Water + Data setting's definition in a module unrelated to Water Data. 6. **The API key stays host-scoped and is not an adapter setting.** ``credentials`` keeps sole ownership of which host honors the key. There is @@ -173,18 +175,18 @@ Settings are scoped to the **adapter**, not the service, and not the host. *Superseded by* :doc:`0011-configuration-profiles`. ``configure()`` takes configuration objects positionally instead, so the adapter is named by the - class rather than by a keyword. The type checking survives -- a setting an + class rather than by a keyword. The type checking remains -- a setting an adapter does not read is not a field of its class -- and the catch-all is no longer needed for a misspelling, because ``WaterdataConfiguration(concurrancy=8)`` is already a ``TypeError`` naming - the keyword that does not exist. What the change buys is that ``configure()`` - no longer enumerates the adapters at all: that enumeration was the roster - this ADR left spelled in four places. + the keyword that does not exist. What the change achieves is that + ``configure()`` no longer enumerates the adapters at all: that enumeration + was the roster this ADR left listed in four places. Consequences ------------ -- **A caller can be gentle with one adapter without throttling the rest** -- +- **A caller can throttle one adapter without throttling the rest** -- the requirement ADR 0009 deferred. Because NGWMN and Water Data share a quota pool, throttling NGWMN now measurably preserves quota for Water Data. @@ -192,9 +194,9 @@ Consequences a hand-maintained table removes the failure mode where a new adapter setting is added and the validation table is not, and over a dataclass per adapter it keeps the payload a plain mapping, so the file and block paths share one - validator and ``configuration`` grows no runtime classes. *Superseded with - decision 5*: the classes exist, and live with their adapters rather than in - the leaf. + validator and ``configuration`` adds no runtime classes. *Superseded with + decision 5*: the classes exist, and are defined with their adapters rather + than in the leaf. - **A configuration object is still refused, but on narrower grounds than ADR 0009 stated.** That ADR rejected an object because it had no way to *reach* @@ -205,25 +207,25 @@ Consequences *Withdrawn by* :doc:`0011-configuration-profiles`, which took the remaining step. Narrowing the objection to a payload-shape preference is what left it - open, and a dataclass turned out to buy the thing a mapping could not: an - instance knows which adapter it targets, so the caller stops naming one and - the roster stops being duplicated. + open, and a dataclass turned out to provide what a mapping could not: an + instance records which adapter it targets, so the caller stops naming one + and the roster stops being duplicated. -- **``show_configuration()`` grows a second section, not a matrix.** It prints +- **``show_configuration()`` gains a second section, not a matrix.** It prints the top-level tier as today, then only those adapter overrides actually set. - A seven-by-eight grid of mostly-inherited values would bury the answer to + A seven-by-eight grid of mostly-inherited values would obscure the answer to "what will this call use". - **The shared quota pool is not modelled.** ``[waterdata]`` and ``[ngwmn]`` - read as independent dials but draw on one 1000/hour allowance. A host or + appear to be independent settings but share one 1000/hour allowance. A host or gateway tier would express it; that is deferred until someone is confused by it, since the pool is a property of the credential, which is already host-scoped. -- **``stall_timeout`` joins the chain.** ``API_USGS_STALL_TIMEOUT`` was read - directly from ``os.environ``, so it could not be set by a block or the file - and never appeared in ``show_configuration()`` -- a gap in ADR 0009's own - claim that every setting resolves through one chain. It is package-wide by +- **``stall_timeout`` is added to the chain.** ``API_USGS_STALL_TIMEOUT`` was + read directly from ``os.environ``, so it could not be set by a block or the + file and never appeared in ``show_configuration()`` -- a gap in ADR 0009's + own claim that every setting resolves through one chain. It is package-wide by default and adapter-scopable. ``dataretrieval/transport/env.py`` existed only to parse it and is deleted, so ``configuration`` is now the only module in the package that reads ``os.environ`` for a setting. @@ -248,8 +250,8 @@ Consequences ``SSL_CERT_FILE`` and ``SSL_CERT_DIR`` on both its sync and async clients -- so that mechanism already covers *every* getter, including the OGC ones that have no ``ssl_check``, and it trusts the corporate CA rather than trusting - nothing. The ``bool`` type cannot even carry a CA bundle path, which is the - value a caller actually wants. + nothing. The ``bool`` type cannot even hold a CA bundle path, which is the + value a caller actually needs. The configuration guide documents ``SSL_CERT_FILE`` for that case. Whether ``ssl_check`` should be deprecated outright is a public-API question left to @@ -281,6 +283,6 @@ Notes ``ContextVar`` delivery, and the leaf constraint are unchanged. - Live-API measurements behind the credential decision were taken 2026-08-11 against ``api.waterdata.usgs.gov`` and ``api.water.usgs.gov``. -- The ``wateruse`` module is renamed ``nwdc`` under separate cover; the service +- The ``wateruse`` module is renamed ``nwdc`` in a separate change; the service names itself "National Water Availability Assessment Data Companion" and serves ten models, only five of which are water use. diff --git a/docs/source/architecture/decisions/0011-configuration-profiles.rst b/docs/source/architecture/decisions/0011-configuration-profiles.rst index 49181f5d3..f10607f54 100644 --- a/docs/source/architecture/decisions/0011-configuration-profiles.rst +++ b/docs/source/architecture/decisions/0011-configuration-profiles.rst @@ -11,7 +11,7 @@ decision 5 (adapter schemas held centrally as ``TypedDict``) and decision 8 environment-above-file rule, inverted for a profile selected in code; and the refusal of a configuration object, which ADR 0010 had already narrowed to a preference about the payload's shape. The chain, the ``ContextVar`` delivery, -host-scoped credentials and the leaf constraint stand. +host-scoped credentials and the leaf constraint remain in effect. Amended after acceptance under :doc:`0000-documenting-decisions`; the ``Notes`` section records every clause added or corrected. @@ -19,7 +19,7 @@ Amended after acceptance under :doc:`0000-documenting-decisions`; the Context ------- -ADR 0010 gave each adapter its own slice of the chain, so ``[ngwmn]`` narrows a +ADR 0010 gave each adapter its own table in the chain, so ``[ngwmn]`` narrows a setting to NGWMN. That covers "tune one service" but not the case a multi-service caller actually has: @@ -28,18 +28,18 @@ multi-service caller actually has: store both. The only named construct is ``[profiles.]``, which switches *every* service at once. - **Composing them.** The two mechanisms do not compose: - ``[profiles.bulk.ngwmn]`` raises, so a profile cannot carry per-service + ``[profiles.bulk.ngwmn]`` raises, so a profile cannot hold per-service detail. That refusal was recorded in ADR 0010 on the grounds that layering them needed a fourth precedence rule nobody had asked for. Someone has now asked for it, and it is the primary use case. -Two further problems ADR 0010 left open feed into the same decision. The -adapter roster is spelled in four places, only one of which is derived -- +Two further problems ADR 0010 left open bear on the same decision. The +adapter roster is listed in four places, only one of which is derived -- adding an adapter needs coordinated edits, and forgetting one leaves a schema no call site can reach, which happened to three adapters and shipped -undetected until a fitness test was written. And a setting's definition lives +undetected until a fitness test was written. And a setting's definition is in ``config`` rather than in the module that reads it, so adding a Water Data -setting edits a file that knows nothing about Water Data. +setting edits a file unrelated to Water Data. Decision -------- @@ -82,18 +82,18 @@ This is the most-typed line the feature exists to enable, and making it wordier is a real cost, accepted deliberately so that every setting is passed the same way. -**Schemas live with their adapter; names live centrally.** ``configuration`` -is a standard-library-only leaf every adapter may import, so it cannot import -adapters. It holds the tuple of adapter *names*, which is what parsing a file -needs (is ``[ngwmn]`` a table or a typo?). Each adapter package owns its -subclass, which is what a setting's definition needs to be local to the -service that reads it. +**Schemas are defined in their adapter; names are defined centrally.** +``configuration`` is a standard-library-only leaf every adapter may import, so +it cannot import adapters. It holds the tuple of adapter *names*, which is what +parsing a file needs (is ``[ngwmn]`` a table or a typo?). Each adapter package +owns its subclass, which is what a setting's definition needs to be local to +the service that reads it. -Registration at import alone would not do: ``dataretrieval`` imports six of +Registration at import alone would not suffice: ``dataretrieval`` imports six of seven adapters eagerly, but NLDI is deliberately on demand for the geopandas extra, so a registry built from imports would reject a valid ``[nldi]`` table until something imported it, and the report would vary by what a caller had -touched. +imported. **Precedence**, highest first: @@ -118,12 +118,12 @@ original rule. **Validation is lazy.** A file's structure is checked when it is parsed; a table's keys are checked when that adapter first resolves a setting. This -keeps the blast-radius rule ADR 0010 established -- a malformed ``[nldi]`` +keeps the isolation rule ADR 0010 established -- a malformed ``[nldi]`` table must not fail a Water Data call -- and it is what allows the schema to -live in a module the parser cannot import. +be defined in a module the parser cannot import. **Base URLs may be configured, from code only.** An adapter's configuration -may carry its base URL, settable in a ``configure()`` block and rejected from +may include its base URL, settable in a ``configure()`` block and rejected from the file and the environment. A file that silently redirects a data-retrieval library to another host is a supply-chain hazard; an in-code block keeps the redirect where a reader sees it. @@ -164,13 +164,13 @@ gain nothing and would turn a stale key into 403s on calls that work anonymously today. The three hosts also keep independent counters, so ADR 0010's "one key, one quota pool" is true of waterdata and ngwmn only. -**An adapter composes shared setting groups; it does not respell their +**An adapter composes shared setting groups; it does not redeclare their fields.** Which settings an adapter reads is the adapter's own knowledge, but what each setting *means* is shared, so the fields come from frozen mixin groups declared once beside their grammar. An adapter's configuration class -names the groups it composes and adds only what is genuinely its own. Spelling +names the groups it composes and adds only what is genuinely its own. Declaring ``retries: int | None = _UNSET`` directly in an adapter module satisfies this -record's letter while losing what it protects: the annotation would enforce +record literally while losing what it protects: the annotation would enforce nothing, could drift from the shared parser, and ``mypy --strict`` would not notice, because it checks the annotation, not whether the field still matches the shared group. @@ -178,7 +178,7 @@ the shared group. Consequences ------------ -- **The multi-service case gets a spelling.** One block, several adapters, at +- **The multi-service case can be expressed.** One block, several adapters, at most one configuration each, any of them from the file or from code. - **The roster stops being duplicated.** An adapter declares itself once. The failure mode where a schema exists that nothing passes becomes impossible by @@ -220,10 +220,10 @@ Satisfied. In ``tests/configuration_test.py``: rung of the seven-rung ladder above, each written against one file that populates every rung with a distinct value. - ``test_inner_block_can_lower_a_setting_an_outer_block_scoped`` -- the - innermost block wins, including over an adapter-scoped outer one. + innermost block takes precedence, including over an adapter-scoped outer one. - ``test_a_table_for_an_unimported_adapter_stays_valid`` and ``test_a_malformed_table_does_not_fail_another_adapters_call`` -- the - blast-radius rule under lazy validation. + isolation rule under lazy validation. - ``test_base_url_applies_from_code_and_is_refused_from_the_file``, with ``test_base_url_is_refused_from_the_environment`` for the other source, and ``test_a_code_base_url_redirects_every_water_data_endpoint_family`` -- one @@ -235,8 +235,8 @@ Satisfied. In ``tests/configuration_test.py``: ``test_every_adapter_is_actually_wired_to_a_read_site`` -- the roster resolves, and no configuration exists that nothing reads. An adapter name the code does not recognize now raises out of ``_resolve`` rather than - falling through to the package-wide value, so the grep is a backstop rather - than the only guard. + falling through to the package-wide value, so the grep is a secondary check + rather than the only one. ``tests/architecture_test.py::test_config_is_a_standard_library_only_leaf`` asserts the module imports no adapter -- ``dataretrieval.exceptions`` is its @@ -251,9 +251,9 @@ Notes - Open, not decided here: whether ``parallel_chunks`` is renamed. ``fan_out`` was suggested and conflicts with the glossary, where fan-out is *executing* chunks concurrently -- which ``concurrency`` already governs -- while - ``parallel_chunks`` asks the planner to *divide* more finely. ADR 0009 + ``parallel_chunks`` instructs the planner to *divide* more finely. ADR 0009 rejected ``parallelism`` and ``chunk_parallelism`` for the same conflation. ``chunk_count`` or ``target_chunks`` would stay on the correct side of it. - The setting-group clause was added after the original decision, consolidating - under ADR 0000 a rule the configuration core was carrying in prose. It does + under ADR 0000 a rule the configuration core was stating in prose. It does not change behavior. diff --git a/docs/source/architecture/decisions/0012-deprecation-horizons.rst b/docs/source/architecture/decisions/0012-deprecation-horizons.rst index b280d2dbb..9bb69a4b1 100644 --- a/docs/source/architecture/decisions/0012-deprecation-horizons.rst +++ b/docs/source/architecture/decisions/0012-deprecation-horizons.rst @@ -11,11 +11,11 @@ Context This package's value is that established calls keep working. Public API compatibility is its first architecture characteristic after artifact integrity. -Names therefore leave slowly: a renamed argument, a retired module, a getter -whose service no longer exists. +Names are therefore removed slowly: a renamed argument, a retired module, a +getter whose service no longer exists. -Four spellings of "tell the caller something is going away" grew up -independently, and only one carried a date. A caller could not tell how long +Four mechanisms for telling the caller something is being removed were written +independently, and only one included a date. A caller could not tell how long they had, a maintainer could not audit what was due, and the warning category was a per-author choice -- which matters, because downstream projects run their suites under ``-W error::DeprecationWarning``. @@ -30,39 +30,39 @@ Every deprecation is announced through the shared mechanism in ``dataretrieval._deprecation``, and every one has a published removal horizon recorded in ``REMOVALS``. -A deprecation advisory names three things: what is going away, what to use +A deprecation advisory names three things: what is being removed, what to use instead, and the date on or after which it may be removed. The mechanism tolerates an advisory with no date -- it then promises nothing specific rather than implying a schedule it does not have. A deprecation of a public name is -expected to carry one, and an advisory naming a replacement the caller cannot +expected to include one, and an advisory naming a replacement the caller cannot yet use is not finished. ``REMOVALS`` is the single table of horizons. One table is auditable -- what is due can be listed, and a horizon can be extended in one place -- whereas four -hand-rolled shims could only be found by grep. A renamed public argument keeps +hand-written shims could only be found by grep. A renamed public argument keeps working under its old name through one shared decorator rather than a shim written for each getter. -The *warning category* an advisory carries is not the author's choice, but the +The *warning category* an advisory uses is not the author's choice, but the rule setting it is not this record's. :doc:`0004-error-retry-resume` decides -when an advisory is a ``DeprecationWarning`` (a name in this package is going -away) and when it is a ``DataCurrencyWarning`` (an upstream dataset has stopped -being updated). This record governs the mechanism and the horizon. +when an advisory is a ``DeprecationWarning`` (a name in this package is being +removed) and when it is a ``DataCurrencyWarning`` (an upstream dataset has +stopped being updated). This record governs the mechanism and the horizon. -A horizon is a floor, not a schedule. Passing it permits removal; it does not +A horizon is a minimum, not a schedule. Passing it permits removal; it does not require one, and removal remains a deliberate change with its own release note. Consequences ------------ -- A caller can see, from the warning alone, how long they have and what to move - to. +- A caller can see, from the warning alone, how long they have and what to + migrate to. - Horizons can be audited and extended centrally, so a removal date cannot arrive unnoticed in a module nobody is reading. - Deprecating something costs more than adding a ``warnings.warn`` call: the replacement must exist and a date must be chosen. That is the intended cost. - The package accumulates long-lived compatibility shims. This is accepted -- - it is what the compatibility characteristic buys, and the table makes the + it is the cost of the compatibility characteristic, and the table makes the accumulation visible rather than hidden. - Nothing is removed on the horizon alone. A removal still needs a release that says so. diff --git a/docs/source/architecture/decisions/0013-core-and-domain-terms.rst b/docs/source/architecture/decisions/0013-core-and-domain-terms.rst index 8e5d1158a..5a8471342 100644 --- a/docs/source/architecture/decisions/0013-core-and-domain-terms.rst +++ b/docs/source/architecture/decisions/0013-core-and-domain-terms.rst @@ -24,7 +24,7 @@ The two sets behave differently because their authority differs. Terms like *chunk*, *page*, *fan-out*, *plan*, *interruption*, *dialect* and *leaf* appear nowhere in any USGS API's vocabulary. They were invented here to -describe machinery this package owns. Nothing external constrains them, so when +describe mechanisms this package owns. Nothing external constrains them, so when the package spells one of them two ways -- the resolution chain's code said *tier* for what its founding records, ADRs 0009 and 0010, call a *source* -- that is simply an inconsistency, and one that can be removed by deciding. @@ -64,7 +64,7 @@ from the API it wraps. Decision -------- -The glossary holds two kinds of term, and they carry different obligations. +The glossary holds two kinds of term, and they impose different obligations. **Core terms are ours.** The package invented them and no service has a claim on them: everything under *Retrieval*, *Failure and resumption*, *Configuration* @@ -73,31 +73,31 @@ enforced everywhere it appears -- prose, identifiers, tests. A second spelling of a core term is a defect, not a variation, and is fixed rather than recorded. This is what makes the lower-level modules shareable: transport, configuration and the OGC engine can be written once because the words they are written in -answer to nothing outside this package. +are constrained by nothing outside this package. **Domain terms belong to the services.** *Monitoring location* and *collection* name things the services define and spell differently. For these the glossary chooses one term for **prose**, so that documents about the package are -internally consistent. It does not choose for the wire, and it does not choose -for an adapter's public surface: each adapter keeps its own service's spelling -in its parameters, and reproduces that service's vocabulary faithfully where it -appears in returned data. +internally consistent. It does not choose the names used in requests, and it +does not choose for an adapter's public surface: each adapter keeps its own +service's spelling in its parameters, and reproduces that service's vocabulary +faithfully where it appears in returned data. -An adapter is where the two meet. Its public surface speaks its service's -language; what it hands to the shared modules speaks the core's. The +An adapter is where the two meet. Its public surface uses its service's +terms; what it passes to the shared modules uses the core terms. The translation is the adapter's job, and a divergence at that boundary is the design working rather than debt. Two rules follow: - **A term the glossary does not define is not used in the glossary.** A word - that earns a place in ``CONTEXT.md``'s prose earns an entry. Naming a term + used in ``CONTEXT.md``'s prose needs an entry. Naming a term only to say what an ADR calls it is a cross-reference, not a definition, and does not license using the word elsewhere. - **Only core misnamings are legacy.** *Known legacy names* records a core term the code spells wrongly and cannot be renamed. A domain term at an adapter's - surface is not a legacy name; it is that adapter speaking its service's - language, and belongs with the term's own entry. + surface is not a legacy name; it is that adapter using its service's + terms, and belongs with the term's own entry. Consequences ------------ @@ -112,7 +112,7 @@ Consequences - *Known legacy names* becomes shorter and means something narrower. The entries it loses are not resolved; they move to the term they belong to, as part of its definition rather than a list of exceptions. -- A glossary entry now carries an obligation to say which kind it is. That is a +- A glossary entry now has an obligation to say which kind it is. That is a small cost per term and the reason the distinction is usable at all. - The package's own inconsistencies in core vocabulary become defects with a deadline rather than curiosities. The resolution chain's ``tier``-for-*source* diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index 4e3983ffe..3890cd5e1 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -43,7 +43,7 @@ are necessary: Context view ------------ -The package sits between Python callers and remote hydrologic services:: +The package is the layer between Python callers and remote hydrologic services:: Python user / notebook / batch process | @@ -102,7 +102,7 @@ Shared components file with optional profiles, and built-in defaults in that order. Service and protocol modules may depend on it; it must not depend back on them. Scoped overrides use ``ContextVar`` so concurrent threads and asyncio - tasks can carry distinct credentials. + tasks can use distinct credentials. ``dataretrieval.ogc`` Protocol subsystem for Water Data and NGWMN. A facade (``__init__.py``) @@ -127,7 +127,7 @@ Shared components Internal service-neutral execution layer. Owns guarded client lifecycle and timeouts, host-scoped authentication, cursor pagination, bounded retry, response aggregation, fan-out execution, progress integration, and - sync-over-async dispatch. ``fanout`` drives an injected plan and fetch + sync-over-async dispatch. ``fanout`` runs an injected plan and fetch callback, owning bounded concurrency, deterministic failure precedence, sparse completion state, resume, and the progress line. It is also the one entry point from synchronous getter code into the async internals: a query @@ -151,8 +151,8 @@ Shared components ``dataretrieval._response_metadata`` ``BaseMetadata``, the second half of every getter's ``(DataFrame, metadata)`` return contract. A dependency-free leaf: nearly every service - module needs this class, and while it lived in ``utils`` beside the legacy - query machinery, importing it pulled in that module's whole HTTP stack + module needs this class, and while it was in ``utils`` beside the legacy + query code, importing it pulled in that module's whole HTTP stack transitively. The implementation module is private; the established public class path remains ``dataretrieval.utils.BaseMetadata``. @@ -210,8 +210,8 @@ Failed requests derive from ``dataretrieval.DataRetrievalError``. Callers can inspect ``status_code``, ``retry_after``, and ``retryable`` without knowing the concrete subtype. A fanned-out call -- an over-large OGC request, or a Water Use query naming several locations -- may raise ``FanOutInterrupted`` subclasses -(formerly, and still aliased as, ``ChunkInterrupted``) carrying a resumable call -handle and completed partial state. +(formerly, and still aliased as, ``ChunkInterrupted``) that hold a resumable +call handle and completed partial state. Package/module exports and documentation define the public surface. Underscore-prefixed symbols are implementation details even where existing @@ -227,7 +227,7 @@ service into one return shape: - Water Data, NGWMN, and Water Use tabular getters return ``(DataFrame, BaseMetadata)``. Geometry-bearing Water Data and NGWMN results may use a ``GeoDataFrame`` in the first position when geopandas is installed. - ``BaseMetadata`` carries request URL, elapsed query time, response headers, + ``BaseMetadata`` holds request URL, elapsed query time, response headers, and comments where the upstream format provides them. - WQP getters return ``(DataFrame, WQP_Metadata)``; the service-specific metadata extends ``BaseMetadata`` with WQP query parameters and site lookup. @@ -293,21 +293,22 @@ architecturally is the behavior around them: ``API_USGS_RETRIES`` Number of retries after the first attempt on supported active request paths; defaults to four. Backoff is exponential with full jitter and honors bounded - ``Retry-After`` values. Only failures a later attempt could survive are - re-sent: 429 and gateway 5xx, not a 500 rejecting the query itself, and not a - transport failure that is settled before the request leaves (unresolvable - host, unsupported scheme). Deprecated NWIS compatibility paths do not opt in. + ``Retry-After`` values. Only failures that may not recur on a later attempt + are re-sent: 429 and gateway 5xx, not a 500 rejecting the query itself, and + not a transport failure that is settled before the request leaves + (unresolvable host, unsupported scheme). Deprecated NWIS compatibility paths + do not opt in. ``API_USGS_STALL_TIMEOUT`` Seconds a call may go without receiving any data before retrying stops and - the failure surfaces; defaults to 60, and ``0`` disables the bound. It + the failure is raised; defaults to 60, and ``0`` disables the bound. It complements ``API_USGS_RETRIES``, which caps attempts rather than elapsed time: without this bound, four retries of a request that times out after a - minute add up to four silent minutes. Progress restarts the budget -- a - page received, or a queued chunk acquiring its concurrency slot. Neither a - slow but productive download nor the tail of a wide fan-out is cut short, - and an attempt already in flight is never interrupted. This bound never - withholds the first retry, so one slow attempt cannot disable retry by + minute add up to four minutes without data. Progress restarts the budget -- + a page received, or a queued chunk acquiring its concurrency slot. Neither a + slow but productive download nor the last chunks of a large fan-out are cut + short, and an attempt already in flight is never interrupted. This bound + never withholds the first retry, so one slow attempt cannot disable retry by itself; after that, the budget decides whether to continue. A dead connection therefore costs about two read timeouts rather than five attempts' worth. @@ -345,9 +346,9 @@ This view records categories and representative locations of debt. - ``waterdata/utils.py`` combines endpoint constants, argument normalization, and the OGC engine wrappers. -These are documented so guardrails distinguish accepted current dependencies -from new erosion. They should be removed through small, test-protected changes, -not a rewrite. +These are documented so the checks distinguish accepted current dependencies +from new violations. They should be removed through small, test-protected +changes, not a rewrite. Change process -------------- diff --git a/docs/source/meta/contributing.rst b/docs/source/meta/contributing.rst index d868a689b..5a862a3ec 100644 --- a/docs/source/meta/contributing.rst +++ b/docs/source/meta/contributing.rst @@ -2,7 +2,7 @@ Contributing ============ Contributions to ``dataretrieval`` are welcome. The repository's contributor -requirements and development commands live in `CONTRIBUTING.md`_. That file is +requirements and development commands are in `CONTRIBUTING.md`_. That file is the single source of truth for issue reports, change proposals, pull requests, coding standards, testing, documentation, and releases. diff --git a/docs/source/reference/exceptions.rst b/docs/source/reference/exceptions.rst index 7b5c29094..7a104db89 100644 --- a/docs/source/reference/exceptions.rst +++ b/docs/source/reference/exceptions.rst @@ -10,9 +10,9 @@ dataretrieval.exceptions Resumable fan-out interruptions ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -These are raised when a fanned-out request is interrupted mid-stream; the +These are raised when a fanned-out request is interrupted partway; the completed work is preserved and ``exc.call.resume()`` continues it. They are -defined in ``dataretrieval.interruptions`` (they carry pandas/httpx state) but +defined in ``dataretrieval.interruptions`` (they hold pandas/httpx state) but are importable from the top level, e.g. ``from dataretrieval import FanOutInterrupted``. diff --git a/docs/source/userguide/configuration.rst b/docs/source/userguide/configuration.rst index 6a1af67ac..ebf31bce7 100644 --- a/docs/source/userguide/configuration.rst +++ b/docs/source/userguide/configuration.rst @@ -9,7 +9,7 @@ want to adjust — a concurrency cap, a retry budget, where requests go — belo to *one* of them. So a **configuration profile** is a named set of settings for one adapter, written in code or stored in your configuration file, and a ``configure`` block puts one profile per adapter into effect for the calls -inside it. The Water Data API key is the exception that proves the rule: it +inside it. The Water Data API key is the one exception: it authenticates to a gateway rather than to an adapter, so it stays package-wide. .. contents:: @@ -24,7 +24,8 @@ One block, several services This is the case the mechanism exists for. Say the file holds what you would write once and keep — the key, a retry budget, and Water Data's everyday -concurrency — plus two named profiles for the shapes you only sometimes want: +concurrency — plus two named profiles for the settings you only sometimes +want: .. code-block:: toml @@ -42,7 +43,7 @@ concurrency — plus two named profiles for the shapes you only sometimes want: concurrency = 2 Then one block configures three services, taking two of them from the file by -name and building the third on the spot: +name and building the third in code: .. code-block:: python @@ -61,8 +62,8 @@ name and building the third on the spot: levels, _ = ngwmn.get_water_level(monitoring_location_id=wells) samples, _ = wqp.get_results(siteid=sites) -Inside the block Water Data runs unbounded and asks the planner for eight -chunks, NGWMN runs two requests at a time, and WQP retries twice. Everything a +Inside the block Water Data runs unbounded and requests eight chunks from the +planner, NGWMN runs two requests at a time, and WQP retries twice. Everything a configuration does *not* name still comes from below it, per setting: Water Data and NGWMN both retry six times and both send the ``api_key``, written once at the top of the file, because a configuration contributes what it names and @@ -74,7 +75,7 @@ file changed nothing on its own — a named profile is inert until a caller selects it, which is what makes one safe to add to a file other people's jobs also read. -Two rules keep a block like that unambiguous. A configuration knows which +Two rules keep a block like that unambiguous. A configuration records which adapter it targets — that is a property of its class — so you never restate it, and ``Configuration`` targets none of them, which is what makes it package-wide. And there is at most one configuration per adapter: naming two @@ -123,9 +124,9 @@ Settings - ``60`` - ``API_USGS_STALL_TIMEOUT`` - Seconds a call may go without receiving *any* data before retrying - stops and the failure surfaces. Bounds the wall-clock cost of a dead + stops and the failure is raised. Bounds the wall-clock cost of a dead connection, which ``retries`` alone does not — it counts attempts, not - seconds. Progress resets the clock; ``0`` disables the bound. + seconds. Progress resets the timer; ``0`` disables the bound. * - ``base_url`` - the service's own - *(none — code only)* @@ -148,8 +149,8 @@ Highest precedence first: ``[]`` table. 5. The package-wide keys at the top of the configuration file — ``~/.dataretrieval/config.toml``, or the path in ``DATARETRIEVAL_CONFIG``. -6. The adapter's own built-in preference, where it has one — NWDC asks for a - ``concurrency`` of 4, because that is as far as the service is +6. The adapter's own built-in preference, where it has one — NWDC defaults to + a ``concurrency`` of 4, because that is as far as the service is stress-tested. It is a default, not a cap: anything you set above outranks it. 7. The package built-in default, which for ``concurrency`` is 32. @@ -177,10 +178,10 @@ file. The one exception is ``API_USGS_PROGRESS``, where blank has always meant The one exception is rung 2 above rung 3 — a profile you name in code. That is a more deliberate act than a variable inherited from whatever started - your process, and having it lose to that variable is the kind of thing you - would file a bug about. The inversion covers what the profile names and - nothing else: every setting you did *not* name still follows the - environment-above-file rule, in the same block. See :doc:`ADR 0011 + your process, and having that variable override it would look like a bug. + The inversion covers what the profile names and nothing else: every setting + you did *not* name still follows the environment-above-file rule, in the + same block. See :doc:`ADR 0011 `. @@ -258,12 +259,12 @@ you import: An adapter table *overrides* the top-level one per setting, so ``[ngwmn]`` above still inherits ``retries`` and the ``api_key``. Precedence is unchanged otherwise: an adapter-scoped value outranks a package-wide one only within the -same source, so ``API_USGS_CONCURRENT`` exported for one run still beats a +same source, so ``API_USGS_CONCURRENT`` exported for one run still outranks a ``[ngwmn] concurrency`` in the file. Between ``configure`` blocks that tie-break applies per block: an adapter -configuration beats a package-wide value set by the *same* block, while -anything set by a block nested inside it wins over both. So a +configuration outranks a package-wide value set by the *same* block, while +anything set by a block nested inside it overrides both. So a ``configure(Configuration(concurrency=1))`` can still throttle a call an enclosing block had scoped to one adapter, and the innermost block decides. @@ -286,8 +287,8 @@ Adapter Configuration Ac ``base_url`` ==================================== ====================================== ======================================== -Each class lives in the module whose code reads those settings, so a setting's -definition sits next to its use rather than in a service-neutral file. +Each class is defined in the module whose code reads those settings, so a +setting's definition is next to its use rather than in a service-neutral file. ``api_key`` is deliberately not per-adapter. It authenticates to the *gateway* in front of a host, and Water Data and NGWMN are served from the same host — @@ -299,8 +300,8 @@ package-wide: there is one progress line per call. Named profiles ~~~~~~~~~~~~~~ -An adapter can hold more than one shape at a time. The ``[]`` table is -that adapter's **default profile** — always in effect, as above — while a +An adapter can have more than one profile at a time. The ``[]`` table +is that adapter's **default profile** — always in effect, as above — while a ``[.]`` table is a **named profile**, inert until you select it: .. code-block:: toml @@ -312,23 +313,23 @@ that adapter's **default profile** — always in effect, as above — while a concurrency = "unbounded" # only when selected parallel_chunks = 8 -So one file can hold an overnight bulk shape beside a polite daytime one, and -name as many of each as an adapter has uses for. +So one file can hold an overnight bulk profile beside a low-rate daytime one, +and name as many of each as an adapter has uses for. A named profile states only what differs: everything it does not name still comes from the adapter's default profile, the package-wide keys, and the rungs below — per setting. -``load`` reads the table and hands you a configuration object, so a name the -file does not define raises there and then, listing the names it does define — +``load`` reads the table and returns a configuration object, so a name the +file does not define raises immediately, listing the names it does define — a profile you just typed is more likely a typo than a request to fall through to settings you did not ask for. What comes back is inert until you pass it to ``configure``; that is what puts a selected profile above the environment, since selecting one is something your code did. A profile holds settings and nothing else: ``[waterdata.bulk-pull.ngwmn]`` is -not a Water Data profile carrying NGWMN detail, and selecting it says so rather -than quietly ignoring the nested table. Two adapters means two profiles, +not a Water Data profile containing NGWMN detail, and selecting it says so +rather than quietly ignoring the nested table. Two adapters means two profiles, selected in the same block, as in :ref:`the example above `. @@ -360,8 +361,9 @@ what makes it package-wide. Settings are not keywords on ``configure``. ``configure(api_key=...)`` and the per-adapter mappings ``configure(ngwmn={"concurrency": 4})`` were an - earlier spelling and are gone; write ``Configuration(api_key=...)`` and - ``NgwmnConfiguration(concurrency=4)`` instead. Passing anything that is not + earlier form and are no longer accepted; write + ``Configuration(api_key=...)`` and ``NgwmnConfiguration(concurrency=4)`` + instead. Passing anything that is not a configuration raises and names the replacement, so an old script says what to write rather than failing obscurely. @@ -450,7 +452,7 @@ selected any of it. A named profile does nothing until a caller selects it, so seeing ``[waterdata.bulk]`` there while no row above mentions it is the answer to "I added a profile and nothing changed". -The last line is the honest cost of validating an adapter's settings lazily: +The last line is the cost of validating an adapter's settings lazily: ``dataretrieval`` cannot say what ``nldi`` accepts until something imports it, so it says that rather than quietly omitting the service. It is named rather than left out, because an omitted service would read as "nothing is configured @@ -458,8 +460,8 @@ for it", which is a different claim. It never raises. A malformed file or a value that fails its grammar is reported in place — on the ``config file`` line for a whole-file problem, or in that -setting's own row — because a broken configuration is exactly when you reach -for this. +setting's own row — because a broken configuration is exactly when you need +this. Why ``parallel_chunks`` has no environment variable @@ -469,9 +471,9 @@ Every other setting can be set from the environment. ``parallel_chunks`` cannot, on purpose. Raising it splits a query into more sub-requests, and *each sub-request spends -rate-limit quota*. Whether that is a good trade depends on the size of the +rate-limit quota*. Whether that is worthwhile depends on the size of the query — which the library cannot know in advance. The setting therefore does -not add another process-global environment knob that could be exported once +not add another process-global environment variable that could be exported once and inherited by every subprocess. Set it per call, which is almost always what you want: @@ -494,8 +496,8 @@ stays a single request. ``parallel_chunks(n)`` is shorthand for ``configure(Configuration(parallel_chunks=n))``: one scoping mechanism, so the -innermost block wins whichever spelling set it, and ``show_configuration()`` -always reports the value the chunker will actually use. +innermost block takes precedence whichever form set it, and +``show_configuration()`` always reports the value the chunker will actually use. .. _configuration-redirect: @@ -517,12 +519,12 @@ a mirror, or a recording proxy — for the duration of a block: ): df, md = waterdata.get_daily(monitoring_location_id="USGS-05114000") -It names one adapter, so nothing else moves: NGWMN is served from the same host -as Water Data, and a ``WaterdataConfiguration`` still leaves it alone. What the -value replaces is that adapter's own base, and the package appends its usual -paths to it — for Water Data that is the root all four of its APIs hang off, so -one value moves the OGC collections, the Samples database, the statistics -service and the STAC catalog together. +It names one adapter, so nothing else changes: NGWMN is served from the same +host as Water Data, and a ``WaterdataConfiguration`` does not affect it. What +the value replaces is that adapter's own base, and the package appends its +usual paths to it — for Water Data that is the root shared by all four of its +APIs, so one value redirects the OGC collections, the Samples database, the +statistics service and the STAC catalog together. **Code only.** The configuration file and the environment both refuse it. A ``base_url`` key anywhere in the file, and an exported ``API_USGS_BASE_URL``, @@ -532,17 +534,17 @@ instead. A file or a shell export that silently redirected a data-retrieval library to another host would be a supply-chain hazard: nothing at the call site would -show it, and a script that reads correctly would be talking to someone else's -service. A ``with`` block keeps the redirect where a reader of the script sees -it. The refusal is loud rather than silent for the same reason — a variable -that was quietly ignored would leave you believing you had redirected -something. +show it, and a script that reads correctly would be sending requests to +someone else's service. A ``with`` block keeps the redirect where a reader of +the script sees it. The refusal raises an error rather than ignoring the value, +for the same reason — a variable that was quietly ignored would leave you +believing you had redirected something. -**The API key does not follow.** It is scoped to the one host that honors it -(:ref:`below `), so a redirected call goes out -without it. That is deliberate: the host you redirected to is not the host you -gave a credential to. If the mirror needs its own credential, it needs its own -mechanism. +**The API key is not sent to the new host.** It is scoped to the one host that +honors it (:ref:`below `), so a redirected call +goes out without it. That is deliberate: the host you redirected to is not the +host you gave a credential to. If the mirror needs its own credential, it needs +its own mechanism. .. _configuration-secret-store: @@ -550,7 +552,7 @@ mechanism. Keeping a key out of your environment entirely ---------------------------------------------- -If your credentials live in a secret manager, nothing needs to touch +If your credentials are stored in a secret manager, nothing needs to modify ``os.environ``: .. code-block:: python diff --git a/docs/source/userguide/errors.rst b/docs/source/userguide/errors.rst index 0708c1bec..c1444cdf2 100644 --- a/docs/source/userguide/errors.rst +++ b/docs/source/userguide/errors.rst @@ -33,7 +33,7 @@ Branch without knowing the concrete type Every :class:`~dataretrieval.exceptions.DataRetrievalError` exposes three read-anywhere fields, so you rarely need to import the specific subclasses: -* ``.status_code`` -- the HTTP status, or ``None`` when the failure carried no +* ``.status_code`` -- the HTTP status, or ``None`` when the failure included no response (a connection error, an over-long URL, ...). * ``.retry_after`` -- seconds the server asked you to wait (its ``Retry-After`` header), or ``None``. @@ -46,7 +46,7 @@ read-anywhere fields, so you rarely need to import the specific subclasses: if e.status_code == 404: ... # not found elif e.retryable: - ... # transient -- see the retry recipe below + ... # transient -- see the retry example below else: raise @@ -79,7 +79,7 @@ Resume an interrupted request Some requests become several: the Water Data and NGWMN getters split an over-large request into chunks, and a Water Use call with several locations becomes one request per location. When a transient failure interrupts one -mid-stream, the work already completed is preserved: catch +partway, the work already completed is preserved: catch ``FanOutInterrupted`` and call ``exc.call.resume()`` once the condition clears -- only the unfinished chunks are re-issued. @@ -115,10 +115,10 @@ extra quota *as long as each chunk still spans many pages*. (Ten states pulled as one request then page nearly as many times as ten per-state requests would; a split that leaves each chunk only a page or two adds its partial final page.) So if you *know* your pull is large, ask for a finer split with -``parallel_chunks(n)``: you trade roughly the same pages for more, smaller +``parallel_chunks(n)``: you get roughly the same pages in more, smaller chunks, which gives smoother progress, more even concurrency, and a smaller unit of retry/resume. ``parallel_chunks`` is a scoped ``with`` block, so -an aggressive setting can't leak into unrelated calls and accidentally spend +a high setting can't leak into unrelated calls and accidentally spend quota: .. code-block:: python @@ -136,13 +136,13 @@ raises ``ValueError`` at the ``with``. ``n`` caps the *total* chunk count across every multi-value argument combined (not per argument), bounded below by what the byte limit already forces and above by how many values there are to split. Several multi-value arguments therefore can't multiply past it, and -``n=1`` asks for no extra fan-out. Each chunk costs a request against your +``n=1`` requests no extra fan-out. Each chunk costs a request against your hourly rate limit. How many run *at once* is capped separately by ``API_USGS_CONCURRENT`` (default 32), so an ``n`` beyond that adds quota without adding parallelism -- the useful range is roughly ``2`` up to ``API_USGS_CONCURRENT``. There is no "off" level: don't enter the block unless you already expect a large, multi-page result -- on a query that would -have fit in a single page, extra chunks only burn quota. +have fit in a single page, extra chunks only spend quota. The full taxonomy ================= diff --git a/docs/source/userguide/timeconventions.rst b/docs/source/userguide/timeconventions.rst index cfbcefea2..ae6029ff0 100644 --- a/docs/source/userguide/timeconventions.rst +++ b/docs/source/userguide/timeconventions.rst @@ -47,7 +47,7 @@ them to any local timezone with the pandas ``.dt`` accessor. 4 2024-02-29 20:00:00-05:00 Name: time, dtype: datetime64[us, America/New_York] -After conversion the timestamps carry New York's offset — ``-05:00`` during +After conversion the timestamps have New York's offset — ``-05:00`` during standard time, or ``-04:00`` during daylight saving time, since New York is 4 or 5 hours behind UTC depending on the time of year. The first midnight-UTC reading rolls back to the previous calendar day (``2024-02-29``) once shifted From fab9d98c2b317774d4b9a31cbb250a1b61727814 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Sun, 6 Sep 2026 17:07:10 -0500 Subject: [PATCH 2/2] docs: second round of copy edits Copy edits only, continuing the previous commit: plain terms for Retry-After handling, the no-progress budget, host acceptance of the API key, and a few remaining figures of speech in the ADRs, glossary, and guides. No decision, glossary term, or code changes. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015Vb9Y5QQdoHTac4cgvTexc --- AGENTS.md | 6 +-- CONTEXT.md | 26 ++++++------- CONTRIBUTING.md | 4 +- NEWS.md | 2 +- .../decisions/0000-documenting-decisions.rst | 2 +- .../0002-sync-api-async-internals.rst | 2 +- .../decisions/0004-error-retry-resume.rst | 38 ++++++++++--------- .../0006-service-neutral-transport.rst | 32 ++++++++-------- .../decisions/0008-fan-out-execution.rst | 7 ++-- .../decisions/0009-layered-configuration.rst | 32 ++++++++-------- .../0010-adapter-scoped-settings.rst | 22 +++++------ .../decisions/0011-configuration-profiles.rst | 10 ++--- .../decisions/0012-deprecation-horizons.rst | 2 +- .../decisions/0013-core-and-domain-terms.rst | 14 +++---- docs/source/architecture/index.rst | 18 ++++----- docs/source/userguide/configuration.rst | 12 +++--- docs/source/userguide/errors.rst | 2 +- 17 files changed, 117 insertions(+), 114 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9b7d42b76..29b443a92 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,16 +109,16 @@ raise states the problem and then the action that fixes it, in that order. failure the remedy is whether to retry, and `transport.pagination. paginated_failure_message()` is the model: cause, then `To recover: …`. - Don't invent a phrasing for a check that recurs. `dataretrieval/_validation.py` - owns the wording for the shared shapes — bad value in a closed vocabulary + owns the wording for the shared cases — bad value in a closed vocabulary (`require_one_of`), missing argument (`require_argument`), incomplete group (`require_together`), no filter at all (`require_any_of`), and conflicting arguments (`require_exactly_one`, `reject_together`). Use one before hand-writing a message. A service-specific pointer is not a reason to hand-write: every check takes a `remedy=` for the action it cannot derive. Every check raises `ValueError` -- one class for a bad argument value, so a - caller catches by shape rather than by which module rejected it. + caller catches by error class rather than by which module rejected it. - `require_argument` returns the narrowed value and `require_exactly_one` the - winning `(name, value)` pair, so use their results rather than re-testing for + selected `(name, value)` pair, so use their results rather than re-testing for `None` to satisfy mypy — a second, unreachable message beside the first is how the two drift apart. - **Paste the remedy back before trusting it.** Whatever a message names must be diff --git a/CONTEXT.md b/CONTEXT.md index 0eaa10901..a443d88b4 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -96,7 +96,7 @@ covering sites, water levels, lithology, well construction, and providers. **NWDC** — The National Water Availability Assessment Data Companion. Serves ten modeled national-scale datasets, of which the water-use models are five; the rest are hydrologic, atmospheric-forcing, and assessment outputs. The -package reaches it through the `nwdc` adapter, named for the service like every +package accesses it through the `nwdc` adapter, named for the service like every other adapter. Legacy: that module was `wateruse`, which named one subset of what the service offers. @@ -106,7 +106,7 @@ what the service offers. from an origin to connected features, flowlines, or basins. **NWIS** — The legacy USGS waterservices interface. Deprecated: it is retained -for compatibility and is not where new work goes. +for compatibility and should receive no new work. **StreamStats** — Basin characteristics and delineation for a point on a stream. @@ -183,12 +183,12 @@ scopes within them. ADR 0010's word for a scope level is *tier* — the top-leve tier that remains, the host or gateway tier it defers. **Package-wide setting** — A setting that applies to every adapter: the retry -count, the progress line, the stall timeout. Set once, honored everywhere. +count, the progress line, the stall timeout. Set once, applied everywhere. **Adapter-scoped setting** — A setting named under one adapter, applying to that adapter and no other. It overrides the package-wide value for that adapter -alone, leaving that value standing for every other adapter. An adapter rejects a setting -it has no use for, rather than accepting and ignoring it. +alone, leaving that value in effect for every other adapter. An adapter +rejects a setting it has no use for, rather than accepting and ignoring it. The scope is the *adapter*, not the service and not the host, because the adapter is what owns the conventions being tuned. The API key shows where the @@ -222,11 +222,11 @@ its identifiers as well as its prose. The code uses both names: `_resolve` returns `(raw, label, source)`, and the parsers take the `label` as the subject of any error message they raise. -**Precedence ladder** — The one linear order resolution walks, highest first: -sources in their order and, within each source, scopes. Each position is a -**rung**; ADR 0011 states the ladder in seven. A rung is finer than a source — -one source can span several rungs — so prose that means a whole category says -*source*, reserving *rung* for when the position itself matters. +**Precedence ladder** — The one linear order resolution follows, highest +first: sources in their order and, within each source, scopes. Each position +is a **rung**; ADR 0011 states the ladder in seven. A rung is finer than a +source — one source can span several rungs — so prose that means a whole +category says *source*, reserving *rung* for when the position itself matters. **Selection** — Naming which profile an adapter should use. Done in code; a profile is never selected by the environment or implied by the file, so the @@ -269,8 +269,8 @@ vocabulary. NWDC is not one: its query fans out per location even though it never chunks by bytes. **Fitness function** — An executable check that an architectural rule still -holds, living in `tests/architecture_test.py`. ADR 0003 divides the work between -these and `.importlinter`. +holds, defined in `tests/architecture_test.py`. ADR 0003 divides the work +between these and `.importlinter`. **Facade** — A module that re-exports a subsystem's public surface and contains no logic of its own, so callers depend on a stable name rather than on internal @@ -288,7 +288,7 @@ and is not public API. ## Known legacy names Core-term spellings recorded so they are not mistaken for drift, and not -re-litigated: frozen misnamings, permanent aliases, and names that agree with +reopened: frozen misnamings, permanent aliases, and names that agree with this glossary by more than coincidence. A domain term at an adapter's surface is not a legacy name and is not listed here; it belongs with that term's own entry (ADR 0013). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e20922bba..c886cb1c6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -96,7 +96,7 @@ Before you submit a pull request, check that it meets these guidelines: test run neither depends on USGS uptime nor spends anyone's rate limit. The exception is a small set of tests marked `live`, which query the real -services to notice when an upstream API changes shape -- something a mock cannot +services to notice when an upstream API changes -- something a mock cannot tell us, because the mock is what would need updating. They are deselected by default and run on a nightly schedule ([live-api.yml](https://github.com/DOI-USGS/dataretrieval-python/blob/main/.github/workflows/live-api.yml)). @@ -169,7 +169,7 @@ ratchet. `xenon` and `complexipy` are complexity ratchets: the thresholds are the tightest the package passes today, so they fail only when a change pushes a score above today's. They disagree because they count different things. `xenon` counts -branches (cyclomatic complexity), so a wide flat dispatch scores high; +branches (cyclomatic complexity), so a large flat dispatch scores high; `complexipy` counts how hard the control flow is to follow (cognitive complexity), so it scores that dispatch lower and nesting higher. Both name the offending block, so the fix is local -- usually extracting a branch rather than diff --git a/NEWS.md b/NEWS.md index 9a6e21e8a..80f5d6aa1 100644 --- a/NEWS.md +++ b/NEWS.md @@ -70,7 +70,7 @@ - Now supports `pandas` 3.x (#221). - The OGC `waterdata` getters (`get_continuous`, `get_daily`, `get_field_measurements`, and the six others built on the same OGC collections) now accept `filter` and `filter_lang` kwargs that are passed through to the service's CQL filter parameter. This enables advanced server-side filtering that isn't expressible via the other kwargs — most commonly, OR'ing multiple time ranges into a single request. A long expression made up of a top-level `OR` chain is transparently split into multiple requests that each fit under the server's URI length limit, and the results are concatenated. -**12/04/2025:** The `get_continuous()` function was added to the `waterdata` module, which provides access to measurements collected via automated sensors at a high frequency (often 15 minute intervals) at a monitoring location. This is an early version of the continuous endpoint and should be used with caution as the API team improves its performance. In the future, we anticipate the addition of an endpoint(s) specifically for handling large data requests, so it may make sense for power users to hold off on heavy development using the new continuous endpoint. +**12/04/2025:** The `get_continuous()` function was added to the `waterdata` module, which provides access to measurements collected via automated sensors at a high frequency (often 15 minute intervals) at a monitoring location. This is an early version of the continuous endpoint and should be used with caution as the API team improves its performance. In the future, we anticipate the addition of an endpoint(s) specifically for handling large data requests, so power users may want to delay heavy development using the new continuous endpoint. **11/24/2025:** `dataretrieval` has a new module, `waterdata`, which gives users access to USGS's modernized [Water Data APIs](https://api.waterdata.usgs.gov/). The Water Data API endpoints include daily values, instantaneous values, field measurements (modernized groundwater levels service), time series metadata, and discrete water quality data from the Samples database. Though there will be a period of overlap, the functions within `waterdata` will eventually replace the `nwis` module, which currently provides access to the legacy [NWIS Water Services](https://waterservices.usgs.gov/). More example workflows and functions coming soon. Check `help(waterdata)` for more information. diff --git a/docs/source/architecture/decisions/0000-documenting-decisions.rst b/docs/source/architecture/decisions/0000-documenting-decisions.rst index 55fb52b32..4ae3da8b5 100644 --- a/docs/source/architecture/decisions/0000-documenting-decisions.rst +++ b/docs/source/architecture/decisions/0000-documenting-decisions.rst @@ -12,7 +12,7 @@ section records the clause added. Context ------- -This package documents itself heavily and deliberately. Its public getters are +This package is documented heavily and deliberately. Its public getters are thin wrappers whose numpydoc parameter tables *are* the deliverable: 55% of all docstring lines in ``dataretrieval/`` are in the service adapters, at a ratio of 2.5 prose lines per line of code. CONTRIBUTING already requires those tables. diff --git a/docs/source/architecture/decisions/0002-sync-api-async-internals.rst b/docs/source/architecture/decisions/0002-sync-api-async-internals.rst index 6219dd504..19c6e3605 100644 --- a/docs/source/architecture/decisions/0002-sync-api-async-internals.rst +++ b/docs/source/architecture/decisions/0002-sync-api-async-internals.rst @@ -23,7 +23,7 @@ Decision Keep public service getters synchronous. Async-capable implementations may run inside a short-lived anyio blocking portal and use ``httpx.AsyncClient`` for pagination and bounded fan-out. Internal async functions are implementation -details, not a second public API promise. +details, not a second public API contract. Ambient per-call policy (the progress reporter) must propagate into the worker context. A resumable OGC call binds the state needed to rebuild its remaining diff --git a/docs/source/architecture/decisions/0004-error-retry-resume.rst b/docs/source/architecture/decisions/0004-error-retry-resume.rst index c99461649..466bb2524 100644 --- a/docs/source/architecture/decisions/0004-error-retry-resume.rst +++ b/docs/source/architecture/decisions/0004-error-retry-resume.rst @@ -28,8 +28,8 @@ All request failures exposed by public service modules derive from ``retryable`` attributes. Status mapping is done in one policy function. Where automatic recovery is supported, retries are bounded, use exponential -backoff with full jitter, honor only bounded ``Retry-After`` delays, and preserve -cancellation. OGC fan-out retains completed chunks and raises a typed +backoff with full jitter, wait only for bounded ``Retry-After`` delays, and +preserve cancellation. OGC fan-out retains completed chunks and raises a typed ``ChunkInterrupted`` with a handle that resumes only missing work. Fatal or unknown failures are not classified as resumable transients. @@ -45,12 +45,12 @@ into one that does. A fan-out over *independent* items may skip one. Where a query asks for many items that do not compose into a single answer, an item failing -deterministically is dropped under a warning naming it, and counts as complete +deterministically is dropped with a warning naming it, and counts as complete so a resume does not re-attempt it. A transient failure is never skipped: it -retries, and exhausts into a resumable interruption like any other. This is the -deliberate exception to the rule that a failed fan-out raises rather than -returning successful siblings; it applies only where the items are independent, -never to the pages of one query. +retries, and once retries are exhausted it raises a resumable interruption like +any other. This is the deliberate exception to the rule that a failed fan-out +raises rather than returning successful siblings; it applies only where the +items are independent, never to the pages of one query. An advisory about *upstream data* -- a dataset the service has stopped updating -- is a ``UserWarning``, never a ``DeprecationWarning``. Downstream projects run @@ -59,12 +59,13 @@ is stale" as a deprecation makes their build fail over something no code change of theirs can fix. ``DeprecationWarning`` means *this package's* API is being removed. -**A ``Retry-After`` hint is honored as information even when it is not -actionable.** A hint too long to wait for, or expressed as a date, is parsed and -surfaced on the failure rather than discarded, so a caller can see what the -service asked for; the retry policy separately declines to wait beyond its cap. -A date already in the past yields no hint at all rather than a zero-second one, -which would read as "retry immediately" -- the opposite of what the header said. +**A ``Retry-After`` value is kept as information even when it is not +actionable.** A value too long to wait for, or expressed as a date, is parsed +and set on the failure rather than discarded, so a caller can see what the +service requested; the retry policy separately does not wait beyond its cap. +A date already in the past yields no value at all rather than a zero-second +one, which would mean "retry immediately" -- the opposite of what the header +specified. **Every error must be reconstructible across a process boundary.** Reconstruction goes through ``__new__`` plus ``__getstate__``/``__setstate__`` @@ -96,17 +97,18 @@ precedence. The skip policy is covered by ``tests/waterdata_ratings_test.py::test_get_ratings_deterministic_download_failure_warns_and_skips`` and its sibling for a feature with no asset; the warning categories by ``tests/deprecation_test.py``, which asserts ``DataCurrencyWarning`` is not a -subclass of ``DeprecationWarning``; the hint-parsing rules by the +subclass of ``DeprecationWarning``; the ``Retry-After`` parsing rules by the ``Retry-After`` date and over-cap cases; and the process boundary by round-tripping error subclasses through ``pickle``. Notes ----- -The warning, hint-parsing, and pickling clauses were added after the original -decision. They record, under ADR 0000, rules the code was stating in prose. The -skip clause records an exception that previously read as contradicting this -record and :doc:`0006-service-neutral-transport`; 0006 now points here for it. +The warning, ``Retry-After`` parsing, and pickling clauses were added after the +original decision. They record, under ADR 0000, rules the code was stating in +prose. The skip clause records an exception that previously read as +contradicting this record and :doc:`0006-service-neutral-transport`; 0006 now +points here for it. The ``Status`` line was also annotated retroactively: this record assigned resumable partial state to OGC, which :doc:`0008-fan-out-execution` superseded diff --git a/docs/source/architecture/decisions/0006-service-neutral-transport.rst b/docs/source/architecture/decisions/0006-service-neutral-transport.rst index 53238fa99..f5cb1ce97 100644 --- a/docs/source/architecture/decisions/0006-service-neutral-transport.rst +++ b/docs/source/architecture/decisions/0006-service-neutral-transport.rst @@ -46,7 +46,7 @@ Three concerns are deliberately *outside* it, as top-level leaves, because they are not HTTP execution policy and every adapter needs them whether or not it goes through transport: -- ``dataretrieval.credentials`` -- which host honors the key, whether a +- ``dataretrieval.credentials`` -- which host accepts the key, whether a destination qualifies, and how the key is withheld. One definition, so the code that attaches a credential and the code that removes it cannot disagree. - ``dataretrieval.progress`` -- terminal rendering. Transport reports *into* it. @@ -70,8 +70,8 @@ at import time without affecting the policy that reads it, so Automatic retry is enabled only on active, idempotent request paths, and only for failures that may not recur on a later attempt -- rate limiting, server errors, and transport failures that are not deterministic. Which server errors -qualify is per adapter: a fanned-out call re-sends any 5xx, to wait out a -transient upstream failure, while a single-shot adapter re-sends only the +qualify is per adapter: a fanned-out call re-sends any 5xx, since a transient +upstream failure may have ended, while a single-shot adapter re-sends only the gateway statuses, because its service responds to a *rejected query* with a 500, and re-sending that would spend a caller's quota on a request that cannot succeed. Both sets are narrower than ``DataRetrievalError.retryable``, @@ -90,20 +90,20 @@ the budget, and an attempt already in flight is never interrupted. **Time spent waiting is not time without progress.** The budget bounds time the *service* left the caller with nothing, so time the package chose to spend -is credited back by the measured amount: a wait the server named in +is excluded by the measured amount: a wait the server named in ``Retry-After``, and time a chunk spent waiting for the concurrency semaphore. The first retry is exempt outright. Without these exemptions a policy that -honors a server's hint would spend its own budget obeying it, and a call would -lose retries for being throttled by settings the caller chose. A credit never -sets the reference time later than now: a timestamp ahead of now would make -the elapsed no-progress time negative and silently disable the bound. Because -half of that accounting is the retry driver's, the concurrency semaphore is -acquired *per attempt* inside the retry driver rather than held by the caller -across one. +follows a server's ``Retry-After`` would spend its own budget doing so, and a +call would lose retries for being throttled by settings the caller chose. An +exclusion never sets the reference time later than now: a timestamp ahead of +now would make the elapsed no-progress time negative and silently disable the +bound. Because half of that exclusion is the retry driver's, the concurrency +semaphore is acquired *per attempt* inside the retry driver rather than held +by the caller across one. **A server-supplied next-page link is untrusted response data.** One shared policy parses it, resolves it against the request, refuses a host the caller -did not ask for, and strips embedded credentials (ADR 0009) before it becomes a +did not request, and strips embedded credentials (ADR 0009) before it becomes a request; a page walk injects only which hosts are acceptable. Three walks follow such links -- OGC ``links``, the ratings STAC search, and Water Use's ``Link`` header -- and a link is the same attacker-influenced input in all @@ -113,7 +113,7 @@ variation. **Refusing credential-shaped keywords belongs to the credentials leaf.** ADR 0009 owns the rule that a general ``**kwargs`` or ``**queryables`` passthrough refuses such names; what belongs here is where the predicate is defined. It is -the fourth question that leaf answers, alongside which host honors the key, +the fourth question that leaf answers, alongside which host accepts the key, whether a destination qualifies, and how the key is withheld -- one definition, so ten getters cannot drift into ten versions of the same check. @@ -128,10 +128,10 @@ Consequences - Retry can increase latency and quota consumption, so attempt counts, waits, and total no-progress time remain bounded, and cancellation signals are never wrapped. -- Guidance the progress reporter prints is gated on the host it applies to, so a +- Guidance the progress reporter prints depends on the host it applies to, so a service that cannot use an API key is not told to obtain one. - The transport package is internal infrastructure, not a new public API - promise. + contract. - Keeping presentation and frame assembly out means transport is roughly 570 lines across five modules, each recognizably HTTP execution policy. Retry is the one complex module, because two independent bounds are what make retry @@ -148,7 +148,7 @@ Component and adapter tests cover cursor termination, row caps, response aggregation, retry exhaustion, ``Retry-After`` limits, the no-progress budget, which failures are re-sent, cancellation, no-partial fan-out behavior, and credential host scoping. The exemptions above are covered by the liveness and -retry tests over credited waits and the first-attempt case. Next-page link +retry tests over excluded waits and the first-attempt case. Next-page link validation is covered by the shared link-policy tests over foreign hosts and embedded userinfo. diff --git a/docs/source/architecture/decisions/0008-fan-out-execution.rst b/docs/source/architecture/decisions/0008-fan-out-execution.rst index 2ff3dc38b..f29fee173 100644 --- a/docs/source/architecture/decisions/0008-fan-out-execution.rst +++ b/docs/source/architecture/decisions/0008-fan-out-execution.rst @@ -32,7 +32,7 @@ siblings, and ADR 0006 grouped them together deliberately. That grouping was correct while a byte plan was the only thing anyone fanned out over. It stopped being correct once Water Use fanned out too: unable to import an OGC-internal executor, ``wateruse._fan_out`` re-implemented the semaphore, the -``asyncio.gather``, and the cancellation-beats-HTTP-error failure precedence, +``asyncio.gather``, and the cancellation-before-HTTP-error failure precedence, with a comment naming ``ChunkedCall._run`` as the original. One rule, two copies, kept in agreement by that comment. @@ -133,7 +133,7 @@ Consequences - Water Use gains resume, progress reporting, and the shared concurrency setting, and removes roughly 75 lines of duplicated orchestration. -- One implementation of failure precedence, so cancellation-beats-error and +- One implementation of failure precedence, so cancellation-before-error and deterministic failure ordering cannot drift between services. - **Breaking:** a Water Use fan-out interrupted by a 5xx, 429, or recoverable connection failure now raises ``ServiceInterrupted`` / ``QuotaExhausted`` @@ -146,7 +146,8 @@ Consequences ``API_USGS_CONCURRENT`` and ``wateruse.DEFAULT_CONCURRENT_REQUESTS``. - Resume re-issues a failed location's entire page walk, so pages fetched before the failure are fetched again. This already applied to OGC -- a partial walk - never enters the completion map -- and is a cost, not a correctness problem. + is never recorded in the completion map -- and is a cost, not a correctness + problem. - Water Use frames have ``huc12_id``, not ``id``, so ``_combine_chunk_frames`` concatenates them without deduplicating. Correct, because locations partition by construction, but the executor's deduplication does not apply there. diff --git a/docs/source/architecture/decisions/0009-layered-configuration.rst b/docs/source/architecture/decisions/0009-layered-configuration.rst index eb466c108..3dc4bad56 100644 --- a/docs/source/architecture/decisions/0009-layered-configuration.rst +++ b/docs/source/architecture/decisions/0009-layered-configuration.rst @@ -44,8 +44,8 @@ could report the effective configuration, and what those parsers accepted was free to drift apart. That mechanism cannot express a per-call credential. An application holding -keys in a secret store, a notebook pulling for two accounts, or a server -handling concurrent users must assign to ``os.environ`` -- which is +keys in a secret store, a notebook retrieving data for two accounts, or a +server handling concurrent users must assign to ``os.environ`` -- which is process-global, so it races across threads and tasks (issue #352). An ``api_key=`` parameter on the public getters is where a per-call value would @@ -93,7 +93,7 @@ Supporting decisions: programmatic path, and a fitness function asserts no getter accepts ``api_key`` / ``session`` / ``token``. The generic ``**queryables`` path also refuses credential-shaped names before request construction so they cannot - enter a URL. That refusal covers names that hold a secret; ``session`` is + appear in a URL. That refusal covers names that hold a secret; ``session`` is deliberately not among them (see Notes). - **The module owns each setting's parser.** ``unbounded``, bounds, and rejection messages are defined in one place. ``tomllib`` returns typed @@ -141,11 +141,11 @@ Supporting decisions: - **Names distinguish execution capacity from planning granularity.** ``concurrency`` names the maximum chunks in flight and maps to the existing ``API_USGS_CONCURRENT`` variable. ``parallel_chunks`` requests optional - extra chunks from the planner; it does not promise that many execute + extra chunks from the planner; it does not guarantee that many execute simultaneously. The name is retained because the context manager is already public. ``parallelism`` and ``chunk_parallelism`` were rejected because they would - conflate this planning hint with ``concurrency``. + conflate this planning setting with ``concurrency``. - **Configuration errors are in the error taxonomy.** ``ConfigurationError`` is a ``DataRetrievalError`` *and* a ``ValueError``. Configuration resolves lazily on the request path, so an invalid file raises from inside whichever getter @@ -160,13 +160,13 @@ Supporting decisions: - **``dataretrieval.configuration`` is a lightweight leaf.** It uses only the standard library, the ``tomli`` backport on Python 3.10, and ``dataretrieval.exceptions`` -- itself a dependency-free leaf, so this adds no - weight and cannot create an import cycle. It is read by ``utils`` (headers), - ``ogc.chunking``, ``ogc.retry``, and ``ogc.progress``, so under ADR 0003 it - must import none of them. The public callable is named ``configure`` rather - than ``config`` so it does not shadow the module. It is a scoped action, not a - ``Configuration`` dataclass: a value object would imply snapshot, equality, - serialization, and representation contracts while risking disclosure of the - API key through generated helpers. + dependencies and cannot create an import cycle. It is read by ``utils`` + (headers), ``ogc.chunking``, ``ogc.retry``, and ``ogc.progress``, so under + ADR 0003 it must import none of them. The public callable is named + ``configure`` rather than ``config`` so it does not shadow the module. It is + a scoped action, not a ``Configuration`` dataclass: a value object would imply + snapshot, equality, serialization, and representation contracts while risking + disclosure of the API key through generated helpers. - **One flat set of setting names, shared by every service.** ``concurrency`` means the same thing to every adapter, so the chain resolves one name rather @@ -183,7 +183,7 @@ Supporting decisions: - **Per-service overrides are deferred, not refused.** One ``configure()`` block cannot currently set one value for Water Use and another for Water Data. Every known service difference is a default, which the caller already - supplies, so nothing needs it yet. If something does, the shape is a namespace + supplies, so nothing needs it yet. If something does, the form is a namespace inside this chain -- a ``[wateruse]`` table beside the top-level keys, read as ``configuration.concurrency(default, service=...)``. It costs a second dimension in resolution, which ``show_configuration()`` must then render as a @@ -192,12 +192,12 @@ Supporting decisions: - **A configuration object would have no way to reach the call.** The public surface is free functions -- ``waterdata.get_daily(...)``, not a client with - methods. An instance would therefore arrive either as a parameter on every + methods. An instance would therefore be passed either as a parameter on every getter, which is the per-call passing the ``ContextVar`` exists to remove and which the ``**queryables`` catch-all makes unsafe, or through a module-level global, which restores the cross-thread and cross-task leakage this ADR - exists to end. A library entered through a constructed client can hold - settings on that client; one entered through free functions cannot, and the + exists to end. A library used through a constructed client can hold + settings on that client; one used through free functions cannot, and the scoped block follows from that. Consequences diff --git a/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst b/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst index af9fc1b18..9b417191c 100644 --- a/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst +++ b/docs/source/architecture/decisions/0010-adapter-scoped-settings.rst @@ -133,11 +133,11 @@ Settings are scoped to the **adapter**, not the service, and not the host. caller's terminal, and there is one progress line per call, so scoping it per adapter could only produce a contradiction. -3. **Precedence stays source-major.** Resolution walks block, then environment, - then file, as ADR 0009 defines; *within* each source an adapter-scoped value - outranks a top-level one. The environment therefore still outranks the file, - so a stale adapter table cannot quietly override a variable exported for one - run. +3. **Precedence stays source-major.** Resolution checks the block, then the + environment, then the file, as ADR 0009 defines; *within* each source an + adapter-scoped value outranks a top-level one. The environment therefore + still outranks the file, so a stale adapter table cannot quietly override a + variable exported for one run. 4. **Adapter-scoped settings get no environment variables.** Every entry in ``ENV_VARS`` stays package-wide, for the reason ``parallel_chunks`` already @@ -158,7 +158,7 @@ Settings are scoped to the **adapter**, not the service, and not the host. Data setting's definition in a module unrelated to Water Data. 6. **The API key stays host-scoped and is not an adapter setting.** - ``credentials`` keeps sole ownership of which host honors the key. There is + ``credentials`` keeps sole ownership of which host accepts the key. There is no ``[ngwmn] api_key``. 7. **Adapters are keyed by their service's name**, matching the module: @@ -234,7 +234,7 @@ Consequences It is a defaulted keyword on 23 shipped getters across four adapters -- ``wqp`` (9), ``nwis`` (10), ``waterdata`` (3) and ``nwdc`` (1) -- and it does reach ``httpx``'s ``verify``. It was added in 2023 to what were then the only - modules; the OGC getters arrived later and never adopted it, so its + modules; the OGC getters were added later and never adopted it, so its distribution records the package's history rather than a boundary. Three reasons not to promote it. It disables certificate verification, so as @@ -243,10 +243,10 @@ Consequences invisible at the call site -- the opposite of the direction this chain narrows everything else. It does not respect adapter boundaries: within ``waterdata`` it applies only to the getters that bypass the OGC engine, so - ``[waterdata] ssl_check`` would be honored by three getters and silently - ignored by the rest, exactly the shape this ADR refuses elsewhere. And the + ``[waterdata] ssl_check`` would be applied by three getters and silently + ignored by the rest, exactly the pattern this ADR refuses elsewhere. And the need it serves is already met better: the legitimate case is a - TLS-intercepting corporate proxy, and ``httpx`` natively honors + TLS-intercepting corporate proxy, and ``httpx`` natively reads ``SSL_CERT_FILE`` and ``SSL_CERT_DIR`` on both its sync and async clients -- so that mechanism already covers *every* getter, including the OGC ones that have no ``ssl_check``, and it trusts the corporate CA rather than trusting @@ -273,7 +273,7 @@ Consequences The two entries covering decision 8's ``**adapters`` catch-all (``test_a_misspelled_setting_is_not_taken_for_an_adapter``) and the central ``TypedDict`` registry (``test_adapter_schema_names_a_real_module``) went with -the clauses ADR 0011 superseded; the checks they stood for are named above in +the clauses ADR 0011 superseded; the checks they represented are named above in their current form. Notes diff --git a/docs/source/architecture/decisions/0011-configuration-profiles.rst b/docs/source/architecture/decisions/0011-configuration-profiles.rst index f10607f54..f5cae383f 100644 --- a/docs/source/architecture/decisions/0011-configuration-profiles.rst +++ b/docs/source/architecture/decisions/0011-configuration-profiles.rst @@ -58,9 +58,9 @@ file gains named profiles beside each adapter's default profile:: [ngwmn.gentle] concurrency = 4 -A named profile never enters the chain unless a caller selects it. The global -``[profiles.]`` table and ``DATARETRIEVAL_PROFILE`` are retired; nothing -has shipped, so nothing is deprecated. +A named profile is never part of the chain unless a caller selects it. The +global ``[profiles.]`` table and ``DATARETRIEVAL_PROFILE`` are retired; +nothing has shipped, so nothing is deprecated. **``configure()`` takes configuration objects.** Positionally, one per adapter, and nothing else:: @@ -135,7 +135,7 @@ the file is withdrawn. The path has never been released, so no alias is needed. **Credentials are unchanged, and measurement settled why.** The API key stays -one package-wide setting scoped to the single host that honours it. Probing +one package-wide setting scoped to the single host that accepts it. Probing the live services: .. list-table:: @@ -165,7 +165,7 @@ anonymously today. The three hosts also keep independent counters, so ADR 0010's "one key, one quota pool" is true of waterdata and ngwmn only. **An adapter composes shared setting groups; it does not redeclare their -fields.** Which settings an adapter reads is the adapter's own knowledge, but +fields.** Which settings an adapter reads is the adapter's own concern, but what each setting *means* is shared, so the fields come from frozen mixin groups declared once beside their grammar. An adapter's configuration class names the groups it composes and adds only what is genuinely its own. Declaring diff --git a/docs/source/architecture/decisions/0012-deprecation-horizons.rst b/docs/source/architecture/decisions/0012-deprecation-horizons.rst index 9bb69a4b1..1bc89c237 100644 --- a/docs/source/architecture/decisions/0012-deprecation-horizons.rst +++ b/docs/source/architecture/decisions/0012-deprecation-horizons.rst @@ -32,7 +32,7 @@ recorded in ``REMOVALS``. A deprecation advisory names three things: what is being removed, what to use instead, and the date on or after which it may be removed. The mechanism -tolerates an advisory with no date -- it then promises nothing specific rather +tolerates an advisory with no date -- it then states no date rather than implying a schedule it does not have. A deprecation of a public name is expected to include one, and an advisory naming a replacement the caller cannot yet use is not finished. diff --git a/docs/source/architecture/decisions/0013-core-and-domain-terms.rst b/docs/source/architecture/decisions/0013-core-and-domain-terms.rst index 5a8471342..6177775d5 100644 --- a/docs/source/architecture/decisions/0013-core-and-domain-terms.rst +++ b/docs/source/architecture/decisions/0013-core-and-domain-terms.rst @@ -11,8 +11,8 @@ Context ``CONTEXT.md`` is one flat glossary. Every term in it reads as equally binding, and every place the code disagrees is filed under *Known legacy names* -- a -list whose framing is that the disagreement is debt, tolerated until someone -gets to it. +list whose framing is that the disagreement is a defect, tolerated until +someone corrects it. For most of the glossary that framing is right. But it is wrong for a small set of terms, and being wrong about those has produced the same review argument @@ -85,15 +85,15 @@ faithfully where it appears in returned data. An adapter is where the two meet. Its public surface uses its service's terms; what it passes to the shared modules uses the core terms. The -translation is the adapter's job, and a divergence at that boundary is the -design working rather than debt. +translation is the adapter's responsibility, and a divergence at that boundary +is the design working as intended rather than a defect. Two rules follow: - **A term the glossary does not define is not used in the glossary.** A word used in ``CONTEXT.md``'s prose needs an entry. Naming a term only to say what an ADR calls it is a cross-reference, not a definition, and - does not license using the word elsewhere. + does not permit using the word elsewhere. - **Only core misnamings are legacy.** *Known legacy names* records a core term the code spells wrongly and cannot be renamed. A domain term at an adapter's surface is not a legacy name; it is that adapter using its service's @@ -116,14 +116,14 @@ Consequences small cost per term and the reason the distinction is usable at all. - The package's own inconsistencies in core vocabulary become defects with a deadline rather than curiosities. The resolution chain's ``tier``-for-*source* - identifiers are the standing example. + identifiers are the current example. Compliance ---------- ``CONTEXT.md`` marks each domain term as such and names the per-service spellings in the entry itself, so a reader who needs to know whether a word is -negotiable can see it without asking. +allowed to vary can see it without asking. The mechanical part is that the glossary must define what it uses: ``tests/architecture_test.py`` asserts every ``ADR NNNN`` citation resolves, and diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index 3890cd5e1..28949674f 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -292,12 +292,12 @@ architecturally is the behavior around them: ``API_USGS_RETRIES`` Number of retries after the first attempt on supported active request paths; - defaults to four. Backoff is exponential with full jitter and honors bounded - ``Retry-After`` values. Only failures that may not recur on a later attempt - are re-sent: 429 and gateway 5xx, not a 500 rejecting the query itself, and - not a transport failure that is settled before the request leaves - (unresolvable host, unsupported scheme). Deprecated NWIS compatibility paths - do not opt in. + defaults to four. Backoff is exponential with full jitter and waits for + bounded ``Retry-After`` values. Only failures that may not recur on a later + attempt are re-sent: 429 and gateway 5xx, not a 500 rejecting the query + itself, and not a transport failure that is settled before the request + leaves (unresolvable host, unsupported scheme). Deprecated NWIS + compatibility paths do not opt in. ``API_USGS_STALL_TIMEOUT`` Seconds a call may go without receiving any data before retrying stops and @@ -310,8 +310,8 @@ architecturally is the behavior around them: short, and an attempt already in flight is never interrupted. This bound never withholds the first retry, so one slow attempt cannot disable retry by itself; after that, the budget decides whether to continue. A dead - connection therefore costs about two read timeouts rather than five - attempts' worth. + connection therefore costs about two read timeouts rather than five full + attempts. ``API_USGS_PROGRESS`` Controls best-effort progress display. Reporting failures must never change @@ -340,7 +340,7 @@ This view records categories and representative locations of debt. - ``ogc/engine.py`` retains a compatibility pagination wrapper alongside OGC orchestration. The sync-dispatch wrapper is gone: every retrieval path now - enters through ``transport.fanout.FanOut``. + goes through ``transport.fanout.FanOut``. - ``utils.py`` combines shaping with compatibility imports for metadata, ambient configuration, transport, and the query path. - ``waterdata/utils.py`` combines endpoint constants, argument normalization, diff --git a/docs/source/userguide/configuration.rst b/docs/source/userguide/configuration.rst index ebf31bce7..e462efaf9 100644 --- a/docs/source/userguide/configuration.rst +++ b/docs/source/userguide/configuration.rst @@ -67,7 +67,7 @@ planner, NGWMN runs two requests at a time, and WQP retries twice. Everything a configuration does *not* name still comes from below it, per setting: Water Data and NGWMN both retry six times and both send the ``api_key``, written once at the top of the file, because a configuration contributes what it names and -inherits the rest. Only WQP named ``retries``, so only WQP departs from the +inherits the rest. Only WQP named ``retries``, so only WQP differs from the file's six. Outside the block nothing has changed, and putting those two profiles in the @@ -233,8 +233,8 @@ useful for a container or a job scheduler that mounts secrets elsewhere. Per-adapter settings ~~~~~~~~~~~~~~~~~~~~ -To tune one service and leave the rest alone, name the adapter — the same name -you import: +To tune one service and leave the rest unchanged, name the adapter — the same +name you import: .. code-block:: toml @@ -391,7 +391,7 @@ keeps the rest: ... Values are validated when the configuration is *constructed*, so a typo raises -on the line you wrote it on rather than deep inside a later request. +on the line you wrote it on rather than inside a later request. Omitted settings inherit from an outer block or a lower-precedence source. Passing ``None`` explicitly suppresses those sources and restores built-in @@ -541,7 +541,7 @@ for the same reason — a variable that was quietly ignored would leave you believing you had redirected something. **The API key is not sent to the new host.** It is scoped to the one host that -honors it (:ref:`below `), so a redirected call +accepts it (:ref:`below `), so a redirected call goes out without it. That is deliberate: the host you redirected to is not the host you gave a credential to. If the mirror needs its own credential, it needs its own mechanism. @@ -584,7 +584,7 @@ organization's CA bundle: # or, for a directory of hashed certificates: export SSL_CERT_DIR=/etc/ssl/certs -``httpx`` honors these natively, so they apply to **every** getter in the +``httpx`` reads these natively, so they apply to **every** getter in the package — including the OGC collection getters (``get_daily``, ``get_continuous``, and the rest), which take no SSL parameter of their own. diff --git a/docs/source/userguide/errors.rst b/docs/source/userguide/errors.rst index c1444cdf2..ba68ac8a5 100644 --- a/docs/source/userguide/errors.rst +++ b/docs/source/userguide/errors.rst @@ -55,7 +55,7 @@ Retry transient failures with backoff ``.retryable`` and ``.retry_after`` make a backoff loop type-agnostic: one loop covers rate limits (429), server errors (5xx), and connection failures alike, -and honors the server's ``Retry-After`` hint when present: +and uses the server's ``Retry-After`` value when present: .. code-block:: python