feat(config): resolve settings through a layered chain - #353
Draft
thodson-usgs wants to merge 10 commits into
Draft
feat(config): resolve settings through a layered chain#353thodson-usgs wants to merge 10 commits into
thodson-usgs wants to merge 10 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 that resolves every setting through one ordered chain: a dataretrieval.configure(...) block, then ~/.dataretrieval/config.toml, then the setting's environment variable, then the built-in default. Precedence applies per setting, so a file that sets only `concurrent` leaves an environment API_USGS_PAT in effect. The module owns each setting's grammar, so a value means the same thing whichever source wrote it, and show_config() reports the effective value and provenance of each without printing the key. Notable choices, with rationale in ADR 0006: - No public getter grows an api_key= or session= parameter. Every Water Data getter forwards unknown keywords to the API via **queryables, so a credential parameter missed in one of ~20 signatures would be serialized into a URL. A fitness function now asserts none of them accept one. - The environment ranks below the file, inverting the AWS/lithops habit: here the environment variable is the legacy mechanism, and a written file is a more deliberate statement of intent than a stale shell export. - parallel_chunks is configurable from the file and a block but has no environment variable, because it spends rate-limit quota and must stay a deliberate choice rather than something exported once and inherited by every subprocess. - TOML via stdlib tomllib; the tomli backport is marker-scoped to Python 3.10 and drops itself when requires-python moves to >=3.11. Host scoping is unchanged and unconditional: a key from any source is sent only to api.waterdata.usgs.gov and stripped on cross-host redirects. Behavior is identical when no file exists and no block is active. Closes DOI-USGS#352 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
Cleanup pass over the layered-configuration change. parallel_chunks had become a second, competing scoping mechanism: its own ContextVar plus a helper that special-cased None to fall back to config. That silently inverted the documented precedence -- inside `with parallel_chunks(2): with configure(parallel_chunks=8):` the chunker used 2 while configure() claimed to be the highest-precedence source and show_config() reported 8. parallel_chunks(n) is now sugar for configure(parallel_chunks=n), so one mechanism backs both spellings, the innermost block wins, and show_config() always reports the value the chunker will use. It keeps its own strict type validation: the Python API rejects a float, a bool, or the numeric string "8", while a value from a TOML file or an env var is a string by nature and parses more leniently. Also: - Delete _CONCURRENCY_ENV/_DEFAULT/_UNBOUNDED and _RETRIES_ENV. All four were assigned and never read once their parsers moved into config; the comments claiming "use sites" described none. _RETRIES_DEFAULT stays -- it is the RetryPolicy field default. - Collapse three copy-pasted integer parsers into one _parse_int(minimum=, default=, examples=). Shrink the eager-validation table to the settings whose grammar can actually reject a value, which lets it be typed instead of Any. - Fold the profile ContextVar into the settings mapping, so nesting and restore-on-exit come from the one merge that already existed. - Replace _display's if-chain, whose last branch was an unguarded fall-through that would print the progress value for any new setting, with a table asserted to cover SETTINGS. - Rename RetryPolicy.from_env to from_config: it reads a TOML file and a ContextVar, and the old name needed a docstring apologizing for itself. Memoize the resolved config path on the raw DATARETRIEVAL_CONFIG value. It sits on the per-request path via _default_headers, and rebuilding it (Path.home() dominating) cost more than the stat it leads to. Measured api_key(): 21 us -> 2.7 us; _default_headers now runs ~1.2 us above the old os.getenv baseline. Per review measurement, caching the merged mapping was NOT worth it -- it is ~1.5% of the cost and would add an invalidation key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
Selecting a profile that the config file doesn't define raised ConfigError unconditionally -- including when there is no config file at all, since _load_file returns an empty _ParsedFile for a missing file. A lingering DATARETRIEVAL_PROFILE export then failed *every* call from inside _default_headers, including legacy NWIS and WQP requests that never read the config, with an error pointing at a file that doesn't exist. Raise only when the file exists: then an undefined profile is a genuine typo. With no file there are no profiles to select from and the whole file layer is inert, so the selection is moot. _ParsedFile carries an `exists` flag to tell the two apart. Also refresh two comments in ogc/chunking.py that still described the concurrency cap as resolving from API_USGS_CONCURRENT, which has been the lowest-precedence of three sources since the config chain landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
Nine of ten verified findings from the high-effort review. Correctness: - A blank-but-set environment variable counted as "configured" and so masked the config file for that setting. Container and CI tooling routinely materializes one (`docker run -e API_USGS_PAT` with nothing to pass, a workflow secret absent on a fork), which silently discarded a file-supplied API key and sent every request unauthenticated. Blank now ranks below the file, and still above the built-in default so the environment-only meaning of blank (progress off, numeric default) is preserved. - ChunkedCall.resume() resolved the concurrency cap inside the construction- time context snapshot, so `with configure(concurrency=2): exc.call.resume()` -- the documented way to recover from QuotaExhausted more gently -- was silently ignored, while the equivalent API_USGS_CONCURRENT export still worked. The cap is now read from the caller's live context; everything the rebuilt sub-requests need, including the credentials the call started with, still comes from the snapshot. - ConfigError moved into the taxonomy as DataRetrievalError + ValueError. Configuration resolves on the request path, so a broken file surfaces from inside whichever getter runs first and escaped `except DataRetrievalError` entirely; the ValueError base keeps older handlers working. - The credential denylist for **queryables matched three exact names, so `x_api_key` -- the spelling the README's `X-Api-Key` header invites -- went into the query string. It now matches credential markers as substrings. Rejecting errs on the safe side: a false positive costs one clear TypeError, a false negative writes a token into a URL. - A non-regular config path was rejected, so `DATARETRIEVAL_CONFIG=/dev/null` (and process substitution) -- the conventional way to isolate a run -- raised on every request. Only a directory is an error now. - A relative DATARETRIEVAL_CONFIG was memoized against the first cwd, so a scheduler or notebook that chdirs per job kept reading the first job's file. The memo key now includes the working directory for relative paths. - show_config() rendered each setting by invoking its parser, so the tool for explaining a broken configuration raised on one. Nothing in it raises now: a whole-file failure is reported once on the file line, a per-setting failure in its own row. Cleanup: - The per-setting type ladder was written twice, for configure() kwargs and for TOML scalars, differing only in wording and int vs Integral. One _coerce_typed covers both, so a tightened rule cannot land on half. - parallel_chunks at the top level of the config file now warns, steering it into a [profiles.<name>] table. It is the one setting that spends quota, and a top-level value applies to every query in every process that reads the file. Not applied: enabling the parsed-file cache fast path on Windows. The os.name != "nt" gate guards a real staleness bug -- Windows ctime is creation time, so a rewrite that restores mtime (cp -p, rsync --times) is invisible to the metadata stamp, and tests/config_test.py::test_file_edit_is_picked_up covers exactly that. Serving a stale API key is worse than the re-read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
Cleanup pass over the review fixes.
Two of the previous fixes were shallower than they looked:
- The blank-environment rule was implemented as a second, lower visit to the
environment -- a fourth precedence tier that the module docstring and ADR
described as three. Whether blank is a value is a property of the setting
(it is one for `progress`, an absence for the rest), so it is now declared
once in `_BLANK_MEANS_SET` and the chain visits each source exactly once.
- Treating a non-regular config path as readable fixed `/dev/null` but broke
the FIFO case it advertised: settings are re-resolved per request, so a
stream would hand its contents to the first getter and nothing to the rest,
making the API key vanish mid-run -- and open() on a FIFO blocks until a
writer appears (verified: it hung). Non-regular paths are now treated as
*empty* configuration without being opened, which is what
DATARETRIEVAL_CONFIG=/dev/null asks for and the only coherent answer for a
stream. That also removes the caching special case.
show_config's error handling did not cover the case its own docstring named:
`_load_file` never raises the undefined-profile error -- `_file_settings`
does -- so it repeated verbatim in all ten cells. It now probes the whole file
layer and dedups any repeated error through one closure, replacing three
helpers and a threaded parameter.
Also:
- config_path probes its memo before doing any work, and identifies the
working directory with stat(".") rather than getcwd(). Relative
DATARETRIEVAL_CONFIG: 16.6us -> 1.29us per call; absolute 0.53 -> 0.28us;
the file-backed api_key() path is unchanged at ~3.5us.
- `_coerce_typed` carried a `from_python` flag switching two things, one of
which was inert: tomllib never yields a non-int Integral, so the widened
check could not change a TOML outcome. It now takes only what varies.
- The parallel_chunks warning moved into `_scalars`, which already receives
`where`, so top-level vs profile falls out instead of being re-derived.
- Dropped two denylist markers that were substrings of an existing one, and
stopped the comment overclaiming: this catches a plausible mistake, it is
not a security control (nothing inspects values).
- exceptions.py now states the taxonomy contract it acquired: the base class
said "every failed-request error" while admitting one member that is not a
request failure.
- Deleted the `#:` block above `__all__` -- Sphinx would have attached that
prose to `__all__`, and the rationale already lives on the class.
- Moved the credential-guard tests to the module that owns the guard, and
rebuilt the resume test on the real chunker harness: it drove a
`__new__`-constructed object, so it would have passed even if resume were
broken for real calls.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
Five findings from the medium-effort review. - `_default_headers` resolved the API key before checking the host, so a malformed ~/.dataretrieval/config.toml (or a stale DATARETRIEVAL_PROFILE naming a profile the file no longer defines) failed *every* request, including legacy NWIS, WQP, and NGWMN calls that never receive the key. Resolution now happens after the host check and only for api.waterdata.usgs.gov. Water Data calls still fail loudly rather than silently going out unauthenticated; unrelated services are unaffected. - config_path's memo watched the working directory for a relative override but nothing for the default branch, which derives from $HOME -- so a process that reassigned HOME after the first resolution kept reading the previous home's file for its lifetime. Each branch now records the guard it was derived from, read back only on a memo hit, so the absolute-override case still pays nothing. - The `assert` guarding "every setting has a show_config renderer" is stripped by `python -O`, leaving the report free to print a neighbour's value under the wrong name. It raises now. - The three config-file warnings used two different stacklevels, neither of which named a user frame. Settings resolve lazily from wherever a getter first needs one, so the user frame is a different depth every time and no fixed stacklevel can name it; they are now consistent, with the reason recorded once at _WARN_STACKLEVEL. - docs: `.. configuration:` was missing its leading underscore, so it parsed as a comment rather than a hyperlink target. Also documents, rather than removes, the credential asymmetry on resume: a configure(api_key=...) value is pinned in the construction-time snapshot while API_USGS_PAT is read live, because a context snapshot cannot capture the environment. Pinning is deliberate -- a resume continues the same logical call, and the documented recovery re-issues it after the authorizing block may have exited -- but the two spellings do differ, and now say so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
`_interpret` ran `_scalars` over every `[profiles.*]` table at parse time, so
a bad value in a profile nobody selected failed every request: a config file
with a good `api_key` plus `[profiles.experimental] concurrency = 0` raised
ConfigError from `_default_headers` for all Water Data calls. The "unknown
setting" and top-level-parallel_chunks warnings fired for unconsulted tables
for the same reason.
Profile tables are now kept raw and validated in `_file_settings` only when
one is actually selected -- the same blast-radius rule `_default_headers`
follows for the key itself. The top-level table stays eager because it always
applies. Selecting a broken profile still reports it. Cost: +0.08us (2%) on a
profile-selected `api_key()`, so no extra caching.
Also wraps `_cwd_id`'s `os.stat(".")`: a relative DATARETRIEVAL_CONFIG cannot
be resolved if the working directory has been removed, and that surfaced as a
bare OSError on the request path instead of the typed error the rest of the
module raises. (Not reproducible on macOS, where stat(".") still succeeds for
a deleted cwd the process holds open, so this is contract hygiene rather than
an observed failure.)
Not applied: enabling the Windows metadata fast path in `_load_file`. This is
its third proposal, so the comment now records why it keeps being rejected --
Windows ctime is creation time, so a ctime-less stamp is identical across a
timestamp-preserving write and would serve a stale API key, which
`test_file_edit_is_picked_up` pins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JGAjg1fDK4EJY3PUi6ZHaG
Two ways of resolving the config-file path could raise something outside
DataRetrievalError, on the per-request path the module claims to protect.
`_cwd_id()` guarded `os.stat(".")`, but a *deleted* working directory
still stats fine -- the process holds the handle -- so the guard passed
and `Path.cwd()` on the next line raised a bare FileNotFoundError.
Resolution now happens in one place that reports a typed ConfigError, and
`_cwd_id`'s docstring no longer claims a guarantee it never provided.
`Path.home()` raises RuntimeError where no home resolves at all -- a
rootless container running as an arbitrary UID with no passwd entry and
no HOME. Before settings were layered those deployments worked on the
environment alone; now every Water Data request, plus progress()/retries()
on every OGC call, died with an untyped error. An unresolvable home is not
a misconfiguration to report, just an absent file, so the unexpanded
`~/...` form is used: it cannot exist, keeps the file layer inert, and
still reads correctly in show_config output.
show_config() resolved the path before entering its own guard, so both of
those escaped the one function documented never to raise. It now reports
an unresolvable path as the file row.
Also, two scope corrections:
`configure(profile=...)` was ignored when no config file existed, which
contradicted its own docstring ("a typo raises here rather than deep in a
later request") and silently dropped the settings the caller asked for. A
lingering DATARETRIEVAL_PROFILE export is still ignored -- that is ambient
state a caller may not know is set -- but a name just typed into
configure() now raises at the `with`. This narrows f05b706 rather than
reverting it.
`session` is no longer treated as a credential. It carries no secret, so
the credentials message misstated the problem, and as a *substring* it
claimed part of a namespace the server owns: any future queryable
containing it was unreachable behind that message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015UVSiGgUyDRSbqNWM5HBbL
The settings were described in four places that would drift apart: the configuration guide, a second per-variable catalogue in the architecture overview, and passing restatements in the README, the errors guide, and the chunking docstrings. The concurrency default appeared in five places counting the code constant; the retry default in four. The configuration guide is now the only place that states a setting's name, default, environment variable, and value grammar. Everything else points at it. What the other surfaces keep is what only they can say. The architecture overview keeps the invariants that constrain design -- the token is host-scoped and stripped across redirects, a semaphore rather than pool waiting is the throttle, progress failures never change results, the config module is a stdlib-only leaf -- and drops the parameter list. ADR 0006 keeps the rationale and the rejected alternatives; it never carried a catalogue. The chunking module docstring keeps the concurrency *mechanism* and drops the value grammar. config.py keeps its precedence summary, the one deliberate duplicate: a maintainer reading that module should not have to open the docs to learn the order. No information is lost. The one fact only the architecture overview stated -- that a concurrency of 1 runs sub-requests one at a time -- moved into the guide's table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015UVSiGgUyDRSbqNWM5HBbL
Collaborator
Author
|
@davetapley, this PR is AI generated but I'm happy to incorporate any high level feedback. Focus on the public interface for now. |
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.
Closes #352.
Adds
dataretrieval.config: one ordered resolution chain for every setting, so acredential can be supplied without mutating process-global
os.environ.Usage
1. A
configure()block — highest precedence, for a key from a secret store, aninteractive prompt, or concurrent callers needing different credentials:
2. Environment variable — unchanged, same variable the
R dataRetrieval package uses:
3. Config file —
~/.dataretrieval/config.toml, for keeping the key out of a shellenvironment that every child process inherits:
Profiles layer over the top-level keys per setting, so
bulk-pullinherits theapi_keyabove — you write the key once. Select one withDATARETRIEVAL_PROFILE=bulk-pull python job.pyorconfigure(profile="bulk-pull").Introspection —
show_config()names the exact source of each setting, includingwhich table in the file, and never prints the key:
A worked example — the same statewide pull under two profiles, timed. On the
PR branch (
pip install -e .), paste this into a file and run it:Measured against the live API:
Same query shape, comparable row counts, ~7× apart — the only difference is which
profile is selected.
Precedence
configure()block → environment variable → config file → built-in default, appliedper setting (an environment that sets only
API_USGS_PATleaves a file-providedconcurrencyfully in effect).