From 1dd136f4de4e49d349ba409c8c9d635504cbf036 Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Fri, 17 Jul 2026 06:03:15 +0900 Subject: [PATCH 1/3] fix(nwis): handle empty peaks response instead of raising KeyError nwis.get_discharge_peaks / get_record(service="peaks") against a site with no annual-peak data returns a peaks RDB body of comment lines only, which read_rdb parses to a column-less empty DataFrame. format_response runs preformat_peaks_response before its own empty-frame check, and that function's first statement pops "peak_dt", so the empty case raised KeyError('peak_dt') instead of returning an empty frame. This is the same empty-result contract fixed for the other services in issue #171; peaks was missed because it is preformatted first. Return the frame unchanged when peak_dt is absent so the empty-frame path in format_response handles it and callers can check df.empty. Adds a regression test alongside the existing #171 coverage. Signed-off-by: Arpit Jain --- dataretrieval/nwis.py | 7 +++++++ tests/nwis_test.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 25e9cf8f4..255ebbe85 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -196,6 +196,13 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: The formatted data frame """ + if "peak_dt" not in df.columns: + # An empty peaks response (e.g. "No sites found") parses to a + # column-less frame, so there is no peak_dt to reformat. Return it + # unchanged and let format_response's empty-frame path handle it, + # matching how the other services treat empty results (issue #171). + return df + df["datetime"] = pd.to_datetime(df.pop("peak_dt"), errors="coerce") df.dropna(subset=["datetime"], inplace=True) return df diff --git a/tests/nwis_test.py b/tests/nwis_test.py index 905ed8db0..f95b40765 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -11,6 +11,7 @@ from dataretrieval.nwis import ( NWIS_Metadata, _read_rdb, + format_response, get_discharge_measurements, get_gwlevels, get_iv, @@ -303,3 +304,20 @@ def test_no_sites_flows_through_format_response(self): df = _read_rdb(no_sites_rdb) assert isinstance(df, pd.DataFrame) assert df.empty + + def test_no_peaks_flows_through_format_response(self): + """The 'peaks' service takes an extra formatting step + (preformat_peaks_response) before the empty-frame check, so an empty + peaks response has to survive that too. Previously this raised + KeyError('peak_dt'); now it returns an empty frame like the other + services (same empty-result contract as issue #171). + """ + no_peaks_rdb = ( + "# //Output-Format: RDB\n" + "# //Response-Status: OK\n" + "# //Response-Message: No sites found matching all criteria\n" + ) + df = _read_rdb(no_peaks_rdb) + df = format_response(df, service="peaks") + assert isinstance(df, pd.DataFrame) + assert df.empty From 751d3b326d99fb60a76d96d7774e25f58a05eb6c Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Thu, 16 Jul 2026 16:22:32 -0500 Subject: [PATCH 2/3] test(nwis): parse with raw read_rdb in empty-peaks regression test The empty-peaks test parsed with _read_rdb, which already runs format_response(service=None); the real get_discharge_peaks path uses the raw read_rdb parser followed by format_response(service="peaks"). Switch to read_rdb so the test exercises the actual call path without the redundant format pass. Signed-off-by: thodson-usgs Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/nwis_test.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/nwis_test.py b/tests/nwis_test.py index f95b40765..9bb24bf75 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -20,6 +20,7 @@ get_record, get_water_use, preformat_peaks_response, + read_rdb, ) START_DATE = "2018-01-24" @@ -317,7 +318,9 @@ def test_no_peaks_flows_through_format_response(self): "# //Response-Status: OK\n" "# //Response-Message: No sites found matching all criteria\n" ) - df = _read_rdb(no_peaks_rdb) + # Mirror get_discharge_peaks: raw read_rdb, then the peaks-specific + # format_response (not _read_rdb, which formats with service=None). + df = read_rdb(no_peaks_rdb) df = format_response(df, service="peaks") assert isinstance(df, pd.DataFrame) assert df.empty From d9d090c4177c056d619b4fbf93cbc75a44e2f52e Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Wed, 26 Aug 2026 16:40:00 -0500 Subject: [PATCH 3/3] fix(nwis): narrow the empty-peaks guard to actually-empty frames Review follow-ups on the empty-peaks guard. The guard fired on any frame missing peak_dt, including a non-empty one from a truncated or altered RDB header. Such a frame is malformed rather than empty, and was being returned silently without its datetime index where it used to raise. Require df.empty too, and pin it with a test. Correct the regression test's docstring, which said the empty responses get_discharge_peaks sees are caught earlier as NoSitesError. That check fires only on a body starting "No sites/data" -- what the live service happens to send today -- so a comment-only RDB does reach the guarded line. As written the docstring read as "this guard is unreachable", inviting its deletion. Widen the test class docstring to cover both arms it now holds, document the pass-through in preformat_peaks_response's public docstring, and add the NEWS entry this behavior change warrants. Co-Authored-By: Claude Opus 5 (1M context) --- NEWS.md | 2 ++ dataretrieval/nwis.py | 12 ++++++++++-- tests/nwis_test.py | 29 ++++++++++++++++++++++------- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/NEWS.md b/NEWS.md index 6dfb3ba9e..f025a9992 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**08/26/2026:** **Bug fix:** `nwis.format_response(df, service='peaks')` and `nwis.preformat_peaks_response` raised `KeyError('peak_dt')` on an empty peaks response instead of returning an empty frame. Both are public, and every other service already treated an empty result as a legitimate empty frame rather than an error (issue #171); the peaks arm slipped through because it reformats the datetime column before the empty-frame check. Callers can now check `df.empty` rather than catching an exception. A *non-empty* frame with no `peak_dt` column is malformed rather than empty, and still raises. + **08/25/2026:** Removed `dataretrieval.ogc.retry`, which only re-exported private helpers. Deprecated `dataretrieval.ogc.interruptions`; import exceptions from `dataretrieval` or `dataretrieval.interruptions` instead. The old path will be removed in a future major release, no earlier than 2027-08-25. **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. diff --git a/dataretrieval/nwis.py b/dataretrieval/nwis.py index 05715d62c..86b32824c 100644 --- a/dataretrieval/nwis.py +++ b/dataretrieval/nwis.py @@ -217,10 +217,18 @@ def preformat_peaks_response(df: pd.DataFrame) -> pd.DataFrame: df: ``pandas.DataFrame`` The formatted data frame. + Notes + ----- + An empty frame with no ``peak_dt`` column is returned unchanged, so that + an empty peaks response reaches :func:`format_response`'s empty-frame path + rather than raising ``KeyError``. + """ - if "peak_dt" not in df.columns: + if df.empty and "peak_dt" not in df.columns: # An empty response parses to a column-less frame; return it so - # format_response's empty-frame path handles it like every other service. + # format_response's empty-frame path handles it like every other + # service. A non-empty frame missing peak_dt is malformed, not empty, + # and still raises. return df df["datetime"] = pd.to_datetime(df.pop("peak_dt"), errors="coerce") diff --git a/tests/nwis_test.py b/tests/nwis_test.py index e11d816dd..f479e9517 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -287,11 +287,12 @@ def test_set_metadata_info_countyCd(self, httpx_mock): class TestReadRdb: - """Tests for the NWIS-specific _read_rdb wrapper. + """Tests for the NWIS-specific parse-then-format path. The format-agnostic parser is exercised in tests/rdb_test.py; this - class pins the wrapper-specific contract — that an empty parser - result flows through format_response without crashing (issue #171). + class pins the NWIS-specific contract — that an empty parser result + flows through format_response without crashing (issue #171), on the + plain arm via _read_rdb and on the peaks arm via format_response. """ def test_no_sites_flows_through_format_response(self): @@ -315,10 +316,13 @@ def test_no_peaks_flows_through_format_response(self): The peaks arm runs ``preformat_peaks_response`` before the "datetime not in columns" check, and that function popped ``peak_dt`` unconditionally, so a column-less frame raised ``KeyError`` where every - other service returned an empty frame (issue #171's contract). Both - functions are public, so this is reachable directly; the empty - responses ``get_discharge_peaks`` itself sees are caught earlier as - ``NoSitesError``. + other service returned an empty frame (issue #171's contract). + + Both functions are public API, so any caller parsing a peaks RDB + reaches this. It is not dead code guarded by ``NoSitesError``: that + check in ``_querying`` fires only on a body starting "No sites/data", + which is what the live service happens to send today -- a comment-only + RDB reaches the guarded line instead. """ no_peaks_rdb = ( "# //Output-Format: RDB\n" @@ -332,6 +336,17 @@ def test_no_peaks_flows_through_format_response(self): assert isinstance(df, pd.DataFrame) assert df.empty + def test_malformed_peaks_frame_still_raises(self): + """Only an *empty* peaks frame is a legitimate empty result. A + non-empty frame with no ``peak_dt`` column is a malformed response -- + a truncated or altered RDB header -- and must stay loud rather than be + returned silently without its datetime index. + """ + df = pd.DataFrame({"peak_va": [1000]}) + + with pytest.raises(KeyError, match="peak_dt"): + format_response(df, service="peaks") + class TestGetRecordDispatch: """``get_record`` is a router; each service must reach its own getter.