feat(waterdata): request v1 of the Water Data API, pinnable through api_version - #422
Conversation
Taken so this branch carries no edit that conflicts with DOI-USGS#422. The `data_gap_interval` fix this branch made to the v0 column list is subsumed by DOI-USGS#422's rewrite of that list for v1, so the resolution keeps DOI-USGS#422's version -- including `statistics_begin` -- and this branch keeps only the monitor. `dataretrieval/waterdata/metadata.py` is byte-identical to DOI-USGS#422's copy after this merge, which is the check worth making: resolving a column list the wrong way would silently revert part of the other PR while every offline test still passed.
1e2e1c8 to
574f448
Compare
Comparing each collection's /schema with its getter's signature found 20 returned columns that were reachable only through **queryables, so the getter documented neither the column nor the filter: - get_field_measurements: control_condition, day, field_measurements_series_id, measurement_rated, month, reading_type, time_of_day, year - get_peaks: qualifier, time_of_day, value - get_monitoring_locations: revision_created, revision_modified, revision_note - get_combined_metadata: data_gap_interval, reading_type - get_time_series_metadata: data_gap_interval, parameter_description - get_field_measurements_metadata: reading_type - get_channel: channel_location_direction Each is now a named parameter, described in the service's own words. day, month and year take the integer annotation get_peaks already uses. The monitoring-location attributes every collection accepts as filters but does not return stay in **queryables. Existing calls send the same request as before. Stacked: this commit also carries DOI-USGS#422 (Water Data API v1), DOI-USGS#423 (continuous method_category and the API-version monitor) and DOI-USGS#424 (the documented-columns monitor), which merge first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
get_daily, get_continuous and get_time_series_metadata list their returned columns in the properties docstring. The list is hand-written and went stale twice without anything noticing: continuous was missing method_category (DOI-USGS#423) and time-series-metadata data_gap_interval (DOI-USGS#422). Add a live test that compares each list with the collection's /schema and names the docstring to edit when they differ. id is excluded on both sides because only time-series-metadata lists it in its schema, though all three accept it. Stacked: this commit also carries DOI-USGS#422 (Water Data API v1) and DOI-USGS#423 (continuous method_category and the API-version monitor), which merge first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Comparing each collection's /schema with its getter's signature found 20 returned columns that could be passed only through **queryables, so the getter documented neither the column nor the filter: - get_field_measurements: control_condition, day, field_measurements_series_id, measurement_rated, month, reading_type, time_of_day, year - get_peaks: qualifier, time_of_day, value - get_monitoring_locations: revision_created, revision_modified, revision_note - get_combined_metadata: data_gap_interval, reading_type - get_time_series_metadata: data_gap_interval, parameter_description - get_field_measurements_metadata: reading_type - get_channel: channel_location_direction Each is now a named parameter, described in the service's own words. day, month and year are typed as integers, as get_peaks already types them. The monitoring-location attributes that every collection accepts as filters but does not return stay in **queryables. Existing calls send the same request as before. Stacked on DOI-USGS#422, DOI-USGS#423 and DOI-USGS#424, which merge first; review this commit alone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
574f448 to
649a208
Compare
Comparing each collection's /schema with its getter's signature found 20 returned columns that could be passed only through **queryables, so their getters documented neither the column nor the filter: - get_field_measurements: control_condition, day, field_measurements_series_id, measurement_rated, month, reading_type, time_of_day, year - get_peaks: qualifier, time_of_day, value - get_monitoring_locations: revision_created, revision_modified, revision_note - get_combined_metadata: data_gap_interval, reading_type - get_time_series_metadata: data_gap_interval, parameter_description - get_field_measurements_metadata: reading_type - get_channel: channel_location_direction Each is now a named parameter, documented from the description the service publishes for it. day, month and year are typed as integers, as get_peaks already types them. The monitoring-location attributes that every collection accepts as filters but does not return stay in **queryables. Existing calls send the same request as before. A parametrized test checks that each one is sent in the request. It also covers DOI-USGS#422's statistics_begin, which no other test sends. The properties lists of get_monitoring_locations and get_channel now name the columns they were missing (the three revision_* columns and channel_location_direction), and get_monitoring_locations joins the documented-columns monitor. get_channel stays out of it, because its list names the output column channel_measurements_id where the schema has id. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
649a208 to
a4bbcf2
Compare
a4bbcf2 to
58ff493
Compare
ehinman
left a comment
ehinman
left a comment
There was a problem hiding this comment.
The diffs look okay. I tried running this fetch:
test,_ = waterdata.get_combined_metadata(state_name = "Iowa")
And I got a warning:
dataretrieval-python/dataretrieval/ogc/shaping.py:292: UserWarning: Could not infer format, so each element will be parsed individually, falling back to `dateutil`. To ensure parsing is consistent and as-expected, please specify a format.
df[col] = pd.to_datetime(df[col], errors="coerce")
What am I doing wrong? Do I have a package version that is causing this warning?
| only: the file and the environment refuse it. The API key is scoped to the host | ||
| that accepts it, so a redirected call sends no key. | ||
| ``/ogcapi/<api_version>``, ``/samples-data``, ``/statistics/v0`` and | ||
| ``/stac/v0``. Code only: the file and the environment refuse it. The API key is |
There was a problem hiding this comment.
What does "the file and the environment refuse it" mean?
There was a problem hiding this comment.
Can we instead say something like "the file and the environment fail loudly if the user tries to specify the version there rather than in the configuration"?
There was a problem hiding this comment.
suggest something more literal like, fail and raise a warning. Follow convention used elsewhere in the package.
There was a problem hiding this comment.
Agreed, "refuse" didn't say what happens. It means a ConfigurationError is raised. That's an error, not a warning, which matches how the rest of configuration handles a setting in the wrong place (e.g. a known key in the wrong table). Fixed in 7cba029: the base_url docstring now says "Code only: setting it in the configuration file or through an environment variable raises ConfigurationError." Same wording in the ADR 0011 note and the code comments.
| _V0_ONLY_FILTERS: dict[str, str] = { | ||
| "begin_utc": "'begin'", | ||
| "end_utc": "'end'", | ||
| "state_name": "get_combined_metadata(state=...)", |
There was a problem hiding this comment.
I'm confused by this. Isn't the input for get_combined_metadata still "state_name"?
| "state_name": "get_combined_metadata(state=...)", | |
| "state_name": "get_combined_metadata(state_name=...)", |
There was a problem hiding this comment.
Good catch. get_combined_metadata takes both state and state_name, so the remedy shouldn't switch a state_name caller over to state. Rather than hard-code either one, 7cba029 repeats whichever the caller passed: state_name= → get_combined_metadata(state_name=...), state= → get_combined_metadata(state=...). A new test (test_time_series_metadata_state_warning_points_to_the_same_argument) covers both, and I confirmed both against the live service.
| requested = set(args.get("properties", ())) | ||
| legacy = [n for n in _V0_ONLY_FILTERS if n in args or n in requested] | ||
| for name in legacy: | ||
| spelled = "state" if name == "state_name" and state_given else name |
There was a problem hiding this comment.
Can we push users to still use "state_name" or "state_code", rather than "state"? I get that it's a convenience function, but I'd rather encourage users to actually use the API's documentation...
There was a problem hiding this comment.
I'll follow up with this point. I elected to sanitize the APIs a bit, since they all follow slightly different conventions, which gets ugly when you're interacting with several APIs at once.
There was a problem hiding this comment.
Following up on this. I'd like to keep state as the recommended argument, but I agree it should be easier to map to the API docs.
Why keep it: the native state parameter is different in each collection. The location collections take a full name (state_name), the statistics getters take state_code in US:XX form, and NGWMN providers take a postal code (state). state accepts a name, postal code, or FIPS code and sends each collection the form it expects. If someone is working across several of these APIs at once, that's one spelling to learn instead of three.
state_name and state_code aren't deprecated, and they won't be. They send the value to the API unchanged, which covers values the conversion doesn't handle, such as non-US FIPS codes. Passing state together with one of them raises a ValueError.
Your point stands, though: state doesn't appear in the API documentation, so a reader there has no way to match it up. 7a48e10 makes every state docstring say it's a dataretrieval argument and name the API field it's sent as, e.g. "A dataretrieval argument rather than an API field: it is sent as the API's state_name." Anyone who starts from the API docs can now find the corresponding argument.
| state_name : string or iterable of strings, optional | ||
| The name of the state or state equivalent in which the monitoring location | ||
| is located. | ||
| Deprecated; see ``state``. The name of the state or state equivalent in |
There was a problem hiding this comment.
Again, not seeing "state" anywhere in API, but maybe I'm missing something. Or, is this artificially created in this PR?
There was a problem hiding this comment.
state isn't from this PR and isn't an API field. It's a dataretrieval argument added with the shared OGC engine (#324). It accepts a name, postal code, or FIPS code and is sent as state_name (or state_code, depending on the collection). This PR only marks it deprecated on get_time_series_metadata, because v1 dropped state_name there. The docstring now says this outright (7cba029): "state is a dataretrieval argument rather than an API field … and is sent as state_name." The bigger question of whether to steer people toward state or the native names is in the thread above.
| ADAPTER_ONLY_SETTINGS: tuple[str, ...] = ("base_url",) | ||
| ADAPTER_ONLY_SETTINGS: tuple[str, ...] = ("base_url", "api_version") | ||
|
|
||
| #: The adapter-only settings the file also refuses, so only a ``configure()`` |
There was a problem hiding this comment.
Again, this language does not make sense to me. What is meant by "the file also refuses"? Can you clarify this language a bit to be useful to both a human and an agent?
There was a problem hiding this comment.
Reworded in 7cba029 to say what happens: "Adapter-only settings that only a configure() block can set: writing one in the configuration file raises ConfigurationError (ADR 0011)." The neighboring _REFUSED_ENV_VARS comment got the same treatment.
| def test_api_version_is_refused_from_the_environment(monkeypatch): | ||
| """Refused, as every adapter-only setting is. The message also points to | ||
| the file, which accepts it.""" | ||
| monkeypatch.setenv("API_USGS_API_VERSION", "v0") |
There was a problem hiding this comment.
Do we actually have instructions anywhere that would prompt a user to do this? "API_USGS_API_VERSION" doesn't even make sense as a variable name. This check might be a little overkill, and the documentation about it being refused in the environment is confusing. I appreciate the defensive programming, but I do not think it's necessary.
There was a problem hiding this comment.
I think the idea was the API should be configurable, and I suspect that the documented method is to use a configuration file. Environment variables are more of a fallback, but fine for a test.
There was a problem hiding this comment.
Nothing tells a user to set it, and you're right that documenting the refusal in user-facing text was more confusing than helpful. 7cba029 removes it there: the api_version docstring and the user guide now just say it can be set in code or in the [waterdata] table of the config file, and that it has no environment variable. The check itself adds no api_version-specific code. It comes from one rule that already guarded API_USGS_BASE_URL: no adapter-only setting can come from an environment variable, since a variable applies to every adapter. The test is there to show the rule also covers the new setting. If someone guesses the variable name, they get an error that tells them where to set it instead of having the value silently ignored. So I've kept the test, and the guarantee now only shows up in the internals.
| def test_key_excluded_for_lookalike_host(self): | ||
| """The key is not sent to a typosquatting/lookalike domain.""" | ||
| url = "https://api.waterdata.usgs.gov.evil.com/ogcapi/v0/daily/items" | ||
| url = "https://api.waterdata.usgs.gov.evil.com/ogcapi/v1/daily/items" |
There was a problem hiding this comment.
How'd I miss the "evil" test, lol.
There was a problem hiding this comment.
Ha. It checks that the API key isn't sent to a lookalike host (api.waterdata.usgs.gov.evil.com). The key is only attached when the host matches exactly, not by prefix. This PR just updated its path from /v0 to /v1.
Review on DOI-USGS#422: "the file and the environment refuse it" did not say what happens. Each mention now names the error. The user-facing api_version docs drop the environment entirely, since nothing directs a caller to set one. get_time_series_metadata's state deprecation now names the argument the caller passed (state or state_name) in its get_combined_metadata remedy, since that getter accepts both.
|
@ehinman re the The column causing it is
|
The service records construction_date at day, month, or year precision (19950812, 199508, 2005). Parsing it as a datetime raised pandas' "Could not infer format" UserWarning and turned every month-precision value into NaT -- 1,716 of 27,307 Iowa rows. No one parse keeps all three, and the schema types it as a string, so it is no longer a time column. The R package also leaves it as character. Reported in review on DOI-USGS#422.
|
Follow-up on the |
Review on DOI-USGS#422: state does not appear in the API documentation, so a reader cannot map it to what they find there. Each state docstring now says it is a dataretrieval argument and names the field it is sent as: state_name (monitoring locations, combined metadata, NGWMN sites), state_code in US:XX form (statistics), or state as a postal code (NGWMN providers).
state and now county are package-named arguments for domain concepts that ADR 0013 otherwise leaves to each service's spelling. Review of DOI-USGS#422 read state as hiding the APIs' own parameters, and nothing recorded why it did not contradict the rule. ADR 0013 gains a clause stating the four conditions both meet: the native parameters stay, the conversion is an exact table kept against the service's reference collection, combining the two raises, and the docstring names what the argument is sent as. CONTEXT.md defines State and County as domain terms with each service's spelling, and Unified argument as a core term.
|
We got asked about the switch to v1 in drpy at the public NWIS decommission meetings. People are excited for this! |
Water Data OGC requests now go to /ogcapi/v1, released September 2026; v0 stays online until June 2027. Statistics and STAC have no v1. WaterdataConfiguration gains api_version, which a configure() block or the [waterdata] file table can set and the environment refuses. The file now refuses only BLOCK_ONLY_SETTINGS (base_url): a base URL can redirect requests to another host, and a version cannot. get_time_series_metadata sends a call that names a field v1 dropped (begin_utc, end_utc, state_name, hydrologic_unit_code) to v0 with a DeprecationWarning, without changing the caller's configured version, and gains statistics_begin. Behavior changes: its begin and end are UTC with a time zone and those four columns are gone; get_field_measurements returns time as a date, with the time of day in time_of_day. Closes DOI-USGS#421. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review on DOI-USGS#422: "the file and the environment refuse it" did not say what happens. Each mention now names the error. The user-facing api_version docs drop the environment entirely, since nothing directs a caller to set one. get_time_series_metadata's state deprecation now names the argument the caller passed (state or state_name) in its get_combined_metadata remedy, since that getter accepts both.
The service records construction_date at day, month, or year precision (19950812, 199508, 2005). Parsing it as a datetime raised pandas' "Could not infer format" UserWarning and turned every month-precision value into NaT -- 1,716 of 27,307 Iowa rows. No one parse keeps all three, and the schema types it as a string, so it is no longer a time column. The R package also leaves it as character. Reported in review on DOI-USGS#422.
Review on DOI-USGS#422: state does not appear in the API documentation, so a reader cannot map it to what they find there. Each state docstring now says it is a dataretrieval argument and names the field it is sent as: state_name (monitoring locations, combined metadata, NGWMN sites), state_code in US:XX form (statistics), or state as a postal code (NGWMN providers).
The field-measurements fixture still carried v0's full datetimes in time, so nothing tested the documented v1 behavior: time is a date, parsed to a tz-naive midnight, with the time of day in time_of_day. The fixture now matches what v1 sends (checked live), and a test pins the parse. Also: the begin_utc/end_utc deprecation remedies now read begin=... and end=..., in the same call form as their get_combined_metadata siblings, and ogc_api_url takes api_version keyword-only.
0e69af6 to
955db3d
Compare
2 participants
TL;DR: The Water Data OGC getters now request v1 of the API, released September 2026; v0 stays online until June 2027, and a new
api_versionsetting can pin v0.get_time_series_metadatasends calls that use a field v1 dropped to v0 with aDeprecationWarning. The maintainer should confirm four choices and decide one open question (field-measurement row order).Closes #421. Merge order is in #426.
Changes
/ogcapi/v1(release post), set byOGC_API_VERSIONinwaterdata/endpoints.py. Statistics and STAC have no v1 and stay onv0.WaterdataConfigurationgainsapi_version, a setting likebase_url(ADR 0010) because it describes the deployment, not the query. Aconfigure()block or the file's[waterdata]table can set it; the environment refuses it. The file now refuses onlyBLOCK_ONLY_SETTINGS(base_url): a base URL can redirect requests to another host, and a version cannot.get_time_series_metadata: v1 responds with 400 or 500 tobegin_utc,end_utc,state_name(fromstate), andhydrologic_unit_code. A call that names one, as a filter or inproperties, goes to v0 with aDeprecationWarning. The version is passed per request (get_ogc_data(api_version=...)) rather than throughconfigure(), so the caller's configured version is unchanged. The getter gainsstatistics_begin, new in v1.get_time_series_metadatareturnsbeginandendin UTC with a time zone, without the four dropped columns.get_field_measurementsreturnstimeas a date (tz-naive midnight, as inget_daily) and the time of day intime_of_day.time-series-metadatafixture are regenerated from v1; the snapshot addsmethod_categoryoncontinuous, which fix(waterdata): accept the continuous method_category queryable #423 makes aget_continuousparameter.For the maintainer
2027-06-01is when the service retires v0, not the NEWS date plus one year, because the v0 routing stops working then.api_version. To make it code-only likebase_url, addapi_versiontoBLOCK_ONLY_SETTINGSand update theapi_versiontests intests/configuration_test.py._accept_legacy_kwargs, the ADR 0012 default for a renamed argument: translatingbegin_utcinsidepropertieswould rename the returned column without an error.get_field_measurementsrow order within a day. Rows sort bytime, thenmonitoring_location_id; withtimea date, same-day rows at one location keep the service's order. Sorting bytime, thentime_of_dayrestores the v0 order, but the same key would reorderget_peaks, so a fix would be specific to field measurements.Verification
ruff check,ruff format --check,mypy --strict(61 files),lint-imports(8 contracts),xenon,complexipy, and pre-commit pass; a Sphinx build without notebook execution adds no warnings.get_time_series_metadatacall raises nothing under-W error::DeprecationWarning; each dropped field warns and returns v0 rows; a caller-pinnedv1still applies after a v0-routed call; the file, top-level, and environment sources behave as documented; the queryables monitor passes.mainagainst the feat(waterdata): request v1 of the Water Data API, pinnable through api_version #422–feat(waterdata): name every column the OGC collections return #425 stack): atapi_version="v0"every frame matchesmain; at v1 the only differences are the behavior changes above and item 4.🤖 Generated with Claude Code