refactor(transport)!: share fan-out execution across services - #355
Merged
thodson-usgs merged 1 commit intoAug 9, 2026
Merged
Conversation
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
force-pushed
the
refactor/phase-3.5-fanout-execution
branch
from
August 9, 2026 14:04
518db33 to
d7b98eb
Compare
thodson-usgs
marked this pull request as ready for review
August 9, 2026 20:56
thodson-usgs
added a commit
to thodson-usgs/dataretrieval-python
that referenced
this pull request
Aug 10, 2026
The branch was cut at 31b0142 and is 37 commits behind: main has since taken the fan-out refactor (DOI-USGS#355), the utils split, and the service->collection glossary work. Rebasing resolved 15 conflicted files; this commit is the integration those resolutions exposed. Four RetryPolicy.from_env() call sites main added after the branch was cut -- _querying, wateruse, waterdata.stats, ogc.engine -- now say from_config(), so the rename is complete rather than partial. Concurrency now resolves through the chain instead of reading the environment in transport.fanout. config.concurrency() takes the caller's default, so the rule that an explicit setting outranks a service preference lives in one place rather than being restated by every adapter. It is resolved OUTSIDE the construction-time context snapshot and passed into the drive, the same treatment DOI-USGS#355 gave the progress reporter: concurrency is the one dial a caller adjusts precisely while retrying, and a configure() block entered after the call was built is invisible inside a copied context. .importlinter gains config as a layer between credentials and exceptions -- it imports only ConfigurationError, and credentials imports it. The contract is exhaustive, so it failed until the module was placed deliberately. ogc/chunking.py drops the _parallel_chunks Ambient: parallel_chunks(n) now delegates to config.configure(parallel_chunks=n), so keeping a second ContextVar would let show_config() report a value the planner does not use. tests/architecture_test.py drops test_ogc_does_not_depend_on_service_adapters, which main deleted when .importlinter's protected contract took it over; the conflict resolution had resurrected it. 889 tests pass, mypy clean across 58 files, all gates and 14 pre-commit hooks pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The distinction
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) andChunkedCall(distribution) sat side by side indataretrieval/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:
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_outre-implemented the semaphore, thegather, and the cancellation-beats-HTTP-error precedence rule, with a comment atwateruse.py:398namingChunkedCall._runas the original. One subtle rule, two copies, synchronized by prose.The duplicate also lost three things:
.call.resume()API_USGS_CONCURRENTMAX_CONCURRENT_REQUESTS = 4The 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::FanOutdrives anyFanOutPlan— aProtocolof 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:
ChunkPlanderives 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.ChunkPlanneeded no edits — it already satisfied the protocol.Water Use sheds ~75 lines and gains resume, progress, and the shared setting.
Breaking changes
ServiceInterrupted/QuotaExhausted, notServiceUnavailable/RateLimited. Both remainDataRetrievalErrorso broad handlers are unaffected, but a narrowexcept ServiceUnavailablearound a Water Use call must widen. This is convergence with the OGC getters — and it is precisely what makes the failure resumable.wateruse.MAX_CONCURRENT_REQUESTSis removed in favor ofAPI_USGS_CONCURRENTandwateruse.DEFAULT_CONCURRENT_REQUESTS.Not breaking:
ChunkInterruptedis a permanent alias of the renamedFanOutInterrupted— the same class object, not a deprecation — soexcept ChunkInterruptedkeeps working. The rename is one name, becauseQuotaExhaustedandServiceInterruptedwere already fan-out-neutral.Concurrency: one setting, per-service defaults
API_USGS_CONCURRENTis 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 makeAPI_USGS_CONCURRENT=1a lie, which is the original defect.Layout
ogc/chunking.py::ChunkedCalltransport/fanout.py::FanOutogc/retry.py::_classify_chunk_errorinterruptions.py(beside the classes it produces)transport/retry.py::_deterministic_failureinterruptions.py; transport imports it backogc/interruptions.pyinterruptions.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 fromogc.chunking;_chunked_clientis the same ambient object transport publishes, not a copy.interruptions.pyis a top-level leaf for the reason ADR 0006 gives forcombining/progress/credentials: adapters need it whether or not they went through transport, and an exception taxonomy is not HTTP execution policy._deterministic_failurefollows it down. Whether a failure is worth retrying and whether it can be resumed are one judgement about what an exception means, not two — and the leaf is where meaning lives. Leaving it in transport would have forced the leaf to import transport just to ask. The fix that made those two answers agree (31b01420) is preserved:tests/transport_test.pystill asserts both on the same failures, so they cannot drift apart.Verification
ruffclean,mypy --strictclean, pre-commit clean.waterusemay contain noasyncio.gather/Semaphore/TaskGroup; both plans satisfyFanOutPlan(including thatiter_sub_args()is stable across passes and agrees withtotal, since resume keys by position); the Water Use plan does not inheritChunkPlan; no interruption taxonomy insidetransport.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 bytransport.retry; patching it through the chunker only ever worked becauseasynciois a shared module object) and one client-factory patch. No assertion changed; each now names the module that actually owns the behavior.Known costs
huc12_id, notid, so_combine_chunk_framesconcatenates without deduplicating. Correct (locations partition by construction), but the dedup safety net does not apply there.Open question
.completed_chunks/.total_chunksare 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 0008 (numbered around #351's 0007).
🤖 Generated with Claude Code