diff --git a/CONTEXT.md b/CONTEXT.md index a6bbbef69..eda9288da 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -6,7 +6,7 @@ conversation use them the same way. Architectural decisions live in `docs/source/architecture/decisions/`. When a term here conflicts with a name in the code, the term wins and the name -is legacy. Legacy names are called out below rather than quietly tolerated. +is legacy. Legacy names are listed below. ## Retrieval @@ -47,8 +47,12 @@ A chunk of a large query commonly spans many pages. 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. The two answers are one -judgement about what a failure means, and must agree. +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. +ADR 0006 sets the policy and calls it the *no-progress budget*. **Interruption** — A transient failure that stopped a fan-out partway, raised with the completed chunks preserved. The caller may wait for the condition to @@ -112,8 +116,11 @@ elapsed time, and the response headers. Describes the *retrieval*, not the data. ## Configuration **Configuration profile** — A named set of settings for one adapter, stored in -the configuration file or built in code. **Configuration** is the short form. -A profile is an *input* to resolution, never its result. +the configuration file or built in code. A profile is an *input* to resolution, +never its result. In prose, **configuration** shortens this; in code, +`Configuration` is the package-wide settings class, and an adapter's profile is +its own `*Configuration` subclass. The two differ only by case, so prefer the +full term wherever a reader could take it either way. **Default profile** — The profile an adapter uses when no other is selected: the `[]` table's own keys. Always in effect. A **named profile** @@ -129,8 +136,9 @@ it. **Configure** is the verb for applying one. concurrency cap, the retry count, the progress line, the fan-out baseline. A setting means the same thing wherever it applies, but it does not apply everywhere: `concurrency` is meaningless to an adapter that issues one request -at a time, and `parallel_chunks` applies only to the two adapters whose queries -chunk. Which settings an adapter accepts is part of that adapter's vocabulary. +at a time, and `parallel_chunks` applies only to the adapters whose chunking the +planner can refine. Which settings an adapter accepts is part of that adapter's +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 @@ -145,15 +153,27 @@ alone; it does not replace the package-wide tier. 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 is the -counter-example that fixes the distinction: it belongs to the gateway fronting -a host, so Water Data and NGWMN — two adapters, one host — necessarily share -one key and one quota pool. Credentials are host-scoped; tunables are -adapter-scoped. - -**Source** — Where a setting's value came from. Sources are ordered, and the -order is resolved per setting rather than per source: a value supplied for one -setting does not displace another setting's value from a lower source. +adapter is what owns the conventions being tuned. The API key shows where the +boundary falls: it belongs to the gateway fronting a host, so Water Data and +NGWMN — two adapters, one host — necessarily share one key and one quota pool. +Credentials are host-scoped; tunables are adapter-scoped. + +**Source** — Where a setting's value came from, as one of the ordered +categories: a `configure()` block, the environment, the file, the built-in +default. The order is resolved per setting rather than per source: a value +supplied for one setting does not displace another setting's value from a lower +source. ADR 0010 calls a source a *tier* and ADR 0011 a *rung*; both are this +term, and the accepted records keep their own wording. + +**Origin label** — The exact thing a value came from, at finer grain than its +source: `$API_USGS_RETRIES`, a path to the config file, the profile a caller +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. + +The code carries both, and spells them the other way around: `_resolve` returns +its origin label under the name `source` and its source under the name `tier`. +Prose uses the terms above. **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 @@ -166,8 +186,8 @@ Package-wide. 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: an adapter able -to override an explicit setting would make that setting a lie. +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 @@ -183,6 +203,22 @@ value. Where the distinction matters — reporting what a call will actually use error shapes, and response quirks. Adapters may use shared machinery; shared machinery may not know about adapters. +**Dialect** — The per-API quirks the shared OGC machinery 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 +stays free of service names. + +**Single-shot adapter** — An adapter whose query is always exactly one request: +WQP, NLDI, StreamStats, and deprecated NWIS. Nothing divides and nothing +distributes, so `concurrency` and `parallel_chunks` are not part of its +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`. + **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 layout. @@ -207,6 +243,12 @@ Recorded so they are not mistaken for the canonical term, and not re-litigated: - `ChunkInterrupted` is a permanent alias of `FanOutInterrupted` — the same class object under the name it was first published as. Both spellings are correct; neither is scheduled for removal. +- *No-progress budget* is ADR 0006's name for the **stall timeout**. Both + spellings are current; the setting is `stall_timeout`. +- `ChunkedCall` is a permanent alias of `FanOut`, published on the OGC + compatibility path. Like `ChunkInterrupted`, both spellings 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. - `site` appears in deprecated NWIS and WQP parameter names where *monitoring location* is meant. These are frozen public surfaces and will not be renamed. Where the Water Data API itself names a thing `site-types` or @@ -216,7 +258,9 @@ Recorded so they are not mistaken for the canonical term, and not re-litigated: 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` - alias remain — a deprecated keyword and a permanent alias respectively. + alias remain — a deprecated keyword and a permanent alias respectively. Two + `OgcDialect` fields also still say it: `cql2_services` and + `date_only_services` are keyed by collection. `service` still means the external system in `transport` and `progress`, where it labels a progress line. That usage is correct. @@ -228,8 +272,8 @@ Recorded so they are not mistaken for the canonical term, and not re-litigated: 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 for a term with no better-evidenced - replacement in reach. + keyword costs a deprecation cycle, and no better-evidenced replacement is in + reach. - `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 3b5af121c..a16ef5d48 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -51,27 +51,25 @@ When reporting a bug, please include: ### Fixing Bugs Look through the GitHub [issues](https://github.com/DOI-USGS/dataretrieval-python/issues) -for known and unresolved bugs. Any issues labeled "bug" that are unassigned, -are open for resolution. You are welcome to comment in the relevant issue to -state your intention to resolve the bug, which will help ensure there is no -duplication of the same work by multiple contributors. +for known and unresolved bugs. Any unassigned issue labeled "bug" is open for +resolution. Comment on the issue to state that you intend to fix it, so that +two contributors do not do the same work. --- ## Code Contributions Code contributions should be made following a ["forking" workflow](https://docs.github.com/en/get-started/quickstart/contributing-to-projects). -This means that first, one should *fork* the repository, essentially creating a -personal mirror of the project. Next, you will want to create a *feature branch* -in your fork, which you can push code changes to. Once you have completed your -modifications and additions, open a pull request from the *feature branch* in -your fork, to the original upstream repository. +First *fork* the repository, creating a personal mirror of the project. Next, +create a *feature branch* in your fork and push your code changes to it. Once +your modifications and additions are complete, open a pull request from the +*feature branch* in your fork to the original upstream repository. ### Implementing Features Look through the GitHub [issues](https://github.com/DOI-USGS/dataretrieval-python/issues) for outstanding feature requests. Anything tagged with "enhancement" -and "please-help" is open to whomever wants to implement it. +and "please-help" is open to whoever wants to implement it. Please do not combine multiple feature enhancements into a single pull request. @@ -117,13 +115,14 @@ about the upstream service rather than about this package. This package keeps its general mechanisms in dependency-free leaves -- `_ambient.Ambient` for scoped context values, `config` for every setting (`API_USGS_*`, the config file, and `configure()` blocks all resolve through -it, and it is the only module that reads the environment for one), -`transport.links.resolve_next_url` for pagination cursors. Each of those has 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 or complicates anything. A grep -for the mechanism you are about to write is the only thing that does. +it, and it is the only module that reads the environment for a setting), +`transport.links.resolve_next_url` for pagination cursors. Each of those has +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. The continuous integration and pre-commit configurations enforce formatting, linting, and strict type checking. Run the relevant checks before opening a PR: @@ -152,28 +151,29 @@ 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 -colour a line green costs a real maintenance slot and catches nothing. If a -path cannot be reached without contorting the code, exclude it in +turn a line green 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. Both are better than a hollow test. +it is. Either costs less than a test that adds maintenance and catches nothing. The blocking run is a single Linux job. The OS/Python matrix reports its own number with `--fail-under=0`, because several tests are POSIX-only and a Windows run genuinely measures a smaller suite. -For the same reason the threshold assumes the whole suite: on Windows, or +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 ratchet. `xenon` and `complexipy` are complexity ratchets: the thresholds are the -tightest the package passes today, so they fail only when a change makes things -worse. They disagree usefully. `xenon` counts branches (cyclomatic complexity), -so a wide flat dispatch scores badly; `complexipy` counts how hard the control -flow is to follow (cognitive complexity), so it forgives the dispatch and -punishes nesting. Both name the offending block, so the fix is local -- usually -extracting a branch rather than restructuring. +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; +`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 +restructuring. `lint-imports` checks the dependency contracts declared in [`.importlinter`](.importlinter) against the *transitive* import graph: the layer @@ -203,7 +203,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 genuinely complex feature is information, not a failure. +gained a complex feature is information, not a failure. #### The periodic deep sweep @@ -211,12 +211,11 @@ Duplication, coupling, cohesion, dependency depth, and dead code are tracked by [`pyscn`](https://github.com/ludo-technologies/pyscn) on a weekly schedule ([code-health.yml](https://github.com/DOI-USGS/dataretrieval-python/blob/main/.github/workflows/code-health.yml)), which attaches an HTML and a JSON report to each run. Nothing gates on it. These -measures move over months rather than commits, and a threshold nobody agreed to -is either noise or theatre. +measures move over months rather than commits. -You do not need it to contribute, but it is the right tool for "what should we -clean up next?" -- including for an agent working on this repo, which gets a -whole-package structural picture from one command: +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: ```bash pip install -e '.[health]' # wheels: macOS ARM64, Linux x86-64, Windows x86-64 @@ -226,8 +225,8 @@ pyscn analyze dataretrieval # HTML report, or --json for the numbers Read its findings as leads, not verdicts. Its clone detector flags this package's per-collection getters -- thin, heavily documented wrappers whose -bodies necessarily rhyme -- and collapsing them into one parameterized function -would trade the documented public surface for a metric. Its +bodies are necessarily similar -- and collapsing them into one parameterized +function would trade the documented public surface for a metric. Its dependency-injection heuristics expect a class-oriented design this package deliberately does not have. @@ -261,6 +260,10 @@ link checking. code. #### Docstrings +* A docstring documents the *contract*. Rationale that argues for a rule binding + other files belongs in an ADR, cited by number; measurements, symptoms, and + what the code used to do belong in the commit message. See + [ADR 0000](docs/source/architecture/decisions/0000-documenting-decisions.rst). * Docstrings should follow the [numpy standard](https://numpydoc.readthedocs.io/en/v1.5.0/format.html): * Example: ``` python @@ -339,26 +342,23 @@ Documentation is built using [sphinx](https://www.sphinx-doc.org/en/master/), and is located within the `docs/source/` subdirectory in the repository. Documentation is written using [reStructuredText](https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html). -Contributions to the documentation should be made in a similar fashion to code -contributions - by following a forking workflow. When opening a pull request -please be sure to have tested your documentation modifications locally, and -clearly describe what it is your proposed changes add or fix. +Contributions to the documentation follow the same forking workflow as code +contributions. Before opening a pull request, test your documentation changes +locally, and describe what they add or fix. ### Adding Examples to the Documentation -A number of examples are provided in the documentation in the form of Jupyter -notebooks. These example notebooks are all contained within the `demos/` -subdirectory of the repository. If you have an example use of the package you -would like to add to the documentation as a run and rendered notebook, you -will need to do the following (in a separate branch of the repository): +The documentation includes examples as Jupyter notebooks, all of which live in +the `demos/` subdirectory. To add one that the documentation runs and renders, +do the following in a separate branch of the repository: 1. Add your notebook to the `demos/` subdirectory after clearing all outputs -2. Add a corresponding `.nblink` file to `docs/source/examples/` subdirectory, - see existing examples for reference, or refer to the [nbsphinx-link](https://nbsphinx-link.readthedocs.io/en/latest/) documentation. +2. Add a corresponding `.nblink` file to the `docs/source/examples/` + subdirectory; see the existing examples for reference, or the [nbsphinx-link](https://nbsphinx-link.readthedocs.io/en/latest/) documentation. 3. Add the example and some text describing it to one of the `.rst` files in the examples subdirectory. 4. Run the documentation locally to ensure it renders as you expect, and then - open a pull request wherein you describe the proposed addition. + open a pull request describing the addition. --- @@ -366,11 +366,11 @@ will need to do the following (in a separate branch of the repository): ### Submitting Feedback -The best way to send feedback is to open an issue at +Send feedback by opening an issue at https://github.com/DOI-USGS/dataretrieval-python/issues. -Please be as clear as possible in your feedback, if you are reporting a bug -refer to [Reporting Bugs](#reporting-bugs). +Please be as clear as possible. If you are reporting a bug, refer to +[Reporting Bugs](#reporting-bugs). ### Feature Requests diff --git a/dataretrieval/_configuration_core.py b/dataretrieval/_configuration_core.py index bfe402727..ee39ceee0 100644 --- a/dataretrieval/_configuration_core.py +++ b/dataretrieval/_configuration_core.py @@ -38,7 +38,7 @@ #: fans a query into more sub-requests, each of which spends rate-limit quota, #: and ``dataretrieval.parallel_chunks`` documents why that must stay a #: deliberate choice rather than a process-wide default. An environment -#: variable is the wrong shape for it -- exported once in a shell profile, +#: variable is process-wide and implicit -- exported once in a shell profile, #: inherited by every subprocess, invisible at the call site. A config-file #: entry is written deliberately and shows up in :func:`show_configuration`, so the #: file and :func:`configure` block are the only sources for it. @@ -51,21 +51,14 @@ } #: Variables the environment is *refused* for, by setting. Named rather than -#: simply left out of :data:`ENV_VARS`, because leaving them out only makes the -#: environment silent: a caller who exports ``API_USGS_BASE_URL`` -- the -#: spelling every other setting's variable predicts -- has redirected nothing -#: and would learn that from the traffic rather than from us. The file refuses -#: the same key in the same words (:func:`_accepted_keys`), for the reason ADR -#: 0011 gives: a redirect a shell profile or a config file can set is one no -#: reader of the script can see. +#: left out of :data:`ENV_VARS`, so a caller who exports ``API_USGS_BASE_URL`` +#: gets an error instead of a silently ignored variable. The file refuses the +#: same key in the same words (:func:`_accepted_keys`): a base URL arriving +#: from outside the code could redirect the library to another host without a +#: reader of the script seeing it (ADR 0011). #: -#: Derived from :data:`ADAPTER_ONLY_SETTINGS` rather than written out beside it, -#: because the two would be spelling one fact -- "this setting is code-only" -- -#: in two tables with nothing keeping them in step. A second adapter-only -#: setting added to the roster alone would be refused by the file (which reads -#: that roster) and *silently ignored* from the environment, which is exactly -#: the defect this table exists to prevent. The predicted spelling is the one -#: the comment above names, so deriving it changes nothing today. +#: Derived from :data:`ADAPTER_ONLY_SETTINGS` so the file and the environment +#: cannot drift apart on which settings are code-only. _REFUSED_ENV_VARS: dict[str, str] = { name: f"API_USGS_{name.upper()}" for name in ADAPTER_ONLY_SETTINGS } @@ -159,21 +152,16 @@ def __repr__(self) -> str: # nesting, per-key inheritance, and restore-on-exit keep falling out of a # single merge, whichever scope a block sets. _ScopeKey = str | tuple[str, str] -# One frame per ``configure`` block, stacked outermost-first. Frames rather than -# a merged mapping are what makes "the innermost block wins" true across *both* -# scopes: an adapter-scoped value outranks a package-wide one only within the -# same frame. Merged, an outer ``configure(WaterdataConfiguration(...))`` would -# beat an inner ``configure(Configuration(concurrency=1))`` -- inverting -# nesting, and silently discarding the per-call ``parallel_chunks(n)`` block. +# One frame per ``configure`` block, stacked outermost-first. Frames rather +# than a merged mapping: merged, an outer adapter-scoped block would beat an +# inner package-wide one, inverting the nesting rule ADR 0011 states. # # Each entry pairs the raw value with the label naming where it came from, the -# same shape the file tier returns (:func:`_adapter_file_settings`). The label -# is built while the configuration object is still in hand, because that is the -# only place the *profile* is known: a value from -# ``WaterdataConfiguration.load("bulk")`` and one from -# ``WaterdataConfiguration(...)`` are indistinguishable by the time they reach -# the frame, so a label rebuilt at resolution time could only ever say -# "some block", never which profile. +# same shape the file tier returns (:func:`_adapter_file_settings`). The +# label is built while the configuration object is still in hand, the only +# point where the *profile* is known -- by the time a value reaches the frame, +# one from ``WaterdataConfiguration.load("bulk")`` and one from +# ``WaterdataConfiguration(...)`` are indistinguishable. _Frame = Mapping[_ScopeKey, tuple[_SettingValue, str]] _scope: Ambient[tuple[_Frame, ...]] = Ambient("dataretrieval_configuration", ()) @@ -210,8 +198,8 @@ class _ParsedFile: base: dict[str, str] = field(default_factory=dict) #: Raw, *unvalidated* ``[]`` tables, keyed by adapter name. Each #: holds that adapter's default-profile keys and, as sub-tables, its named - #: profiles. Left unvalidated because a bad value in ``[nldi]`` must not - #: fail a Water Data call that never reads it. + #: profiles. Left unvalidated because an invalid value in ``[nldi]`` must + #: not fail a Water Data call that never reads it. adapters: dict[str, dict[str, Any]] = field(default_factory=dict) exists: bool = False @@ -227,21 +215,14 @@ class _ParsedFile: # A setting means the same thing wherever it applies, but it does not apply # everywhere (ADR 0010). Each adapter declares the settings it accepts as the # fields of a ``BaseConfiguration`` subclass, defined *in the adapter's own -# module* so a setting's definition sits with the code that reads it -- adding -# a Water Data setting no longer edits a service-neutral file (ADR 0011). The -# *which* is the adapter's own knowledge; the setting itself is drawn from the -# shared groups below, so ``retries`` is declared once rather than six times. +# module* (ADR 0011). Which settings an adapter accepts is the adapter's own +# knowledge; the setting itself is drawn from the shared groups below, so +# ``retries`` is declared once. # -# Two settings are deliberately absent from every adapter: -# -# ``api_key`` belongs to the gateway fronting a host, not to an adapter. -# Water Data and NGWMN are two adapters on one host sharing -# one key and one quota pool -- measured, see ADR 0010 -- so -# a per-adapter key would model a distinction that does not -# exist. ``credentials`` keeps sole ownership of it. -# ``progress`` describes the caller's terminal, not a service. There is one -# progress line per call, so scoping it per adapter could only -# produce a contradiction. +# Two settings are deliberately absent from every adapter (ADR 0010): +# ``api_key`` belongs to the gateway fronting a host and stays solely owned by +# ``credentials``; ``progress`` describes the caller's terminal rather than a +# service. #: Bound to the concrete subclass so ``WaterdataConfiguration.load(...)`` is #: typed as a ``WaterdataConfiguration`` rather than the base. ``typing.Self`` @@ -261,11 +242,7 @@ def _settings_of(cls: type[BaseConfiguration]) -> frozenset[str]: A class constant in everything but spelling: the fields cannot change after the class is created, and every adapter-scoped read asks for it -- through - :func:`_accepts`, before the frame walk and before the file, so the cost is - paid even when a ``configure`` block answers. Rebuilding the frozenset per - read measured as a fifth of an adapter-scoped resolution: two generator - passes over :func:`~dataclasses.fields` to rebuild six strings that cannot - have changed. + :func:`_accepts`, before the frame walk and before the file. Keyed on the *class* rather than on the adapter name because tests replace a registry entry to stand in for an unimported adapter; a name-keyed memo @@ -290,8 +267,8 @@ class BaseConfiguration: change under the block that entered it. Values are checked when the configuration is *constructed*, so a typo - raises where it was written rather than at a later ``with`` statement or, - worse, inside a request. + raises where it was written rather than at a later ``with`` statement or + inside a request. """ #: The adapter this configuration targets, by the name of the module a @@ -410,35 +387,17 @@ def _provenance(self) -> str: # --- shared setting groups ----------------------------------------------- # -# Which settings an adapter accepts is the adapter's own knowledge, and it says -# so by naming the groups below. What a setting *is* -- its type, its default, -# the fact that ``None`` suppresses the tiers under it -- is not: that is this -# module's, and it already was, since :func:`_coerce_typed` keys the type check -# by setting *name* and :data:`_VALIDATORS` holds the grammar. Spelling -# ``retries: int | None = _UNSET`` in six adapter modules therefore bought -# nothing and cost a guarantee: the annotations are decorative, so an adapter -# that drifted to ``retries: str | None`` would type-check clean under -# ``mypy --strict`` and fail only when a value reached the chain. -# -# So each group declares one shared setting once, and an adapter composes the -# groups it reads:: -# -# class NgwmnConfiguration( -# _Chunked, _Concurrent, _Redirectable, _Retrying, BaseConfiguration -# ): -# adapter: ClassVar[str] = "ngwmn" -# -# Widening a shared setting's accepted type, or adding one, is now one edit -# rather than six. Each adapter still documents the settings it takes in its own +# Each group declares one shared setting once; an adapter composes the groups +# it reads (ADR 0011). Each still documents the settings it takes in its own # ``Parameters`` section, because that is the signature a caller writes and # ``base_url`` means something different for every service. # -# Plain mixins rather than ``BaseConfiguration`` subclasses: a group is not a -# configuration -- it has no adapter and cannot be passed to :func:`configure` -# -- and keeping them off that branch of the tree leaves one linear base for the -# behavior. Frozen because a dataclass may not mix frozen and non-frozen bases. -# Fields are collected in reverse MRO order, so an adapter composing all four -# reads ``retries, stall_timeout, base_url, concurrency, parallel_chunks``. +# Plain mixins rather than ``BaseConfiguration`` subclasses: a group has no +# adapter and cannot be passed to :func:`configure`, so keeping it off that +# branch leaves one linear base for the behavior. Frozen because a dataclass +# may not mix frozen and non-frozen bases; fields collect in reverse MRO order, +# so an adapter composing all four reads ``retries, stall_timeout, base_url, +# concurrency, parallel_chunks``. @dataclass(frozen=True) @@ -540,9 +499,9 @@ class Configuration(BaseConfiguration): #: what the adapter side already does (:func:`settings_for`); only the #: package-wide side was hand-maintained. #: -#: Declared here, below the class, for the obvious reason: it cannot be derived -#: before the class exists. Every reader is a call-time lookup or a ``def`` -#: default evaluated further down the module. +#: Declared here, below the class, because it cannot be derived before the +#: class exists. Every reader is a call-time lookup or a ``def`` default +#: evaluated further down the module. SETTINGS: tuple[str, ...] = tuple(f.name for f in fields(Configuration)) #: Every setting name this module knows a grammar for. @@ -555,9 +514,9 @@ class Configuration(BaseConfiguration): #: #: Holding the names here rather than deriving them from the registry below is #: what lets a ``[nldi]`` table stay valid in a file: NLDI is imported on demand -#: for the geopandas extra, so a roster built from imports would reject a -#: perfectly good table until something happened to import that module, and the -#: verdict would vary by what a caller had touched. +#: for the geopandas extra, so a roster built from imports would reject a valid +#: table until something happened to import that module, and the verdict would +#: vary by what a caller had touched. ADAPTERS: tuple[str, ...] = ( "waterdata", "ngwmn", @@ -614,8 +573,6 @@ def _toml_parser() -> Any: ``import dataretrieval`` imports this module, but the parser is reachable only once a configuration file actually exists -- the minority case. - Importing it eagerly costs every caller ~4 ms of ``tomllib`` regex - compilation for a file most of them do not have. """ if sys.version_info >= (3, 11): import tomllib @@ -628,10 +585,9 @@ def config_path() -> Path: """Path to the configuration file, honoring ``DATARETRIEVAL_CONFIG``. Memoized on the raw ``DATARETRIEVAL_CONFIG`` value, because this sits on - the per-request path via :func:`api_key` and building the default costs - more than the ``stat`` it leads to (``Path.home()`` alone dominates the - whole resolution). Returning a stable object also lets :func:`_load_file` - check its cache by identity instead of re-normalizing a fresh ``Path``. + the per-request path via :func:`api_key`. Returning a stable object also + lets :func:`_load_file` check its cache by identity instead of + re-normalizing a fresh ``Path``. Returns ------- @@ -652,9 +608,7 @@ def config_path() -> Path: # is anchored to the working directory (a later ``os.chdir`` in a # per-job notebook or scheduler must not keep reading the previous # job's file); the default branch is anchored to ``$HOME``. An absolute - # override depends on neither and guards with ``None``. ``stat(".")`` - # identifies the directory ~17x cheaper than ``getcwd()``, which - # reifies the whole path string. + # override depends on neither and guards with ``None``. if cached_guard is None or cached_guard == _path_guard(cached_guard[0]): return path @@ -681,8 +635,8 @@ def _default_home_path() -> Path: ``Path.home()`` raises ``RuntimeError`` where no home can be resolved at all -- a rootless container running as an arbitrary UID with no passwd entry and no ``HOME``. That is not a misconfiguration to report: such a deployment - simply has no config file, and before settings were layered it worked fine - on the environment alone. So the unexpanded ``~/...`` form is returned + has no config file, and before settings were layered it worked on the + environment alone. So the unexpanded ``~/...`` form is returned instead: it does not exist, which keeps the whole file layer inert rather than failing every request from inside the header builder, and it still reads correctly in :func:`show_configuration` output. @@ -743,8 +697,8 @@ def _home_id() -> str: the path. Which variable that is differs by platform, and the memo has to agree with - the resolver or it watches the wrong thing. ``posixpath.expanduser`` reads - ``HOME``; ``ntpath.expanduser`` reads ``USERPROFILE`` (then + the resolver or it watches a different variable. ``posixpath.expanduser`` + reads ``HOME``; ``ntpath.expanduser`` reads ``USERPROFILE`` (then ``HOMEDRIVE``/``HOMEPATH``) and ignores ``HOME`` outright. Preferring ``HOME`` everywhere means that on Windows -- where Git Bash and MSYS do set it -- the memo invalidates on a variable that cannot move the path, and @@ -1083,7 +1037,7 @@ def _adapter_file_settings( one. The table's sub-tables are its named profiles, which are inert until a caller selects one, so they are skipped here (see :func:`_accepted_keys`). - Validated on first use, not at parse time, so a bad value in ``[nldi]`` + Validated on first use, not at parse time, so an invalid value in ``[nldi]`` cannot fail a Water Data call -- the blast-radius rule ADR 0010 set. """ table = parsed.adapters.get(adapter) @@ -1158,10 +1112,10 @@ def _cached_parse_by_metadata(path: Path, st: os.stat_result) -> _ParsedFile | N POSIX ``st_ctime_ns`` advances on any inode change, so the metadata stamp catches even a rewrite that restores the original mtime. Windows ctime is *creation* time, so there the stamp cannot see that class of edit and the - content compare in :func:`_parse_or_reuse_cache` is the only correct check - -- the re-read it forces is deliberate, and ``test_file_edit_is_picked_up`` - pins it. Do not drop the ctime gate (or extend the stamp to Windows) - without a Windows-safe change detector. + content compare in :func:`_parse_or_reuse_cache` is the only check that + catches it -- the re-read it forces is deliberate, and + ``test_file_edit_is_picked_up`` pins it. Do not drop the ctime gate (or + extend the stamp to Windows) without a Windows-safe change detector. """ cached = _file_cache if ( @@ -1217,10 +1171,10 @@ def _interpret(data: dict[str, Any], path: Path) -> _ParsedFile: Only the top-level table is validated here, because it always applies. An adapter's table is kept raw and validated when that adapter first resolves a - setting: a bad value in ``[nldi]`` must not fail a Water Data call, the same - blast-radius rule :func:`~dataretrieval.utils._default_headers` follows for - the key itself. It is also what lets an adapter's vocabulary live in the - adapter, which this module cannot import. + setting: an invalid value in ``[nldi]`` must not fail a Water Data call, + the same blast-radius rule :func:`~dataretrieval.utils._default_headers` + follows for the key itself. It is also what lets an adapter's vocabulary + live in the adapter, which this module cannot import. """ top: dict[str, Any] = {} adapters: dict[str, dict[str, Any]] = {} @@ -1292,10 +1246,10 @@ def _accepted_keys( if key not in allowed: if key in SETTINGS: # A real setting, in a table that does not read it. Unlike an - # unrecognized name -- which may simply belong to a newer - # release -- this cannot become meaningful later, and silently - # ignoring it would leave a caller believing they had tuned - # something. See ADR 0010. + # unrecognized name -- which may belong to a newer release -- + # this cannot become meaningful later, and silently ignoring it + # would leave a caller believing they had tuned something. See + # ADR 0010. raise ConfigurationError( f"{path}: {key!r} at {where} is not a setting that table " f"accepts. It accepts: {', '.join(sorted(allowed))}." diff --git a/dataretrieval/_deprecation.py b/dataretrieval/_deprecation.py index acb726314..de486f920 100644 --- a/dataretrieval/_deprecation.py +++ b/dataretrieval/_deprecation.py @@ -1,19 +1,10 @@ """One advisory mechanism, and one place to read the removal horizons. -Four spellings of "tell the caller something is going away" had grown up -independently -- a dated decorator in :mod:`~dataretrieval.nwis`, an undated -kwarg shim in :mod:`~dataretrieval.waterdata.utils`, an undated module-level -notice in :mod:`~dataretrieval.wqp`, and one bare :func:`warnings.warn` with -no category at all. Only one carried a date, so the horizons could not be -audited or bumped in one place, and the category was a per-author choice. - -The category matters more than the wording. A ``DeprecationWarning`` is a -promise that a *name in this package* is going away, so a downstream project -running ``-W error::DeprecationWarning`` is right to fail on it. An advisory -that an upstream *dataset* has stopped being updated is not that -- the API is -fine and the caller has nothing to migrate to -- and it belongs under -:class:`~dataretrieval.exceptions.DataCurrencyWarning` instead. See -:data:`REMOVALS` for the horizons this package has published. +Every deprecation is announced through this module, with a horizon in +:data:`REMOVALS` (ADR 0012). A ``DeprecationWarning`` promises that a *name in +this package* is going away, while an advisory that an upstream *dataset* has +stopped being updated belongs under +:class:`~dataretrieval.exceptions.DataCurrencyWarning` (ADR 0004). """ from __future__ import annotations diff --git a/dataretrieval/_querying.py b/dataretrieval/_querying.py index bb127fca5..aa360a122 100644 --- a/dataretrieval/_querying.py +++ b/dataretrieval/_querying.py @@ -136,7 +136,7 @@ def _single_request_policy(adapter: str | None = None) -> RetryPolicy: These services answer a rejected query with a 500, so only the gateway statuses are worth re-sending; the Water Data chunker keeps the broader - default, where a 5xx is an upstream hiccup worth riding out. + default, where a 5xx is a transient upstream failure worth riding out. ``adapter`` names which settings table supplies ``retries`` and ``stall_timeout`` -- these three services share a retry *shape* but not diff --git a/dataretrieval/_response_metadata.py b/dataretrieval/_response_metadata.py index 4cc520e7a..e19321ff0 100644 --- a/dataretrieval/_response_metadata.py +++ b/dataretrieval/_response_metadata.py @@ -1,11 +1,8 @@ """The metadata object every getter returns alongside its DataFrame. -A dependency-free leaf on purpose. This class is the second half of the -``(DataFrame, metadata)`` return contract, so nearly every service module needs -it -- and while it lived in :mod:`dataretrieval.utils` beside the legacy query -machinery, needing it meant inheriting that module's whole HTTP stack -(transport, credentials, error policy) transitively. Here it costs its -consumers nothing but ``httpx``. +A dependency-free leaf on purpose (ADR 0003). This class is the second half of +the ``(DataFrame, metadata)`` return contract (ADR 0007), so nearly every +service module needs it; importing it pulls in nothing but ``httpx``. ``dataretrieval.utils.BaseMetadata`` remains the public import. """ diff --git a/dataretrieval/_wqx.py b/dataretrieval/_wqx.py index dc21afa7d..733a1c5cb 100644 --- a/dataretrieval/_wqx.py +++ b/dataretrieval/_wqx.py @@ -3,9 +3,8 @@ The Samples database and the Water Quality Portal both split an instant across three columns -- a date, a time, and a time-zone abbreviation -- and they spell the trio two different ways. Recognizing either spelling and folding it into one -UTC column is knowledge about those response formats, so it lives in its own -leaf rather than in :mod:`dataretrieval.utils`, whose docstring reserves that -module for shaping that is not service-specific. +UTC column is service-specific knowledge, so it lives in its own leaf rather +than in :mod:`dataretrieval.utils` (ADR 0001). Depends on pandas and the time-zone table only; nothing here issues a request. """ diff --git a/dataretrieval/combining.py b/dataretrieval/combining.py index d052ea712..167c1814b 100644 --- a/dataretrieval/combining.py +++ b/dataretrieval/combining.py @@ -8,9 +8,7 @@ Separated from :mod:`dataretrieval.ogc.planning` so that module stays focused on *what* to split, while this module owns *how* to reassemble. -A top-level leaf rather than part of :mod:`dataretrieval.transport`: these are -pandas transforms over already-fetched results, with no HTTP or event-loop -concern, consumed by chunk planning and service fan-out as well as by pagination. +A top-level leaf rather than part of :mod:`dataretrieval.transport` -- ADR 0006. """ from __future__ import annotations diff --git a/dataretrieval/configuration.py b/dataretrieval/configuration.py index 4c6992bc4..d856ee9e6 100644 --- a/dataretrieval/configuration.py +++ b/dataretrieval/configuration.py @@ -18,23 +18,16 @@ selects it with ``Configuration.load("")``. 4. The built-in default. -Those are the four *sources*, which is the decomposition this module is built -around -- one branch each in :func:`_resolve`. ADR 0011 states the same order -as seven rungs by splitting three of them into the scopes inside: source 1 into -a configuration instance and a selected profile, which cannot disagree because -both name one adapter and two configurations for one adapter raise; source 3 -into the ``[]`` table above the top-level keys; and source 4 into an -adapter's own built-in preference above the package default. That last scope is -invisible here because this module never supplies it -- it arrives as the -``default`` a read site like :func:`concurrency` passes for its own service. +Each of those four sources is one branch of :func:`_resolve`; ADR 0011 lists the +same order in finer grain, splitting three of them into the scopes they contain. +An adapter's own default is not one of the four: a read site such as +:func:`concurrency` passes it in as the ``default`` argument. Precedence applies **per setting**, not per source: an environment that sets only -``API_USGS_PAT`` leaves a file-provided ``concurrency`` fully in effect. Putting -the environment above the file follows common deployment conventions and keeps -the original environment-variable interface authoritative (see ADR 0009) -- with -one exception ADR 0011 carves out: a profile named *in code* is a more -deliberate act than a variable inherited from a shell, and a profile reaches the -chain by being passed to :func:`configure`, which is above the environment. +``API_USGS_PAT`` leaves a file-provided ``concurrency`` fully in effect. The +environment ranks above the file (ADR 0009). ADR 0011 makes one exception: a +profile named *in code* reaches the chain through :func:`configure`, above the +environment. A caller configures by passing configuration objects, at most one per adapter:: @@ -46,24 +39,18 @@ ... Settings are scoped **per adapter** (ADR 0010): a ``[ngwmn]`` table in the file, -or an ``NgwmnConfiguration``, applies to NGWMN calls and no others, so one block -can be gentle with one service while leaving the rest alone. Precedence stays -*source-major*: the chain still walks block, then environment, then file, and an -adapter-scoped value outranks a package-wide one only *within* the same source. -So a variable exported for one run still beats a stale adapter table. Within the -block source that tie-break applies per block: an adapter configuration outranks -a package-wide value set by the same ``configure`` call, while a value set by a -block nested inside it wins over both, so the innermost block still decides. - -Which settings an adapter accepts is its own vocabulary -- ``concurrency`` means -nothing to an adapter that issues one request -- so each adapter declares them -on its own :class:`BaseConfiguration` subclass, defined in the module that -*reads* them. The API key is not among them: it belongs to the gateway fronting -a host, which Water Data and NGWMN share. +or an ``NgwmnConfiguration``, applies to NGWMN calls and no others. Precedence +stays *source-major* (ADR 0010): an adapter-scoped value outranks a package-wide +one only *within* the same source, and within the block source the innermost +block decides. + +Each adapter declares the settings it accepts on its own +:class:`BaseConfiguration` subclass, defined in the module that *reads* them +(ADR 0011). The API key is not among them -- it belongs to the gateway fronting +a host, not to an adapter (ADR 0010). This module is a leaf: it imports only the standard library plus the Python 3.10 -``tomli`` backport, so any module can depend on it without an import cycle or -pulling in httpx or pandas. That is also why it holds the adapter *names* but +``tomli`` backport (ADR 0009). That is also why it holds the adapter *names* but never imports an adapter -- see :data:`ADAPTERS`. It centralizes each setting's parser while retaining legacy environment behavior and stricter validation for the new Python/TOML surfaces. @@ -195,8 +182,8 @@ def configure(*configurations: BaseConfiguration) -> Iterator[None]: ------ ConfigurationError If an argument is not a configuration, or two of them target the same - adapter. Raised on entry, before any request. A bad *value* raises - earlier still, where the configuration was constructed. + adapter. Raised on entry, before any request. An invalid *value* + raises earlier still, where the configuration was constructed. Examples -------- @@ -355,7 +342,7 @@ class _ErrorDeduplicatingCell: most in need of explaining are the broken ones -- an unparseable file, a value that fails its grammar, a profile that no longer exists. Nothing here raises: each distinct failure is printed once, in the first place - it shows up; a repeat is collapsed, so one bad file does not bury the + it shows up; a repeat is collapsed, so one invalid file does not bury the rows that did resolve under ten copies of the same message. """ @@ -383,8 +370,8 @@ def _show_file_status( """Probe and print the config file status line, returning the parsed file. Probing the file once here means a whole-file problem -- unparseable TOML, - a bad value at the top level -- is reported on the file row rather than - repeated in every setting's row below. + an invalid value at the top level -- is reported on the file row rather + than repeated in every setting's row below. """ parsed = _NO_FILE try: @@ -526,8 +513,8 @@ def _show_unimported_adapters(out: TextIO) -> None: vocabulary has been imported, and NLDI is deliberately imported on demand for the geopandas extra. So the rows above cannot cover it. Omitting it silently would read as "nothing is configured for nldi", which is a - different claim and the wrong one -- this is the honest cost of validating - an adapter's keys lazily (ADR 0011). + different claim and an incorrect one -- this is the cost of validating an + adapter's keys lazily (ADR 0011). """ unimported = [a for a in ADAPTERS if settings_for(a) is None] if unimported: @@ -681,8 +668,10 @@ def _resolve(name: str, adapter: str | None = None) -> tuple[str | None, str, st Precedence is *source-major*: the chain walks block, then environment, then file, exactly as ADR 0009 defines it -- and *within* each source an adapter-scoped value outranks a package-wide one. So a variable exported - for one run still beats a stale ``[wqp]`` table in the config file, which - scope-major ordering would have quietly inverted (ADR 0010). + for one run still beats a stale ``[wqp]`` table in the config file -- + ordering by scope first, putting every adapter-scoped value ahead of every + package-wide one whatever its source, would have quietly inverted that + (ADR 0010). ``adapter`` names the adapter on whose behalf the setting is being read. ``None`` resolves the package-wide value, which is also what an adapter @@ -737,11 +726,9 @@ def _check_env_not_refused(name: str) -> None: """Raise if an environment variable is set for a code-only setting. Refused before anything is consulted, not at the environment's turn in - the chain. The file refuses ``base_url`` whether or not a block also set - one -- it raises while the file is read -- and the two surfaces are one - rule, so a variable that cannot work must not be silently outranked by a - block that happens to work. Unsetting it is the only fix, and the message - says so. + the chain. The file and the environment refuse ``base_url`` as one rule + (ADR 0011), so a variable that cannot work is not silently outranked by a + block that happens to work. """ refused = _REFUSED_ENV_VARS.get(name) if refused is not None and refused in os.environ: @@ -789,10 +776,7 @@ def _resolve_from_env(name: str) -> tuple[str | None, str, str] | None: def _resolve_from_file(name: str, scoped: str | None) -> tuple[str | None, str, str]: """Fall through to the configuration file, then the built-in default. - One load serves both file tiers. Reading the file twice -- once for the - adapter table, once for the top level -- cost a second stat on every - adapter-scoped resolution, and the common case (no table for this adapter) - is the one that paid it. + One load serves both file tiers. """ path, parsed = _current_file() diff --git a/dataretrieval/credentials.py b/dataretrieval/credentials.py index 66545ba8a..6e0b7ebdb 100644 --- a/dataretrieval/credentials.py +++ b/dataretrieval/credentials.py @@ -3,18 +3,13 @@ One leaf owns every answer about the ``API_USGS_PAT`` credential: the host that accepts it, whether a given destination qualifies, how it is stripped back off a request bound somewhere else, and which keyword names are a caller *asking* to -send it. Splitting those answers across the layers that happen to need them is -how a credential reaches a host nobody authorized: the code that attaches a key -and the code that removes it have to agree, and the only way to guarantee they -agree is to have them read the same predicate. - -This sits below HTTP mechanics (which attaches the header) and below progress -reporting (which tells an unauthenticated caller where to register), so neither -has to depend on the other to learn the same fact. Its only first-party -dependency is :mod:`dataretrieval.configuration`, which is itself a -standard-library-only leaf and sits directly beneath this module in the layers -contract -- it supplies the key's *value*, while the questions this module -owns are which host may receive it and how it is withheld from every other. +send it. ADR 0006 assigns that sole ownership, and ADR 0010 keeps the key out +of every adapter's settings. + +This sits below HTTP mechanics and below progress reporting in the layers +contract. Its only first-party dependency is +:mod:`dataretrieval.configuration` -- itself a standard-library-only leaf -- +which supplies the key's *value*. """ from __future__ import annotations @@ -58,12 +53,9 @@ def accepts_api_key(target_url: str | httpx.URL | None) -> bool: when deciding whether "get an API key" is useful advice rather than noise, so the three can't drift apart. - The scheme has to be ``https``, not just the host. A bearer token sent over - cleartext is readable by anything on the path, and the destination that would - receive it is reachable through data we do not control: a redirect, or a - server-supplied next-page link naming ``http://`` on the very host that is - otherwise authorized. Matching on the host alone would hand the key over in - the clear on the strength of a hostname the attacker chose to keep. + The scheme has to be ``https``, not just the host (ADR 0009): a redirect or + a server-supplied next-page link can name ``http://`` on the very host that + is otherwise authorized. """ if target_url is None: return False @@ -77,26 +69,20 @@ def accepts_api_key(target_url: str | httpx.URL | None) -> bool: def without_embedded_credentials(url: httpx.URL) -> httpx.URL: """Drop any ``user:pass@`` from a URL we were *handed* rather than built. - A next-page link is data, not configuration. ``httpx`` derives an - ``Authorization: Basic`` header from userinfo in a URL, so a poisoned link - carrying ``user:pass@`` mints a credential the caller never configured and - sends it onward -- next to the real API key, when the host still checks out - and the host check therefore raises nothing. No USGS service authenticates - that way, so stripping it costs a legitimate caller nothing. + A next-page link is data, not configuration, and ``httpx`` derives an + ``Authorization: Basic`` header from userinfo in a URL (ADR 0009). No USGS + service authenticates that way, so stripping it costs a caller nothing. """ return url.copy_with(userinfo=b"") if url.userinfo else url # Credential-shaped keyword names must never reach a getter's generic query # passthrough: URLs are retained by clients, proxies, logs, and response -# metadata. Kept here rather than in the adapter that first needed it, because -# the fact that motivates the check is package-wide -- ``configure()`` now takes -# ``Configuration(api_key=...)``, so a caller who has not read that far reaches -# for ``api_key=`` on whichever getter they are already calling, and every -# adapter with a ``**kwargs`` passthrough is that getter. +# metadata. The predicate lives in this leaf rather than in any one adapter so +# that ten getters cannot drift into ten spellings of it (ADR 0006). # # Matched as *substrings* of the separator-stripped name, not as exact names: -# an exact-match list missed the spelling the library's own docs make most +# an exact-match list misses the spelling the library's own docs make most # tempting -- ``x_api_key``, after the ``X-Api-Key`` header. _CREDENTIAL_MARKERS = ( "apikey", @@ -112,9 +98,9 @@ def without_embedded_credentials(url: httpx.URL) -> httpx.URL: # substrings without catching legitimate query parameters. # # ``session`` is deliberately absent from both lists: it carries no secret, so -# rejecting it with a credentials message told users 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 message. +# rejecting it with a credentials message reports an incorrect reason, and as a +# substring it claims part of a namespace the *server* owns -- any future query +# parameter containing it would be unreachable behind that message. _CREDENTIAL_NAMES = frozenset({"auth", "key", "pat", "pw"}) @@ -127,14 +113,11 @@ def refuse_credential_keywords(names: Iterable[str]) -> None: own list, so a spelling learned from one adapter's mistake is refused by the other on the same day. - This catches the plausible mistake; it is not a security control. Nothing - inspects *values*, so a secret pasted into ``state_name=`` travels just the - same, and the name space belongs to the server (``get_queryables``) rather - than to us. The point is to answer the caller who reasonably guesses that a - credential goes here, with a ``TypeError`` naming - ``with configure(Configuration(api_key=...)):`` instead of a token in a - URL (the bare call is a no-op -- ``configure`` is a context manager). It - errs toward rejecting for that reason. + A usability check, not a security control (ADR 0009). It answers the + caller who reasonably guesses that a credential goes here, with a + ``TypeError`` naming ``with configure(Configuration(api_key=...)):`` + instead of a token in a URL -- the bare call is a no-op, since + ``configure`` is a context manager. """ forbidden = set() for name in names: diff --git a/dataretrieval/exceptions.py b/dataretrieval/exceptions.py index 809d7c246..96cc1d6f3 100644 --- a/dataretrieval/exceptions.py +++ b/dataretrieval/exceptions.py @@ -270,7 +270,7 @@ class NetworkError(DataRetrievalError): retryable: ClassVar[bool] = True -# --- Bad configuration --------------------------------------------------- +# --- Invalid configuration ----------------------------------------------- class ConfigurationError(DataRetrievalError, ValueError): @@ -278,14 +278,9 @@ class ConfigurationError(DataRetrievalError, ValueError): request was issued -- an environment variable, a policy field, a malformed ``config.toml``, or a profile the file does not define. - It is a :class:`DataRetrievalError` so ``except`` around a retrieval catches - it rather than letting a bare ``ValueError`` escape a request path. That - matters because settings resolve lazily, on the request path: a broken - config file surfaces from inside whichever getter runs first, and belongs in - the same handler as any other failure of that call. It is *also* a - :class:`ValueError`, so code that already treats a bad setting as one keeps - working whether the value came from the environment, a file, or a - :func:`dataretrieval.configure` block. + Both bases matter: ``except DataRetrievalError`` around a retrieval + catches it, and code that already treats an invalid setting as a + :class:`ValueError` keeps working. The dual base is ADR 0009. """ @@ -318,7 +313,7 @@ class DataCurrencyWarning(UserWarning): Distinct from ``DeprecationWarning``, which promises that a *name in this package* is going away and gives the caller something to migrate to. Here - the API is fine and there is nothing to migrate: the service's own data + the API is unchanged and there is nothing to migrate: the service's own data has stopped moving, and only the caller can judge whether that matters. It is a ``UserWarning`` for that reason. Emitting it as a @@ -337,7 +332,7 @@ class SkippedItemWarning(UserWarning): The policy for batch getters whose items are independent documents: an item that fails *deterministically* -- so retrying would reproduce the failure -- is dropped from the result under a warning naming it, because - aborting would discard every other item's data over one bad entry. + aborting would discard every other item's data over one failing entry. Transient failures (429 / 5xx / timeouts / connection drops) are never skipped -- they are retried and, if retries run out, raised as a resumable interruption. Rate limiting in particular is systematic, so @@ -433,7 +428,7 @@ def parse_retry_after(value: str | None) -> float | None: except ValueError: pass else: - # ``inf``/``nan`` parse cleanly but poison every later comparison: an + # ``inf``/``nan`` parse without error but poison every later comparison: an # infinite hint would refuse retry forever and travel to the caller on # ``.retry_after``. Treat them as no hint at all. return max(0.0, seconds) if math.isfinite(seconds) else None diff --git a/dataretrieval/interruptions.py b/dataretrieval/interruptions.py index daa1510f9..a7aa09a9b 100644 --- a/dataretrieval/interruptions.py +++ b/dataretrieval/interruptions.py @@ -279,11 +279,10 @@ def _walk_causes( def _deterministic_failure(exc: BaseException) -> bool: """Whether a transport failure would fail identically on every retry. - An unsupported scheme or a request we built wrong is settled before a byte - goes out, and a hostname the resolver rejects outright won't be accepted on - the next attempt either -- so retrying only delays the error the caller - needs. A *temporary* resolver failure is not in that class and stays - retryable (see :data:`_PERMANENT_DNS_ERRORS`). + True for an unsupported scheme, a malformed request, or a hostname the + resolver rejects permanently. A *temporary* resolver failure is not in that + class and stays retryable (see :data:`_PERMANENT_DNS_ERRORS`). Bounding + retry to failures a later attempt could survive is ADR 0006. Walks ``__context__`` as well as ``__cause__``, because the original failure is several layers down and not always an explicit ``raise ... @@ -311,9 +310,9 @@ def _classify_transient( if isinstance(exc, TransientError): return ServiceInterrupted, exc.retry_after if isinstance(exc, (httpx.HTTPError, httpx.InvalidURL)): - # Some failures will fail the same way every time -- a bad scheme, a - # hostname that doesn't resolve. Offering to resume one would just - # hide the real error behind a retry that can never work. + # Some failures will fail the same way every time -- an unsupported + # scheme, a hostname that doesn't resolve. Offering to resume one + # would hide the real error behind a retry that can never work. if _deterministic_failure(exc): return None return ServiceInterrupted, None diff --git a/dataretrieval/ngwmn.py b/dataretrieval/ngwmn.py index ff68486c9..d44cf8dfd 100644 --- a/dataretrieval/ngwmn.py +++ b/dataretrieval/ngwmn.py @@ -49,12 +49,8 @@ ] -# The Water Data API base URL, from the credentials leaf rather than OGC policy -# internals: it names the same authority the API key is scoped to. Spelling the -# host out here instead would put a second copy of it in the package, which -# ``tests/architecture_test.py::test_credential_policy_has_one_definition`` -# rejects: the code that attaches the API key and the code that strips it at -# redirect time must not be able to disagree about which host is authorized. +# The Water Data API base URL, from the credentials leaf -- the one place the +# API-key host is named (ADR 0006). BASE_URL = WATERDATA_BASE_URL # The National Ground-Water Monitoring Network exposes its own OGC API at a @@ -116,9 +112,8 @@ def _get(service: str, local_vars: dict[str, Any]) -> tuple[pd.DataFrame, BaseMe service, output_id=_NGWMN_OUTPUT_ID, # A ``NgwmnConfiguration(base_url=...)`` from an enclosing block, or - # this service's own base. Resolved per call because the block is - # scoped to a ``with`` statement, and read here because this is the one - # place the NGWMN base is named. + # this service's own base. Read here because this is the one place the + # NGWMN base is named (ADR 0011). base_url=_configuration.base_url(adapter="ngwmn", default=NGWMN_OGC_API_URL), spatial=service == "sites", dialect=NGWMN_DIALECT, @@ -459,10 +454,8 @@ class NgwmnConfiguration( dials. The API key is not among them: one gateway fronts both adapters, so one key and one quota pool serve them (ADR 0010). - Lives here rather than in :mod:`dataretrieval.configuration` because - *which* settings a service reads is the service's own knowledge (ADR - 0011); what each of them means is shared, so the fields come from the - setting groups declared beside their grammar. + Declared here rather than in :mod:`dataretrieval.configuration` + (ADR 0011). Parameters ---------- @@ -483,10 +476,6 @@ class NgwmnConfiguration( rate-limit quota, so raise it only for pulls you know are large. """ - # NGWMN rides the same OGC engine as Water Data, so it reads the same - # groups: retry dials, a redirectable base, and both fan-out dials. The - # settings themselves are declared once in - # :mod:`dataretrieval.configuration`, beside the grammar that parses them. adapter: ClassVar[str] = "ngwmn" diff --git a/dataretrieval/nldi.py b/dataretrieval/nldi.py index c46afd931..3e12faa07 100644 --- a/dataretrieval/nldi.py +++ b/dataretrieval/nldi.py @@ -71,14 +71,7 @@ def _api_base() -> str: Every URL below is built from this rather than from :data:`NLDI_API_BASE_URL` directly, so a ``NldiConfiguration(base_url=...)`` - reaches every navigation, basin, and catalog request alike -- a redirect - that covered only some of them would leave the library asking the real - service about the mirror's data. Resolved per call, because a ``configure`` - block is scoped to a ``with`` statement rather than to the process. - - Six call sites, which is what this seam is for; choosing between the - redirect and the service's own base is the accessor's job, not each - service's. + reaches every navigation, basin, and catalog request alike (ADR 0011). """ return _configuration.base_url(adapter="nldi", default=NLDI_API_BASE_URL) @@ -108,7 +101,7 @@ def _features_to_gdf(feature_collection: dict[str, Any]) -> gpd.GeoDataFrame: upstream), and :func:`_query_nldi` returns ``{}`` when a 200 response carries no JSON body. ``GeoDataFrame.from_features`` raises on both cases (there's no geometry column to attach the CRS to), so return an empty - GeoDataFrame with the correct CRS instead of crashing. + GeoDataFrame carrying ``_CRS`` instead of crashing. """ features = feature_collection.get("features") if feature_collection else None if not features: @@ -722,10 +715,8 @@ class registers itself later than the rest -- which is exactly why the adapter roster lives in :data:`~dataretrieval.configuration.ADAPTERS` rather than being derived from what has been imported. - Lives here rather than in :mod:`dataretrieval.configuration` because - *which* settings a service reads is the service's own knowledge (ADR - 0011); what each of them means is shared, so the fields come from the - setting groups declared beside their grammar. + Declared here rather than in :mod:`dataretrieval.configuration` + (ADR 0011). Parameters ---------- @@ -741,9 +732,6 @@ class registers itself later than the rest -- which is exactly why environment refuse it. """ - # One request per call, so this service reads the retry dials and a - # redirectable base and no fan-out dial. Each setting is declared once, - # in :mod:`dataretrieval.configuration`, beside its grammar. adapter: ClassVar[str] = "nldi" diff --git a/dataretrieval/nwdc.py b/dataretrieval/nwdc.py index 614bf9585..dd38b5283 100644 --- a/dataretrieval/nwdc.py +++ b/dataretrieval/nwdc.py @@ -10,13 +10,9 @@ Unlike the main Water Data getters (:mod:`dataretrieval.waterdata`) and NGWMN (:mod:`dataretrieval.ngwmn`), the NWDC is a plain CSV REST service rather than -an OGC API Features collection. This module supplies the NWDC-specific bits — +an OGC API Features collection. This module supplies the NWDC-specific bits -- request building, CSV parsing, the ``Link``-header cursor, and the ``{detail}`` -error envelope. The service-neutral transport layer supplies cursor pagination, -response aggregation, client lifecycle, and sync-from-async dispatch. The module -follows the same conventions: host-scoped request headers, the typed -:class:`~dataretrieval.exceptions.DataRetrievalError` taxonomy, and a -``(DataFrame, BaseMetadata)`` return. +error envelope -- over the service-neutral transport layer (ADR 0006). See https://api.water.usgs.gov/docs/nwaa-data/ for the API reference and https://water.usgs.gov/nwaa-data/ for the catalog of available models and @@ -251,9 +247,8 @@ def get_wateruse( base_params = {k: v for k, v in base_params.items() if v is not None} # An ``NwdcConfiguration(base_url=...)`` from an enclosing block, or this - # service's own endpoint. Resolved once per call -- the block is scoped to - # a ``with`` statement -- and threaded through every request and the page - # walk, so a redirected call cannot half-follow the redirect. + # service's own endpoint, threaded through every request and the page walk + # (ADR 0011). service_url = _configuration.base_url(adapter="nwdc", default=WATERUSE_URL) # The NWDC queries one location per request, so fan a multi-value selector @@ -359,12 +354,10 @@ def _fan_out( carrying the NWDC ``detail``, and shape the result. :func:`~dataretrieval.transport.pagination.run_paginated` owns the rest. - The plan is the request list itself. The executor asks a plan only to be - sized and iterable, and the NWDC accepts one ``location=`` per request, so - the caller's locations arrive already separate -- there is nothing to - divide and so nothing for a plan class to hold. + The plan is the request list itself: the NWDC accepts one ``location=`` + per request, so the caller's locations arrive already separate (ADR 0008). - The broad retry status set is on purpose: NWDC reports a bad query as a 400 + The broad retry status set is on purpose: NWDC reports an invalid query as a 400 with a ``{"detail": ...}`` envelope, so unlike WQP and StreamStats its 5xx really is an upstream fault worth re-sending. """ @@ -478,10 +471,8 @@ class NwdcConfiguration(_Concurrent, _Redirectable, _Retrying, BaseConfiguration fans out per location rather than being divided along a URL byte budget. There is nothing for the planner to divide more finely. - Lives here rather than in :mod:`dataretrieval.configuration` because - *which* settings a service reads is the service's own knowledge (ADR - 0011); what each of them means is shared, so the fields come from the - setting groups declared beside their grammar. + Declared here rather than in :mod:`dataretrieval.configuration` + (ADR 0011). Parameters ---------- @@ -499,9 +490,6 @@ class NwdcConfiguration(_Concurrent, _Redirectable, _Retrying, BaseConfiguration Cap on simultaneous sub-requests, or ``"unbounded"``. """ - # One request per location, fanned out but never chunked, so this service - # reads the retry dials, a redirectable base and ``concurrency`` -- but not - # ``parallel_chunks``, which divides a query it never divides. adapter: ClassVar[str] = "nwdc" diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 44d74828f..b4e9c0563 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -125,7 +125,7 @@ def wrapper(*args: Any, **kwargs: Any) -> Any: def _parse_json_or_raise(response: httpx.Response) -> pd.DataFrame: - """Parse a JSON NWIS response, raising a helpful error on HTML responses.""" + """Parse a JSON NWIS response, raising an error that names an HTML body.""" try: return _read_json(response.json()) except (ValueError, JSONDecodeError) as e: diff --git a/dataretrieval/ogc/chunking.py b/dataretrieval/ogc/chunking.py index 8fa98dd00..00b1e163b 100644 --- a/dataretrieval/ogc/chunking.py +++ b/dataretrieval/ogc/chunking.py @@ -5,17 +5,16 @@ which splits along its top-level OR clauses. Any of them can fan the URL past the server's ~8 KB byte limit. ``ChunkPlan`` picks a fan-out for each axis that minimizes total chunks while keeping every -chunk URL under the budget. Requests that already fit get a -trivial single-step plan — the executor has one code path either way. +chunk URL under the budget. Requests that already fit get a single-chunk +plan — the executor has one code path either way. This module owns the OGC-specific half: the byte budget, the ``parallel_chunks`` dial, and the ``multi_value_chunked`` decorator that -ties a plan to a fetcher. Driving the resulting chunks to -completion — bounded concurrency, retry, failure precedence, resume — is -API-neutral and belongs to -:class:`dataretrieval.transport.fanout.FanOut`, which this module hands -its plan to. :class:`~dataretrieval.ogc.planning.ChunkPlan` satisfies -:class:`~dataretrieval.transport.fanout.FanOutPlan` structurally. +ties a plan to a fetcher. It hands the plan to +:class:`dataretrieval.transport.fanout.FanOut`, which drives the chunks to +completion; :class:`~dataretrieval.ogc.planning.ChunkPlan` satisfies +:class:`~dataretrieval.transport.fanout.FanOutPlan` structurally. That split +is ADR 0008. Parallel chunks: the planner is conservative by default — it splits only as far as the byte limit forces. A caller who knows their result is large can opt @@ -58,14 +57,9 @@ from .planning import ChunkPlan -# Compatibility aliases. ``ChunkedCall`` was this module's executor before it -# moved down to transport as the API-neutral ``FanOut``; ``get_active_client`` -# and ``_chunked_client`` named its shared per-call client. Only the -# chunking/progress test modules still use these names, and the rename is not -# worth churning them over -- package code imports the canonical spellings from -# :mod:`dataretrieval.transport.fanout`. They are aliases, not copies: the -# ambient in particular must be the *same* object transport publishes, or a -# test reading it here would never see the running client. +# Compatibility aliases for the chunking/progress test modules. The client +# names bind the *same* objects transport publishes, not copies -- a test +# reading a copy here would never see the running client. ChunkedCall = FanOut get_active_client = active_client _chunked_client = _active_client @@ -85,7 +79,7 @@ def parallel_chunks(n: int) -> Iterator[None]: By default the Water Data / NGWMN getters chunk a request only as much as the server's ~8 KB URL-byte limit forces — the fewest chunks that - fit. That is the safe default, but it can be *needlessly* conservative. + fit. That default can be more conservative than a large pull needs. Because every chunk paginates, splitting a large result further costs little or no extra quota *as long as each chunk still spans many pages* — rows-per-chunk far exceeding the page size (ten states pulled as @@ -97,14 +91,10 @@ def parallel_chunks(n: int) -> Iterator[None]: smoother progress, more even concurrency, and a smaller unit of retry/resume. - This is a *deliberate* per-call knob rather than an automatic behavior or a - process-wide environment variable, because the library can't tell in - advance whether a query is large (ten states over a short window might fit - in a single page, where extra chunks would only burn quota). Scoping it to - a ``with`` block keeps an aggressive setting from leaking into unrelated - calls and accidentally spending quota. Outside any block the getters use - the conservative default. Only the OGC getters (Water Data, NGWMN) read - this; wrapping a legacy NWIS call in the block is a harmless no-op. + A per-call knob rather than an environment variable, and scoped to a + ``with`` block: ADR 0009. Outside any block the getters use the + conservative default. Only the OGC getters (Water Data, NGWMN) read this; + wrapping a legacy NWIS call in the block is a no-op. Parameters ---------- @@ -116,7 +106,7 @@ def parallel_chunks(n: int) -> Iterator[None]: cannot multiply past it. The cap is a ceiling, never exceeded: the actual count is bounded below by what the ~8 KB URL limit already forces and above by ``n``. So an ``n`` larger than the input allows - simply yields one chunk per value, and with several multi-value + yields one chunk per value, and with several multi-value arguments the total may land somewhat below ``n`` because splits are whole (the plan can't always divide evenly onto ``n``). ``n=1`` asks for no extra fan-out. @@ -136,7 +126,7 @@ def parallel_chunks(n: int) -> Iterator[None]: ------ ValueError If ``n`` is not a positive integer — raised on ``with`` entry, before - any request is issued, so a bad value fails loudly rather than silently + any request is issued, so an invalid value fails loudly rather than silently doing nothing. Notes @@ -174,22 +164,13 @@ def parallel_chunks(n: int) -> Iterator[None]: -------- ChunkPlan._refine : the planning-side effect of ``n``. """ - # Fail loudly on a bad ``n`` at ``with`` entry, before any request -- and - # fail by the *setting's* grammar, not a second one written here. ``n`` is - # ``parallel_chunks``: the same bool/Integral rejection and the same lower - # bound, from the table that owns them, so raising the floor there cannot - # leave this block accepting a value the chain would then refuse. Spelled - # with the source label this block is written as, so the message names - # ``parallel_chunks(n)`` rather than the ``Configuration`` built below. - # ``ConfigurationError`` is a ``ValueError``, so callers catching that - # still catch this. + # Validate at ``with`` entry, before any request, through the setting's + # own parser -- one grammar, ADR 0009. The source label makes the message + # name ``parallel_chunks(n)`` rather than the ``Configuration`` built + # below. _configuration._validated_raw("parallel_chunks", n, "parallel_chunks(n)") - # Sugar for a package-wide ``Configuration`` rather than a second scope of - # its own: two competing ContextVars would let ``show_configuration()`` report a - # value the chunker does not use. Sharing one means the innermost block - # wins, whichever spelling opened it -- and package-wide rather than scoped - # to one adapter, because this block is a per-call request that must reach - # whichever adapter the call goes to. + # Reuses the one package-wide ``Configuration`` ContextVar rather than + # opening a second scope of its own (ADR 0009). with _configuration.configure(_configuration.Configuration(parallel_chunks=n)): yield diff --git a/dataretrieval/ogc/engine.py b/dataretrieval/ogc/engine.py index b50216957..b26c49170 100644 --- a/dataretrieval/ogc/engine.py +++ b/dataretrieval/ogc/engine.py @@ -127,8 +127,8 @@ def _ogc_parse_response( The parse strategy :func:`_walk_pages` hands to :func:`~dataretrieval.transport.pagination.paginate`. Coerces falsy cursors (empty href, etc.) to ``None`` so the paginate loop's - ``while cursor is not None`` terminates instead of spinning on a - meaningless value. + ``while cursor is not None`` terminates instead of looping forever on an + empty href, which ``is not None`` and so never ends the walk. """ body = resp.json() return ( @@ -247,18 +247,12 @@ def get_ogc_data( max_rows : int, optional Stop paginating once this many rows have been collected and truncate the result to exactly ``max_rows``. ``None`` (default) - fetches the full result. Intended for cheap previews of large, + fetches the full result. Intended for few-request previews of large, un-chunked tables (e.g. :func:`get_reference_table`). base_url : str OGC API base URL to target. Required: this package is API-neutral and names no API of its own, so each adapter passes its own base (e.g. - ``waterdata.utils.OGC_API_URL``, ``ngwmn.NGWMN_OGC_API_URL``). It was - once optional, falling back to whatever was in ambient scope -- which - defaults to the empty string, so omitting it built a *relative* - ``/collections/{id}/items`` that planning accepted and only httpx - rejected at send time, surfacing as a NetworkError about an unknown - service. Requiring it moves that mistake to the call site, where mypy - catches it. + ``waterdata.utils.OGC_API_URL``, ``ngwmn.NGWMN_OGC_API_URL``). spatial : bool Whether this collection's result contract includes feature geometry. The adapter supplies this semantic fact; ``skip_geometry`` and diff --git a/dataretrieval/ogc/interruptions.py b/dataretrieval/ogc/interruptions.py index cb88d03a8..3b76f12ee 100644 --- a/dataretrieval/ogc/interruptions.py +++ b/dataretrieval/ogc/interruptions.py @@ -1,16 +1,15 @@ """Deprecated alias for :mod:`dataretrieval.interruptions`. -The resume contract is no longer OGC-specific -- Water Use raises it too -- so -the classes live in :mod:`dataretrieval.interruptions`, where the base class is -named :class:`~dataretrieval.interruptions.FanOutInterrupted`. This path is the -one v1.2.0 published, when the classes were defined here. +The classes live in :mod:`dataretrieval.interruptions`, where the base class is +named :class:`~dataretrieval.interruptions.FanOutInterrupted`. That move, and +``ChunkInterrupted`` staying a permanent alias rather than a shim, are ADR 0008. +This path is the one v1.2.0 published, when the classes were defined here. Importing this module emits a :class:`DeprecationWarning` and re-exports the taxonomy. The re-exported objects are the *same objects*, not copies, so ``ogc.interruptions.ChunkInterrupted is dataretrieval.ChunkInterrupted`` and ``except`` clauses behave identically through either spelling. Only the module -*path* is deprecated: ``ChunkInterrupted`` itself remains a permanent alias of -``FanOutInterrupted``. +*path* is deprecated. ``dataretrieval.ogc.__init__`` deliberately does not import this module, so ``import dataretrieval`` and ``import dataretrieval.ogc`` stay silent. The diff --git a/dataretrieval/ogc/planning.py b/dataretrieval/ogc/planning.py index ba208a516..2693327c6 100644 --- a/dataretrieval/ogc/planning.py +++ b/dataretrieval/ogc/planning.py @@ -164,8 +164,8 @@ def _check_unchunkable_request( """Enforce the byte budget on a request with no chunkable axis. Passthrough when the single request fits or when the filter is in a - language the chunker doesn't manage (cql-json) — the server, not us, - judges that one. Raises + language the chunker doesn't manage (cql-json) — the server, not the + chunker, judges that request. Raises :class:`~dataretrieval.exceptions.Unchunkable` when the request is over budget and has nothing to split. """ @@ -333,7 +333,7 @@ class ChunkPlan: all axes, and stores the result. Passthrough requests (no chunkable axes, or already fitting) are - represented as a trivial plan with empty ``axes`` / ``chunks`` and + represented as a single-chunk plan with empty ``axes`` / ``chunks`` and ``total == 1``; :meth:`iter_chunk_args` yields the original args unchanged so the ``ChunkedCall`` loop is the same shape either way. @@ -549,7 +549,7 @@ def _largest_chunk_in( def _best_refine_candidate( self, total: int, max_chunks: int ) -> tuple[_Axis, int] | None: - """Find the best chunk to split during the refine pass. + """Choose the next chunk to split during the refine pass. Returns the largest splittable chunk (by atom count) among axes whose split stays within the ``max_chunks`` cap, or ``None`` when no @@ -621,11 +621,10 @@ def iter_chunk_args(self) -> Iterator[dict[str, Any]]: chunk_args[axis.arg_key] = axis.render(chunk) yield chunk_args - # ``total`` and ``iter_chunk_args`` are this class's domain vocabulary and - # stay as they are. The dunders are how a plan satisfies - # :class:`~dataretrieval.transport.fanout.FanOutPlan`, which asks for a - # sized iterable and nothing chunking-specific. They delegate rather than - # duplicate, so ``len(plan)`` cannot disagree with what iterating yields. + # ``__len__`` and ``__iter__`` delegate to ``total`` and + # ``iter_chunk_args`` so a plan satisfies + # :class:`~dataretrieval.transport.fanout.FanOutPlan` without a second + # count of its own. ADR 0008. def __len__(self) -> int: return self.total diff --git a/dataretrieval/ogc/requests.py b/dataretrieval/ogc/requests.py index f5ed26fe0..151fb50a6 100644 --- a/dataretrieval/ogc/requests.py +++ b/dataretrieval/ogc/requests.py @@ -2,10 +2,8 @@ The API to target and its quirks are explicit parameters (``base_url``, ``dialect``) -- construction states everything it needs. Queryables and schema -execution live in :mod:`dataretrieval.ogc.schema`, not re-exported from here -- -importing the schema helper only to forward it would give this module an edge -to the one part of OGC that executes HTTP, which is exactly what request -*construction* is supposed to be free of. +execution live in :mod:`dataretrieval.ogc.schema` and are not re-exported here, +so this module does not depend on the part of OGC that executes HTTP (ADR 0007). """ from __future__ import annotations diff --git a/dataretrieval/ogc/shaping.py b/dataretrieval/ogc/shaping.py index eb8a692b7..f59f4ba10 100644 --- a/dataretrieval/ogc/shaping.py +++ b/dataretrieval/ogc/shaping.py @@ -379,10 +379,8 @@ def _finalize_ogc( as :class:`~dataretrieval.utils.BaseMetadata`. Injected into the chunker as its ``finalize`` hook (see - :data:`~dataretrieval.ogc.chunking._Finalize`) so the - un-interrupted return *and* a resumed ``ChunkInterrupted.call.resume()`` - produce the same post-processed ``(DataFrame, BaseMetadata)`` shape, not - the chunker's raw frame and bare ``httpx.Response``. + :data:`~dataretrieval.ogc.chunking._Finalize`); ADR 0008 makes that hook + part of the fan-out contract. ``max_rows`` is applied here (after dedup/sort, on the *combined* frame) rather than only per-chunk, so a chunked call's total is bounded diff --git a/dataretrieval/progress.py b/dataretrieval/progress.py index ea7cc2d37..3d2acff82 100644 --- a/dataretrieval/progress.py +++ b/dataretrieval/progress.py @@ -14,13 +14,12 @@ page/row/rate-limit counts) both update without knowing about each other. Call :func:`progress_context` to activate one and :func:`current` to reach it. -This is a top-level leaf rather than part of :mod:`dataretrieval.transport`: it -is terminal presentation, not HTTP execution policy. Transport modules report -*into* it, so keeping it outside means the execution layer owns no rendering, and -every service adapter -- OGC or not -- reaches the same reporter. +This is a top-level leaf rather than part of :mod:`dataretrieval.transport`: +transport modules report *into* it and the execution layer owns no rendering +(ADR 0006), so every service adapter -- OGC or not -- reaches the same reporter. By default the line is shown for interactive use — an interactive terminal or a -Jupyter/IPython kernel, like ``tqdm`` — while redirected logs and CI stay clean. +Jupyter/IPython kernel, like ``tqdm`` — while redirected logs and CI get no line. ``API_USGS_PROGRESS`` forces it on (``1``/``true``) or off (``0``/``false``). """ @@ -131,8 +130,8 @@ def __init__( self.retry_note: str | None = None self._last_len = 0 # Whether anything was actually written to the stream — drives whether - # close() needs a terminating newline. (``current_chunk`` is a poor - # proxy: ``start_chunk`` sets it even when it doesn't render.) + # close() needs a terminating newline. (``current_chunk`` doesn't + # track that: ``start_chunk`` sets it even when it doesn't render.) self._rendered = False self._closed = False diff --git a/dataretrieval/streamstats.py b/dataretrieval/streamstats.py index f3b67074b..91a18ffa9 100644 --- a/dataretrieval/streamstats.py +++ b/dataretrieval/streamstats.py @@ -38,8 +38,7 @@ def _service_base() -> str: Both endpoints below hang off this, so a ``StreamstatsConfiguration(base_url=...)`` moves the whole service rather - than the one endpoint a caller happened to reach first. Resolved per call, - because a ``configure`` block is scoped to a ``with`` statement. + than one endpoint (ADR 0011). """ return _configuration.base_url(adapter="streamstats", default=STREAMSTATS_URL) @@ -249,10 +248,8 @@ class StreamstatsConfiguration(_Redirectable, _Retrying, BaseConfiguration): No fan-out dials: a StreamStats query is answered by a single request. - Lives here rather than in :mod:`dataretrieval.configuration` because - *which* settings a service reads is the service's own knowledge (ADR - 0011); what each of them means is shared, so the fields come from the - setting groups declared beside their grammar. + Declared here rather than in :mod:`dataretrieval.configuration` + (ADR 0011). Parameters ---------- @@ -267,9 +264,6 @@ class StreamstatsConfiguration(_Redirectable, _Retrying, BaseConfiguration): the file and the environment refuse it. """ - # One request per call, so this service reads the retry dials and a - # redirectable base and no fan-out dial. Each setting is declared once, - # in :mod:`dataretrieval.configuration`, beside its grammar. adapter: ClassVar[str] = "streamstats" diff --git a/dataretrieval/transport/fanout.py b/dataretrieval/transport/fanout.py index d812f7754..84a5a87b9 100644 --- a/dataretrieval/transport/fanout.py +++ b/dataretrieval/transport/fanout.py @@ -1,32 +1,22 @@ """Bounded, resumable fan-out execution over a plan of chunks. -A fan-out is one logical query the service forces into several requests. Two -unrelated reasons produce one: - -- a Water Data / NGWMN query whose URL exceeds the server's byte limit, split - along its multi-value axes by :class:`dataretrieval.ogc.planning.ChunkPlan`; -- a Water Use query naming several locations, which the NWDC accepts only one - at a time. - -Chunking is how you divide the data structurally; fan-out is how you distribute -the work operationally. The two are orthogonal, 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. Only -the Water Data / NGWMN case above involves chunking at all — Water Use fans out -without dividing anything, because the caller's locations were never one body to -split. - -So this module owns distribution and nothing else: concurrency bounded by a +A fan-out executes the chunks one logical query was split into: a Water Data / +NGWMN query whose URL exceeds the server's byte limit, split along its +multi-value axes by :class:`dataretrieval.ogc.planning.ChunkPlan`; or a Water +Use query naming several locations, which the NWDC accepts only one at a time. + +This module owns distribution and nothing else: concurrency bounded by a semaphore, per-attempt retry, deterministic failure precedence, sparse -completion tracking, and resume. It names no protocol concept — an adapter +completion tracking, and resume. It names no protocol concept -- an adapter supplies a :class:`FanOutPlan` (whatever structure it divided into, if any) and -an ``async def fetch(item) -> (df, response)``. +an ``async def fetch(item) -> (df, response)``. Dividing a query is protocol +knowledge and stays in OGC; distributing the pieces is protocol-neutral and +lives here. That split is ADR 0008. Concurrency: :meth:`FanOut._run` dispatches every pending chunk under one ``asyncio.gather`` sharing a single ``httpx.AsyncClient``. An ``asyncio.Semaphore`` -- not the client's connection pool, which is merely sized -to match -- caps the chunks in flight at ``N``; see :meth:`FanOut._run` -for why the gate must be the semaphore rather than the pool. +to match -- caps the chunks in flight at ``N`` (ADR 0008). The ``concurrency`` setting resolves ``N`` -- a ``configure()`` block, then ``API_USGS_CONCURRENT``, then the config file, and per adapter as well as package-wide: an integer N > 1 allows N chunks in flight; ``1`` forces @@ -94,8 +84,7 @@ # :func:`dataretrieval.configuration.concurrency`, which owns the setting's name, its # grammar (``1`` sequential, >1 bounded, ``unbounded`` uncapped) and its # built-in default. Naming any of those here too would let this module and the -# chain disagree about what a value means. The concurrency model -- why the cap -# is a semaphore rather than the connection pool -- is in the module docstring. +# chain disagree about what a value means. # --------------------------------------------------------------------------- @@ -108,35 +97,18 @@ class FanOutPlan(Protocol[_ChunkCo]): The contract a plan satisfies for a fan-out to execute it. A **plan** is defined in ``CONTEXT.md``. This protocol is that enumeration - and nothing more, which is why it is named for the role it plays here - rather than for its contents: + and nothing more: ``len`` must agree with the number of items iteration + yields, and iteration must repeat the same items in the same order, since + :meth:`FanOut.resume` keys completed work by position. The item type is + whatever an adapter's own ``fetch`` accepts -- this executor passes each + item through untouched and never inspects it, so the OGC getters yield + kwargs dicts while Water Use yields ready :class:`httpx.Request` objects. + The query's ``canonical_url`` is not part of the plan; it is an argument to + :class:`FanOut`. + :class:`~dataretrieval.ogc.planning.ChunkPlan` is *a* plan, and so is a - plain list of requests. - - Deliberately the two standard protocols rather than bespoke members, since - an enumeration of chunks is exactly ``__len__`` + ``__iter__`` -- so a - plain ``list`` of pre-built - requests satisfies this with no adapter class, and a real planner - satisfies it by delegating (see - :class:`~dataretrieval.ogc.planning.ChunkPlan`, whose domain vocabulary is - ``total`` / ``iter_chunk_args``). Naming them ``total`` and - ``iter_chunk_args`` here would mean two names for ``len`` that could report - different counts, and a shim class for every adapter whose chunks - are already a list. - - The item type is whatever an adapter's own ``fetch`` accepts: this executor - passes each item through untouched and never inspects it, so the OGC - getters yield kwargs dicts while Water Use yields ready - :class:`httpx.Request` objects. - - Iteration order is load-bearing: :meth:`FanOut.resume` keys completed work - by position, so a plan that yielded a different order on a second pass - would resume the wrong chunks. ``len`` must agree with the number of - items iteration yields — the usual contract for a sized collection. - - The identity of the query as a whole is *not* here: it is a value stamped - on the combined response, not a property of how the work divides, so it is - the ``canonical_url`` argument to :class:`FanOut`. + plain ``list`` of requests. Standard protocols rather than custom + members: ADR 0008. """ def __len__(self) -> int: ... @@ -473,7 +445,7 @@ def resume(self) -> tuple[pd.DataFrame, Any]: Idempotent: only chunks whose index isn't already in ``self._chunks`` are re-issued. Item order is the plan's own and is deterministic, so a partial completion (sparse indices) - resumes correctly. + resumes onto the same items. Returns ------- @@ -592,17 +564,9 @@ async def _run(self, max_concurrent: int | None) -> tuple[pd.DataFrame, Any]: (``httpx.Limits(max_connections=N, max_keepalive_connections=N)``) so the in-flight fetches reuse keepalive connections. - The semaphore, not the pool, is deliberately the throttle. If the - pool throttled instead, the excess chunks would queue - *inside* httpx waiting for a connection, and that wait counts - against the pool-acquire timeout (60 s, from ``HTTPX_ASYNC_DEFAULTS``). - A batch of slow pages that keeps every connection busy past that - window would then trip ``httpx.PoolTimeout`` on the queued tail — - a purely client-side failure that consumes the retry budget and - surfaces as a spurious resumable ``ServiceInterrupted``. Holding - chunks at the semaphore keeps them out of the pool until a - slot frees, so the pool timeout only fires for a genuinely stuck - connection. + The semaphore, not the pool, is the throttle (ADR 0008); holding + chunks at the semaphore keeps them out of the pool, so the pool + timeout only fires for a genuinely stuck connection. The shared client is published on :data:`_active_client` so the paginated-loop helpers reuse its connection pool. @@ -630,11 +594,9 @@ async def _run(self, max_concurrent: int | None) -> tuple[pd.DataFrame, Any]: holding the sparse completed chunks; ``.call.resume()`` re-issues the unfinished ones. """ - # The semaphore is the throttle; the pool is merely sized to match - # it. Left at httpx's default client limits (``max_connections=100``, - # keepalive 20) the pool would bottleneck a wider cap or churn - # connections by keeping too few alive. See the method docstring for - # why the gate can't be the pool itself. ``unbounded`` + # At httpx's default client limits (``max_connections=100``, + # keepalive 20), the pool would bottleneck a wider cap or churn + # connections by keeping too few alive. ``unbounded`` # (``max_concurrent=None``) is a degenerate cap at the plan total — a # semaphore that can never block — so gated is the only code path. limits = httpx.Limits( diff --git a/dataretrieval/transport/liveness.py b/dataretrieval/transport/liveness.py index 983e82e1d..d72ba3d81 100644 --- a/dataretrieval/transport/liveness.py +++ b/dataretrieval/transport/liveness.py @@ -1,7 +1,7 @@ """When data last arrived, shared by the loops that produce and consume it. A retrieval can be slow for two very different reasons: it is downloading a lot -(fine, however long it takes) or it is receiving nothing at all (worth giving up +(progressing, however long it takes) or it is receiving nothing at all (worth giving up on). Telling those apart needs one fact -- when data last arrived -- that the page-walking loop knows and the retry loop acts on. Keeping it in this leaf lets both point *down* at it rather than at each other, and leaves any future producer diff --git a/dataretrieval/transport/pagination.py b/dataretrieval/transport/pagination.py index 721c81b49..3312cb595 100644 --- a/dataretrieval/transport/pagination.py +++ b/dataretrieval/transport/pagination.py @@ -46,8 +46,7 @@ async def _client_for( """Borrow a client: the caller's, else the running drive's, else a new one. Preferring the executor's published client over a fresh one keeps every - page of every request on one connection pool. Both callers wanted that and - each spelled it itself before it moved here. + page of every request on one connection pool. """ borrowed = client if client is not None else active_client() if borrowed is not None: @@ -178,8 +177,8 @@ def run_paginated( The adapter supplies its strategies (``parse_response``, ``follow_up``, ``raise_for_status``, and optionally ``finalize``); this driver owns the - composition three adapters used to copy -- each request paginated on the - client the executor publishes unless ``client`` is injected, the retry + composition -- each request paginated on the client the executor publishes + unless ``client`` is injected, the retry policy, bounded concurrency, and the canonical URL the aggregate reports (the first request's, unless overridden). diff --git a/dataretrieval/transport/retry.py b/dataretrieval/transport/retry.py index d38d61454..91af0c956 100644 --- a/dataretrieval/transport/retry.py +++ b/dataretrieval/transport/retry.py @@ -26,16 +26,11 @@ ) # Which error statuses a request may be re-sent for. Both are narrower than -# :attr:`~dataretrieval.exceptions.DataRetrievalError.retryable`, deliberately: -# that field tells a caller re-issuing *might* work, while spending someone's -# quota unasked needs a stricter bar. -# -# The default keeps every 5xx, because for a query interface like the Water Data -# OGC API a 500 is an upstream hiccup and re-sending is how a chunked call rides -# one out. The gateway-only set is for the single-shot adapters whose services -# answer a *bad query* with a 500 -- WQP does that for an over-large request, -# StreamStats for out-of-network coordinates -- where re-sending multiplies load -# on a request that can never succeed and delays the caller's error. +# :attr:`~dataretrieval.exceptions.DataRetrievalError.retryable`. The default +# keeps every 5xx, for chunked calls riding out a transient upstream failure; +# the gateway-only set is for the single-shot adapters whose service answers a +# *rejected query* with a 500 -- WQP for an over-large request, StreamStats for +# out-of-network coordinates. Which failures may be re-sent is ADR 0006. _RETRYABLE_STATUSES = frozenset({429, *range(500, 600)}) _GATEWAY_STATUSES = frozenset({429, 502, 503, 504}) _RETRY_BASE_BACKOFF = 0.5 @@ -275,7 +270,7 @@ async def retry_async( holding one while it isn't touching the server, and the time spent waiting for it is credited back to the no-progress budget rather than counted as silence. A caller that gated its own body would have to rediscover both, and - nothing would catch it getting them wrong. + nothing would flag a caller that broke either. """ policy = RetryPolicy.from_configuration() if policy is None else policy attempt = 0 diff --git a/dataretrieval/utils.py b/dataretrieval/utils.py index 924f88816..0eda32048 100644 --- a/dataretrieval/utils.py +++ b/dataretrieval/utils.py @@ -1,14 +1,13 @@ -"""Data-shaping helpers, and the historical home of the legacy query path. +"""Data-shaping helpers, plus re-exports for names documented at this path. What is *defined* here is frame munging that names no service: building a UTC datetime column out of the separate date/time/zone columns a caller points at. -The one-shot HTTP query path that used to sit alongside it now lives in -:mod:`dataretrieval._querying`, and the WQX3 / legacy-WQP column conventions -live in :mod:`dataretrieval._wqx`; nothing here depends on either -- the names -below are re-exported so their documented ``dataretrieval.utils`` paths keep -resolving. +The one-shot HTTP query path lives in :mod:`dataretrieval._querying` and the +WQX3 / legacy-WQP column conventions in :mod:`dataretrieval._wqx`; nothing here +depends on either -- the names below are re-exported so their documented +``dataretrieval.utils`` paths keep resolving. -By default, do not add new service-specific behavior here. +By default, do not add new service-specific behavior here (ADR 0001). """ from __future__ import annotations diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 357ada2e2..5cc5081c5 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -3,7 +3,7 @@ The other families expose a fixed argument per filter, which covers the common cases and keeps them discoverable. This is the escape hatch: an arbitrary CQL2 filter against any collection, for the query nobody anticipated. Prefer a typed -getter when one fits -- it validates more and reads better. +getter when one fits -- it validates more and names its filters. """ from __future__ import annotations diff --git a/dataretrieval/waterdata/endpoints.py b/dataretrieval/waterdata/endpoints.py index 4a8cc6e63..c9efea66d 100644 --- a/dataretrieval/waterdata/endpoints.py +++ b/dataretrieval/waterdata/endpoints.py @@ -1,15 +1,9 @@ """Every Water Data endpoint this package talks to, in one place. -"Which services does Water Data reach, and at what URL" is a single question -with a single answer, so the answer lives in one file rather than being spelled -out again in each family module. The host is the authority of the credentials -leaf -- the host that serves these endpoints is the host that honors the API -key -- while the paths below stay here rather than importing OGC policy -internals. - -This module imports only leaves -- the credentials host and the configuration -chain -- so a family module can name its endpoint, and honor a caller's -redirect, without also taking on an OGC or transport edge. +The host is the authority of the credentials leaf -- the host that serves +these endpoints is the host that honors the API key -- while the paths below +stay here rather than importing OGC policy internals. This module imports only +leaves: the credentials host and the configuration chain (ADR 0003). """ from __future__ import annotations @@ -18,9 +12,8 @@ from dataretrieval.credentials import WATERDATA_BASE_URL #: Canonical paths below the Water Data root. They are not endpoints on their -#: own: callers obtain complete destinations through the functions below, which -#: makes scoped redirection part of endpoint acquisition rather than a wrapper -#: every use site must remember. +#: own: callers obtain complete destinations through the request-time functions +#: below (ADR 0011). _OGC_API_PATH = "/ogcapi/v0" _SAMPLES_PATH = "/samples-data" _STATISTICS_API_PATH = "/statistics/v0" diff --git a/dataretrieval/waterdata/ratings.py b/dataretrieval/waterdata/ratings.py index 4c1b6ea65..683cdf2c3 100644 --- a/dataretrieval/waterdata/ratings.py +++ b/dataretrieval/waterdata/ratings.py @@ -385,7 +385,7 @@ def _download_all( The plan is the feature list itself -- ``FanOut`` asks a plan only to be sized and iterable -- so the downloads get bounded concurrency, per-attempt retry, the progress line, and the resumable interruption - taxonomy in place of the previous serial loop, which had none of them. + taxonomy. Failure policy (rationale on :class:`~dataretrieval.exceptions.SkippedItemWarning`): a *transient* diff --git a/dataretrieval/waterdata/reference.py b/dataretrieval/waterdata/reference.py index 84a683588..8791b5576 100644 --- a/dataretrieval/waterdata/reference.py +++ b/dataretrieval/waterdata/reference.py @@ -54,9 +54,9 @@ def get_reference_table( call. max_rows : int, optional Cap the total number of rows returned, stopping pagination early - instead of downloading the whole table. Useful for cheaply - previewing large tables (e.g. ``hydrologic-unit-codes`` has ~125k - rows). Unlike ``limit`` (the per-page size), this bounds the total + instead of downloading the whole table. Useful for previewing + large tables in a few requests (e.g. ``hydrologic-unit-codes`` has + ~125k rows). Unlike ``limit`` (the per-page size), this bounds the total result. The default (None) downloads every page. Returns @@ -97,7 +97,8 @@ def get_reference_table( # Give the ID column the collection name, singularized and underscored. # ``removesuffix`` rather than an ``endswith`` branch, whose non-plural arm - # was unreachable and would stay correct if a singular collection appeared. + # was unreachable; ``removesuffix`` returns a singular collection name + # unchanged if one appears. if collection in ("counties", "countries"): output_id = collection.removesuffix("ies") + "y" else: diff --git a/dataretrieval/waterdata/stats.py b/dataretrieval/waterdata/stats.py index f6e5fd15d..2ec7eb9e1 100644 --- a/dataretrieval/waterdata/stats.py +++ b/dataretrieval/waterdata/stats.py @@ -2,10 +2,9 @@ Wraps ``https://api.waterdata.usgs.gov/statistics/v0`` — the daily-statistics service (period-of-record and date-range normals/intervals). This is a -*separate*, non-OGC API: it has no chunkable multi-value axes, so it drives -:func:`dataretrieval.transport.pagination.paginate` as a one-item -:class:`~dataretrieval.transport.fanout.FanOut` rather than going through -``multi_value_chunked``. The typed getters +*separate*, non-OGC API with no chunkable multi-value axes, so it runs as a +one-item :class:`~dataretrieval.transport.fanout.FanOut` rather than going +through ``multi_value_chunked`` (ADR 0008). The typed getters ``get_stats_por`` and ``get_stats_date_range`` in :mod:`dataretrieval.waterdata.api` call :func:`get_data` here. @@ -212,13 +211,6 @@ def get_data( processes results, and formats output according to the specified parameters. - The stats path doesn't go through ``multi_value_chunked`` (its query - shape has no chunkable list axes), so it drives transport pagination as a - one-item :class:`~dataretrieval.transport.fanout.FanOut`. The executor - runs the pagination loop in a short-lived worker thread, so this works - whether or not the caller is already inside an event loop, and the single - request gets the same retry and resume semantics as every other getter. - Parameters ---------- args : Dict[str, Any] @@ -250,7 +242,7 @@ def get_data( The typed subclass for an HTTP error response (see :func:`transport.pagination.paginate`); or :class:`~dataretrieval.exceptions.NetworkError` if the request - can't reach the service in a way retrying cannot fix (bad scheme, + can't reach the service in a way retrying cannot fix (unsupported scheme, a hostname that does not resolve), the ``httpx`` exception chained on ``__cause__``. FanOutInterrupted diff --git a/dataretrieval/waterdata/utils.py b/dataretrieval/waterdata/utils.py index a41ef7f86..cfdf10c65 100644 --- a/dataretrieval/waterdata/utils.py +++ b/dataretrieval/waterdata/utils.py @@ -9,9 +9,8 @@ OGC machinery (request construction, pagination, response shaping, the chunked ``get_ogc_data`` entry point) lives in :mod:`dataretrieval.ogc` and its implementation submodules. This adapter consumes the public facade -for dialects, argument normalization, and retrieval; callers that need an OGC -implementation helper import its canonical module directly rather than using -this module as a re-export layer. +for dialects, argument normalization, and retrieval; it is not a re-export +layer for OGC helpers (ADR 0003). """ from __future__ import annotations @@ -175,9 +174,9 @@ def _with_state(local_vars: dict[str, Any], *, to: str, into: str) -> dict[str, format-flexible parameter (full name / postal / FIPS); it is normalized via :func:`~dataretrieval.codes.states.to_state` to the ``to`` representation and stored under ``into`` (the API parameter this endpoint filters on). - It is additive sugar over the native ``state_code`` / ``state_name`` - parameters, which still accept the API's raw values (e.g. non-US FIPS); - passing ``state`` together with either raises ``ValueError``. + It adds to, rather than replaces, the native ``state_code`` / + ``state_name`` parameters, which still accept the API's raw values (e.g. + non-US FIPS); passing ``state`` together with either raises ``ValueError``. """ # Flatten ``**queryables`` first so a native state param arriving that way # (e.g. ``get_time_series_metadata``'s ``state_code``, which isn't an diff --git a/dataretrieval/wqp.py b/dataretrieval/wqp.py index f516a4891..595d47df2 100644 --- a/dataretrieval/wqp.py +++ b/dataretrieval/wqp.py @@ -656,10 +656,7 @@ def _service_base() -> str: The portal serves the legacy and WQX3 interfaces from one root under different paths, so a ``WqpConfiguration(base_url=...)`` names that root and - both follow it. Redirecting only the interface a caller happened to use - first would leave the other pointed at the service they were trying not to - talk to. Resolved per call, because a ``configure`` block is scoped to a - ``with`` statement. + both follow it (ADR 0011). """ return _configuration.base_url(adapter="wqp", default=_WQP_BASE_URL) @@ -824,10 +821,8 @@ class WqpConfiguration(_Redirectable, _Retrying, BaseConfiguration): No fan-out dials: a WQP query is answered by a single request, so a concurrency cap could only report a number nothing honours. - Lives here rather than in :mod:`dataretrieval.configuration` because - *which* settings a service reads is the service's own knowledge (ADR - 0011); what each of them means is shared, so the fields come from the - setting groups declared beside their grammar. + Declared here rather than in :mod:`dataretrieval.configuration` + (ADR 0011). Parameters ---------- @@ -843,9 +838,6 @@ class WqpConfiguration(_Redirectable, _Retrying, BaseConfiguration): the environment refuse it. """ - # One request per call, so this service reads the retry dials and a - # redirectable base and no fan-out dial. Each setting is declared once, - # in :mod:`dataretrieval.configuration`, beside its grammar. adapter: ClassVar[str] = "wqp" diff --git a/docs/source/architecture/decisions/0000-documenting-decisions.rst b/docs/source/architecture/decisions/0000-documenting-decisions.rst new file mode 100644 index 000000000..34089e609 --- /dev/null +++ b/docs/source/architecture/decisions/0000-documenting-decisions.rst @@ -0,0 +1,155 @@ +ADR 0000: Record each explanation once, in the place that owns it +================================================================= + +Status +------ + +Accepted + +Amended after acceptance under the reader's-position rule below; the ``Notes`` +section records the clause added. + +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 +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. +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 +65 docstring lines while citing ADRs 0009, 0010, and 0011 in the course of it, +the no-progress budget is argued from first principles in five places across +``transport/``, and which failures may be retried is enumerated in three lists +that can drift apart. + +Duplication is not a tidiness problem here; it is a correctness problem. Every +copy is a place the rule can be updated while the others are not, and the audit +found copies that had already gone stale -- an overview paragraph describing +concurrency caps that a later ADR had removed, and an ADR clause describing a +credential rejection the code deliberately no longer performs. A reader has no +way to tell which copy is current. + +Decision +-------- + +Each explanation is recorded once, in the venue that owns that kind of +knowledge, and referenced from anywhere else that needs it. + +**Docstrings own the contract.** What a caller must know to use the documented +object: the numpydoc ``Parameters``, ``Returns``, ``Raises``, and ``Examples`` +sections, what the function does, and what it guarantees. Public getters keep +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. + +**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. + +**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 +somewhere else. The code cites the record by number rather than restating its +argument. Adding a clause to an existing ADR is preferred over a new record; +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. + +Three rules follow: + +- **Cite, do not restate.** Prose that argues for a rule an ADR already owns is + 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. +- **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 + that fact before the Decision text rather than after it. + +Consequences +------------ + +- A rule has one current statement, so updating it cannot leave stale copies + behind in modules nobody thought to grep. +- Reading a module gets slower in one respect: some rationale now requires + 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. +- 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. + +Compliance +---------- + +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 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. +``tests/architecture_test.py`` asserts that every ``ADR NNNN`` reference in +``dataretrieval/`` resolves to a record in +``docs/source/architecture/decisions/``, so a renumbered or deleted record fails +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. + +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 +broke this record's own history rule. The venue rules say where an explanation +goes; none of them asked who it reads for. + +``Context`` and the measurements below are this package's. ``Decision``, +``Consequences``, and the review questions in ``Compliance`` are written to hold +for any project; the paragraph naming ``tests/architecture_test.py`` is not. A +project adopting this record writes its own ``Context`` from its own audit and +keeps the rest. + +This record was written after ADRs 0001 through 0011. It is numbered 0000 +because it governs how every record is written, not because it came first. Its +``Context`` describes the package as the audit found it, and the migration it +authorizes is incremental, so some of the prose described there is still in +place. + +Prose measurements were taken over ``dataretrieval/`` on 2026-08-26: 9,283 +docstring lines and 1,157 comment lines against 6,127 lines of code; public +service adapters at 2.46 prose lines per code line, internal modules at 1.29. +The ~500-line restatement estimate comes from the subsystem audit recorded in +the pull request that introduced this ADR. diff --git a/docs/source/architecture/decisions/0004-error-retry-resume.rst b/docs/source/architecture/decisions/0004-error-retry-resume.rst index 70202c65c..c5c159de1 100644 --- a/docs/source/architecture/decisions/0004-error-retry-resume.rst +++ b/docs/source/architecture/decisions/0004-error-retry-resume.rst @@ -4,16 +4,21 @@ ADR 0004: Use typed failures and bounded recovery Status ------ -Accepted +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. + +Amended after acceptance under :doc:`0000-documenting-decisions`; the +``Notes`` section records every clause added or corrected. Context ------- Remote hydrologic services fail through HTTP statuses, rate limits, timeouts, invalid payloads, and mid-pagination interruptions. Returning partial data after -an undetected page failure is worse than raising. At the same time, retrying -without limits can stall callers, multiply quota usage, or hide persistent -faults. +an undetected page failure leaves the caller a truncated result they cannot tell +from a complete one. At the same time, retrying without limits can stall +callers, multiply quota usage, or hide persistent faults. Decision -------- @@ -33,14 +38,50 @@ 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 +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. + +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 +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. + +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. + +**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. + +**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. + Consequences ------------ -- Callers can catch one stable base error and still branch on useful fields. +- Callers can catch one stable base error and still branch on ``status_code``, + ``retry_after``, and ``retryable``. - Mid-pagination failure cannot silently look like a complete dataset. - Retry can increase latency and request quota, so policy and defaults are part of observable behavior. -- Partial OGC state requires careful serialization and finalization tests. +- Partial OGC state requires serialization and finalization tests. - Expanding retry to another service requires service-specific idempotency and failure-contract tests. @@ -50,4 +91,22 @@ Compliance Tests cover status-to-type mapping, uniform fields, transport wrapping, pagination failure, retry exhaustion and jitter bounds, ``Retry-After`` limits, resume equivalence, partial-state stability, pickling, and cancellation -precedence. +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 +``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 carrying in prose. The +skip clause records a carve-out 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 +without noting it here. The supersession is 0008's; only the backlink is new. diff --git a/docs/source/architecture/decisions/0006-service-neutral-transport.rst b/docs/source/architecture/decisions/0006-service-neutral-transport.rst index 67b3c2c6d..b8d0f5c2a 100644 --- a/docs/source/architecture/decisions/0006-service-neutral-transport.rst +++ b/docs/source/architecture/decisions/0006-service-neutral-transport.rst @@ -8,6 +8,9 @@ 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. +Amended after acceptance under :doc:`0000-documenting-decisions`; the +``Notes`` section records every clause added or corrected. + Context ------- @@ -16,8 +19,8 @@ clients, cursor pagination, bounded retry, response aggregation, progress, and a sync-over-async bridge. Locating those capabilities inside a protocol package would make unrelated services depend on protocol-specific implementation details -- Water Use previously imported its page walker and sync bridge from -``ogc.engine``, a dependency with no conceptual basis. Duplicating them would -allow authentication, timeout, retry, and failure behavior to drift. +``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 @@ -27,9 +30,8 @@ that invites generality no caller needs. Decision -------- -``dataretrieval.transport`` is the internal service-neutral execution layer -- -neutral across the USGS services this package talks to, not across HTTP APIs in -general. It owns: +``dataretrieval.transport`` is the internal service-neutral execution layer. It +owns: - synchronous and asynchronous HTTP client lifecycle and timeout defaults; - attaching the API key and stripping it at redirect time, over the predicate @@ -66,13 +68,19 @@ at import time without reaching 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, gateway 5xx, and -transport failures that are not settled before the request leaves. A server -error reporting that *this* request was rejected is surfaced on the first -attempt rather than multiplied against an already-failing service. 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. +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`. 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 @@ -80,6 +88,34 @@ 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. + +**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 +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 +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. + Consequences ------------ @@ -97,8 +133,8 @@ Consequences promise. - Keeping presentation and frame assembly out means transport is roughly 570 lines across five modules, each recognizably HTTP execution policy. Retry is - the one intricate module, and it is intricate because two independent bounds - are what make retry safe against a slow service. + the one complex module, because two independent bounds are what make retry + safe against a slow service. Compliance ---------- @@ -110,4 +146,21 @@ transport, and that only ``dataretrieval.credentials`` names the API-key host. 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. +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 +validation is covered by the shared link-policy tests over foreign hosts and +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 +across ``transport/retry.py``, ``transport/liveness.py``, and +``transport/fanout.py``. + +One sentence of the original Decision was also corrected rather than added to: +it scoped automatic retry to "gateway 5xx", which was never true of a fanned-out +call -- those re-send any 5xx, and only the single-shot adapters are limited to +the gateway statuses. The decision is unchanged; the sentence now describes it. diff --git a/docs/source/architecture/decisions/0007-adapter-facades.rst b/docs/source/architecture/decisions/0007-adapter-facades.rst index 2dc5552d8..de740d011 100644 --- a/docs/source/architecture/decisions/0007-adapter-facades.rst +++ b/docs/source/architecture/decisions/0007-adapter-facades.rst @@ -6,15 +6,19 @@ Status Accepted +Amended after acceptance under :doc:`0000-documenting-decisions`; the +``Notes`` section records every clause added or corrected. + Context ------- A service facade can remain stable while its implementation grows for unrelated upstream collections. Keeping every Water Data getter in one module coupled -changes to time series, monitoring metadata, field measurements, reference -catalogs, Samples, statistics, and generalized CQL queries. Active service -modules also relied on Python's implicit wildcard-export behavior, making their -intended public surfaces difficult to distinguish from imported helpers. +together changes to time series, monitoring metadata, field measurements, +reference catalogs, Samples, statistics, and generalized CQL queries. Active +service modules also relied on Python's implicit wildcard-export behavior, +making their intended public surfaces difficult to distinguish from imported +helpers. Decision -------- @@ -27,10 +31,10 @@ selection, Statistics API execution, shared Water Data policy, and type vocabularies. The facade re-exports the established functions and preserves their signatures, -their identity at ``dataretrieval.waterdata``, and the private Samples constants -compatibility tests rely on. It does not rewrite their ``__module__``: each -function reports the family module that defines it, so a traceback names a file -that contains code. Collection-family modules do not +their identity at ``dataretrieval.waterdata``, and the private Samples +constants that compatibility tests rely on. It does not rewrite their +``__module__``: each function reports the family module that defines it, so a +traceback names a file that contains code. Collection-family modules do not import one another; shared behavior belongs in Water Data policy, OGC, or transport modules. @@ -38,6 +42,31 @@ Active service and focused implementation modules declare explicit ``__all__`` exports. Deprecated NWIS remains outside this modernization. Service adapters do not import another adapter's implementation to obtain transport behavior. +The ``__module__`` rule above is scoped to this facade, where the family module +is a real file a traceback can name. It is not a package-wide prohibition: the +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 +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 +by the number of collections while making each family's surface partly untyped. +One hatch, 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 +how it is organized. HUCs, parameter codes, FIPS codes, and monitoring-location +identifiers (``site_no`` in NWIS) carry 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 +identifier columns as ``str`` before parsing, which is why a two-pass header +read is not a redundancy to be optimized away. + Return contracts remain service-specific. Tabular services generally return a ``(DataFrame, metadata)`` pair, while NLDI returns geospatial values directly, StreamStats exposes response/domain objects, and ratings return parsed tables or @@ -47,14 +76,13 @@ contracts. Consequences ------------ -- Collection changes have a smaller implementation and test blast radius. +- Collection changes touch fewer implementation and test files. - Existing package and ``waterdata.api`` import paths remain stable. - Explicit exports make accidental public-surface growth reviewable. -- More modules require a maintained facade and executable signature/export +- More modules mean a facade to maintain, plus executable signature and export snapshots. - Tests are described as public-contract, adapter-contract, component, or - cross-component layers without forcing a disruptive move of established - files. + cross-component layers without moving established files. Compliance ---------- @@ -64,5 +92,16 @@ 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 lateral adapter -reach-through. +(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 +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 +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 44b324fc4..82da7044a 100644 --- a/docs/source/architecture/decisions/0008-fan-out-execution.rst +++ b/docs/source/architecture/decisions/0008-fan-out-execution.rst @@ -8,6 +8,9 @@ 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. +Amended after acceptance under :doc:`0000-documenting-decisions`; the +``Notes`` section records every clause added or corrected. + Context ------- @@ -18,10 +21,10 @@ is split because the NWDC accepts one ``location=`` per request -- its URLs run around 63 bytes against an 8000-byte budget, so the byte limit has nothing to do with it. -Chunking is how you divide the data structurally; fan-out is how you distribute -the work operationally. The two are orthogonal, 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. +Chunking is how you divide the data; fan-out is how you distribute the work. +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 @@ -30,14 +33,14 @@ 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 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 subtle rule, -two copies, synchronized by prose. +with a comment naming ``ChunkedCall._run`` as the original. One rule, two +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 progress. And it read its own module-global concurrency cap, so a user setting -``API_USGS_CONCURRENT`` to be polite to the service found one adapter ignoring +``API_USGS_CONCURRENT`` to lower the request rate found one adapter ignoring them. Decision @@ -46,7 +49,30 @@ Decision ``dataretrieval.transport.fanout`` owns fan-out execution for every service: bounded concurrency, per-attempt retry, deterministic failure precedence, sparse completion tracking, and resume. It names no protocol concept. An adapter -supplies a ``FanOutPlan`` and an ``async def fetch(item) -> (df, response)``. +supplies a ``FanOutPlan``, an ``async def fetch(item) -> (df, response)``, and +optionally a ``finalize`` hook applied to the combined frame. + +**``finalize`` is part of the contract, not a convenience.** Post-processing is +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``. + +**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. + +**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 +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. ``FanOutPlan`` is a ``Protocol`` of ``__len__`` and ``__iter__``, generic in the item type -- a sized, iterable collection of chunk descriptions, and @@ -54,7 +80,7 @@ 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 getters yield kwargs dicts, Water Use yields ready ``httpx.Request`` objects. -The standard protocols, rather than bespoke members, are a deliberate choice. +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 @@ -66,10 +92,10 @@ Use passes. ``ChunkPlan`` keeps ``total`` and ``iter_chunk_args`` as its own vocabulary and defines the dunders to delegate to them, so the two cannot disagree. -The protocol is structural rather than nominal for the original reason: -``ChunkPlan`` derives 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 protocol is structural rather than nominal -- satisfied by having the +members, not by inheriting -- for the original reason: ``ChunkPlan`` derives +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 @@ -93,13 +119,13 @@ published in the user guide and caught in user code. The subclasses (``QuotaExhausted``, ``ServiceInterrupted``) were already neutral and are unchanged. -Concurrency is one general setting with per-service defaults. -``API_USGS_CONCURRENT`` applies to every fanned-out call; a service may declare +Concurrency is one general setting with per-adapter defaults. +``API_USGS_CONCURRENT`` applies to every fanned-out call; an adapter may declare a different default for when it is unset. The precedence is deliberate: an -explicitly set environment variable outranks a service default, never the -reverse. A service that could override the general setting would make -``API_USGS_CONCURRENT=1`` a lie. Service defaults say "absent instruction, this -service prefers N"; they do not say "this service knows better than you". +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. Consequences ------------ @@ -124,8 +150,9 @@ Consequences 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, pagination-borrowed clients, and combining. It remains HTTP - execution policy, which is the test the package applies. + drives retry, publishes the client pagination borrows, and calls + ``combining``. It remains HTTP execution policy, which is the test the + package applies. Compliance ---------- @@ -143,3 +170,19 @@ progress ticks, and the concurrency precedence rule. Conformance itself is left to the type checker rather than asserted at runtime: with the protocol reduced to ``__len__`` and ``__iter__``, a missing member is a ``mypy`` error at the call site, not an ``AttributeError`` discovered mid-fan-out. + +Resume equivalence tests cover the ``finalize`` rule: a resumed call must return +what an uninterrupted one would. + +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 -- +the semaphore rule was stated four times in ``transport/fanout.py`` alone, and +that file now states it once and cites this record. + +The sentence naming the executor contract was extended in place to include +``finalize``; it previously listed only the plan and the fetch callback. The +hook already existed, so this records what the executor always required rather +than adding a requirement. diff --git a/docs/source/architecture/decisions/0009-layered-configuration.rst b/docs/source/architecture/decisions/0009-layered-configuration.rst index 43ba1e8d7..e5bce3902 100644 --- a/docs/source/architecture/decisions/0009-layered-configuration.rst +++ b/docs/source/architecture/decisions/0009-layered-configuration.rst @@ -7,8 +7,8 @@ Status Accepted, with clauses superseded twice. :doc:`0010-adapter-scoped-settings` supersedes "One flat set of setting names" -and "Per-service overrides are deferred" below, having found the premise of the -first -- that every service accepts the same settings -- to be false. +and "Per-service overrides are deferred" below: the premise of the first -- that +every service accepts the same settings -- is false. :doc:`0011-configuration-profiles` supersedes three more: @@ -25,33 +25,36 @@ first -- that every service accepts the same settings -- to be false. scoped action, not a ``Configuration`` dataclass") and in "A configuration object would have no way to reach the call". ``configure()`` now takes exactly such objects. The grounds were that an instance had no way to reach a - free function; the ``ContextVar`` this ADR established is that way, and ADR - 0010 had already narrowed the objection to a payload-shape preference. + free function; the ``ContextVar`` this ADR established is one, and ADR 0010 + had already narrowed the objection to a payload-shape preference. The chain itself, the ``ContextVar`` delivery, host-scoped credentials, and the leaf constraint stand. +Amended after acceptance under :doc:`0000-documenting-decisions`; the +``Notes`` section records every clause added or corrected. + Context ------- Settings reached the library through one mechanism: process-global environment variables (``API_USGS_PAT``, ``API_USGS_CONCURRENT``, ``API_USGS_RETRIES``, -``API_USGS_PROGRESS``), each with its own hand-rolled parser at its point of -use. Nothing could report the effective configuration, and the grammars were +``API_USGS_PROGRESS``), each with its own parser at its point of use. Nothing +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 +handling concurrent users must assign to ``os.environ`` -- which is process-global, so it races across threads and tasks (issue #352). -The obvious fix, an ``api_key=`` parameter on the public getters, 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 also object to an ``api_key=`` -parameter on the separate ground that it invites keys pasted into shared -scripts. +An ``api_key=`` parameter on the public getters is where a per-call value would +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. Decision -------- @@ -71,12 +74,12 @@ Supporting decisions: - **Precedence is per setting, not per source.** An environment that sets only ``API_USGS_PAT`` leaves a file-provided ``concurrency`` in effect. A *blank* environment variable does not count as set, so it cannot shadow the - file: container and CI tooling routinely materializes one. The exception is + file: container and CI tooling routinely creates one. The exception is ``progress``, where a blank ``API_USGS_PROGRESS`` has always meant "off" -- so "does blank count as a value?" is a property of the setting (``configuration._BLANK_MEANS_SET``) rather than an extra tier in the chain. -- **The environment ranks above the file.** This follows the established - precedence used by `pip +- **The environment ranks above the file.** This follows the precedence used by + `pip `_ and `AWS `_, @@ -89,77 +92,108 @@ Supporting decisions: - **No public getter grows 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 - rejects those names before request construction so they cannot enter a URL. + refuses credential-shaped names before request construction so they cannot + enter a URL. That refusal covers names carrying 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 arbitrary non-empty progress value, remain compatible - without making the new surfaces equally permissive. + 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 + fallback branch happened to do. The rejected alternative -- an ``if``/``elif`` + chain with an implicit integer default -- fails by omission, and fails + quietly. +- **The file format is forward compatible; the table layout is not.** A key the + running version does not recognize warns and is ignored, so a file written for + a newer release still loads rather than breaking a caller who downgraded. A + 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 + 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 + 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. - **TOML, read with** ``tomllib``. Stdlib from Python 3.11; the ``tomli`` - backport is a marker-scoped dependency that disappears when + backport is declared under an environment marker and disappears 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`` - spends rate-limit quota, and ADR-adjacent documentation on - ``dataretrieval.parallel_chunks`` argues it must stay a deliberate choice. - It does not add a new exported process-global knob; the file and ``configure`` - block are its only sources, with a scoped block as the recommended use. + spends rate-limit quota and must stay a deliberate choice: the library cannot + tell in advance whether a query is large -- ten states over a short window + might fit in one page, where extra chunks would only spend quota -- so a high + value is a judgement the caller makes per query, not a process default. It + adds no new process-global variable; the file and ``configure`` block are its + only sources, with a scoped block as the recommended use, which also keeps the + setting from leaking into unrelated calls. - **Names distinguish execution capacity from planning granularity.** - ``concurrency`` is the noun for the maximum in-flight subrequests and maps to - the established ``API_USGS_CONCURRENT`` variable. ``parallel_chunks`` asks - the planner for optional extra chunks; it does not promise that many requests - 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``. + ``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. + 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``. - **Configuration errors are in the error taxonomy.** ``ConfigurationError`` is a ``DataRetrievalError`` *and* a ``ValueError``. Configuration resolves lazily - on the request path, so a broken file surfaces from inside whichever getter + on the request path, so an invalid file raises from inside whichever getter runs first; ``except DataRetrievalError`` around a call has to catch it like any other failure of that call, while the ``ValueError`` base keeps the handlers that predate the file layer working. - **``parallel_chunks`` at the top level of the file warns.** It is the one setting that spends rate-limit quota, so a value left there applies to every splittable query in every process that reads the file. A - ``[profiles.]`` table is opt-in per run, which is the shape this - setting wants; the top-level form still works but says so. -- **``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 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. + ``[profiles.]`` table is opt-in per run, which is the scope this setting + needs; the top-level form still works, but warns. +- **``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. - **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 than one per service. Services differ in the *value* they want, not the vocabulary, and that difference is expressed as a caller-supplied default: ``wateruse`` passes its ``DEFAULT_CONCURRENT_REQUESTS`` of 4 to - ``configuration.concurrency()`` where the OGC getters take the package default of - 32, and the single-shot adapters pass ``_GATEWAY_STATUSES`` to - ``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 + ``configuration.concurrency()`` where the OGC getters take the package default + of 32, and the single-shot adapters pass ``_GATEWAY_STATUSES`` to + ``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. - **Per-service overrides are deferred, not refused.** One ``configure()`` - block cannot currently ask for a gentler Water Use than 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 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 real requirement before it is paid. + 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 + 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. - **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 - getter, which is the threading the ``ContextVar`` exists to remove and which - the ``**queryables`` catch-all makes unsafe, or through a module-level + 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 @@ -172,13 +206,13 @@ Consequences ``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. -- ``show_configuration()`` reports the effective value and provenance of each setting - without ever printing the key. +- ``show_configuration()`` reports the effective value and source of each + setting without ever printing the key. - Behavior is unchanged when no file exists and no block is active, so existing environment-variable users are unaffected. - A configuration file becomes a supported artifact whose format is now a compatibility surface. -- The Python floor and the file format are coupled: raising +- The minimum Python version and the file format are coupled: raising ``requires-python`` to ``>=3.11`` drops the ``tomli`` dependency with no other change. @@ -192,3 +226,24 @@ asserts the module imports nothing from ``dataretrieval`` other than the ``tests/configuration_test.py`` covers the precedence chain, per-setting merging, thread and asyncio isolation, host scoping for file-sourced keys, redaction in ``show_configuration``, and rejection of credential parameters on public getters. + +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 "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 +the prose being consolidated. + +The ``**queryables`` clause above originally named ``session`` among the +rejected spellings. It was corrected after the fact: ``session`` carries 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 +message. ``dataretrieval/credentials.py`` records the exclusion at the +predicate. The decision the clause makes -- credential-shaped names never reach +a URL -- is unchanged. diff --git a/docs/source/architecture/decisions/0011-configuration-profiles.rst b/docs/source/architecture/decisions/0011-configuration-profiles.rst index 95a5efa5b..db0c34b9f 100644 --- a/docs/source/architecture/decisions/0011-configuration-profiles.rst +++ b/docs/source/architecture/decisions/0011-configuration-profiles.rst @@ -13,6 +13,9 @@ 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. +Amended after acceptance under :doc:`0000-documenting-decisions`; the +``Notes`` section records every clause added or corrected. + Context ------- @@ -20,10 +23,10 @@ ADR 0010 gave each adapter its own slice of the chain, so ``[ngwmn]`` narrows a setting to NGWMN. That covers "tune one service" but not the case a multi-service caller actually has: -- **Several named configurations per adapter.** A caller with an overnight - bulk shape and a polite daytime shape for Water Data cannot store both. The - only named construct is ``[profiles.]``, which switches *every* - service at once. +- **Several named configurations per adapter.** A caller with an overnight bulk + configuration profile and a lower-rate daytime one for Water Data cannot + 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 detail. That refusal was recorded in ADR 0010 on the grounds that layering @@ -71,12 +74,13 @@ adapter, and nothing else:: The adapter an instance targets is a property of its class, so the caller never restates it -- which is what removes the roster duplication. Naming two -configurations for one adapter raises: they would be the one pairing with no -defined order. +configurations for one adapter raises: they would be the one pairing the +precedence rules do not order. Keyword settings are removed, so ``configure(api_key=...)`` no longer works. This is the most-typed line the feature exists to enable, and making it wordier -is a real cost, accepted deliberately for one shape everywhere. +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 @@ -108,9 +112,9 @@ means they cannot tie. Position 2 above 3 inverts ADR 0009's environment-above-file rule for this one case. A profile named in code is a more deliberate act than a variable -inherited from a shell, and losing to that variable is the behaviour a caller -would file a bug about. Everything the caller did *not* name in code still -follows the original rule. +inherited from a shell, which would otherwise override a selection the caller +made explicitly. Everything the caller did *not* name in code still follows the +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 @@ -121,8 +125,8 @@ live 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 the file and the environment. A file that silently redirects a data-retrieval -library to another host is a supply-chain-shaped hazard; an in-code block -keeps the redirect where a reader sees it. +library to another host is a supply-chain hazard; an in-code block keeps the +redirect where a reader sees it. **The module is renamed** ``dataretrieval.config`` to ``dataretrieval.configuration``, @@ -140,7 +144,7 @@ the live services: * - Host - No key - With key - - Bad key + - Invalid key * - ``api.waterdata.usgs.gov`` (waterdata, ngwmn) - no limit header - ``x-ratelimit-limit: 4000`` @@ -160,12 +164,22 @@ 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 +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 +``retries: int | None = _UNSET`` directly in an adapter module satisfies this +record's letter 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. + Consequences ------------ -- **The multi-service case gets a spelling**, which is the point. One block, - several adapters, at most one configuration each, any of them from the file - or from code. +- **The multi-service case gets a spelling.** 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 construction rather than caught by a fitness test. @@ -176,14 +190,14 @@ Consequences the same commit. - **``show_configuration()`` can only resolve the settings an adapter accepts once that adapter has been imported.** It names the adapters it could not - check rather than omitting them silently, which is the honest cost of lazy + check rather than omitting them silently, which is the cost of lazy validation. The *profile list* is not import-limited: what a profile is called is a fact about the file, so every ``[.]`` table it defines is listed, imported or not -- withholding one would make the section's answer depend on which optional extras happened to be installed. - **Two names differ only by case** -- the ``configuration`` module and the ``Configuration`` class. The module stays out of the package's public - exports so the confusing import line cannot arise. + exports, so ``from dataretrieval import configuration, Configuration`` cannot arise. - **Separate quota pools are still not modelled.** Three exist. Nothing in the library needs to know yet. - **``ssl_check`` is unaffected** and remains a per-call argument, for the @@ -213,9 +227,9 @@ Satisfied. In ``tests/configuration_test.py``: ``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 public-getter contract covering OGC, Samples, Statistics, and Ratings. The - endpoint module exposes request-time acquisition functions rather than raw - usable endpoints, so a family module gets the active scoped root without a - wrapper obligation at every use site. + endpoint module exposes functions that build each URL at request time rather + than ready-made endpoint constants, so a family module gets the root in effect + for the call without wrapping every use site. - ``test_adapter_roster_names_real_modules_that_register_themselves`` and ``test_every_adapter_is_actually_wired_to_a_read_site`` -- the roster resolves, and no configuration exists that nothing reads. An adapter name @@ -239,3 +253,6 @@ Notes ``parallel_chunks`` asks 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 + not change behavior. diff --git a/docs/source/architecture/decisions/0012-deprecation-horizons.rst b/docs/source/architecture/decisions/0012-deprecation-horizons.rst new file mode 100644 index 000000000..b280d2dbb --- /dev/null +++ b/docs/source/architecture/decisions/0012-deprecation-horizons.rst @@ -0,0 +1,85 @@ +ADR 0012: Announce every removal through one advisory with a published horizon +============================================================================== + +Status +------ + +Accepted + +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. + +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 +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``. + +ADR 0005 sets a removal date for legacy NWIS, but it is scoped to that adapter. +Nothing recorded the general rule, and the deprecations kept accumulating. + +Decision +-------- + +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 +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 +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 +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 +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. + +A horizon is a floor, 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. +- 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 + accumulation visible rather than hidden. +- Nothing is removed on the horizon alone. A removal still needs a release that + says so. + +Compliance +---------- + +``tests/deprecation_test.py`` covers the shared mechanism: that an advisory +names its replacement, and that the horizon it prints is the one in +``REMOVALS``. Per-surface tests assert the individual advisories, including that +a renamed argument still works under its old name. The *warning category* +assertions -- that ``DataCurrencyWarning`` is not a ``DeprecationWarning`` +subclass -- belong to :doc:`0004-error-retry-resume`. + +Notes +----- + +ADR 0005 remains the record for legacy NWIS specifically, including its +2027-05-06 date. This record generalizes the mechanism without changing that +decision. diff --git a/docs/source/architecture/decisions/index.rst b/docs/source/architecture/decisions/index.rst index ad468728a..a59787073 100644 --- a/docs/source/architecture/decisions/index.rst +++ b/docs/source/architecture/decisions/index.rst @@ -12,11 +12,13 @@ links back to the old record. Keep records concise and commit them with the change that makes the decision effective. Use :doc:`template` when proposing a decision. Number accepted and proposed -records sequentially. +records sequentially. :doc:`0000-documenting-decisions` says which explanations +belong here rather than in a docstring, an inline comment, or a commit message. .. toctree:: :maxdepth: 1 + 0000-documenting-decisions 0001-modular-monolith 0002-sync-api-async-internals 0003-dependency-direction @@ -28,4 +30,5 @@ records sequentially. 0009-layered-configuration 0010-adapter-scoped-settings 0011-configuration-profiles + 0012-deprecation-horizons template diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index 28462770d..85fa47e81 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -7,7 +7,7 @@ Purpose and scope ``dataretrieval`` is a Python client library for discovering and retrieving hydrologic data from several independently operated USGS and partner services. It is a modular monolith: one installable distribution with public service -facades, service- and protocol-specific adapters, and shared infrastructure. +adapters, protocol subsystems, and shared infrastructure. The architecture favors incremental evolution and stable user-facing functions over a framework that forces unlike upstream APIs into one shape. @@ -63,25 +63,25 @@ contracts but does not hide meaningful service-specific behavior. Composition and dependency view ------------------------------- -Public service facades -^^^^^^^^^^^^^^^^^^^^^^ +Public service adapters +^^^^^^^^^^^^^^^^^^^^^^^ ``dataretrieval.waterdata`` - Modern USGS Water Data API facade. ``waterdata.api`` is a logic-free + Modern USGS Water Data API adapter. ``waterdata.api`` is a logic-free compatibility facade over collection-family modules: ``time_series``, ``metadata``, ``measurements``, ``reference``, ``samples``, and ``cql``. - Focused modules own ratings, nearest-value selection, statistics execution, + Separate modules own ratings, nearest-value selection, statistics execution, shared service policy, and type vocabularies. Internal modules import protocol helpers from their canonical OGC modules rather than re-exporting them through Water Data utilities. ``dataretrieval.ngwmn`` - NGWMN facade. Its only OGC dependency is the public OGC facade, which it + NGWMN adapter. Its only OGC dependency is the public OGC facade, which it configures with an NGWMN-specific base URL, output identifiers, state translation, and :class:`OgcDialect`. ``dataretrieval.nwdc`` - NWDC Water Use facade. Builds CSV requests, follows ``Link`` headers, and + NWDC Water Use adapter. Builds CSV requests, follows ``Link`` headers, and uses service-neutral transport for bounded fan-out, retry, pagination, response aggregation, and synchronous dispatch. It does not depend on OGC modules. @@ -90,25 +90,25 @@ Public service facades policy. Their return types intentionally reflect their upstream data models. ``dataretrieval.nwis`` - Deprecated legacy NWIS facade, scheduled for removal on or after + Deprecated legacy NWIS adapter, scheduled for removal on or after 2027-05-06. Modern code must not depend on it. Shared components ^^^^^^^^^^^^^^^^^ ``dataretrieval.configuration`` - Lightweight configuration leaf: standard library plus the ``tomli`` - backport on Python 3.10. It resolves scoped overrides, environment - variables, a TOML 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. + Configuration leaf: standard library plus the ``tomli`` backport on + Python 3.10. It resolves scoped overrides, environment variables, a TOML + 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. ``dataretrieval.ogc`` - Protocol subsystem for Water Data and NGWMN. A small facade - (``__init__.py``) exposes the service-adapter seam: ``OgcDialect``, - ``prepare_request_args``, and ``get_ogc_data`` (whose ``cql_body`` - parameter covers verbatim-CQL2 queries). + Protocol subsystem for Water Data and NGWMN. A facade (``__init__.py``) + exposes the service-adapter seam: ``OgcDialect``, ``prepare_request_args``, + and ``get_ogc_data`` (whose ``cql_body`` parameter covers verbatim-CQL2 + queries). Internally, ``policy`` defines the dialect type, control validation, and endpoint constants; ``requests`` owns argument normalization and HTTP request construction, taking the target ``base_url`` and ``dialect`` as @@ -118,9 +118,9 @@ Shared components ``planning`` determines chunk boundaries; ``chunking`` connects those plans to the shared fan-out executor and retains compatibility aliases; ``interruptions`` retains its deprecated public import path; and - ``shaping``, ``dates``, ``filters``, and ``errors`` isolate - their named protocol concerns. The full runtime OGC graph, including the - facade, is acyclic -- enforced by the package-wide fitness function in + ``shaping``, ``dates``, ``filters``, and ``errors`` isolate their named + protocol concerns. The full runtime OGC graph, including the facade, is + acyclic -- enforced by the package-wide fitness function in ``tests/architecture_test.py``. ``dataretrieval.transport`` @@ -152,7 +152,7 @@ Shared components ``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 that module's whole HTTP stack in + query machinery, 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``. @@ -183,9 +183,9 @@ Shared components The intended direction is:: - public facade -> service/protocol adapter -> service-neutral transport - -> stable policy/infrastructure - -> third-party library / network + public service adapter -> protocol subsystem -> service-neutral transport + -> stable policy/infrastructure + -> third-party library / network Dependencies must not point from shared infrastructure back to a public service adapter. ``.importlinter`` declares this as a layer stack and ``lint-imports`` @@ -205,7 +205,7 @@ Interface view The primary API is a collection of synchronous functions grouped by data portal. Most tabular download functions return ``(DataFrame, metadata)``. NLDI and StreamStats retain service-specific geospatial or response-object -contracts; consistency alone is not sufficient reason for a breaking change. +contracts. Failed requests derive from ``dataretrieval.DataRetrievalError``. Callers can inspect ``status_code``, ``retry_after``, and ``retryable`` without knowing the @@ -304,8 +304,8 @@ architecturally is the behavior around them: the failure surfaces; 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 + 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 @@ -316,23 +316,21 @@ architecturally is the behavior around them: ``API_USGS_PROGRESS`` Controls best-effort progress display. Reporting failures must never change retrieval results. + * The API token is attached only to requests for ``api.waterdata.usgs.gov``. Shared synchronous and asynchronous clients re-check every redirected request and strip the token before following a link to any other host, including external rating assets. * A semaphore, not connection-pool waiting, is the execution throttle for sub-request concurrency. -* Retry backoff is exponential with full jitter and honors bounded - ``Retry-After`` values. -* Progress reporting is best-effort: a reporting failure must never change - retrieval results. -* ``dataretrieval.configuration`` is a stdlib-only leaf, so any module may depend on - it without an import cycle. +* ``dataretrieval.configuration`` is a stdlib-only leaf, so any module may + depend on it without an import cycle. ``dataretrieval.transport`` centralizes HTTP timeout, redirect, and -authentication policy. OGC chunk fan-out and Water Use location -fan-out retain separate explicit concurrency caps because their upstream costs -and request shapes differ. +authentication policy. OGC chunk fan-out and Water Use location fan-out share +one ``concurrency`` setting with per-adapter defaults, because their upstream +costs and request shapes differ but a caller's instruction should not (ADR +0008). Known architectural debt ------------------------ diff --git a/docs/source/userguide/configuration.rst b/docs/source/userguide/configuration.rst index 99db38095..85472629d 100644 --- a/docs/source/userguide/configuration.rst +++ b/docs/source/userguide/configuration.rst @@ -492,7 +492,7 @@ The value limits optional refinement only. URL-byte safety can require more sub-requests than the configured value, and an input with nothing to split stays a single request. -``parallel_chunks(n)`` is sugar for +``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. diff --git a/tests/architecture_test.py b/tests/architecture_test.py index 4af377b77..874065153 100644 --- a/tests/architecture_test.py +++ b/tests/architecture_test.py @@ -28,6 +28,7 @@ import ast import configparser import functools +import re import sys from graphlib import CycleError, TopologicalSorter from importlib.util import resolve_name @@ -372,11 +373,11 @@ def test_credential_policy_has_one_definition() -> None: """ host = "api.waterdata.usgs.gov" # Walked as AST string *values*, not as source text. A line-substring match - # is wrong in both directions: it missed the ``"https://…"`` form three - # modules use to spell the same authority, and it flagged docstring prose - # that merely names the service. Docstrings are excluded here (they are - # documentation, not a second source of truth) while every other literal -- - # bare host or full base URL -- counts. + # produces both false negatives and false positives: it missed the + # ``"https://…"`` form three modules use to spell the same authority, and it + # flagged docstring prose that merely names the service. Docstrings are + # excluded here (they are documentation, not a second source of truth) + # while every other literal -- bare host or full base URL -- counts. offenders: list[str] = [] for path in sorted(PACKAGE_ROOT.rglob("*.py")): if path.name == "credentials.py": @@ -608,8 +609,8 @@ def test_fan_out_plans_are_sized_and_repeatably_iterable() -> None: protocol would prove only that the methods exist. What is actually load-bearing and *not* guaranteed by the type is repeatability: resume keys completed work by position, so a plan whose second pass differed -- - a generator mistaken for a collection, say -- would re-issue the wrong - chunks. + a generator mistaken for a collection, say -- would re-issue chunks that + no longer match the completed positions. """ import httpx @@ -630,3 +631,41 @@ def _build(**args: object) -> httpx.Request: f"{name} yielded {len(first)} items but reports len {len(plan)}" ) assert list(plan) == first, f"{name} is not repeatably iterable" + + +def test_adr_references_resolve_to_a_record() -> None: + """Every ``ADR NNNN`` citation names a record that exists, in every venue. + + ADR 0000 routes cross-cutting rationale into the decision records and asks + the code to cite rather than restate. A renumbered or deleted record has to + fail here rather than leave a dangling pointer for a reader to chase. + + Scoped to every venue ADR 0000 names, not just the package: the glossary, + the contributor guide, and the architecture docs cite records too, and a + pointer rots there too. + """ + repo_root = PACKAGE_ROOT.parent + decisions = repo_root / "docs" / "source" / "architecture" / "decisions" + recorded = {path.name[:4] for path in decisions.glob("[0-9][0-9][0-9][0-9]-*.rst")} + assert recorded, f"no ADR records found under {decisions}" + + cited: list[Path] = sorted(PACKAGE_ROOT.rglob("*.py")) + cited += sorted((repo_root / "docs" / "source").rglob("*.rst")) + cited += [repo_root / "CONTEXT.md", repo_root / "CONTRIBUTING.md"] + cited += [repo_root / "AGENTS.md"] + + pattern = re.compile(r"ADR\s+(\d{4})") + dangling: list[str] = [] + for path in cited: + if not path.is_file(): + continue + text = path.read_text(encoding="utf-8") + for lineno, line in enumerate(text.splitlines(), start=1): + for number in pattern.findall(line): + if number not in recorded: + rel = path.relative_to(repo_root) + dangling.append(f"{rel}:{lineno} cites ADR {number}") + + assert not dangling, "citations name no such decision record:\n " + "\n ".join( + dangling + ) diff --git a/tests/configuration_test.py b/tests/configuration_test.py index e72e52220..c0b6775e3 100644 --- a/tests/configuration_test.py +++ b/tests/configuration_test.py @@ -148,7 +148,7 @@ def test_explicit_none_suppresses_lower_sources(monkeypatch): ], ) def test_a_configuration_validates_its_own_settings(settings): - """A bad value raises where it was written, not inside a later request. + """An invalid value raises where it was written, not inside a later request. Construction is earlier than the ``with``, which is earlier than the request the value would otherwise have broken. @@ -459,7 +459,7 @@ def test_loading_an_undefined_profile_raises(config_file): """A name the caller just typed is a typo, not a silent fall-through. The message lists what the file *does* define, because a misspelling is - only obvious next to the spelling that was meant -- and only for this + only recognizable next to the spelling that was meant -- and only for this adapter, since selecting a profile is per adapter and another service's profile names are not candidates for what the caller meant to type. """ @@ -1049,7 +1049,7 @@ def test_show_config_renderers_cover_every_setting(): def test_unselected_profile_is_not_validated(config_file): - """A bad value in a profile nobody selected must not fail every request. + """An invalid value in a profile nobody selected must not fail every request. Profile tables are kept raw at parse time and validated only when one is actually selected -- the same blast-radius rule ``_default_headers`` @@ -1075,9 +1075,10 @@ def test_unknown_setting_in_an_unselected_profile_is_silent(config_file, recwarn def test_a_malformed_table_does_not_fail_another_adapters_call(config_file): """The blast-radius rule, on the tier a whole adapter table sits in. - Keys are checked when *that* adapter first resolves a setting, so a bad - value in ``[nldi]`` costs a Water Data call nothing -- which is also what - lets an adapter's vocabulary live in a module this leaf cannot import. + Keys are checked when *that* adapter first resolves a setting, so an + invalid value in ``[nldi]`` costs a Water Data call nothing -- which is + also what lets an adapter's vocabulary live in a module this leaf cannot + import. """ config_file( 'api_key = "good"\n\n[nldi]\nretries = -1\n\n[waterdata]\nretries = 2\n' @@ -1232,8 +1233,9 @@ def test_a_misspelled_setting_is_not_silently_swallowed(): """A typo must fail, not be accepted and ignored. ``Configuration(concurrancy=8)`` is not a field, so the dataclass refuses - it by name -- the worst outcome for a module whose job is to be - trustworthy about what a call will use would be to take it and drop it. + it by name -- taking it and dropping it would leave a caller believing a + setting is in force that no call reads, from a module whose job is to be + trustworthy about what a call will use. """ with pytest.raises(TypeError, match="concurrancy"): Configuration(concurrancy=8) @@ -1271,7 +1273,7 @@ def test_settings_for_an_unimported_adapter_is_not_an_error(monkeypatch): """``None`` means "cannot validate these keys yet", never "invalid". NLDI is imported on demand for the geopandas extra, so a roster built from - imports would reject a perfectly good ``[nldi]`` table until something + imports would reject a valid ``[nldi]`` table until something happened to import that module. """ monkeypatch.delitem(configuration._REGISTRY, "nldi", raising=False) @@ -1280,13 +1282,13 @@ def test_settings_for_an_unimported_adapter_is_not_an_error(monkeypatch): def test_every_adapter_is_actually_wired_to_a_read_site(): - """A schema nothing passes is worse than no schema. + """A schema nothing passes costs the caller a report they cannot trust. ``show_configuration()`` would report a ``[nwis]`` override as live while every call ignored it -- the report whose whole job is answering "what will - this call use" being confidently wrong. Importability is the weaker half of - the invariant: it passed while ``waterdata.get_cql``, eight of nine WQP - getters, and all of ``nwis`` silently resolved package-wide. + this call use" being confidently incorrect. Importability is the weaker + half of the invariant: it passed while ``waterdata.get_cql``, eight of nine + WQP getters, and all of ``nwis`` silently resolved package-wide. """ import pathlib @@ -1310,7 +1312,7 @@ def test_a_misspelled_adapter_at_a_read_site_raises(): matches the typo, every setting is accepted because nothing knows the schema, and a ``[waterdata]`` table or a ``WaterdataConfiguration`` is then ignored with nothing raised anywhere. The grep only sees that the correctly - spelled string occurs somewhere; it cannot see a second, wrong one. + spelled string occurs somewhere; it cannot see a second, misspelled one. """ with pytest.raises(configuration.ConfigurationError, match="not a configurable"): configuration.retries(adapter="waterdatas") @@ -1385,7 +1387,7 @@ def test_base_url_is_refused_from_the_environment(monkeypatch): ``API_USGS_BASE_URL`` is the spelling every other setting's variable predicts, so a caller who exports it believes they have redirected - something. Leaving it out of ``ENV_VARS`` would make that belief wrong and + something. Leaving it out of ``ENV_VARS`` would make that belief false and silent; the error names the block to write instead. """ monkeypatch.setenv("API_USGS_BASE_URL", "https://evil.example") @@ -1475,7 +1477,7 @@ def test_a_redirected_adapter_is_not_sent_the_api_key(httpx_mock): ``credentials.accepts_api_key`` is checked where the header is attached, so a redirect needs no second rule to be safe -- but "needs no rule" is exactly the kind of claim that stops being true silently, and the cost of it being - wrong is a credential handed to whatever host the block named. + false is a credential handed to whatever host the block named. """ httpx_mock.add_response(method=None, url=_MIRROR_RE, json=_DAILY_PAGE) httpx_mock.add_response(method=None, url=_WATERDATA_RE, json=_DAILY_PAGE) @@ -1645,11 +1647,11 @@ def test_a_loaded_profile_beats_the_environment(config_file, monkeypatch): """Rung 2 over rung 3 -- the one inversion ADR 0011 exists to make. ADR 0009 put the environment above the file, and a named profile lives in - the file, so the naive reading is that ``API_USGS_CONCURRENT`` in the shell - wins. It does not: what reaches the chain is the caller *naming* the - profile in code, which is a more deliberate act than a variable inherited - from whatever started the process, and losing to that variable is the - behaviour a caller would file a bug about. + the file, so those two rules alone predict that ``API_USGS_CONCURRENT`` in + the shell wins. It does not: what reaches the chain is the caller *naming* + the profile in code, which is a more deliberate act than a variable + inherited from whatever started the process, and losing to that variable + is the behaviour a caller would file a bug about. The inversion is also bounded, which the second half asserts: it covers what the profile names and nothing else, so ``retries`` -- which the file @@ -1798,7 +1800,8 @@ def test_load_returns_an_instance_carrying_only_the_profiles_keys(config_file): # # The report exists to answer "why is this call using that value?", so every # row names the source that supplied it. A value from a profile is the case a -# bare "configure() block" answers badly: a configuration written in code and +# bare "configure() block" label cannot distinguish: a configuration written in +# code and # one loaded from a table reach the chain by the same route, and only the # latter has a name in a file the caller can go and read. @@ -1838,9 +1841,10 @@ def test_show_configuration_names_the_profile_a_value_came_from(config_file): ``WaterdataConfiguration.load("bulk")`` and ``WaterdataConfiguration(...)`` enter the chain by the same route and are indistinguishable once their values are in the block, so a report that said only ``configure() block`` - left a caller who selected the wrong profile -- or who had forgotten a - profile was selected at all -- with nothing to look at. The label is the - table's own spelling, so it is greppable in the file that defines it. + left a caller who selected a profile they did not intend -- or who had + forgotten a profile was selected at all -- with nothing to look at. The + label is the table's own spelling, so it is greppable in the file that + defines it. """ config_file("[waterdata.bulk]\nconcurrency = 6\n") out = io.StringIO() @@ -1927,7 +1931,7 @@ def test_show_configuration_reports_an_unimported_adapter(config_file, monkeypat """An adapter this process cannot report on is named, never omitted. NLDI is imported on demand for the geopandas extra, so a process that has - not touched it cannot say which settings it accepts -- the honest cost of + not touched it cannot say which settings it accepts -- the cost of validating an adapter's keys lazily (ADR 0011). Leaving it out of the report would read as "nothing is configured for nldi", which is a different claim from "this report could not check", and the caller cannot @@ -2085,7 +2089,7 @@ class TestConfigPathResolutionFailures: def test_an_unresolvable_home_leaves_the_file_layer_inert(self, monkeypatch): """A container with no passwd entry raises from ``Path.home()``. The unexpanded ``~`` form is returned instead: it does not exist, so the - file layer is simply empty, and the environment alone still works -- + file layer is empty, and the environment alone still works -- which is how this package behaved before settings were layered.""" monkeypatch.setattr( _core.Path, diff --git a/tests/headers_host_scoping_test.py b/tests/headers_host_scoping_test.py index 99acee35d..713bf39af 100644 --- a/tests/headers_host_scoping_test.py +++ b/tests/headers_host_scoping_test.py @@ -80,7 +80,7 @@ def test_non_auth_headers_always_present(self): assert "X-Api-Key" not in headers def test_key_excluded_over_cleartext_on_the_authorized_host(self): - """The right host over plain http is still the wrong destination. + """The authorized host over plain http is still an unauthorized destination. Matching on the host alone would send a bearer token in the clear on the strength of a hostname an attacker chose to keep -- reachable via a diff --git a/tests/nldi_test.py b/tests/nldi_test.py index e448272eb..e08282881 100644 --- a/tests/nldi_test.py +++ b/tests/nldi_test.py @@ -218,7 +218,7 @@ def test_get_features_rejects_ambiguous_origins(kwargs, problem, remedy): """Origin validation runs ahead of the request, and names the way out. Both halves are asserted because the caller is usually a program: the - problem alone tells it something is wrong, and only the remedy tells it + problem alone tells it the call is invalid, and only the remedy tells it what to send instead. """ with pytest.raises(ValueError) as excinfo: @@ -566,7 +566,7 @@ def test_navigation_without_a_data_source_says_what_to_add(kwargs, monkeypatch): def test_a_bad_navigation_mode_is_reported_before_the_missing_data_source(): - """Both arguments are wrong; the mode is the one the caller typed. + """Both arguments are invalid; the mode is the one the caller typed. Requiring ``data_source`` ahead of validating the mode would answer a mistyped ``navigation_mode`` with a message about a different argument, @@ -604,7 +604,7 @@ def test_a_200_with_a_non_json_body_becomes_an_empty_frame(httpx_mock): This is the one place the package returns an empty frame rather than raising on a malformed response. Pinned because it is deliberate: the - swallow is easy to mistake for an oversight and 'fix' into a raise, which + swallow reads as an oversight and could be 'fixed' into a raise, which would turn a legitimate empty navigation into a crash. """ mock_request_data_sources(httpx_mock) diff --git a/tests/nwdc_test.py b/tests/nwdc_test.py index 5b7504902..a089833ef 100644 --- a/tests/nwdc_test.py +++ b/tests/nwdc_test.py @@ -280,7 +280,8 @@ def test_multiple_states_fan_out_preserves_input_order(httpx_mock): def test_fan_out_is_serial_when_concurrency_is_one(httpx_mock, monkeypatch): - """``API_USGS_CONCURRENT=1`` still fans out correctly (serial path).""" + """``API_USGS_CONCURRENT=1`` still fans out one request per state + (serial path).""" monkeypatch.setenv("API_USGS_CONCURRENT", "1") httpx_mock.add_response( method="GET", url=re.compile(r".*location=stateCd%3ARI.*"), text=_CSV_P1 @@ -976,7 +977,7 @@ def test_importing_dataretrieval_does_not_warn(): itself should see the warning. If ``__init__`` ever imports the alias, every user of the library gets a DeprecationWarning they cannot act on. - Runs in a subprocess: a fresh interpreter is the only honest way to test + Runs in a subprocess: a fresh interpreter is the only way to observe an import side effect, and clearing ``sys.modules`` in-process would hand every later test a second copy of the package. """ diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 9c424fa01..1e76078c1 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -385,7 +385,8 @@ def test_a_non_html_parse_failure_is_re_raised_unchanged(): def test_deprecating_a_getter_with_no_named_replacement_is_refused(): """``@_deprecated`` promises the caller a replacement, so the decorator refuses to be applied to a function whose replacement nobody recorded -- - a deprecation warning naming nothing is worse than none.""" + a deprecation warning naming nothing leaves the caller with nothing to + migrate to.""" with pytest.raises(RuntimeError, match="_REPLACEMENTS missing entry"): @nwis._deprecated @@ -408,7 +409,7 @@ def test_utc_localization_of_a_single_datetime_index(): def test_metadata_site_info_is_none_when_no_site_filter_was_used(): """``site_info`` fetches the sites a query named. A query filtered by something else (a parameter code alone) has no sites to describe, and - guessing one would describe the wrong thing.""" + guessing one would describe a site the query never named.""" md = NWIS_Metadata(mock.MagicMock(), parameterCd="00060") assert md.site_info is None diff --git a/tests/transport_test.py b/tests/transport_test.py index 18533f69c..e2c0f3968 100644 --- a/tests/transport_test.py +++ b/tests/transport_test.py @@ -452,7 +452,8 @@ def test_bad_retry_environment_raises_a_catchable_error(monkeypatch) -> None: monkeypatch.setenv("API_USGS_STALL_TIMEOUT", "10") assert retry.RetryPolicy.from_configuration().stall_timeout == 10.0 - # Still a ValueError, so existing handling of a bad setting keeps working. + # Still a ValueError, so existing handling of an unparseable setting keeps + # working. assert issubclass(ConfigurationError, ValueError) @@ -628,8 +629,8 @@ def test_an_unusable_next_page_link_is_reported_not_swallowed(): class TestErrorForStatus: def test_a_success_status_is_a_usage_error(self): """``error_for_status`` builds an exception for a failure. Handing it a - 200 means the caller's branch is wrong, and returning some default - exception would hide that.""" + 200 means the caller took the error branch on a success status, and + returning some default exception would hide that.""" with pytest.raises(ValueError, match="expects an HTTP error status"): exceptions.error_for_status(200, "not an error") diff --git a/tests/utils_test.py b/tests/utils_test.py index 4c00a27eb..fa5521d43 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -503,7 +503,7 @@ def test_resolves_an_iterable_element_wise(self): "Ohio", ] assert to_state(["WI", "CA"], "fips_us") == ["US:55", "US:06"] - # A bad element fails the whole call (fail-fast). + # An unrecognized element fails the whole call (fail-fast). with pytest.raises(ValueError, match="not a recognized US state"): to_state(["WI", "XX"]) @@ -628,8 +628,9 @@ def test_joins_date_time_and_zone_into_utc(self): def test_warns_and_keeps_going_when_a_timestamp_will_not_parse(self): """An unparseable row becomes NaT rather than failing the whole frame, - but silently dropping timestamps would be a wrong answer -- so it - warns, and names the switch that avoids the loss.""" + but silently dropping timestamps would return an incomplete frame with + nothing to mark the gap -- so it warns, and names the switch that + avoids the loss.""" df = pd.DataFrame( { # A missing date field, as an RDB row with an unrecorded diff --git a/tests/waterdata_chunking_test.py b/tests/waterdata_chunking_test.py index 569c13806..092641c77 100644 --- a/tests/waterdata_chunking_test.py +++ b/tests/waterdata_chunking_test.py @@ -276,8 +276,8 @@ def test_chunk_plan_minimizes_total_chunks(): } # Tight limit forces both axes to participate. plan = ChunkPlan(args, _fake_build, url_limit=380) - # Plan must beat the bail-floor-style worst case (8 singletons × 16 - # filter chunks = 128 chunks) by a healthy margin. + # Plan must stay under the all-singleton worst case (8 singletons × 16 + # filter chunks = 128 chunks). assert plan.total < 128 @@ -296,7 +296,7 @@ def test_chunk_plan_raises_when_smallest_plan_doesnt_fit(): def test_chunk_plan_passthrough_when_request_fits(): - """URL under limit → trivial passthrough plan (no axes, total=1), + """URL under limit → single-chunk passthrough plan (no axes, total=1), and ``iter_chunk_args`` yields exactly one sub-args dict equal to the original args.""" args = {"monitoring_location_id": ["A", "B", "C"], "limit": 100} @@ -588,7 +588,7 @@ def test_quota_exhausted_resume_can_reraise_on_persistent_429(): """If the window is still empty when the caller resumes, ``call.resume()`` raises ``QuotaExhausted`` again — the ``ChunkedCall``'s in-flight state carries forward, so a - subsequent resume after a longer wait still picks up cleanly.""" + subsequent resume after a longer wait still picks up from the pending chunk.""" # Key the failure on the chunk's CONTENT (one persistently-429ing # site) rather than a global call counter: under the async fan-out # every other chunk completes, and the same still-pending @@ -1346,8 +1346,8 @@ def test_extract_axes_skips_filter_passed_as_list(): def test_extract_axes_skips_scalar_contract_params(): """``limit`` and ``skip_geometry`` are scalars by contract (``int | None`` and ``bool | None`` respectively). If a caller smuggles - a list through type erasure (e.g. ``limit=["100","200"]`` after a - bad cast), ``_extract_axes`` must NOT treat it as a multi-value + a list through type erasure (e.g. ``limit=["100","200"]`` after an + incorrect cast), ``_extract_axes`` must NOT treat it as a multi-value axis. Chunking ``limit`` would silently fan into separate paginated queries with different per-request caps; chunking ``skip_geometry`` would emit chunks with conflicting @@ -1364,9 +1364,9 @@ def test_extract_axes_skips_scalar_contract_params(): def test_joint_planner_url_construction_long_filter_and_long_sites(): """Realistic stress: 20 datetime OR-clauses combined with 100 USGS site IDs. Every chunk URL built from the plan must fit the - 8000-byte limit, the joint planner must beat the naive "filter at - bail-floor, chunk lists" approach, and the partitioned filters - must union to the user's original filter expression. + 8000-byte limit, the joint planner must emit fewer chunks than splitting + the filter to singletons and chunking the lists separately, and the + partitioned filters must union to the user's original filter expression. Uses the real ``_construct_api_requests`` builder so the test catches URL-encoding surprises that a fake builder would miss. @@ -1393,7 +1393,7 @@ def test_joint_planner_url_construction_long_filter_and_long_sites(): url_limit = 8000 plan = ChunkPlan(args, _construct_api_requests, url_limit) - assert plan.total > 1, "expected non-trivial plan for over-limit request" + assert plan.total > 1, "expected a multi-chunk plan for over-limit request" # Walk every chunk the plan would issue and assert URL fits. over_limit = [] @@ -1414,7 +1414,7 @@ def test_joint_planner_url_construction_long_filter_and_long_sites(): f"axis {axis.arg_key} partition lost or duplicated atoms" ) - # Plan must beat the bail-floor-style worst case (singleton sites + # Plan must stay under the all-singleton worst case (singleton sites # × all filter clauses singleton = 500 * 20 = 10,000) — uniform # greedy halving of these inputs cuts that by at least 20×. assert plan.total < 500, f"joint plan emitted {plan.total} chunks (expected <500)" @@ -1734,7 +1734,7 @@ def do_GET(self): self.end_headers() self.wfile.write(body) - def log_message(self, *args): # keep pytest output clean + def log_message(self, *args): # silence the server's request log pass server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), _SlowHandler) @@ -2248,7 +2248,7 @@ async def fetch(args): def test_default_preserves_passthrough(): """The default ``max_chunks`` (1 = off) must not perturb the existing - plan: a multi-value request that fits the byte limit is still the trivial + plan: a multi-value request that fits the byte limit is still the single-chunk passthrough (no axes, ``total == 1``), byte-for-byte the pre-feature behavior.""" args = {"monitoring_location_id": ["A", "B", "C", "D"]} @@ -2260,7 +2260,7 @@ def test_default_preserves_passthrough(): def test_unit_cap_preserves_passthrough(): """``max_chunks=1`` means "no extra fan-out", so a fitting multi-value - request stays the trivial passthrough (no axes, ``total == 1``, + request stays the single-chunk passthrough (no axes, ``total == 1``, ``iter_chunk_args`` yields the original args verbatim) — identical to the default (off), not a materialized one-chunk-per-axis plan.""" args = {"monitoring_location_id": ["A", "B", "C", "D"]} @@ -2339,7 +2339,7 @@ def test_cap_below_byte_split_does_not_reduce_fan_out(): def test_cap_never_exceeds_the_byte_budget(): - """Refining on top of an over-budget request keeps the hard invariant: + """Refining on top of an over-budget request keeps the invariant: every chunk still fits ``url_limit`` (splitting only ever shrinks a chunk), and the fan-out is at least what the byte pass required.""" args = {"monitoring_location_id": ["X" * 30, "Y" * 30, "Z" * 30, "W" * 30]} @@ -2388,10 +2388,9 @@ def test_cap_bounds_fan_out_across_many_axes(): a cap of 30 fan out to *at most* 30 chunks total — never the ``30 ** 3`` a per-axis cap would allow, and never *over* the cap either. 30 is deliberately not evenly reachable by these axes: a single split - multiplies the plan by more than one, so the naive ``while total < cap`` + multiplies the plan by more than one, so the ``while total < cap`` loop the first refine used stepped past 30 (to 32). The cap is a hard ceiling — - the property neither the single-axis-only cap nor that naive loop - guaranteed.""" + the property neither the single-axis-only cap nor that loop guaranteed.""" cap = 30 # Three chunkable axes (two list axes + the filter OR-axis), each with 10 # atoms — under the old per-axis cap this would have been cap**3. @@ -2415,7 +2414,7 @@ def test_cap_bounds_fan_out_across_many_axes(): def test_cap_is_a_hard_ceiling_never_overshoots(atoms_per_axis, cap): """The cap is a hard ceiling, not a soft target. With two multi-value axes a single split multiplies the plan by ``(k+1)/k`` for the split axis — - adding the product of the *other* axes, not one — so a naive + adding the product of the *other* axes, not one — so a ``while total < cap`` loop steps *past* the cap. These are exactly the (atoms, cap) combos that loop overshot (5->6, 10->12, 7->8). The plan must fan out and cover every atom once, but never exceed the cap, landing below @@ -2435,7 +2434,7 @@ def test_cap_is_a_hard_ceiling_never_overshoots(atoms_per_axis, cap): def test_cap_does_not_mask_unchunkable(): """A request with nothing to split that still busts the byte limit must raise ``Unchunkable`` regardless of the cap — the soft pass has no axis to - act on and must not swallow the hard failure.""" + act on and must not swallow the raise.""" args = {"monitoring_location_id": "one-huge-scalar"} with pytest.raises(Unchunkable): ChunkPlan(args, _fake_build, url_limit=10, max_chunks=32) @@ -2443,7 +2442,7 @@ def test_cap_does_not_mask_unchunkable(): def test_parallel_chunks_publishes_n_as_the_effective_setting(): """The context manager sets ``n`` for the block and restores the previous - value on exit — including proper nesting. + value on exit — including across nested blocks. ``parallel_chunks(n)`` is sugar for ``configure(parallel_chunks=n)``, so both forms share one scoping mechanism and the innermost block wins. @@ -2472,7 +2471,7 @@ def test_parallel_chunks_publishes_n_as_the_effective_setting(): "8", # a string, even a numeric one "high", # the old level names are gone None, # None not accepted - True, # bool is an int subclass but nonsensical here + True, # bool is an int subclass but not a chunk count ["8"], # a list ], ) diff --git a/tests/waterdata_filters_test.py b/tests/waterdata_filters_test.py index 774fbf169..1272602a7 100644 --- a/tests/waterdata_filters_test.py +++ b/tests/waterdata_filters_test.py @@ -90,8 +90,8 @@ def test_split_top_level_or_respects_quotes(): def test_split_top_level_or_handles_doubled_quote_escape(): """CQL text escapes a single quote inside a literal as ``''``. The - two quotes are adjacent, so the scanner's naive toggle-on-quote logic - happens to land back in the correct state with nothing between the + two quotes are adjacent, so the scanner's escape-unaware toggle-on-quote + logic happens to land back in the correct state with nothing between the toggles to misclassify. Lock that behavior in so a future refactor can't regress it.""" cases = [ diff --git a/tests/waterdata_nearest_test.py b/tests/waterdata_nearest_test.py index 426be8607..cca36f6b4 100644 --- a/tests/waterdata_nearest_test.py +++ b/tests/waterdata_nearest_test.py @@ -422,7 +422,7 @@ def test_caller_properties_keep_the_columns_the_match_needs(patch_get_continuous Without the injection a list like ``['time', 'value']`` reached the service unchanged, the response came back with no ``monitoring_location_id``, and every site but one was silently dropped -- - a wrong answer with nothing for a caller to notice it by. + an incomplete result with nothing for a caller to notice it by. """ patch_get_continuous.return_value = ( pd.DataFrame( diff --git a/tests/waterdata_progress_test.py b/tests/waterdata_progress_test.py index b05ba8f2a..7c0076eda 100644 --- a/tests/waterdata_progress_test.py +++ b/tests/waterdata_progress_test.py @@ -50,7 +50,7 @@ def _run_walk_pages(*, geopd, req, client): @pytest.fixture(autouse=True) def _reset_api_key_hint_latch(monkeypatch): """The 'no API key' pointer is latched once per process; reset it so each - test sees a clean slate regardless of order.""" + test sees the latch unset regardless of order.""" monkeypatch.setattr(_progress, "_api_key_hint_shown", False) diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index d17ef585e..99938de48 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -318,7 +318,7 @@ def value_for(snake): def test_check_profiles(): - """Tests that correct errors are raised for invalid profiles.""" + """Tests that ``ValueError`` is raised for invalid profiles.""" with pytest.raises(ValueError): _check_profiles(service="foo", profile="bar") with pytest.raises(ValueError): @@ -1183,7 +1183,7 @@ def test_get_reference_table_rejects_unknown_collection_by_its_own_name(httpx_mo """The rejection names ``collection`` -- the parameter actually passed. Regression: this check was copied from ``get_codes``, message and local - variable name included, so a bad ``collection=`` was reported as an + variable name included, so an unknown ``collection=`` was reported as an invalid *code service* -- a parameter this function does not have. """ with pytest.raises(ValueError, match="Invalid collection: 'agency-codez'"): @@ -1192,7 +1192,8 @@ def test_get_reference_table_rejects_unknown_collection_by_its_own_name(httpx_mo def test_get_reference_table_serves_countries(httpx_mock): - """``countries`` is a real reference collection and singularizes correctly. + """``countries`` is a real reference collection and singularizes to + ``country``. It sits beside ``counties`` in the service catalog but was missing from the accepted vocabulary, so the rejection told a caller asking for a real @@ -1257,7 +1258,7 @@ def test_get_cql_max_rows_is_excluded_from_request_and_forwarded(): """``get_cql`` caps the total like every other Water Data getter. It was the only one without ``max_rows``, and its ``limit`` is the page - size -- so the obvious way to ask for a few rows instead paged the whole + size -- so asking for a few rows through ``limit`` instead paged the whole match a few rows at a time. A bounded probe written that way spent ~400 requests of an hourly quota of 1000 before the service refused it. """ diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index 62b461545..2f4c45bcd 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -300,8 +300,8 @@ def test_walk_pages_raises_with_class_name_when_cause_stringifies_empty(): def test_walk_pages_raises_on_5xx_mid_pagination(): """A 5xx mid-pagination must raise — partial data is no longer returned - because the API has no resume cursor, so silently truncating is the - wrong default.""" + because the API has no resume cursor, so silently truncating would return + an incomplete frame the caller cannot tell from a complete one.""" page2_503 = mock.MagicMock() page2_503.status_code = 503 page2_503.json.return_value = { @@ -504,7 +504,7 @@ def test_get_data_raises_on_mid_pagination_failure(monkeypatch): same ``_paginate`` strategy helper, so error-routing behaviour is exercised by the ``_walk_pages`` triplet above. This single ``get_data`` mid-pagination case proves the stats-specific - follow-up callback is wired into ``_paginate`` correctly. + follow-up callback is wired into ``_paginate``. Statistics drives that page walk as a one-item ``FanOut``, the same executor every other getter uses, so a transient mid-walk failure is @@ -1398,7 +1398,8 @@ def test_real_queryables_still_pass_through(name): class TestWireIdSwitch: """The API keys every collection on ``id``; callers spell it after the collection (``monitoring_location_id``). The switch happens here, and - dropping the wrong key silently sends an unfiltered query.""" + dropping an alias without carrying its value to ``id`` silently sends an + unfiltered query.""" def test_the_collection_scoped_spelling_becomes_id(self): from dataretrieval.ogc.requests import _switch_arg_id diff --git a/tests/wqp_test.py b/tests/wqp_test.py index c4e4d0b4a..8ddf4632e 100644 --- a/tests/wqp_test.py +++ b/tests/wqp_test.py @@ -323,7 +323,8 @@ def test_what_query(httpx_mock, func, service, fixture, profile_column): def test_check_kwargs(): - """Tests that correct errors are raised for invalid mimetypes.""" + """An unsupported mimetype raises ``NotImplementedError``; an unknown one + raises ``ValueError``.""" kwargs = {"mimeType": "geojson"} with pytest.raises(NotImplementedError): kwargs = _check_kwargs(kwargs)