Skip to content

fix(waterdata): Raise on mid-pagination failures instead of silently truncating#279

Merged
thodson-usgs merged 1 commit into
DOI-USGS:mainfrom
thodson-usgs:fix-walk-pages-429
May 16, 2026
Merged

fix(waterdata): Raise on mid-pagination failures instead of silently truncating#279
thodson-usgs merged 1 commit into
DOI-USGS:mainfrom
thodson-usgs:fix-walk-pages-429

Conversation

@thodson-usgs
Copy link
Copy Markdown
Collaborator

@thodson-usgs thodson-usgs commented May 16, 2026

The paginated request loops in waterdata (_walk_pages and get_stats_data) have, since #273, logged page failures correctly but kept a "best effort" contract: when any follow-up page failed, log the failure and return whatever pages had been collected so far. That contract was the wrong default — the waterdata API exposes no resume cursor once a paginated walk is interrupted, so the partial DataFrame couldn't reliably be extended. Callers silently received truncated data they had no way to detect.

Both loops now raise on any mid-pagination failure (429, 5xx, network error). The wrapper RuntimeError carries:

  • the number of pages successfully collected,
  • the upstream cause (as both the message text and __cause__ for programmatic inspection),
  • a short menu of recovery actions: wait and retry, reduce request size (fewer locations, shorter time range, smaller limit), or obtain an API token.

A small shared helper _paginated_failure_message builds the user-facing string so both loops stay aligned.

RuntimeError: Paginated request failed after collecting 7 page(s):
429: Too many requests made. Please obtain an API token or try again
later. To recover: wait for the rate-limit window to reset and retry,
reduce the request size (e.g. fewer locations, a shorter time range,
or a smaller ``limit``), or obtain an API token.

Behavior change

Callers that previously consumed partial DataFrames on transient upstream blips (5xx, network errors) will now see an exception they need to handle — typically: retry, possibly with a smaller limit or narrower query. Called out in NEWS.

This is intentional. With no resume API, "best effort partial" was just "silent truncation with friendly framing." A loud error is strictly safer; programmatic callers can try / except RuntimeError and use e.__cause__ (isinstance(..., requests.ConnectionError), status-code parsing, etc.) to branch on the failure mode.

Tests

Three new tests cover the new contract in both _walk_pages and get_stats_data_raises_on_connection_error_mid_pagination, _raises_on_5xx_mid_pagination, _raises_on_mid_pagination_429. The two pre-existing best-effort tests were replaced (same scenarios, new assertion shape).

Ordering / relationship to other open PRs

This PR is independent of #276 (the multi-value GET chunker) and can land in either order.

This PR should land before #278 (the draft parallel chunker). Parallel sub-requests make the silent-truncation behavior easy to trigger empirically (one of two runs returned 330,174 rows instead of 337,808 with no exception). With this fix in place, the chunker can be enhanced separately to catch the RuntimeError and wrap it in QuotaExhausted with partial_frame — but the silent-data-loss bug is closed at the source.

Refs #273

@thodson-usgs thodson-usgs changed the title fix(waterdata): Raise on mid-pagination 429 instead of silently truncating fix(waterdata): Raise on mid-pagination failures instead of silently truncating May 16, 2026
@thodson-usgs thodson-usgs requested a review from Copilot May 16, 2026 16:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes waterdata pagination behavior to fail loudly (raise) on any mid-pagination error rather than returning a silently truncated DataFrame, and updates tests + release notes to reflect the new contract.

Changes:

  • Introduce _paginated_failure_message(...) and wrap mid-pagination exceptions in a RuntimeError that chains the original failure via __cause__.
  • Update _walk_pages and get_stats_data to raise on mid-pagination failures (429/5xx/network errors) instead of returning partial results.
  • Replace the prior “best-effort” pagination tests with new assertions that validate the raised exception and chaining behavior; document the behavior change in NEWS.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
dataretrieval/waterdata/utils.py Adds shared failure-message helper and switches mid-pagination handling to raise with chained cause.
tests/waterdata_utils_test.py Updates pagination-failure tests to assert raising behavior instead of logging/partial returns.
NEWS.md Documents the behavior change for paginated waterdata requests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread dataretrieval/waterdata/utils.py Outdated
Comment thread NEWS.md Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread dataretrieval/waterdata/utils.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread dataretrieval/waterdata/utils.py
Comment thread dataretrieval/waterdata/utils.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

`_walk_pages` and `get_stats_data`'s pagination loops have, since
PR DOI-USGS#273, logged failures correctly but preserved a "best effort"
contract of returning whatever pages had been collected when a
follow-up page failed. The waterdata API exposes no resume cursor
once a paginated walk is interrupted, so the partial DataFrame
couldn't reliably be extended — silently returning it handed
callers truncated data they had no way to know was truncated.

Both loops now wrap any mid-pagination exception (429, 5xx,
network error) in a ``RuntimeError`` carrying:

- the number of pages successfully collected,
- the upstream cause (as both the message text and ``__cause__``
  for programmatic inspection),
- a short menu of recovery actions (wait and retry, reduce
  request size, or obtain an API token).

The shared helper ``_paginated_failure_message`` builds the user-
facing string so both loops stay aligned.

Behavior change: callers that previously consumed partial
DataFrames on transient upstream blips will now see an exception
they need to handle (typically: retry, possibly with a smaller
``limit`` or narrower query). Called out in NEWS.

Tests:
- Replaced the prior best-effort-preserves-partial assertions
  with raises-with-cause-chain assertions for all three failure
  modes (connection error, 5xx, 429), in both ``_walk_pages``
  and ``get_stats_data`` variants.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@thodson-usgs thodson-usgs marked this pull request as ready for review May 16, 2026 23:54
@thodson-usgs thodson-usgs merged commit 4a65fb1 into DOI-USGS:main May 16, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants