Skip to content

refactor(transport)!: share fan-out execution across services - #5

Closed
thodson-usgs wants to merge 2 commits into
refactor/phase-2-transport-boundariesfrom
refactor/phase-3.5-fanout-execution
Closed

refactor(transport)!: share fan-out execution across services#5
thodson-usgs wants to merge 2 commits into
refactor/phase-2-transport-boundariesfrom
refactor/phase-3.5-fanout-execution

Conversation

@thodson-usgs

Copy link
Copy Markdown
Owner

Stacked on DOI-USGS#350, and opened here on the fork deliberately: refactor/phase-2-transport-boundaries only exists on the fork, so this is the only base that yields a 1-commit diff. Targeting DOI-USGS:main today would carry DOI-USGS#350's three commits along with it — the duplicated-history shape that got DOI-USGS#345 closed.

When DOI-USGS#350 merges, retarget this at DOI-USGS:main (it will then be a clean single commit). Independent of DOI-USGS#351, which touches waterdata/api.py only.

The distinction

Chunking is how you divide the data structurally; fan-out is how you distribute the work operationally.

The two are orthogonal, and only the first is protocol knowledge: dividing a query needs the byte budget, the CQL2 grammar, and which parameters are list-valued. Distributing the pieces needs none of it.

The package had not drawn that line. ChunkPlan (division) and ChunkedCall (distribution) sat side by side in dataretrieval/ogc/ as siblings — and ADR 0006 grouped them together deliberately, which was right while a byte plan was the only thing anyone fanned out over.

Measuring OGC domain vocabulary in each class's code, docstrings and comments stripped:

ChunkPlan     7 distinct -> CQL, axes, axis, axis_chunks, bytes, filter_lang, url_limit
ChunkedCall   0 distinct -> []

What that cost

Water Use fans out for an entirely different reason — the NWDC accepts one location= per request, and its URLs run ~63 bytes against an 8000-byte budget. Unable to reach an OGC-internal executor, wateruse._fan_out re-implemented the semaphore, the gather, and the cancellation-beats-HTTP-error precedence rule, with a comment at wateruse.py:398 naming ChunkedCall._run as the original. One subtle rule, two copies, synchronized by prose.

The duplicate also lost three things:

OGC Water Use (before)
resume after 429 .call.resume() none — one exhausted location discarded all completed ones
progress line yes no
concurrency knob API_USGS_CONCURRENT module global MAX_CONCURRENT_REQUESTS = 4

The resume gap is the one that bites: a multi-state county pull is hundreds of requests against a 1000/hr quota.

What this does

Moves execution down, leaves planning up. transport/fanout.py::FanOut drives any FanOutPlan — a Protocol of exactly the three members the executor already used (total, canonical_url, iter_sub_args()).

Structural rather than nominal because the two implementations share an interface and no implementation at all: ChunkPlan derives sub-requests from a byte budget over multi-value axes; a Water Use plan lists locations the caller already named separately. Neither has anything the other could inherit, so an ABC would be ceremony. ChunkPlan needed no edits — it already satisfied the protocol.

Water Use sheds ~75 lines and gains resume, progress, and the shared setting.

Breaking changes

  1. A Water Use fan-out interrupted by 5xx/429 now raises ServiceInterrupted/QuotaExhausted, not ServiceUnavailable/RateLimited. Both remain DataRetrievalError so broad handlers are unaffected, but a narrow except ServiceUnavailable around a Water Use call must widen. This is convergence with the OGC getters — and it is precisely what makes the failure resumable.
  2. wateruse.MAX_CONCURRENT_REQUESTS is removed in favor of API_USGS_CONCURRENT and wateruse.DEFAULT_CONCURRENT_REQUESTS.

Not breaking: ChunkInterrupted is a permanent alias of the renamed FanOutInterrupted — the same class object, not a deprecation — so except ChunkInterrupted keeps working. The rename is one name, because QuotaExhausted and ServiceInterrupted were already fan-out-neutral.

Concurrency: one setting, per-service defaults

API_USGS_CONCURRENT is general; a service declares a default for when it is unset (Water Use 4, package-wide 32). An explicitly set env var outranks a service default, never the reverse — a service able to override it would make API_USGS_CONCURRENT=1 a lie, which is the original defect.

Layout

From To
ogc/chunking.py::ChunkedCall transport/fanout.py::FanOut
ogc/retry.py::_classify_chunk_error interruptions.py (beside the classes it produces)
ogc/interruptions.py interruptions.py (top-level leaf)

Stays in ogc: ChunkPlan, multi_value_chunked, parallel_chunks, _OGC_URL_BYTE_LIMIT. Compatibility aliases (ChunkedCall, get_active_client, _chunked_client) remain importable from ogc.chunking; _chunked_client is the same ambient object transport publishes, not a copy.

interruptions.py is a top-level leaf for the reason ADR 0006 gives for combining/progress/credentials: adapters need it whether or not they went through transport, and an exception taxonomy is not HTTP execution policy.

Verification

  • 685 tests pass (5 new), ruff clean, mypy --strict clean, pre-commit clean.
  • New fitness functions: wateruse may contain no asyncio.gather/Semaphore/TaskGroup; both plans satisfy FanOutPlan (including that iter_sub_args() is stable across passes and agrees with total, since resume keys by position); the Water Use plan does not inherit ChunkPlan; no interruption taxonomy inside transport.
  • New adapter tests: Water Use resume re-issues only unfinished locations, progress ticks, concurrency precedence.

One deviation from plan: I predicted the moves would change zero OGC tests. They changed three call sites — 9 patches of _chunking.asyncio.sleep (the backoff sleep is issued by transport.retry; patching it through the chunker only ever worked because asyncio is a shared module object) and one client-factory patch. No assertion changed; each now names the module that actually owns the behavior.

Known costs

  • Resume re-issues a failed location's entire page walk, so pages fetched before the failure are fetched again. Already true for OGC — a partial walk never enters the completion map — so a cost, not a correctness problem.
  • Water Use frames carry huc12_id, not id, so _combine_chunk_frames concatenates without deduplicating. Correct (locations partition by construction), but the dedup safety net does not apply there.

Open question

.completed_chunks / .total_chunks are left as-is. They are read rather than caught, and the message text already says "sub-requests", so renaming them would churn ~30 assertions for cosmetics. Happy to do it if you want the vocabulary uniform.

Supersedes one clause of ADR 0006; see the new ADR 0007.

🤖 Generated with Claude Code

