fix(nwis): handle empty peaks response instead of raising KeyError

[arpitjain099]

Calling nwis.get_discharge_peaks or get_record(service="peaks") for a site with no annual-peak data raises KeyError('peak_dt') instead of returning an empty result.

When peaks has no data, the RDB body is comment lines only, so read_rdb parses it to a column-less empty DataFrame. format_response runs preformat_peaks_response before its own "datetime not in columns" empty-frame check, and that function's first line pops peak_dt, which blows up on the empty frame.

This is the same empty-result behavior that #171 fixed for the other services; peaks slipped through because it gets preformatted first. The fix returns the frame unchanged when peak_dt is absent, so the existing empty-frame path in format_response takes over and callers can check df.empty rather than catching an exception.

Added a regression test in TestReadRdb next to the existing #171 coverage. It fails on main with KeyError('peak_dt') and passes with the change. ruff check and ruff format --check are clean on both touched files.

I work on supply-chain and data-tooling robustness and hit this while looking at the empty-response paths. Happy to adjust if you would rather guard this inside format_response instead.

[thodson-usgs]

Thanks for this — the fix is right and I've merged current main into the branch and pushed. Net change is still just the 7-line guard plus one regression test.

One correction to the premise, because I couldn't reproduce the headline symptom and I'd rather the test docstring not enshrine a repro that doesn't hold.

get_discharge_peaks doesn't actually raise KeyError. Every genuinely empty peaks response the live service returns is the legacy sentinel body, which _querying converts to NoSitesError before format_response is reached. I probed six cases against nwis.waterdata.usgs.gov/nwis/peaks:

case	body
streamgage, empty future window	No sites/data found… → NoSitesError
groundwater-only site	No sites/data found… → NoSitesError
nonexistent site	No sites/data found… → NoSitesError
streamgage, pre-record window	HTML error page → ValueError from read_rdb
stateCd=HI, narrow window	651 KB of data (peaks ignores the date filter)
control, site with peaks	normal RDB

So the getter path is already guarded.

The defect is real anyway, one level down. format_response and preformat_peaks_response are both public, documented API (automodule :members:), and on current main:

read_rdb(comment_only_rdb) -> (0, 0) empty frame
format_response(df, service="peaks") -> KeyError: 'peak_dt'
format_response(df, service=None)    -> OK, empty frame

That asymmetry is exactly what issue #171 established shouldn't happen, and peaks is the one arm that still violates it — because preformat_peaks_response runs before the "datetime" not in df.columns check. So the guard belongs where you put it. I've reworded the test docstring to describe that path rather than the getter.

This also sits inside ADR 0005, which permits "compatibility, security, and correctness fixes only" on the deprecated nwis facade — a correctness fix, no new capability.

What I changed while resolving:

• read_rdb has since moved to the dataretrieval.rdb leaf, so the test imports it from there rather than through the nwis adapter, and passes _NWIS_RDB_DTYPES to mirror what get_discharge_peaks now does.
• Trimmed the inline comment to the constraint; the history lives in the commit message.
• Conflict was TestGetRecordDispatch landing right where this branch appends; both sides kept.

Verified: the new test fails with KeyError: 'peak_dt' without the guard and passes with it; 1130 tests pass; mypy --strict, ruff, Xenon, complexipy and import-linter all clean.

One thing I deliberately left alone: there are now two different empty-result behaviors for NWIS — NoSitesError when the body starts with the sentinel, an empty frame when the RDB is comment-only. That predates this PR and isn't its job to reconcile, but it's worth a follow-up issue.

[thodson-usgs]

Ran a review pass over the branch and pushed the follow-ups. No functional bug in the fix itself; these are contract and documentation issues.

The guard was broader than intended. if "peak_dt" not in df.columns also fired on a non-empty frame missing that column — a truncated or altered RDB header. That frame is malformed rather than empty, and was being returned silently without its datetime index where it previously raised. Now if df.empty and "peak_dt" not in df.columns, with a test pinning that a malformed non-empty frame still raises.

I had overstated the NoSitesError guard in the test docstring I wrote on the last push. I said the empty responses get_discharge_peaks sees are caught earlier as NoSitesError — but that check fires only on a body starting No sites/data, which is what the live service happens to send today. A comment-only RDB does reach the guarded line. As written the docstring read as "this guard is unreachable", which invites someone deleting it. Reworded.

Also: widened the test class docstring so it still describes its contents, documented the pass-through in preformat_peaks_response's public docstring, and added the NEWS entry this behavior change warrants.

Verified: 1131 tests pass; mypy --strict, ruff, Xenon, complexipy and import-linter all clean.

One thing I deliberately left out: a mocked test driving get_discharge_peaks end-to-end. It would pass, but it pins a response shape the live service doesn't produce — I probed six empty cases and every one returns the No sites/data sentinel. The unit-level test covers the path callers actually reach.

Separate follow-up, not folded in here: rdb.read_rdb has the same empty-result hole one step earlier — read_rdb("# //Output-Format: RDB\n#\n\n") raises pandas.errors.EmptyDataError rather than returning an empty frame, because header_idx lands on the blank line. Confirmed, but it lives in the shared rdb leaf, so it belongs in its own change rather than growing this one.

[arpitjain099]

@thodson-usgs Your narrowing is right. df.empty and "peak_dt" not in df.columns is the correct guard. Mine fired on a malformed non-empty frame as well and would have handed it back silently without its datetime index.

On the conflicts: I resolved them locally and the branch comes out empty against main. #395 already carries the guard and an equivalent malformed-frame test, and merging this as-is would revert the censored-peaks work. It restores df.pop("peak_dt") and the dropna(subset=["datetime"]) that #395 removed, and drops the three peaks tests. So I'd close this as superseded unless you want it open for something.

Happy to take the rdb.read_rdb follow-up you flagged. read_rdb("# //Output-Format: RDB\n#\n\n") raising EmptyDataError because header_idx lands on the blank line is the same hole one layer down, and it is self-contained enough for its own change. I'll open an issue for it first unless you'd rather go straight to a patch.

[thodson-usgs]

@arpitjain099 , the bot found #395 while reviewing your PR, but I merged out of order and it clobbered your fix. Sorry about that. I trimmed this PR to a regression test rather than closing it. Thanks for finding the bug!