Make failures actionable and harden validation, APIs, and CI by thodson-usgs · Pull Request #387 · DOI-USGS/dataretrieval-python

thodson-usgs · 2026-08-20T14:49:46Z

Summary

This PR started as an agent-documentation refresh and an error-message cleanup. Exercising each proposed remedy exposed several cases where following the message caused a second failure or, worse, returned a plausible but wrong result. The scope now covers the resulting validation, correctness, API, geographic-code, test, and CI changes as one caller-correctness pass.

Actionable argument validation

dataretrieval._validation now owns the recurring argument-check shapes and their message vocabulary:

Helper	Contract
`require_one_of`	one scalar from a closed vocabulary
`require_argument`	one required argument, returned with its type narrowed
`require_together`	an all-or-none argument group
`require_any_of`	at least one filter from a permissive group
`require_exactly_one`	one sufficient alternative, returned as `(name, value)`
`reject_together`	mutually exclusive but individually optional arguments

The helpers are adopted across NLDI, NWDC, NWIS, WQP, OGC dates, Water Data CQL/reference/nearest, and state normalization. Messages identify the caller's parameter spelling, state the problem, and name a correction the caller can execute.

The ratings file_type validator remains local deliberately: it validates a collection and reports every invalid member, while require_one_of validates one scalar. Their shared formatting is centralized through render_options; adding a second collection-helper interface for one caller would be speculative.

Compatibility note

Malformed argument values now consistently raise ValueError. The deprecated nwis.query_waterdata, query_waterservices, and get_record paths therefore no longer use their historic TypeError for missing filters, incomplete bounding boxes, or unknown services. TypeError remains for genuinely mistyped arguments such as a non-string sites value.

Correctness fixes found by executing the remedies

NLDI validates origin conflicts in one pass and requires data_source before constructing a navigation URL. A missing source can no longer become a literal /None path whose 200 response looks like a genuine empty result.
waterdata.get_nearest_continuous adds time and monitoring_location_id to an explicit properties list because both are required to select one row per target and monitoring location. This is a documented returned-column-shape change.
nwdc empty selectors and error envelopes now produce actionable text rather than advice that repeats the same failure or prints a raw validation-object list.
Shared metadata and state helpers no longer recommend parameters or replacement getters that their caller does not accept.
NWIS's invalid query_waterdata("ratings", ...) route now points to waterdata.get_ratings, including the dict return shape and feature-ID keys callers need to use it.
Date formatting names the public argument, rejects overlong intervals rather than truncating them, and does not advertise durations on the ratings path that refuses them.
Pagination, credential-install, RDB, and adapter-specific failures now distinguish retryable recovery from deterministic corrections.

nldi._query_nldi still turns a 200 non-JSON body into an empty GeoDataFrame. That is an existing documented exception, not changed here, and is called out in AGENTS.md so it is not mistaken for the general error policy.

Additional API and data behavior

waterdata.get_cql(..., max_rows=N) exposes the OGC engine's existing total-row cap. limit remains page size; a small limit alone can increase request count rather than cap results.
State normalization now includes American Samoa, Guam, the Northern Mariana Islands, Puerto Rico, and the US Virgin Islands under their ANSI/FIPS codes. Name, postal, FIPS, and US:<FIPS> forms normalize alike for adapters whose upstream service supports them.
The Water Data metadata collection vocabulary includes countries, and its hand-maintained reference docstring is contract-tested against that vocabulary.
NEWS.md records the public behavior and exception changes.

Coverage and CI

Coverage is now a branch-coverage ratchet rather than an informational number. The branch adds behavior-focused tests for dispatch, malformed response envelopes, NLDI navigation, nearest-row grouping, configuration fallbacks, date handling, pagination, and previously uncovered public getters.

The ratchet runs in the existing OS/Python test matrix, blocks on every non-Windows leg, and reports informationally on Windows where POSIX-only skips measure a smaller suite. Version-conditional and environment-unreachable import arms are excluded explicitly so Python 3.10, 3.13, and 3.14 enforce the same measurement. Complexity and dependency-direction jobs remain fast and independent.

