Skip to content

Feat/adaptive retries - #77

Open
geoffhancock wants to merge 5 commits into
mainfrom
feat/adaptive-retries
Open

geoffhancock wants to merge 5 commits into
mainfrom
feat/adaptive-retries

Conversation

@geoffhancock

Copy link
Copy Markdown
Collaborator

Proposal: failure-class-aware retries -- split slow requests instead of repeating them

This is a proposal, not a settled design. It is one concrete attempt at making the client absorb the kind of slowness seen in August 2026 without the user having to touch anything. Several of the decisions below could reasonably go the other way; each one is called out under "Discussion points" with the alternative.

Stacked on the knobs PR (#76): this PR's base branch is feat/configurable-timeout, so the diff here is only the retry change. It uses chunk_size, _is_timeout and the timeout hint from #76; once #76 merges, GitHub retargets this PR to main.

Why

Today the transport retries every failure the same way: re-issue the identical request, up to four times, with backoff. That is the right response to a dropped connection or a transient 5xx. It is the wrong response to a read timeout on a 30-day request, where the server was too slow to answer that much data. Repeating the request four times does not make it faster; it just multiplies the load on a backend that is already struggling and turns a one-minute failure into a four-minute one. The knobs PR lets a user react to that after it happens. This PR aims to make the common case need no reaction at all.

The idea

Two layers, each owning the failures it can actually fix.

  • Transport (urllib3 Retry) owns transient, connection-level faults where the identical request is the right thing to retry: connection errors, 500/502/503, and 429 (honouring Retry-After).
  • Client owns faults where something has to change before retrying: a read timeout or 504 on a span request (ask for less at once), and a 401 on a token the client thought was valid (refresh it).
  • Everything else (400/403/404, exhausted 5xx, connection errors that outlast the transport's retries) fails immediately, as today.

Every adaptation the client makes is logged at WARNING, so a run that succeeds because the client split requests still leaves a record that the API was slow.

What changes

  1. Transport retry policy: Retry(total=3, read=0, status_forcelist=[429, 500, 502, 503], ...). Read timeouts are never retried identically; 504 leaves the forcelist; 429 joins it.
  2. _fetch_adaptive: every chunk fetched through _fetch_data now goes through this. On a timeout-class failure where start/end are datetimes, it halves the span with _get_chunks (so the 5-minute trim between halves stays correct) and recurses, up to _MAX_TIMEOUT_SPLITS = 2 levels. Splitting is depth-first and fails fast: the first sub-span that still times out at the last level fails the call.
  3. 401 handling in _make_rate_limited_request: a 401 on a token the client considers valid triggers one re-login and one retry; a second 401 is a real auth failure. Login is serialised with a lock so concurrent workers that all see the same rejected (or expired) token log in once, not once each.
  4. WattTimeRequestError(RuntimeError) replaces the bare RuntimeError on the data path, carrying kind ("timeout", "http_401", "http_4xx", "http_5xx", "connection", "other"), url and params. It subclasses RuntimeError so existing except RuntimeError handlers keep working unchanged; the dispatch above branches on kind rather than parsing messages, and callers can too.

What a user sees

For the August case (a 30-day co2_moer request taking >180 s server-side, 15-day requests taking ~15 s):

  • Before: 60 s timeout, retried identically three more times -> RuntimeError after ~4 minutes, four slow queries issued against the backend.
  • After: 60 s timeout -> one WARNING -> two 15-day requests succeed in ~30 s -> the call returns normally in ~90 s, one slow query issued.

For an API that never answers: WattTimeRequestError(kind="timeout") with the hint from the knobs PR, after about 3 x read timeout (one per split level) rather than 4 x.

Implications and behaviour changes

  • Read timeouts are no longer retried identically anywhere, including login. A login that times out fails on the first attempt with the timeout hint.
  • 504 is no longer retried by the transport; it is treated as a timeout (split on span requests, fail otherwise).
  • 429 is now retried with backoff and Retry-After; previously it failed immediately.
  • A 401 mid-run now costs one automatic re-login instead of failing the call.
  • On the chunked endpoints a call can return more responses than chunks when a split occurred. With include_meta=True that means more meta rows, one per response, as today for normal chunks.
  • Data-path failures are WattTimeRequestError, a RuntimeError subclass. Message format is unchanged.
  • Response shapes, default chunking, and the public method signatures are unchanged.

Discussion points

  1. read=0 on the transport. Chosen so a slow span request costs one timeout before the client splits. The cost is that login loses its identical retries. Alternative: read=1 everywhere (login gets two attempts; span requests waste one extra timeout before splitting).
  2. Depth cap of 2. A class attribute, not a knob, so it can be overridden without a signature change. Alternative: expose it, or replace it with a wall-clock budget per call (a better bound, but a new parameter).
  3. 504 out of the forcelist. Trades away transient-504 recovery on single-shot endpoints for not repeating a slow query on span endpoints. Alternative: keep 504 in the forcelist and split only on read timeouts.
  4. Typed exception. WattTimeRequestError.kind is what makes the dispatch clean and gives callers something better than string matching, but it is more API surface than the retry change strictly needs. Alternative: keep bare RuntimeError and classify internally.
  5. Fail fast on an exhausted split. Mirrors today's "one bad chunk fails the call". Alternative: attempt every sibling sub-span and raise once at the end, which returns more partial data at the cost of more wall-clock on a dead API.

Tests

Nine mocked tests, no network: transport policy asserts; kind classification for eight exception shapes including the retry-wrapped timeout; 401 refreshes once and retries with the new token; a second 401 raises http_401; other 4xx not retried and no re-login; a timed-out 30-day chunk becomes two non-overlapping 15-day requests with one warning; the depth cap holds and is depth-first; no split for non-span or string-dated params; the multithreaded path splits inside the worker. The knobs PR's tests and the existing suite pass unchanged.

The README paragraph on timeouts is updated in a separate commit, as in the knobs PR.

🤖 Generated with Claude Code

geoffhancock and others added 5 commits September 18, 2026 13:14
…ical_csv

get_historical_csv passed include_imputed_marker as the sixth positional
argument to get_historical_pandas, whose sixth parameter is include_meta.
As a result, get_historical_csv(..., include_imputed_marker=True) wrote a
CSV containing a meta column and no imputed_data_used column.

Pass all defaulted parameters by keyword in the internal call chain
(get_historical_csv -> get_historical_pandas -> get_historical_jsons) so
argument order differences between signatures cannot misroute a flag.
Public signatures are unchanged.

Adds a regression test asserting the CSV contains imputed_data_used and
not meta when include_imputed_marker=True.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eout

Add a keyword-only `timeout` to WattTimeBase.__init__ (default (10, 60),
matching the existing login/register literals) and pass it at all three
HTTP call sites. The data request previously used a scalar 60, so its
default connect timeout drops from 60 s to 10 s; default read timeouts
are unchanged.

Add a keyword-only `chunk_size` to get_historical_jsons/_pandas/_csv,
forwarded to _get_chunks, which now accepts None (30 days) and rejects
sizes of 5 minutes or less -- the per-chunk trim -- instead of looping
forever or producing inverted chunks.

When a request fails on a timeout, append a hint to the RuntimeError
naming both knobs. Once the session's retries are exhausted, requests
raises ConnectionError -> MaxRetryError -> ReadTimeoutError rather than
ReadTimeout, so _is_timeout walks the exception chain.

Motivation: in August 2026, 30-day historical pulls for some regions took
over 180 s server-side while the same pulls at 10 days took ~11 s. Users
hit an opaque read timeout after ~4 minutes with no supported lever; the
only workaround was monkeypatching _get_chunks.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add a short "Tuning requests that time out" note under the historical
data example showing both options and that the timeout is per attempt.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… them

The transport retried every failure identically, up to four times. For a
read timeout on a 30-day request that multiplies load on an already slow
backend and turns a one-minute failure into a four-minute one. Split the
retry responsibility in two:

- Transport (urllib3 Retry) keeps connection errors, 500/502/503, and now
  429 (honouring Retry-After). read=0: read timeouts are never retried
  as-is, and 504 leaves the forcelist -- both mean the API was too slow to
  answer this much data, which repeating does not fix.
- Client: _fetch_adaptive halves a span request's time range on a
  timeout-class failure and recurses, up to _MAX_TIMEOUT_SPLITS = 2
  levels, depth-first and failing fast. A 401 on a token the client
  considered valid triggers one serialised re-login and one retry.

Data-path failures are now WattTimeRequestError, a RuntimeError subclass
carrying `kind`, `url` and `params`; existing `except RuntimeError`
handlers and the message format are unchanged. Every split and re-login
is logged at WARNING so a run that succeeds by adapting still records
that the API was slow.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@geoffhancock
geoffhancock marked this pull request as ready for review September 18, 2026 19:12
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.

1 participant