refactor(settings)!: resolve settings with pydantic-settings - #370
Closed
thodson-usgs wants to merge 20 commits into
Closed
refactor(settings)!: resolve settings with pydantic-settings#370thodson-usgs wants to merge 20 commits into
thodson-usgs wants to merge 20 commits into
Conversation
Settings reached the library through process-global environment variables only, each with its own parser at its point of use. That cannot express a per-call credential: an application holding keys in a secret store, or a service handling concurrent users, had to assign to os.environ, which races across threads and tasks (DOI-USGS#352). Add dataretrieval.config, a stdlib-only leaf resolving every setting through one ordered chain, highest precedence first: an active configure(...) block (a ContextVar, so it cannot leak across threads or tasks), the setting's environment variable, the TOML file at ~/.dataretrieval/config.toml, then the built-in default. Precedence applies per setting, so a file that sets only `concurrency` leaves an environment API_USGS_PAT in effect. The environment sits above the file to keep the original interface authoritative; rationale in ADR 0009. The module owns each setting's grammar, so a value means the same thing whichever source wrote it, and show_configuration() reports the effective value and provenance of each without printing the key. Consequences at the read sites: - RetryPolicy.from_env becomes from_configuration. It reads a configure() block and a config file too, so a name saying "environment" is the drift this change exists to prevent. - Concurrency resolves through the chain instead of reading the environment in transport.fanout, so the rule that an explicit setting outranks an adapter preference lives in one place rather than being restated. - ConfigError is dropped in favor of the taxonomy's existing ConfigurationError -- same bases, same purpose, and only one has shipped. - default_headers checks the host before resolving the key. Resolution now reads a file and can raise, so the old order let a Water Data config problem break an NWIS, WQP, or NGWMN call that would never have received the key. - parallel_chunks(n) becomes sugar for configure(parallel_chunks=n) rather than owning a second ContextVar, so show_configuration() always reports the value the chunker will use. - ChunkedCall.resume resolves the concurrency cap per drive rather than fixing it at construction: it is the one dial a caller adjusts precisely while retrying, so a configure() block entered between the interruption and the resume has to win. _home_id mirrors expanduser per platform: posixpath reads HOME, while ntpath reads USERPROFILE then HOMEDRIVE+HOMEPATH and ignores HOME entirely. The POSIX ordering made the memo watch a variable that cannot move the resolved path on Windows -- where Git Bash and MSYS both set HOME. Settings are scoped per adapter (ADR 0010) ------------------------------------------ ADR 0009 assumed one flat namespace, on the premise that a setting means the same thing to every service. Surveying all seven APIs showed the premise is false: concurrency and parallel_chunks are meaningless to the four single-shot adapters, which have nothing to fan out, and a flat namespace accepted configure(streamstats={"parallel_chunks": 8}) without complaint -- the typo class ADR 0009 exists to catch. A [ngwmn] table in the file, or configure(ngwmn={...}), now applies to that adapter alone, so one block can be gentle with NGWMN while leaving Water Data untouched. A profile could not express this: it switches a whole configuration, not one service's slice. An adapter table overrides the top-level one per setting, inheriting everything it does not name. Precedence stays source-major -- block, then environment, then file, with an adapter-scoped value outranking a package-wide one only within the same source. Scope-major ordering would have inverted ADR 0009's environment-above-file rule the moment anyone added a table, letting a stale [wqp] entry beat a variable exported for one run. Each adapter is a named, typed parameter on configure() carrying its own TypedDict, so mypy --strict rejects a setting that adapter does not read, by schema name, before the code runs. The remaining **unknown catch-all exists only to turn a misspelled setting -- configure(concurrancy=8) -- into a message naming the settings rather than a bare TypeError. api_key is absent from every schema, and that was settled by measurement rather than argument. NGWMN is served from the Water Data host, so it already receives that key; probing both live, each returns 200 with no key and x-ratelimit-limit: 1000 with one, and alternating authenticated calls decrement a single counter (997, 996, 996, 994, 993, 992). One key, one quota pool, two adapters. The key belongs to the gateway fronting the host, so a per-adapter key would model a distinction that does not exist, and credentials keeps its host scoping unchanged. progress is likewise package-wide: there is one progress line per call. The deprecated nwis has no table. Its calls pin max_retries=0, so a [nwis] entry could only be reported as live and then ignored -- the failure this decision exists to prevent. stall_timeout joins the chain. API_USGS_STALL_TIMEOUT was read straight from os.environ, so it could not be set by a block or the file and never appeared in show_configuration() -- a gap in ADR 0009's own claim that every setting resolves through one chain. transport/env.py existed only to parse it and is deleted; config is now the only module that reads the environment for a setting. ssl_check deliberately does not join it. It disables certificate verification, so as a per-call keyword it is a visible, scoped decision, while a file key or environment variable would make a security downgrade process-wide and invisible at the call site. Its legitimate use -- a TLS-intercepting proxy -- is already served better by SSL_CERT_FILE, which httpx honors on both its sync and async clients, so that covers every getter including the OGC ones that have no ssl_check, and trusts the corporate CA rather than trusting nothing. The configuration guide documents it. Renames dataretrieval.wateruse to dataretrieval.nwdc ---------------------------------------------------- Every other adapter is named for the service it retrieves from -- ngwmn, nldi, wqp, streamstats, nwis. This one was named for a subset of what its service offers: the National Water Availability Assessment Data Companion serves ten modeled datasets, of which the water-use models are five. dataretrieval.wateruse remains a deprecated alias until 2027-08-11 or later, per the dated-removal convention nwis uses. It re-exports rather than copies, so wateruse.get_wateruse IS nwdc.get_wateruse. It is an alias for reading, not a second name for the module: assignment and private names do not forward, and the docstring says so. `import dataretrieval` stays silent -- the package imports nwdc directly, so only code naming wateruse sees the warning, pinned by a subprocess test because an import side effect cannot be tested honestly in an interpreter that already imported the package. 906 tests, mypy clean across 58 files, 7/7 contracts, all 14 hooks. BREAKING CHANGE: RetryPolicy.from_env() is renamed to RetryPolicy.from_configuration(), and resolves through the full chain rather than the environment alone. BREAKING CHANGE: dataretrieval.wateruse is renamed to dataretrieval.nwdc. The old module remains as a deprecated alias emitting a DeprecationWarning on import, and will be removed on or after 2027-08-11. Closes DOI-USGS#352. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An adapter-scoped block entry outranked a package-wide one unconditionally,
and the scope was one flat mapping with no notion of which block wrote a key.
So an *outer* configure(waterdata={...}) beat an *inner* configure(...),
inverting the nesting rule configure() documents.
Two user-visible consequences, both reproduced:
configure(waterdata={"parallel_chunks": 2}):
parallel_chunks(16) -> resolved 2, not 16
configure(waterdata={"concurrency": 32}):
configure(concurrency=1) -> resolved 32, not 1
The first silently discarded a per-call request: parallel_chunks(n) is sugar
for configure(parallel_chunks=n), which writes the package-wide key, so any
enclosing adapter table outranked it. The second made the documented recovery
from QuotaExhausted -- wait, then re-issue more gently -- inexpressible once
any adapter table was in play.
Each scope entry now carries the depth of the block that wrote it. An
adapter-scoped value outranks a package-wide one only at the *same* depth,
where one block named both and the adapter is the more specific of the two; a
deeper package-wide value was written by an inner block and wins. Same-source
precedence between the two scopes is unchanged for the case it was designed
for, and nesting behaves as documented again.
Found by the xhigh review, which flagged both and whose verifiers reproduced
them independently; the earlier /simplify pass had blessed the flat mapping as
correct, which it was for merging within one scope and not across two.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The "check whether a leaf already generalizes it" rule named `transport.retry._read_env_number` as the home for `API_USGS_*` settings. That module is deleted: every setting now resolves through `config`, which is also the only module left that reads the environment for one. The rule is binding, so a stale entry is worse than a missing one -- a contributor following it imports a symbol that does not exist, and the leaf that would actually have answered their question goes unmentioned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docstring said "deliberately a leaf" after the module gained a module-level import of `dataretrieval.config` (for `API_KEY_ENV`, so the environment variable's name has one home). A reader acting on the stale claim would remove that import to restore the property and break `API_KEY_ENV` at import time. Says what is actually true instead: config is its one first-party dependency, is itself a standard-library-only leaf, and sits directly beneath this module in the layers contract. It supplies the key's value; the questions this module owns -- which host may receive it, and how it is withheld from every other -- are unchanged. Also drops the function-local `import config` in `api_key()`, which duplicated the module-level import and carried a comment explaining a laziness that no longer applied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eout
configure() grew seven parameters this branch documents nowhere: the six
per-adapter tables and stall_timeout. The rendered API reference showed a
signature it never described, so the headline feature of ADR 0010 was
undiscoverable from the docs -- a caller had to read the ADR or the source to
learn that configure(ngwmn={...}) exists at all.
The adapter entry states what a reader cannot infer from the signature: that a
table overrides the package-wide value per key, that each adapter accepts only
the settings it reads and is annotated so a type checker says which, and why
api_key, progress and nwis are absent.
Also sharpens the module docstring's precedence paragraph, which said an
adapter-scoped value outranks a package-wide one "within the same source" --
true, but no longer the whole rule now that block entries carry their nesting
depth. Within the block source the tie-break is per block, and a nested block
still wins over both.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six docstrings across three modules still presented the environment variable
as *the* mechanism for concurrency and retries, which stopped being true when
those settings began resolving through the chain -- and stopped being the
whole story again when they became scopable per adapter.
The most user-visible was `nwdc.get_wateruse`, whose own docstring told a
reader that `API_USGS_CONCURRENT` was how to change fan-out, so a caller
reading the getter they were calling would never learn that
`configure(nwdc={"concurrency": 2})` or an `[nwdc]` table exists. The rest are
the fan-out and chunking module docstrings and `FanOut.default_concurrent`,
which described the env var as the thing a service default fills in for.
Each now names the *setting* and points at the chain, mentioning the
environment variable as one source among several rather than the interface.
No behavior change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six cleanups from the xhigh review, none changing behavior except where noted. show_configuration()'s docstring sample was missing two settings the function always prints and the adapter-overrides section entirely, so a reader comparing real output against the docs saw rows the docs denied. Regenerated from actual output rather than edited by hand -- the same sample had already gone stale twice by being edited. The `keyword in SETTINGS` branch in _normalize_adapters was unreachable once every setting became a named parameter: nothing that reaches the catch-all can be a setting. Removed, and the surviving message says why anything arriving there is unknown. The unknown-setting warning hard-coded stacklevel=2 while _WARN_STACKLEVEL exists to name that number once; changing the constant would have moved two of three warnings. _resolve loaded the config file twice on an adapter-scoped read -- once for the adapter table, once for the top level -- so the common case (no table for that adapter) paid an extra stat per setting. It now loads once and passes the parsed file to both tiers: measured 2 stat calls -> 1 for retries(adapter="waterdata"), and 4 -> 2 per RetryPolicy.from_configuration. _show_adapter_overrides re-resolved each package-wide source label it needed, though show_configuration had just computed all of them. Beyond the repeated work, cell() mutates the shared error-dedupe state, so a broken config's message could be consumed by a throwaway comparison before the row that needed to print it. It now takes the labels already resolved. tests/conftest.py drops the flaky_api marker and its rerun patterns. They are deliberate, documented infrastructure, but nothing references them and they have nothing to do with configuration -- they belong in the change that adds the live tests using them, not in this one. Not applied: the adapter roster appears in the TypedDicts, _ADAPTER_SCHEMAS and configure()'s parameter list. Deriving any of those from another would give up exactly what the explicit parameters buy -- a type checker naming the schema in an error about a call -- so the repetition is the price of the static check, not an oversight. 909 tests, mypy clean across 58 files, 7/7 contracts, all 14 hooks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d values The precedence fix encoded each block's nesting depth into every value it wrote, then re-derived that depth on entry by scanning the outer scope. Keeping the frames themselves says the same thing with less machinery: "the innermost block wins" falls out of iterating them in reverse, so the depth field, the scan, the tuple unwrapping at every read site, and two comments restating the tie-break rule all go away. _resolve returns to nearly its pre-fix shape, wrapped in one loop. Net 11 lines lighter. It also removes a subtlety worth not having: `max(...) + 1` computed "one more than the deepest *recorded* depth", not the nesting level -- an enclosing block that set nothing was invisible, so the stored number was only decision-equivalent to depth, and a reader had to prove that before trusting the comparison. `len(frames)` needs no proof. The depth carried on the profile key was write-only state for the same reason. And it undoes a regression the depth version introduced: entering a block was O(all keys ever set), measured 18% slower at any nesting and 36% inside a large outer scope. Appending a frame is O(1). Reads become O(nesting depth) rather than O(1), which is 1-3 in practice and below the noise floor. Behavior is unchanged and re-verified, including three-deep nesting and an adapter block inside a different adapter's block, which the depth version was never exercised against. Also from the review: - The user guide still stated the pre-fix rule -- that an adapter-scoped value outranks a package-wide one "only within the same source" -- which stopped being the whole truth for blocks. It was the one of three user-facing copies the fix missed, and this diff had just pointed nwdc's docstring at it. - `_file_settings` took `path`/`parsed` as optional while its sibling `_adapter_file_settings` required them, so the file tier had two entry shapes and a caller could hand the two tiers different parsed files -- the drift the single-load change existed to remove. Both now take the pair, from one `_current_file()` helper. - The `for keyword in unknown` loop always raises on its first iteration; it is an `if`, and now says so. - configure()'s new docstring restated the api_key/progress rationale a fourth time. It points at ADR 0010 and keeps only the `nwis` clause, which appears nowhere else. 909 tests, mypy clean across 58 files, 7/7 contracts, all 14 hooks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Swept into the squashed commit by a `git add -A`, the same way CONFIG-PLAN.md was. 4549 lines of resolver output in a library whose pyproject declares ranges and which is not deployed from a pinned set, so it constrains nothing and dates immediately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ADR 0011 proposes per-adapter configuration profiles, class-based configure(), schemas owned by adapters with names held centrally, and the precedence ladder that follows -- including the one inversion of ADR 0009's environment-above-file rule, for a profile a caller selects in code. CONTEXT.md gains 'configuration profile' (short: configuration), 'default profile' vs 'named profile', 'effective configuration' for the resolved result, and 'selection'. The config-as-abbreviation rule is withdrawn. Model first, before the code that implements it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prefactor for the configuration-profile work: the concept is spelled in full everywhere else, and ADR 0011 withdraws the rule that reserved `config` as an abbreviation for the module and the file. The path has never been released, so no alias and no deprecation. The user-facing file keeps its name -- `~/.dataretrieval/config.toml` is a separate decision nobody has made, and renaming it would break every example for no gain. No behaviour change; 909 tests, mypy clean, 7/7 contracts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A configuration profile is now a named set of settings for ONE adapter, and
``configure()`` takes those configurations positionally instead of keywords
(ADR 0011).
The adapter a configuration targets is a property of its class, so a caller
never restates it -- which is what removes the adapter roster from every call
site. Each adapter owns its class, defined in the module that *reads* those
settings; ``dataretrieval.configuration`` keeps only the adapter names, which
is what parsing a file needs and all a standard-library-only leaf can hold.
with dataretrieval.configure(
Configuration(api_key=vault.read("usgs/pat")),
WaterdataConfiguration.load("bulk"),
NgwmnConfiguration(concurrency=4),
):
...
- ``BaseConfiguration``: frozen dataclass, all fields optional, ``adapter``
ClassVar, a ``validate()`` hook for rules no single setting can express,
``settings()``, ``values()``, and ``load(profile)``. Values are checked at
construction, so a typo raises on the line that wrote it.
- ``Configuration`` carries the package-wide settings; ``ADAPTERS`` holds the
roster and ``_register`` populates a registry at adapter import.
``settings_for()`` returns None for an adapter this process has not imported
-- "cannot validate these keys yet", never an error, since nldi is
import-on-demand for the geopandas extra.
- The file gains named profiles beside each adapter's default profile:
``[waterdata]`` always applies, ``[waterdata.bulk]`` only when selected, and
a named profile inherits the tiers below it per key. Selecting one is code,
so it outranks the setting's environment variable -- the one place ADR 0011
inverts ADR 0009.
- An adapter configuration may carry ``base_url``, settable in a block and
refused from the file and the environment: a file that silently redirects a
data-retrieval library to another host is a supply-chain-shaped hazard.
Breaking, none deprecated because nothing shipped: ``configure(api_key=...)``,
the ``configure(ngwmn={...})`` mappings and their public ``*Settings``
TypedDicts, the global ``[profiles.<name>]`` table and ``DATARETRIEVAL_PROFILE``
are all gone. A file still using the retired table gets an error naming its
replacement. ``show_configuration()`` drops its ``profile`` row and now names
any adapter it could not cover rather than omitting it -- the honest cost of
validating an adapter's keys lazily.
Tests migrated to the new API; those exercising the retired global profile
table are deleted. Added: same-adapter collision, a non-configuration
argument, a package-wide configuration resolving end to end through every
tier, an adapter configuration narrowing to one adapter, named profiles
selected independently per adapter, a code-selected profile beating the
environment, base_url from code and refused from the file, and the
``validate()`` hook.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
ADR 0011's file grammar -- top-level scalars, a ``[<adapter>]`` default profile, and inert ``[<adapter>.<name>]`` named profiles -- landed with the configuration-object refactor but was only partly pinned by tests. Cover each rule the grammar promises, and close the one hole the coverage found. A table nested inside a named profile was silently dropped, so a file migrated from the retired ``[profiles.bulk.ngwmn]`` -- where a profile did carry per-service detail -- would look like it had tuned NGWMN and tune nothing. Selecting such a profile now raises and names the shape to write instead. The check is lazy, like every other key check on a profile: a malformed profile for one adapter must not fail another adapter's call. Tests added for: a named profile layering per key over the default profile and the package-wide keys; a file gaining a profile changing no resolved setting until code selects it; a nested table inside a profile; ``DATARETRIEVAL_PROFILE`` staying retired rather than quietly honored under the new grammar; a malformed ``[nldi]`` table costing a Water Data call nothing; and a ``[nldi]`` table staying valid while nldi has never been imported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
…file ADR 0011's ladder was implemented by the file-grammar step but only partly pinned: the suite asserted rungs 1, 3 and 5 and one instance of 2-beats-3, so a refactor that fused two rungs could still pass. Verified each of the ADR's requirements against the code and a live probe, then covered the ladder as a chain -- each test knocks out the rung above and asserts the next takes over. `nwdc` and `concurrency` are the pair that can express all seven rungs: NWDC is the one adapter shipping a built-in preference of its own (4) distinct from the package default (32), which is the only way rungs 6 and 7 can be told apart at all. Resolution goes through the adapter's own read site so that preference is really in the chain. Confirmed non-vacuous by mutation: moving the environment above the configure() frame in _resolve fails the 2-beats-3 test, dropping the [<adapter>] tier fails both rung-4 tests, and ignoring the read site's `default` fails both rung-6 tests. Also covered: rungs 1 and 2 cannot tie, because both name one adapter and the same-adapter rule refuses the pairing -- with the neighbouring case a reader of the ladder gets wrong, a package-wide Configuration beside a loaded profile, where the adapter-scoped value wins for that adapter and the package-wide one still governs the rest. And load() itself: it returns an instance of the class it was called on carrying only the profile's keys, and an undefined name lists the names the file does define, for that adapter alone. No behaviour change. The docs said precedence in four tiers where the ADR says seven, so the user guide now spells all seven -- the tier it was hiding is an adapter's own built-in preference -- and the module docstring says how its four sources map onto them.
show_configuration() could say a value came from a configure() block but
not *which* configuration put it there, so a caller who selected the wrong
profile -- or who had forgotten one was selected -- had nothing to look at.
A loaded configuration now remembers the profile it was read from, and each
block override carries the label naming it, so a row reads
``configure() block [waterdata.bulk]``: the table's own spelling, greppable
in the file that defines it.
The label is built in ``_frame`` rather than at resolution time because
that is the last point where the profile is known -- values from
``load("bulk")`` and from ``WaterdataConfiguration(...)`` are
indistinguishable once they are in the frame. Frames therefore hold
(value, source) pairs, the shape the file tier already returned.
The report also lists the named profiles the file defines, selected or
not. A named profile changes nothing until a caller selects it, which is
the thing readers of a file get wrong; without the line, "I added
[waterdata.bulk] and nothing changed" has nothing to explain it. Names come
from the parsed file, so an unimported adapter's profiles are listed too --
what a table means needs the import, what it is called does not. Nothing in
the section validates, so a malformed profile is reported rather than
taking the report down with it.
The unimported-adapter caveat moves to its own printer, keeping section
order (overrides, profiles, caveat) and leaving each function one section.
Behaviour there is unchanged: nldi is named, never omitted.
Both documented samples were regenerated by running the function -- the
docstring had wrapped a line the function prints whole, and the guide had
lost a paragraph. A new test rebuilds the scenario, compares the captured
output verbatim, and fails when either drifts, since a sample kept by hand
is only as fresh as the last person who remembered it.
Every adapter configuration already carried a base_url; nothing read it. Each adapter now resolves its base per call -- ``base_url(adapter=...) or <the service's own>`` -- at the one place its request URLs are built, so a ``configure`` block redirects that adapter and no other. Resolution is per call rather than at import because a block is scoped to a ``with`` statement and delivered through a ContextVar; a constant computed once could only describe the process. Water Data serves four APIs from one root, so its value names that root and ``endpoints.redirected()`` moves the OGC collections, the Samples database, the statistics service and the STAC catalog together -- a redirect that reached only the family a caller happened to call first would send the rest to the host they were redirecting away from. WQP's two interfaces move together for the same reason. NWDC threads the resolved host into its page walk as well: its ``water.usgs.gov`` alias list and host rewrite are facts about that service, so a redirected call gets the general rule instead (follow a link only back to the host that served the page), rather than having page two rewritten onto the USGS. The environment is now refused out loud instead of merely unread: ``API_USGS_BASE_URL`` is the spelling every other setting's variable predicts, so exporting it and being silently ignored would leave a caller believing they had redirected something. It raises before any source is consulted, matching the file, which raises whether or not a block also set one -- a variable that cannot work must not be quietly outranked by a block that happens to work, since it survives to a run where nothing outranks it. The API key needs no new rule to stay put: credentials checks the host when attaching it, so a redirected adapter simply does not receive it. Tested anyway, with a control on the same key and the same call to the real host, because "needs no rule" is the kind of claim that stops being true silently. Tests: a code override moving real traffic for two adapters with unrelated request machinery (OGC engine, one-shot GET), the four Water Data families moving together, the environment refusal (including that show_configuration() reports it rather than raising), NGWMN redirected alone while Water Data stays on the host they share, the NWDC page walk following the mirror, WQP's two interfaces, NLDI's catalog probe and query, and the credential not following a redirect. Each was proved non-vacuous by mutating the source and reverting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
`configure(api_key=...)` no longer exists, so every place that showed it, or
the per-adapter `configure(ngwmn={...})` mappings, was wrong. This is the
documentation half of that change; no behaviour moves.
The configuration guide now leads with the case the feature exists for --
one block, three services, two loaded from the file by name and one built in
code -- instead of burying it under "Named profiles" as a second example.
That section keeps only what is specific to the file grammar and `load()`,
and points back rather than restating a worked example. The example was run
before it was written: the resolved values in the prose are what the chain
actually produces for that file, including the two that are easy to get wrong
(WQP departs from the file's `retries`, and the key reaches only the host
that honours it). The `configure` block section gains a note naming the
retired spellings, since that is what a reader of an old script will search
for.
ADR 0011 moves to Accepted: every item in its own Compliance list is covered
by a shipped test, so the section now names them instead of asking for them.
ADRs 0009 and 0010 are marked, not rewritten -- their Status sections say
which clauses 0011 supersedes, and the clauses themselves carry a marker
where a reader could otherwise act on superseded text (0010's decisions 5 and
8, the `configure()` spelling in its decision 1, and the two consequences
that turned on refusing a configuration object). Broken pointers left by the
`config` -> `configuration` rename are fixed in place; no decision text is
edited to reverse its meaning.
`show_configuration()`'s sample is unchanged and re-verified by running it:
`test_show_configuration_sample_output_is_current` rebuilds the scenario and
compares the captured output verbatim against both the docstring and the
guide.
Also: the `**queryables` credential refusal now names the current spelling in
the TypeError a caller actually sees, and two comments in `nwdc` and
`transport.fanout` stop illustrating adapter scope with a keyword that is
gone.
949 tests, mypy --strict clean, 7/7 contracts, pre-commit clean, docs build
adds no new warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
The migration removed one duplication (the adapter roster at every call site) and introduced several smaller ones. This collapses them, and closes the two seams the review found guarded only by convention. Duplication removed - Shared setting groups. `retries` / `stall_timeout` / `base_url` / `concurrency` / `parallel_chunks` were each spelled once per adapter class, with verbatim docstrings and nothing keeping them in step -- the annotations are decorative, since `_coerce_typed` keys the type check by setting *name*, so a drift to `retries: str | None` type-checked clean. Each is now declared once as `_Retrying` / `_Redirectable` / `_Concurrent` / `_Chunked` in `configuration`, and an adapter composes the groups it reads. *Which* groups is still the adapter's own knowledge, which is what ADR 0011 asked for. - `SETTINGS` is derived from `fields(Configuration)` rather than restated, in the one module whose job is to stop rosters being duplicated. A field added and forgotten used to work from `configure()` and be silently dropped from the file. - `_REFUSED_ENV_VARS` is derived from `ADAPTER_ONLY_SETTINGS`. The two tables spelled one fact -- "this setting is code-only" -- and a second adapter-only setting added to the roster alone would have been refused by the file and *silently ignored* from the environment, which is the defect the table was written to prevent. - `base_url()` takes the fallback: `base_url(adapter="nldi", default=NLDI_API_BASE_URL)`. The `... or SERVICE_DEFAULT` rule was spelled at five read sites; the service's own URL still lives beside the service. - `_checked_table()` is the one check-one-table routine; `_scalars` and `_named_profile` each take the half they need. They had been the same loop, including the source-label format, written twice. - The credential-name predicate moves to the `credentials` leaf as `refuse_credential_keywords()`. The fact that motivates it -- `api_key=` is a plausible kwarg now that `configure()` takes it -- is package-wide, and WQP's ten getters forward `**kwargs` straight into the query payload. BEHAVIOR CHANGE, noted in NEWS: a credential-shaped WQP filter now raises `TypeError` rather than putting a secret in a URL. Work removed - `_frame()` renders with `_coerce_typed` instead of re-running the grammar `__post_init__` already ran. Construction is the single validation point. - `parallel_chunks(n)` validates through `_validated_raw`, the table that owns the setting's bound, rather than a second grammar in `ogc.policy`. `ConfigurationError` is a `ValueError`, so the caller contract holds; the matched text in the test moves to the shared message. - `settings()` is memoized on the class. Rebuilding a six-element frozenset from `fields()` was 17-19% of every adapter-scoped read, and `_accepts` asks for it before the frame walk and before the file. Keyed on the class, not the name, because tests replace registry entries. Seams closed - `_resolve` raises on an adapter name it does not recognize. A typo used to resolve silently package-wide, so a `[waterdata]` table would be ignored with nothing raised; the fitness test could only see that the correct string occurred somewhere. - New AST fitness test asserts every Water Data endpoint constant reaches `redirected()`. Water Data is the one adapter that cannot resolve its base at a single choke point, and a bare use sends traffic to the host a caller redirected away from with nothing failing. Also: `_WARN_AT_TOP_LEVEL` declares the `parallel_chunks` warning as data beside the other per-setting policies rather than as a name test in a loop body; `progress` imports `configuration` at module level like its two siblings (no cycle exists); the Windows stamp-cache comment carries the measured cost so the next reviewer does not re-derive it; NEWS collapses three entries describing one unreleased feature's superseded designs into one; and ADR 0011's `show_configuration()` consequence is reworded to what shipped -- the profile list is deliberately not import-limited, and a test forbids filtering it. Skipped: the per-service `_api_base()` / `_service_base()` wrappers stay. The duplication the review named was the fallback *rule*, which `base_url()` now owns; deleting the wrappers would push the adapter name and the service constant back out to ten call sites. Two performance findings and one Windows finding were explicit no-ops.
CI pins ruff 0.16.1 and runs `ruff format --check .` over the whole tree, which formats code blocks inside Markdown. The local pre-commit hook only targets Python files, so a README example never reached it -- pre-commit passing does not imply CI lint passing, which is how this got pushed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reimplements the layered settings chain from PR DOI-USGS#353 on pydantic-settings instead of a hand-rolled one, and renames the vocabulary to the library's. The behavior is the same; the implementation is not. Each adapter's settings profile is now a BaseSettings subclass, so its field annotations are enforced rather than decorative -- ADR 0010 noted that an adapter drifting to `retries: str | None` would type-check clean under mypy --strict and fail only when a value reached the chain. Each rung of the ladder is a PydanticBaseSettingsSource, listed in `_CHAIN` highest first, so precedence being per setting rather than per source is an ordering rather than a stack of hand-written fallbacks. Deleted: _coerce_typed, _validated_raw, the _UNSET sentinel (pydantic's model_fields_set already distinguishes an omitted setting from an explicit None), the memoized _settings_of, and the hand-written merge. Kept, because pydantic-settings has no opinion about them: the TOML grammar of adapter tables and named profiles, the file cache, the provenance labels show_settings() reports, and the ContextVar that carries a configure() block. Vocabulary: Configuration -> Settings, BaseConfiguration -> AdapterSettings, <Adapter>Configuration -> <Adapter>Settings, show_configuration() -> show_settings(), RetryPolicy.from_configuration() -> from_settings(), and dataretrieval.configuration -> dataretrieval.settings. The file keeps the name config.toml and the variable keeps DATARETRIEVAL_CONFIG: both are compatibility surfaces, and config is the conventional name for a file on disk. BREAKING CHANGE: pydantic-settings is a required dependency. Settings resolve on the request path -- every call reads the API key -- so an optional one would mean shipping a second standard-library implementation of the same chain and testing both. BREAKING CHANGE: a setting an adapter does not read, or a misspelled one, raises ConfigurationError from extra=forbid rather than a bare TypeError. The same mistake written into the settings file has always raised ConfigurationError, so the two surfaces now agree, and the message lists the settings the adapter does accept. ADR 0012 records the decision, including why dynaconf and typed-settings were rejected, and withdraws ADR 0009's standard-library-only clause -- its first-party half (no adapter imports, so no cycle) still stands and is still asserted by a fitness function. ADRs 0009-0011 are amended where superseded, and CONTEXT.md's glossary follows the vocabulary. Resolution deliberately does not go through BaseSettings.__init__, which builds four stock sources per instantiation -- two of them snapshotting and case-folding the whole of os.environ -- before settings_customise_sources can discard them. That is right for a settings object built once at start-up and wrong for lazy per-read resolution: profiling put 74% of one read there. A read now costs about twice the hand-rolled version (26-37us -> 68-82us with no settings file), which at the eight reads a one-chunk query performs is ~0.3ms against ~0.6ms on a 100-500ms round trip. PR DOI-USGS#353's 144 settings tests carry over intact and pass unchanged, which is the evidence that the behavior is preserved. Full suite: 956 passed, mypy --strict clean over 59 files, ruff clean, import-linter 7/7.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #353. Same behavior, different implementation: the layered settings
chain is now built on
pydantic-settings
rather than hand-rolled, and the vocabulary follows the library's.
#353 should stay open until this is reviewed — this branch is its content
plus the refactor, so whichever lands, the other closes.
Why
#353 hand-wrote a settings library: a per-setting type check (
_coerce_typed),a grammar per setting (
_VALIDATORSand the_parse_*family), a merge acrosstiers, and a frozen dataclass per adapter whose annotations ADR 0010 itself
called "decorative" — an adapter that drifted to
retries: str | Nonewouldtype-check clean under
mypy --strictand fail only when a value reached thechain. That library already exists and is maintained by people whose job it is.
What pydantic-settings replaces
Field declaration, type coercion, bounds,
extra="forbid"typo detection, andthe source-ordering framework. Each adapter's profile is a
BaseSettingssubclass, so the annotations are now enforced. Each rung of the ladder is a
PydanticBaseSettingsSource, listed in_CHAINhighest-first, so ADR 0009's"precedence is per setting, not per source" is an ordering rather than a stack
of hand-written fallbacks.
Deleted:
_coerce_typed,_validated_raw, the_UNSETsentinel (pydantic'smodel_fields_setalready distinguishes an omitted setting from an explicitNone), the memoized_settings_of, and the merge in_resolve.What it does not replace
The TOML grammar of adapter tables and named profiles, the file cache, the
provenance labels
show_settings()reports, and theContextVarthat carries aconfigure()block. pydantic-settings has no opinion about any of them, so theysurvive from #353 largely unchanged.
TomlConfigSettingsSourceis deliberatelyunused: it reads a flat mapping per instantiation with no adapter tables, no
profiles, no cache and no permission check.
Naming
ConfigurationSettingsBaseConfigurationAdapterSettingsWaterdataConfigurationWaterdataSettingsshow_configuration()show_settings()RetryPolicy.from_configuration()RetryPolicy.from_settings()dataretrieval.configurationdataretrieval.settingsBaseConfiguration.validate()AdapterSettings.validate_settings()configure()keeps its name — it is the verb, and pydantic-settings has nocompeting spelling.
validate()had to move because pydantic'sBaseModelalready owns it. The file stays
~/.dataretrieval/config.tomland the variablestays
DATARETRIEVAL_CONFIG: both are compatibility surfaces, andconfigisthe conventional name for a file on disk.
CONTEXT.md's glossary follows.Breaking changes
on the request path — every call reads the API key — so an optional dependency
would mean shipping a second standard-library implementation of the same chain
and testing both, which is more hand-rolled settings code than this PR exists
to delete. ~9 MB installed against the ~100 MB of pandas and numpy already
required.
ConfigurationErrorfromextra="forbid"rather than a bareTypeError.The same mistake written into the settings file has always raised
ConfigurationError, so the two surfaces now agree, and the message lists thesettings the adapter does accept.
Everything #353 broke is still broken the same way; nothing new.
Alternatives considered
Recorded in full in ADR 0012.
dynaconf — rejected. Schemaless (
Validatorobjects are runtime assertions,not annotations), so the per-adapter vocabularies ADR 0011 is built around go
back to a hand-maintained table — the failure mode that shipped undetected for
three adapters. And its environments switch every service at once, which is
exactly the global
[profiles.<name>]table ADR 0011 retired.typed-settings — near miss. Its loader chain is arguably a cleaner statement
of a tiered resolution, and its attrs/dataclass backend would have left the
adapter classes as the dataclasses they already were. Rejected because the
custom work does not shrink — adapter tables, profiles, the ContextVar tier and
the
base_urlrefusal are custom loaders either way — while it is a muchsmaller project, has no
extra="forbid"equivalent for unknown keys in anarbitrary TOML table, and exposes no stable per-value provenance. Provenance is
load-bearing:
show_settings()exists to report it.Performance
Resolution deliberately does not go through
BaseSettings.__init__. Thatbuilds the four stock sources on every instantiation — two of which snapshot and
case-fold the whole of
os.environ— beforesettings_customise_sourcescandiscard them. Right for a settings object built once at start-up; wrong for lazy
per-read resolution. Profiling put 74% of a single read inside
_settings_init_sources._resolvedtherefore walks_CHAINitself; thesources, their order, the field schema,
extra="forbid"andmodel_post_initare all still the library's.
Measured against #353 in isolated environments, per read:
About 2× the CPU cost. The file case is dominated by the file open, which both
re-read on Windows for the reason ADR 0009 gives and which on-access virus
scanning inflates on the measuring machine. At the eight reads a one-chunk query
performs that is ~0.3 ms against ~0.6 ms, on a 100–500 ms round trip — a
fraction of a percent either way.
Verification
pass unchanged — that is the evidence the behavior is preserved, and the four
that did change are message-wording and the
TypeError→ConfigurationErrorcase above.
mypy --strictclean over 59 filesruff checkandruff format --checkcleanimport-linter7/7 contracts keptfeat(config)!: resolve settings through a layered chain #353)
test_config_is_a_standard_library_only_leafis nowtest_settings_is_a_first_party_leaf: ADR 0012 withdraws the third-party halfof that constraint deliberately. The first-party half — no adapter imports, so
no cycle — still stands and is still asserted, against a pinned roster so a leaf
that quietly grew
httpxorpandasstill fails.Not done here
Raising
requires-pythonto>=3.11would drop thetomlibackport and lettyping.Selfreplace the_STypeVar. Both are consequences of the floorrather than of this refactor, so they belong to whichever change moves it.