Agent and contributor documentation

AGENTS.md now starts from CONTEXT.md, explains placement rules rather than carrying a stale file inventory, names .importlinter as the authoritative dependency map, documents the current test/tooling gates, and records the executable-remedy policy.
CONTEXT.md corrects the definition of a setting: a public keyword such as ssl_check is not automatically part of configuration resolution.
CONTRIBUTING.md documents branch coverage, platform-dependent local measurements, complexity ratchets, import-contract ownership, and the periodic structural-health sweep.

Validation

Focused validation/ratings tests: 45 passed
Full offline suite: 1114 passed, 12 deselected
Branch coverage: 98.97%, ratchet met
ruff check . and ruff format --check .
mypy in strict mode
lint-imports: 8 contracts kept
xenon and complexipy
Sphinx HTML build succeeds (5 pre-existing warnings)

Four message defects found reviewing DOI-USGS#387, all of the shape the new AGENTS.md rule names: following the remedy literally must produce a working call. nldi._validate_feature_source_comid checked the feature pair before the origin conflict, so `get_features(comid=13294314, feature_id='X')` was told "Pass both, e.g. feature_source='WQP'" -- and the corrected call then raised "Provide exactly one of comid or feature_source". Two round trips for one mistake, in both directions of the pair. A comid now rejects either half up front, and both checks offer the same two ways forward from one shared hint. nwdc._resolve_locations closed its empty-value complaint with "(exactly one of state, county, or huc must be given)" -- but `state=[]` did give exactly one, so the parenthetical sent the caller to change a different selector than the one at fault. _nwdc_error_detail is annotated `str | None` and returned `body["detail"]` unguarded; a validation envelope spells that as a list of error objects, which would have been interpolated verbatim into the message. It also appended a period to a detail that already ended in one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Point agents at the shared vocabulary and authoritative dependency map, replace stale file inventories with placement rules, document the current quality gates, and correct the definition of a configuration setting.

Centralize the recurring missing, grouped, alternative, and conflicting argument checks. Adopt them across adapters, make every rejection name an executable remedy, prevent NLDI and nearest-observation corrections from producing silent wrong answers, and keep the associated NLDI tests offline.

Expose the OGC engine's existing max_rows limit through get_cql so callers can cap total results rather than accidentally turning a small page size into hundreds of requests.

Enable branch coverage, close behaviorally meaningful gaps, exclude only generated or environment-unreachable paths, and set the threshold at the measured value so future regressions fail without encouraging hollow tests.

Correct stale service and collection documentation, make the date-formatting hint parameter describe rather than imply enforcement, and document how to interpret the coverage threshold when local platform skips differ from CI.

Move the coverage gate into the existing test matrix, keep Windows informational where POSIX-only tests skip, and exclude version-conditional branches so every supported Linux interpreter measures the same ratchet.

Add the permissive any-of shape and the closed-vocabulary remedy extension needed by legacy NWIS and CQL callers, then replace the remaining hand-written recurring checks without changing their caller-facing intent.

Record the public behavior changes in NEWS and remove comments that merely restated measured values or nearby code while retaining rationale and constraints.

Add the five inhabited US territories to state normalization, allow Water Data and NGWMN queries the upstream services already support, and make endpoint-specific remedies use arguments their public getter accepts.

Consolidate shared message rendering and context handling, finish adoption in NLDI, NWIS, states, and ratings, return validated selections instead of re-deriving them, and standardize malformed argument values on ValueError while improving the NWIS ratings replacement guidance.

ehinman · 2026-08-24T18:06:00Z

+      - name: Coverage ratchet
+        # Windows skips POSIX-only tests, so its number measures a smaller
+        # suite. Negative condition: a matrix edit cannot silently unenforce it.
+        if: runner.os != 'Windows'


