docs: add CONTEXT.md and map the package onto it - #365
Merged
Conversation
Fixes what the package's words mean so code, docstrings, ADRs and conversation use them the same way. A glossary, not a spec -- decisions stay in docs/source/architecture/decisions/. The term that needed deciding was 'chunk'. ADR 0008 defined it narrowly as a byte-driven slice and introduced 'sub-request' for the general unit, which left two words for one idea and made completed_chunks / total_chunks / set_chunks read as violations of the package's own vocabulary. This takes Dask's sense instead -- chunks describe how an array is split into sub-arrays -- so a chunk is a piece of a query, named for being a piece rather than for why it was made one. 'Sub-request' becomes redundant and is dropped, the existing chunk-counting names become correct, and the deferred rename is unnecessary. Three boundaries are recorded because each is a mistake someone would otherwise make: a page is not a chunk (chunks divide a query, pages divide a chunk's response); a query needing no split has one chunk rather than zero, matching ChunkPlan's passthrough; and chunking is not defined by being forced, since parallel_chunks() produces chunks on a query that already fits. Also records the legacy names -- 'site' where monitoring location is meant, ChunkInterrupted as a permanent alias -- so they are not mistaken for canonical or re-litigated.
Applies the chunk definition CONTEXT.md fixes. 'Sub-request' is gone: 244 occurrences across 23 files become 'chunk', which is what they always meant. Two places defined the vocabulary rather than merely using it, and were rewritten rather than word-swapped. interruptions.py's module docstring drew the distinction the glossary removes -- chunk as byte-driven slice, sub-request as the general unit -- and now states the three terms as CONTEXT.md defines them. ADR 0008 justified the FanOutInterrupted rename on the grounds that 'Water Use raises it without chunking anything'. Under the new definition Water Use chunks one request per location, so that reasoning was false while the rename remained right. The ADR now gives the reason that survives: what fails is the execution, not the division. Both services chunk; they chunk for different reasons. No behavior change. The user-visible interruption message now counts 'chunks' rather than 'sub-requests', matching the completed_chunks / total_chunks attributes it was already reporting. 779 tests pass, mypy clean across 58 files.
The last identifier still carrying the retired 'sub-request' term. ChunkPlan's method and the FanOutPlan protocol it satisfies both yield chunk descriptions, so they say so; the local sub_args variables become chunk_args with them. Mechanical -- no behavior change, and the protocol still requires only __len__ and __iter__, so the method name is ChunkPlan's own vocabulary rather than anything FanOut depends on.
_Item/_ItemCo were placeholders chosen while 'chunk' still meant a byte-driven slice and no word covered the general unit. It does now, so FanOutPlan is generic in _ChunkCo and FanOut in _Chunk, which says what the executor is carrying rather than that it is carrying something. The variance split stays: the protocol only ever yields, so it is covariant; FanOut both accepts and yields, so it is invariant.
Three residues the earlier passes missed. ADR 0008 still named iter_sub_args after the method was renamed. The architecture overview and ADR 0004 spelled it 'subrequest' without the hyphen, which every previous search pattern walked past -- a good argument for the glossary existing, since the term was drifting in spelling as well as meaning. The package now has one word for the unit.
thodson-usgs
changed the title
The glossary defined chunk, chunking, fan-out and page but not plan, while 'plan' was doing load-bearing work in two class names. That gap, not the names, was the inconsistency: ChunkPlan and FanOutPlan looked like competing claims on one idea when they are a concrete plan and the contract a plan satisfies. A plan enumerates a query's chunks -- how many, and what each one is. It says how a query divides; it does not execute. Computing one is protocol-specific (a byte budget, a per-location rule), executing one is not, which is the sentence that explains why planning and fan-out live in different packages. With the term defined, FanOutPlan's docstring cites it rather than re-deriving it, and says plainly that it is named for the role it plays -- ChunkPlan is a plan, and so is a plain list of requests.
thodson-usgs
marked this pull request as ready for review
August 9, 2026 23:00
Auditing the remaining 25 terms against the code turned up one real conflict. The glossary says a service is an external system and a collection is one named set of records it offers, but the OGC machinery calls a collection a service -- including the public waterdata.get_cql(service="daily"), where 'daily' is a collection by any reading. The rename was attempted and reverted rather than reported as a plan. 'service' also legitimately means the external system in transport and progress, where it labels a progress line, so one word carries both senses at different layers and no mechanical rename separates them: renaming the OGC half broke FanOut's service label and every waterdata call site, and left get_cql(service=) -- a documented public keyword -- still disagreeing. The inconsistency survives either way, so the churn buys nothing. Recorded as legacy instead, with the rule for reading it: inside dataretrieval.ogc, 'service' means collection; in transport and progress it means the external system; new code should prefer 'collection' where it means one. This is the same treatment 'site' and ChunkInterrupted already get. Also sharpens the Collection entry to state the distinction the conflict obscured, and notes that where the API itself says 'site-types', that is the service's vocabulary and is reproduced rather than translated.
The OGC spec settles this, and our own code already proved it: requests.py
builds the URL as f'{base}/collections/{service}/items'. OGC API - Features
defines that path as /collections/{collectionId}/items, so the value
interpolated there is a collectionId by the standard's own naming. 'daily' is a
collection; Water Data is the service.
get_cql's parameter is now 'collection' and the Literal is WATERDATA_COLLECTIONS.
service= still works and resolves to collection with a DeprecationWarning,
removable on or after 2027-08-09; WATERDATA_SERVICES stays as a permanent alias
of the same object. service was the first positional parameter, so
get_cql('daily', cql) -- the form the docstring's own second example uses -- is
untouched. Only the service= keyword spelling is affected.
The error text changes with it: 'Unknown service' becomes 'Unknown collection'.
One contract loosened deliberately. public_api_test froze get_cql's required
arguments as ('service', 'cql'); both now carry defaults, because a caller still
spelling service= has to reach the alias resolution before the required-argument
check runs. The baseline records why and what to restore it to when service is
removed.
780 tests pass, mypy clean across 58 files, xenon/complexipy/lint-imports pass.
The internals matched the public surface's old spelling: 'service' named a
collectionId throughout dataretrieval.ogc and waterdata.utils. Only public
functions need backward compatibility, and dataretrieval.ogc is internal by
ADR 0003 -- only ngwmn and waterdata may consume it -- so this renames outright
rather than deprecating.
'service' keeps its correct meaning where it has one: the external system in
transport and progress, where it labels a progress line, and the public
waterdata.get_samples / get_codes parameters, which are frozen.
Two things a blanket rename got wrong, both caught by tests rather than review:
FanOut and its ChunkedCall alias take service= as a progress label, not a
collection; renaming that kwarg silently broke construction. The callee names
are protected explicitly now.
More subtly, ogc/requests.py excluded {'service', 'output_id'} from the query
string by matching the CALLER's local variable name -- a string literal, not an
identifier. Renaming it to 'collection' stopped excluding 'service', so
get_samples leaked service=results into the samples URL. It excludes both
spellings now, which it must anyway while get_cql's deprecated keyword lives.
780 tests pass, mypy clean across 58 files, xenon/complexipy/lint-imports pass,
pyscn unchanged at 82.
Two accuracy fixes to this branch's own claims, which is where a vocabulary
change has to be exact.
The justification said 'OGC API - Features calls these collections'. It does
not. 17-069r4 defines only 'feature collection' (a set of features from a
dataset) and notes that 'collection' there is broader than its use elsewhere;
bare 'collection' has no entry in its Terms and Definitions, and the formal
definition lives in OGC API - Common Part 2, still a draft. What IS normative
and adopted is the name: Requirement 20 fixes /collections/{collectionId}/items
and Requirement 18 defines collectionId as each id in the collections response.
NEWS and the deprecation message now cite that, which a reviewer can check.
The get_samples entry claimed 'service' there was correct. The Samples OpenAPI
document disproves it -- results/locations/activities/projects/organizations
are declared as tags, the document titles itself the Resource Center, and
'service' appears 46 times without once naming them. They are not collections
either: they share 22 of 23 query parameters, so they are five projections of
one query, and OGC scopes 'collection' to access mechanisms defined by OGC API
standards, which Samples does not implement. Recorded as a deliberate exception
with that evidence rather than mislabelled as correct.
get_codes(code_service=) is recorded as correct: the Samples docs call it a
'code service' in prose and serve it from /codeservice/.
Found by /code-review. base_url was optional and fell back to the ambient
_ogc_base_url, which defaults to the empty string. Omitting it therefore built
a *relative* /collections/{id}/items that planning accepted -- the URL is
well-formed, just not absolute -- and only httpx rejected at send time, as an
UnsupportedProtocol. That is an httpx.TransportError, so the fan-out wraps it
as a NetworkError, potentially reading 'could not reach the service at unknown
service'. The comment on the default promised it 'fails loudly on the malformed
URL'; it fails late, and says the wrong thing.
The parameter's own docstring already said 'Required in practice', so this
makes it required in fact. Both permitted consumers already pass it -- ngwmn
and the waterdata wrapper -- and ADR 0003 admits no others, so nothing changes
behaviourally; mypy now catches omission at the call site instead of httpx
catching it mid-flight.
The empty default stays. A real endpoint there would silently send an adapter
that forgot to set it to whichever API happened to be the default, which is the
worse failure. The comment no longer claims the default is a good error, and
says the required argument is the guard.
Not a regression from this branch: the empty default arrived in c298fe7, on
main. Folded in here because this branch already touches these two files.
780 tests pass, mypy clean across 58 files, all gates pass.
Four cleanup agents; efficiency clean, the other three found real work. The rename stopped short. Eleven typed getters in metadata, time_series and measurements still assigned service = "<collection-name>" as the local feeding _get_args(locals()), so the package spoke both vocabularies at one seam and CONTEXT.md's claim that 'the internals now say collection' was overstated again. They say collection now, and the glossary says what is actually true. The two id-switch helpers kept a service_id local through the same rename. get_cql hand-rolled a deprecated-keyword shim that waterdata.utils already provides as _accept_legacy_kwargs, used twice in samples.py -- the exact rule this branch added to CONTRIBUTING, broken in the same branch. Reusing it is strictly better: passing both service= and collection= is now a TypeError rather than silently dropping service, collection and cql go back to being required so the public_api_test baseline this branch had to loosen is restored, and roughly twenty lines of shim and justifying comment go away. The decorator gained an optional detail= so the OGC requirement citation and removal date survive; that generalizes the shared helper rather than duplicating it for one sentence. Requiring base_url on get_ogc_data left the same trap next door: schema.py's _check_ogc_requests and queryables_frame still defaulted to None and fell back to the empty ambient. All three callers pass it explicitly, so it is required there too. shaping._finalize_ogc keeps its fallback -- get_cql genuinely relies on the ambient it enters. Also trims two pieces of duplicated prose: the relative-URL failure was explained twice near-verbatim, and FanOutPlan restated CONTEXT.md's Plan definition immediately after citing it. Skipped: prepare_request_args keying to_exclude on the caller's local variable names is a fair design criticism, but 'service' is genuinely load-bearing there for get_samples and changing the contract is well outside this diff. 780 tests, mypy clean across 58 files, all gates 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
1 participant
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.
Adds
CONTEXT.md, the package glossary, and maps the code, docs, and ADRs onto it. Five commits, each one concern.The term that needed deciding
ADR 0008 defined chunk narrowly as a byte-driven slice and introduced sub-request for the general unit. Two words for one idea — which made
completed_chunks/total_chunks/set_chunks()read as violations of the package's own vocabulary, and left a rename deferred as "~30 assertions of churn for cosmetics".CONTEXT.mdtakes Dask's sense instead — chunks describe how an array is split into sub-arrays:sub-request becomes redundant and is dropped. The chunk-counting names become correct, and the deferred rename is unnecessary.
Three boundaries are recorded because each is a mistake someone would otherwise make:
ChunkPlan's passthroughtotal == 1.parallel_chunks()produces chunks on a query that already fits.Commits
7dc254a0CONTEXT.md— the glossary11b6419d50a630f7iter_sub_args→iter_chunk_args,sub_args→chunk_argsc05b29f9_Item/_ItemCo→_Chunk/_ChunkCosubrequestunhyphenated in the architecture overview and ADR 0004What the glossary caught
ADR 0008 justified the
FanOutInterruptedrename on the grounds that "Water Use raises it without chunking anything." Under the new definition Water Use chunks one request per location, so that reasoning was false while the rename remained right. The ADR now gives the reason that survives: what fails is the execution, not the division. Both services chunk; they chunk for different reasons.Nothing automated could have found that — it is a sentence that stopped being true when a word changed meaning.
The final commit found the same thing in a different register: the term was drifting in spelling as well as meaning.
subrequestwithout the hyphen sat in the architecture overview and ADR 0004, invisible to every search pattern used earlier.Verification
sub-request,sub_args,subrequest,sub requestall return zero acrossdataretrieval/,tests/,docs/source/,CONTEXT.md,CONTRIBUTING.mdmypyclean across 58 source files;lint-imports,xenon,complexipy, and 14 pre-commit hooks passOne surface change
The user-visible interruption message now counts "chunks" rather than "sub-requests", matching the
completed_chunks/total_chunksattributes it was already reporting. Anyone matching on the literal string "sub-requests" in an interruption message would break — almost certainly nobody, but it is a real surface change rather than pure documentation.🤖 Generated with Claude Code
https://claude.ai/code/session_01DSMyMqQ4uuQ9TAbJLnJ6Yi