@thodson-usgs
thodson-usgs force-pushed the refactor/phase-3.5-fanout-execution branch from 9563779 to 3ec883c Compare August 6, 2026 18:10
thodson-usgs and others added 2 commits August 6, 2026 13:39
…al layer (DOI-USGS#350)

* feat(transport): bounded retry for active services, over an API-neutral layer

WQP, NLDI, StreamStats, and Water Use now retry transient failures instead
of surfacing the first one. That is a resilience change, not a refactor, so
it leads here; the layering change that made it tractable follows.

Retry costs latency and quota on failing requests, so it is bounded on two
independent axes and narrowed to failures a later attempt could survive.

API_USGS_STALL_TIMEOUT (new; default 60 s, 0 disables) bounds how long a
call may go without receiving any data. API_USGS_RETRIES counts attempts,
not seconds, so on its own four retries of a request that times out after a
minute is four silent minutes. Progress restarts the budget -- a page
received, or a queued sub-request acquiring its concurrency slot, credited
as the wait it was rather than restamped -- and an attempt already in
flight is never interrupted. The first retry is never withheld, so one slow
attempt cannot disable retry by itself. A dead connection costs about two
read timeouts rather than five attempts.

Which statuses are re-sent is per-adapter. WQP answers an over-large query
with a 500 and StreamStats answers out-of-network coordinates with one, so
those one-shot adapters re-send only for 429/502/503/504. The Water Data
OGC API is a query interface where a 500 is an upstream fault, so the
chunker keeps re-sending for every 5xx, as it always has.

Failures already settled are not retried: an unsupported scheme, a request
we built wrong, or a hostname the resolver rejects outright. A temporary
resolver failure (EAI_AGAIN) stays retryable. Backoff always includes
jitter, including on a server-named Retry-After, so sub-requests handed one
hint do not wake in lockstep and a hint of 0 cannot become a zero-delay
re-send. An unusable setting raises ConfigurationError -- both a
DataRetrievalError and a ValueError -- rather than escaping a request path
untyped.

Measured against the live API: a 4-state, 30-year get_daily over 800 sites
at parallel_chunks(1) runs 91.8 s and returns 581,070 rows with a worst
inter-page silence of 12.1 s, so the budget does not threaten long
successful queries.

The layering half adds dataretrieval.transport, an internal API-neutral
execution layer owning guarded client lifecycle and timeout defaults,
host-scoped authentication, cursor pagination, bounded retry, response
aggregation, progress, and sync-over-async dispatch. dataretrieval.ogc
keeps its protocol concerns: dialects, CQL2, request construction, feature
shaping, URL-byte chunk planning, resumable ChunkedCall state, and
interruption types. Before this, generic execution behavior lived under OGC
even where non-OGC services used it, so Water Use depended on private
protocol modules and retry policy was uneven across services; there is now
one policy to reason about. transport.liveness is a stdlib-only leaf
recording when data last arrived, so the page loop that observes progress
and the retry loop that acts on it depend on it rather than on each other.
Architecture fitness functions enforce the dependency direction, an acyclic
transport graph, and Water Use's isolation from OGC; ADR 0006 records the
decision.

Compatibility: public imports, service signatures, return shapes, metadata,
deprecations, exception types, OGC chunking/resume behavior, and the
four-symbol OGC facade are unchanged, and utils.query keeps its exact
signature and performs no retry. Private compatibility aliases are kept
where a consumer exists. Two modules were removed rather than aliased,
since nothing imported them: ogc.progress and ogc.combining, now
transport.progress and transport.combining. ogc.retry keeps only its OGC
interruption classifiers.

Also pins the CI test step to bash on every OS. Windows defaults to
PowerShell, which does not halt on a failing native command and takes the
step's exit status from the last one, so a coverage report following a
failed pytest reported success -- every Windows test failure in this
repository has been invisible.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015UVSiGgUyDRSbqNWM5HBbL

* Simplify pass

* refactor: move presentation and credential policy out of transport

Splits three concerns out of the shared transport layer and closes two
credential-leak paths in server-supplied pagination links.

The layer was named "API-neutral" but held `api.waterdata.usgs.gov`, read
`API_USGS_PAT`, pinned `x-ratelimit-remaining`, and printed a USGS signup
URL. It is neutral across USGS *services*, not across HTTP APIs, and the
aspirational name invited generality nobody needs. ADR 0006 now says so
plainly and is renamed to match.

Two modules were in transport only because they had to leave `ogc/`
during the earlier extraction:

- `progress.py` is terminal presentation (Jupyter detection, status-line
  rewriting, broken-pipe handling), called *from* transport rather than
  part of it, and the sole reason a `progress -> http` edge existed.
- `combining.py` is DataFrame assembly, consumed by `ogc/planning` and
  `wateruse` for reasons unrelated to HTTP.

Both move to top-level leaves. Transport goes 1290 -> 766 lines and 7 ->
5 modules, and `http`/`liveness` become leaves.

A new `credentials.py` leaf owns every answer about the API key. The code
that attaches a credential and the code that strips it back off have to
agree on which host is authorized, and the way they stop agreeing is a
second copy of the host string. `waterdata/utils`, `ogc/policy`, and
`ngwmn` each carried their own `BASE_URL` spelling of that same
authority -- two of them with a comment documenting the duplication as
deliberate -- so they now derive it from the one definition.

Closes two ways a poisoned response body reached a credential:

- `accepts_api_key` matched on host alone, so `http://` on the
  authorized host sent the key in cleartext. It now requires https.
- `ogc/engine` checked the next-link host but not its userinfo, and
  `waterdata/ratings` followed STAC `next` hrefs with no check at all.
  httpx derives `Authorization: Basic` from userinfo, so a link carrying
  `user:pass@` minted a credential the caller never configured and sent
  it beside the real API key -- past the host check, which passes in
  exactly that case.

The credential fitness function matched the quoted bare host, so the
`https://`-prefixed form slipped past it and it reported success with
three copies live. It now walks AST string values, excluding docstrings
so prose naming the service is not mistaken for a second source of truth.

Every new test was verified to fail against the unfixed source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(ogc): don't offer a failure we refuse to retry as resumable

"Should we retry this?" and "can the caller resume it?" are the same
question asked twice, and the two answers disagreed. Retry already
declines to re-send a failure no later attempt could survive -- a bad
URL scheme, a malformed request, a hostname the resolver rejects
outright. The interruption classifier mapped every httpx error to
ServiceInterrupted regardless, so the caller got a .call.resume() whose
every attempt fails identically, with the NetworkError that actually
explained the problem buried underneath it.

Both answers now come from one predicate in transport. A deterministic
failure classifies as unrecognized, which is the existing "re-raise raw"
path, so the caller sees the real error.

The test asserts both answers on the same failures, so they cannot drift
apart again. Note the chain shape matters: our wrapper raises with
`from`, so the chunker's explicit-link walk reaches the httpx error,
whose implicit links then lead to the resolver code -- a temporary
resolver failure stays both retryable and resumable, decided only by the
errno.

Addresses finding 3 of the chunking review; finding 4 (a 5xx sibling
masking a 429's Retry-After) remains open.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Update NEWS.md

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Chunking is how you divide the data structurally; fan-out is how you
distribute the work operationally. The two are orthogonal, and only the
first is protocol knowledge -- dividing a query needs the byte budget,
the CQL2 grammar, and which parameters are list-valued, while
distributing the pieces needs none of it.

The package had not drawn that line. ChunkPlan (division) and
ChunkedCall (distribution) sat side by side in dataretrieval.ogc as
siblings. Unable to reach an OGC-internal executor, wateruse._fan_out
re-implemented the semaphore, the gather, and the failure-precedence
rule, with a comment naming ChunkedCall._run as the original -- one
subtle rule, two copies, synchronized by prose. The duplicate lacked
resume (a 429 partway through discarded every completed location),
reported no progress, and ignored API_USGS_CONCURRENT.

Move execution down; leave planning up. transport.fanout.FanOut drives
any FanOutPlan -- a Protocol of the three members the executor already
used (total, canonical_url, iter_sub_args). It is structural because its
two implementations share an interface and no implementation: ChunkPlan
derives sub-requests from a byte budget, a Water Use plan lists locations
the caller already named separately.

Water Use sheds ~75 lines and gains resume, progress, and the shared
concurrency setting. Concurrency is now one general knob with per-service
defaults, and an explicitly set API_USGS_CONCURRENT outranks a service
default -- never the reverse, or the setting would be a lie.

The interruption taxonomy moves to the dataretrieval.interruptions leaf,
since adapters need it whether or not they went through transport. Its
base is renamed FanOutInterrupted, because Water Use raises it without
chunking anything; ChunkInterrupted stays as a permanent alias of the
same class object, so `except ChunkInterrupted` keeps working.

_deterministic_failure moves to that leaf too, and transport.retry
imports it back. Whether a failure is worth retrying and whether it can
be resumed are one judgement about what the exception means, not two --
and the leaf is where meaning lives. Leaving it in transport would have
forced the leaf to import transport to ask.

BREAKING CHANGE: a Water Use fan-out interrupted by 5xx/429 now raises
ServiceInterrupted/QuotaExhausted rather than ServiceUnavailable/
RateLimited. Both remain DataRetrievalError, so broad handlers are
unaffected, but a narrow `except ServiceUnavailable` must widen. This is
convergence with the OGC getters, and it is what makes the failure
resumable. wateruse.MAX_CONCURRENT_REQUESTS is removed in favor of
API_USGS_CONCURRENT / wateruse.DEFAULT_CONCURRENT_REQUESTS.

Supersedes the ADR 0006 clause assigning resumable ChunkedCall state to
OGC; see ADR 0008.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thodson-usgs
thodson-usgs force-pushed the refactor/phase-3.5-fanout-execution branch from 3ec883c to 518db33 Compare August 6, 2026 18:50
@thodson-usgs

Copy link
Copy Markdown
Owner Author

Superseded by DOI-USGS#355, which is the same commit rebased onto main now that DOI-USGS#350 has merged. This one existed only because its base branch lived on the fork; with phase 2 in main the work can target upstream directly.

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