Aren't these tests run with Linux on GitHub?

The test matrix is defined a few lines earlier https://github.com/thodson-usgs/dataretrieval-python/blob/5d7ff503220332438cb27b698725926f8c794c3e/.github/workflows/python-package.yml#L128-L135 Which the diff won't show.

Duhhh got it, got it.

ehinman

Interesting work with the error message building. Happy to see additional states and territories added.

ehinman · 2026-08-24T18:08:36Z

+    and only those. They are the endpoint's own state
+    parameters *as the caller spells them*: the mutual-exclusion guard below is
+    proof the getter accepts them as keyword arguments. ``into`` is deliberately
+    not offered, because it is a wire queryable that need not exist on the


wire queryable?

ehinman · 2026-08-24T18:32:32Z

+        offered = dict.fromkeys(n for n in (into, *reject) if n in reject)
+        raise ValueError(
+            f"{err} Pass {' or '.join(offered)} instead -- they take the "
+            f"values the API itself uses, unnormalized."


I don't think this error would make sense to a user, since they wouldn't know what is normalized vs not normalized.

ehinman · 2026-08-24T18:49:38Z

+        filter matching more rows than ``limit`` still returns every matching
+        row across multiple pages, so a small ``limit`` makes *more* requests,
+        not fewer. Use ``max_rows`` to cap the total instead.
+    max_rows : int, optional


Why is this an input? Trying to think of when someone would want to use this...

ehinman · 2026-08-24T21:42:06Z

+    ``monitoring_location_id`` is what it groups by, so a ``properties`` list
+    omitting either silently collapses every site into one row per target
+    rather than failing. Added rather than rejected: the caller asked for
+    columns, not for a lecture about which ones this getter needs.


This is hilarious.

ehinman · 2026-08-24T22:50:34Z

        # ever returns one rather than leaking a bare pandas exception.
        raise DataRetrievalError(
-            f"NWDC returned an empty response body (URL: {response.url})."
+            f"NWDC returned an empty response body (URL: {response.url}). "


Did this happen and that's why there's a guard against it?

ehinman · 2026-08-24T22:54:27Z

 ]
+# The major filters each query function accepts, hoisted beside the service
+# lists so the checks and their remedies read from one roster.
+_WATERDATA_MAJOR_FILTERS = ("site_no", "stateCd")


Why is this named WATERDATA? It's confusing since there's a whole module for water data.

ehinman · 2026-08-24T22:55:19Z

+    require_together(
+        {name: kwargs.get(name) for name in _WATERDATA_BBOX_CORNERS},
+        context="to describe a bounding box",
+        remedy=(


Seems like all these remedies could be in a table somewhere rather than hidden in the functions?

ehinman · 2026-08-25T15:14:09Z

+      - name: Coverage ratchet
+        # Windows skips POSIX-only tests, so its number measures a smaller
+        # suite. Negative condition: a matrix edit cannot silently unenforce it.
+        if: runner.os != 'Windows'


Duhhh got it, got it.

ehinman · 2026-08-25T15:43:26Z

@@ -1,3 +1,7 @@
+**08/20/2026:** The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. **Behavior change:** a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast.
+
+**08/20/2026:** Argument checks now share one vocabulary, and every rejection names a move the caller can execute. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their *code service* was invalid. Each check takes the caller's own spelling of the parameter and a remedy for the move it cannot derive, and every check raises `ValueError` -- one class for a bad argument value. **Behavior change:** the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. **Behavior change:** the deprecated `nwis` query entry points (`query_waterdata`, `query_waterservices`, `get_record`) now answer a missing major filter, an incomplete bounding box, or an unknown service with `ValueError` rather than their historic `TypeError` -- `TypeError` remains for a genuinely mistyped argument, such as a non-string `sites`. Code catching `TypeError` there, or matching on the old strings, must update. **Bug fix:** a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. **Bug fix:** four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. **Bug fix:** `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. **Behavior change:** `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. **Behavior change:** `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns.


Suggest using term "user" rather than "caller". This is more relevant to people reading the news.

ehinman · 2026-08-25T15:49:51Z

Also, it looks like ogc/retry.py and ogc/interruptions.py aren't needed anymore, since they're just passing through the dataretrieval-level scripts.

thodson-usgs · 2026-08-25T19:15:30Z

@ehinman Addressed separately in #391: dataretrieval.ogc.retry is removed because it exposed only private classifiers, while the published dataretrieval.ogc.interruptions path is deprecated for one year before removal so existing imports keep working.

thodson-usgs changed the title ~~docs: refresh AGENTS.md and correct the setting definition~~ Make errors actionable for programmatic callers, and refresh AGENTS.md Aug 20, 2026

thodson-usgs force-pushed the docs/refresh-agents-context branch from 595a8ef to b0fd59d Compare August 20, 2026 18:36

thodson-usgs added 10 commits August 21, 2026 09:13

docs: refresh contributor and agent guidance

19321f9

Point agents at the shared vocabulary and authoritative dependency map, replace stale file inventories with placement rules, document the current quality gates, and correct the definition of a configuration setting.

feat(cql): add a total-row cap to get_cql

410ef02

Expose the OGC engine's existing max_rows limit through get_cql so callers can cap total results rather than accidentally turning a small page size into hundreds of requests.

test: enforce a branch-coverage ratchet

77fd19d

Enable branch coverage, close behaviorally meaningful gaps, exclude only generated or environment-unreachable paths, and set the threshold at the measured value so future regressions fail without encouraging hollow tests.

fix: align branch documentation with behavior

a118086

Correct stale service and collection documentation, make the date-formatting hint parameter describe rather than imply enforcement, and document how to interpret the coverage threshold when local platform skips differ from CI.

ci: enforce coverage across supported interpreters

4e8dbb5

Move the coverage gate into the existing test matrix, keep Windows informational where POSIX-only tests skip, and exclude version-conditional branches so every supported Linux interpreter measures the same ratchet.

refactor(validation): complete shared check adoption

e01ed4d

Add the permissive any-of shape and the closed-vocabulary remedy extension needed by legacy NWIS and CQL callers, then replace the remaining hand-written recurring checks without changing their caller-facing intent.

docs: record behavior changes and remove stale commentary

a839f9a

Record the public behavior changes in NEWS and remove comments that merely restated measured values or nearby code while retaining rationale and constraints.

feat(states): support US territories

60a0c6d

Add the five inhabited US territories to state normalization, allow Water Data and NGWMN queries the upstream services already support, and make endpoint-specific remedies use arguments their public getter accepts.

thodson-usgs changed the title ~~Make errors actionable for programmatic callers, and refresh AGENTS.md~~ Make failures actionable and harden validation, APIs, and CI Aug 21, 2026

thodson-usgs force-pushed the docs/refresh-agents-context branch from a939ca4 to 5d7ff50 Compare August 21, 2026 14:23

thodson-usgs requested a review from ehinman August 21, 2026 14:46

thodson-usgs marked this pull request as ready for review August 21, 2026 14:46

ehinman reviewed Aug 24, 2026

View reviewed changes

ehinman reviewed Aug 25, 2026

View reviewed changes

thodson-usgs mentioned this pull request Aug 25, 2026

Remove OGC retry and deprecate OGC interruptions #391

Open

Address validation review feedback

ff9294c

		@@ -1,3 +1,7 @@
		08/20/2026: The `state` filter now accepts the five US territories. `dataretrieval.codes.states` held the 50 states and DC, so `ngwmn.get_sites(state='Puerto Rico')`, `waterdata.get_monitoring_locations(state_name='Puerto Rico')` via the unified `state`, and `nwdc.get_wateruse(state='PR')` were refused locally -- while all three services carry the data (NGWMN answers with 36 Puerto Rico monitoring locations, the Water Data monitoring-locations collection returns Puerto Rico sites, and legacy NWIS lists 1,148 stream sites for `stateCd=PR`). American Samoa, Guam, the Northern Mariana Islands, Puerto Rico and the US Virgin Islands are now in both code tables under their real ANSI/FIPS codes, so every encoding resolves: `'Puerto Rico'`, `'PR'`, `'72'` and `'US:72'` all normalize alike. Behavior change: a territory that used to raise `ValueError` now produces a request. A value the table genuinely does not hold still fails fast.

		08/20/2026: Argument checks now share one vocabulary, and every rejection names a move the caller can execute. `dataretrieval._validation` owns the message shape for each check that recurs across the adapters -- a value outside a closed vocabulary (`require_one_of`), a missing argument (`require_argument`, `require_together`), a query with no filter at all (`require_any_of`), and arguments that conflict (`require_exactly_one`, `reject_together`) -- so a new check cannot invent its own phrasing, which is how `get_reference_table` came to tell callers who passed a bad `collection` that their code service was invalid. Each check takes the caller's own spelling of the parameter and a remedy for the move it cannot derive, and every check raises `ValueError` -- one class for a bad argument value. Behavior change: the text of those rejections moves with them -- `"Unrecognized service: 'x'. get_record serves …"` is now `"Invalid service: 'x'. Valid options are: …"`, and the major-filter and bounding-box complaints in `query_waterdata` / `query_waterservices` are rendered in the shared form. Behavior change: the deprecated `nwis` query entry points (`query_waterdata`, `query_waterservices`, `get_record`) now answer a missing major filter, an incomplete bounding box, or an unknown service with `ValueError` rather than their historic `TypeError` -- `TypeError` remains for a genuinely mistyped argument, such as a non-string `sites`. Code catching `TypeError` there, or matching on the old strings, must update. Bug fix: a `None` passed as a major filter (`query_waterservices(service='dv', sites=None)`) counted as a filter and reached the service as an empty `sites=`; `None` now means not supplied, and the call is refused with the filters that would have served. Bug fix: four messages told callers to do something that raised again or named a parameter their getter does not accept -- `nldi.get_features(comid=…, feature_id=…)` said to supply the missing half of the feature pair, and the corrected call then failed on the conflict with `comid`; `nwdc` answered `state=[]` by saying exactly one of `state`, `county` or `huc` must be given, when exactly one was; `codes.states.apply_state` offered NGWMN callers a `state_name` / `state_code` parameter no NGWMN getter accepts; and `BaseMetadata` pointed NGWMN and NWDC callers at a Water Data getter. Bug fix: `nldi.get_features(navigation_mode=…)` without a `data_source` spelled `None` into the URL path and returned an empty frame from a 200; it now raises and names the source to pass. Behavior change: `waterdata.get_nearest_continuous` appends `time` and `monitoring_location_id` to an explicit `properties` list rather than honoring it verbatim -- omitting either silently collapsed every monitoring location into one row per target, a wrong answer rather than an error -- so a caller who passed `properties=['time', 'value']` now gets a third column. Behavior change: `nwis.query_waterdata` serves `'peaks'` only; the `'ratings'` URL it used to build was never an NwisWeb program and answered with an HTML error page. `nwis.get_record(service='ratings')` is unaffected -- it routes to `get_ratings`, which is served from a different endpoint. New: `waterdata.get_cql(..., max_rows=N)` caps the total rows a CQL query returns.

Uh oh!

Conversation

thodson-usgs commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Actionable argument validation

Compatibility note

Correctness fixes found by executing the remedies

Additional API and data behavior

Coverage and CI

Agent and contributor documentation

Validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thodson-usgs Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehinman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ehinman commented Aug 25, 2026

Uh oh!

thodson-usgs commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thodson-usgs commented Aug 20, 2026 •

edited

Loading

thodson-usgs Aug 25, 2026 •

edited

Loading