compare: hand-rolled settings vs pydantic-settings (review aid for #370) - #7
Draft
thodson-usgs wants to merge 1 commit into
Draft
compare: hand-rolled settings vs pydantic-settings (review aid for #370)#7thodson-usgs wants to merge 1 commit into
thodson-usgs wants to merge 1 commit into
Conversation
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.
Review aid, not a merge target. This PR exists so the two settings
implementations can be read side by side. Its base is DOI-USGS#353's branch
(
worktree-config-fallback-352) rather thanmain, so the diff is only thepydantic-settings refactor — 46 files — instead of the 63 files and ~7.7k added
lines you get diffing the whole feature against
main.main)The single commit here is a direct child of DOI-USGS#353's head, so every line below is
attributable to the library swap.
Where to look first
dataretrieval/settings.pydataretrieval/configuration.py(2,079 lines).dataretrieval/waterdata/settings.py+ the five adapter modulesBaseSettingssubclass. Should be near-identical apart from the decorator and the defaults.tests/settings_test.pydocs/source/architecture/decisions/0012-pydantic-settings.rstWhat actually differs in the module
Deleted —
_coerce_typed,_validated_raw, the_UNSETsentinel, thememoized
_settings_of, and the per-tier merge inside_resolve.Replaced — the four branches of
_resolvebecome fourPydanticBaseSettingsSourcesubclasses (_BlockSource,_EnvSource,_AdapterTableSource,_TopLevelSource) listed in_CHAIN, highest first.Unchanged in substance —
config_path,_load_file,_interpret,_accepted_keys,_named_profile, the file cache and its Windows ctimecaveat,
_warn_on_loose_permissions, and the whole ofshow_settings().pydantic-settings has no opinion about any of it.
New — a provenance recorder (a
ContextVareach source writes into,first-writer-wins), because pydantic-settings does not report which source
supplied a value and
show_settings()exists to answer exactly that.The four test changes
Everything else in
tests/settings_test.pyis carried over verbatim, which isthe point.
test_adapter_rejects_a_setting_it_does_not_read—TypeError→ConfigurationErrortest_api_key_is_never_adapter_scoped— sametest_a_misspelled_setting_is_not_silently_swallowed— sametest_the_validate_hook_can_refuse_a_combination—validate()→validate_settings(), because pydantic'sBaseModelownsvalidatePlus message-wording assertions that follow the rename, and
_UNSETchecks thatbecome
model_fields_setchecks.One thing to weigh
Resolution deliberately bypasses
BaseSettings.__init__, which builds fourstock sources per instantiation — two of them case-folding all of
os.environ—before
settings_customise_sourcescan discard them. That put 74% of a singleread in
_settings_init_sourcesbefore I worked around it. It is the oneplace this design sets the library's shape aside, and it is worth a reviewer's
attention: see
_resolvedandAdapterSettings.__init__, both commented atlength, and the "Resolution does not go through
BaseSettings.__init__" sectionof ADR 0012.
Net cost after the workaround is ~2× per read (26–37 µs → 68–82 µs with no
settings file), or ~0.3 ms → ~0.6 ms per query against a 100–500 ms round trip.
Verification
956 passed, 6 skipped;
mypy --strictclean over 59 files; ruff clean;import-linter 7/7; Sphinx adds no new warnings.