Repository navigation
refactor(ogc): one entry into the engine; retire fetch_ogc_request - #368
Conversation
The base URL, dialect, and row cap traveled by three mechanisms at once: partial-bound finalizer arguments, ambient ContextVars in ogc.context, and .get() fallbacks deep in request construction and shaping. No interface stated that they were required, and the service-neutral FanOut executor snapshotted a whole execution context at construction solely to keep that adapter state alive across a resume fired after the originating with-blocks had exited. Bind them the way the finalizer was already bound instead: - _construct_api_requests / _construct_cql_request take base_url (and dialect) as explicit parameters; get_ogc_data binds them with functools.partial into both the plan's build_request and the fetch, so planning-time sizing and a later exc.call.resume() rebuild every chunk against the values the call was created with. - row_cap is a real parameter of _paginate/_walk_pages, forwarded to the transport paginate parameter that already existed, instead of being laundered through an ambient read inside a wrapper. - _finalize_ogc / _deal_with_empty require base_url; the empty-frame schema lookup can no longer silently target the ambient default. - FanOut drops the copy_context() snapshot and _resume_in_context: transport carries no adapter state (ADR 0006), and the progress reporter reaches the worker through the portal's normal calling-context copy. - waterdata.get_cql states its target explicitly instead of entering a private ambient scope. - ogc/context.py is deleted. The captured-context regression test becomes a creation-time-binding test; the request-construction unit tests bind the Water Data target with the same partial pattern the engine uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fetch_ogc_request's interface could not state its own precondition: it took no base_url, so request construction inside it targeted whatever was ambient -- and its only production caller, waterdata.get_cql, imported the private ambient to make it usable, then re-implemented get_ogc_data's body by hand across four OGC internals (the properties id-switch, request construction, execution, finalization). Give get_ogc_data one extra entry shape instead: a verbatim ``cql_body``. The engine's existing prologue handles the id-switch and finalizer; the body is driven as a one-item FanOut with the same retry, progress, and interruption semantics as the chunked path -- and the same finalizer, so a resumed get_cql now returns the finished (df, BaseMetadata) shape rather than a raw (frame, response) pair (previously the finalizer was applied outside the executor and a resume skipped it). - ogc facade shrinks to three names, each usable through the facade alone; fetch_ogc_request is deleted. - waterdata.cql consumes only the facade (get_ogc_data + prepare_request_args), dropping four private cross-package imports; a new import-linter contract pins that, mirroring the NGWMN one. - The planner's cql-json passthrough semantics are preserved: a verbatim body's size stays the server's judgement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Health metrics vs
|
main (f31b4730) |
this PR (8d2eb59e) |
|
|---|---|---|
| Modules | 56 | 56 |
| Resolved dependency edges | 135 | 132 |
| Max dependency depth | 8 | 8 |
| Technical-debt estimate | 160 h | 160 h |
| Functions | 361 | 359 |
| High-risk entries | 19 | 20 |
| Package LOC (non-blank) | 13,958 | 13,936 |
Architecture 87 → 82 — new, and it arrived with the /simplify commit
This is the one number that genuinely moved, and it is worth being precise about, because it is not what it looks like.
pyscn reports 26 architecture violations on both sides — the same 26 modules, with byte-identical descriptions. What changes is the severity of exactly two:
responsibility dataretrieval.transport.retry warning -> error
"mixes 10 dependency concerns with fan-in 6 and fan-out 5" (identical text both sides)
responsibility dataretrieval.waterdata.utils warning -> error
"mixes 9 dependency concerns with fan-in 6 and fan-out 5" (identical text both sides)
Weighted violations 26 → 34; compliance 86.73% → 82.38%. Measured across three trees:
| compliance | banded | |
|---|---|---|
main (f31b4730) |
86.73% | 87 |
PR before /simplify (3fcb0242) |
86.67% | 87 |
PR head (8d2eb59e) |
82.38% | 82 |
I isolated the cause: checking out 8d2eb59e and reverting only waterdata/cql.py to its pre-/simplify form restores 87, with every other /simplify edit still in place.
The trigger is that cql.py no longer imports dataretrieval.ogc — it now reaches the engine through waterdata.utils.get_ogc_data, like every other Water Data getter. That is the change ADR 0007 asks for, and it is what lets the facade-only import contract hold. Removing that edge takes the graph from 135 to 132 resolved edges, and the two modules above tip across a severity band whose own inputs — their fan-in, fan-out, and concern counts — did not change.
I did not reverse-engineer pyscn's severity formula, so I'll claim only what I measured: neither module gained a dependent or a dependency, their violation text is identical on both sides, and the flip is reproducibly caused by removing an edge elsewhere in the graph. That reads as a banding artifact of a smaller dependency graph, not as new coupling on transport.retry or waterdata.utils.
If the banded number matters more than the direction of the change, the way back to 87 is to restore the cql.py -> dataretrieval.ogc import — but that reintroduces exactly the duplication the /simplify pass removed (base_url, dialect, extra_id_cols, and the output_id default restated at the call site). My recommendation is to accept the 82.
High-risk entries 19 → 20
Still not a new complex function — it is ogc/engine.py's module scope crossing pyscn's cognitive-complexity-25 line:
ogc/engine.py::<module> cyclomatic 2 -> 2 cognitive 24 -> 26 risk: low -> high
The /simplify pass shaved this from 27 to 26 by replacing the _fetch_cql closure with a functools.partial; it is now one point over the threshold.
18 of the 19 entries already flagged on main are <module> scopes (ogc/planning.py 140, nwis.py 119, nldi.py 88, ogc/requests.py 83, progress.py 82, transport/fanout.py 80, …), so at module scope this metric tracks "how much is in the file", not how gnarly any function is.
Per-function on the same change:
get_ogc_datacyclomatic 5 → 6, cognitive 2 → 3fetch_ogc_requestand its closure: removed- no new function at all —
/simplifyreplaced the_fetch_cqlclosure withfunctools.partial(_walk_pages, GEOPANDAS, row_cap=max_rows)
complexipy — the per-function cognitive gate CI actually enforces, also at 25 — passes: the highest function in engine.py is _next_req_url at 8, unchanged by this PR, and get_ogc_data is 5.
Splitting the CQL branch into a private _fetch_cql_body helper would move engine.py's module aggregate back under pyscn's line. My read is still that it is not worth it — the aggregate would sit just under a threshold it is already only one point over, and the branch reads better where it is, next to the finalizer it shares.
Elsewhere
- Maintainability index:
ogc/engine.py61.92 → 63.39 (up — deletingfetch_ogc_requestmore than pays for the added branch).ogc/requests.py(49.62) andogc/policy.py(57.20) are unchanged now that refactor(ogc): close over request state, retiring ogc.context #367 is inmainand no longer part of this diff.waterdata/cql.py84.09 → 75.65 is the one regression; it stays rank A, and MI swings hard on a file this small — it shed ~18 lines while keeping the same two functions. - Duplication: 8.8968% on both sides, with clone groups (5), pairs (50), and total clones (25) identical. The denominator effect the earlier version of this comment described is gone now that
maincontains refactor(ogc): close over request state, retiring ogc.context #367. - Import contracts: 8 → 7, back to
main's count —/simplifymerged the newcql-facadecontract intoogc-facadeas a secondsource_modulesentry instead of keeping a near-verbatim copy, and updated the ADR 0003 / ADR 0007 compliance paragraphs that described only the NGWMN half of that seam.
Merge gates (xenon --max-absolute C --max-modules B --max-average A, complexipy, lint-imports, mypy --strict, 781 tests): pass on both sides.
Route get_cql through Water Data's own wrapper rather than the OGC facade. waterdata.utils.get_ogc_data exists to bind base_url, dialect, and extra_id_cols and to default output_id from the collection map; get_cql was restating all four, making it the only Water Data getter reaching past the wrapper. It gains a cql_body passthrough instead, so the Water Data binding stays stated once. Also in the verbatim-CQL branch: - Bind the fetcher with functools.partial, the idiom the comment three lines below already names, instead of a nested closure. - Drop `or None` from the properties lookup: _switch_properties_id returns [] and never None, and _ogc_query_params guards with `if properties:`, so both spellings build the same request. - Trim the cql_body docstring to the load-bearing contract, dropping the mechanism narration the inline comment below already carries. Merge the new cql-facade import contract into ogc-facade, which it duplicated except for source_modules, and update the two ADR compliance paragraphs that still described only the NGWMN half of the seam. The branch itself stays: an over-budget body with no args["filter"] would raise Unchunkable client-side if folded into the planner, rather than letting the server judge it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Absorbs DOI-USGS#368, DOI-USGS#369, and DOI-USGS#371. Eight files conflicted; three of the resolutions were more than textual: - ``wateruse.py``: main edited the implementation this branch had already renamed to ``nwdc.py``, so git conflicted the shim against it. Kept the shim and ported DOI-USGS#371's ``run_paginated`` migration into ``nwdc.py``, preserving ``adapter="nwdc"``. - ``cql.py``: took main's version wholesale. This branch's two changes there (``redirected(OGC_API_URL)`` and ``adapter="waterdata"``) are subsumed by DOI-USGS#368 routing ``get_cql`` through ``waterdata.utils.get_ogc_data``, which already applies both. - ``ratings.py``: took main's rewritten implementation and re-applied this branch's only contribution to it (``redirected(STAC_URL)``), plus adapter scoping on both drives. Three breaks were semantic, not textual -- git merged them cleanly and they would have failed at import or call time, because main added callers of names this branch renamed or deleted: - ``transport/pagination.run_paginated`` imported ``_CONCURRENCY_DEFAULT`` (deleted here in favour of ``configuration.DEFAULT_CONCURRENCY``) and called ``RetryPolicy.from_env`` (renamed to ``from_configuration``). It now takes an ``adapter`` argument and threads it to both the retry policy and the executor, which is what makes per-adapter ``retries`` and ``concurrency`` tables reach the three getters that use it. - ``ogc/engine``'s ``cql_body`` branch (new in DOI-USGS#368) called ``from_env`` and dropped the adapter; both fixed. - ``get_ogc_data`` gained ``cql_body`` from main and ``adapter`` here; both parameters kept. 969 passed, mypy --strict clean, all hooks including import-linter pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BTaSm7HmVb94RSJiKW4WAS
1 participant
What
Retires
fetch_ogc_requestand givesget_ogc_dataone extra entry shape — a verbatimcql_body— so the OGC facade has a single orchestrated entry that is fully usable through the facade alone.Why
fetch_ogc_request's interface could not state its own precondition: it took nobase_url, so request construction inside it targeted whatever was ambient. Its only production caller,waterdata.get_cql, had to import the private ambient to make it usable, and then re-implementedget_ogc_data's body by hand across four OGC internals — the properties id-switch, request construction, execution, and finalization. The deletion test pointed one way: folding that orchestration into the engine concentrates it where id-switching, base-URL handling, and finalization already live.How
get_ogc_data(..., cql_body=...)drives the prepared POST as a one-itemFanOut(ADR 0008's plan shape) with the same retry, progress-line, and interruption semantics as the chunked path — and the same finalizer, so a resumedget_cqlnow returns the finished(df, BaseMetadata)shape; previously the finalizer was applied outside the executor andexc.call.resume()skipped it, returning a raw(frame, response)pair. A new test pins this.waterdata.cqlconsumes only the facade (get_ogc_data+prepare_request_args), dropping its four private cross-package imports; a new import-linter contract (cql-facade) pins that, mirroring the NGWMN facade contract.ogcfacade shrinks from four names to three;fetch_ogc_requestis deleted (it was internal — no public-surface change).Testing
mypy --strict,ruff,xenon,complexipy, andlint-imports(8 contracts, including the new one) all pass.🤖 Generated with Claude Code