From 375c4892d59a57c36b38a80ce536341862ff68d1 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 09:15:58 -0500 Subject: [PATCH 1/7] ci(health): report the signals that apply, demote the composite The weekly report led with a number whose remaining gap is this package's API surface. pyscn grades six sub-scores and four of them do not describe this codebase: Complexity counts synthetic per-file rows that are not functions, CBO and LCOM sit pinned at 100 in a package that is deliberately function-oriented, and Architecture penalises a leaf for being depended upon -- extracting BaseMetadata to a dependency-free leaf, an unambiguous improvement, lowered it. Averaging all six produces a composite that moves for reasons unrelated to the work. Three signals do apply, and they are now the headline. Dead code, which nothing else in the stack checks. Dependency depth, which .importlinter enforces the direction of but not the length of. And a NEW clone group -- the event actually worth catching, and until now invisible: five accepted getter families dominate the duplication score, so a sixth group moves it about two points and nobody looks. .pyscn-known-clones.json was committed for exactly this and nothing read it; tools/health_signals.py now diffs against it and names the offending functions. Verified by dropping a group from the baseline and watching the run report it. Added alongside them: the count of public functions carrying required arguments. It is the closest automatable proxy for interface cost, and measures something no other check does -- every Water Data getter is currently fully keyword-optional, which is what makes a 157-parameter module deep rather than shallow. A getter growing its first required argument is a real event. The full pyscn scores stay, in a collapsed block with a note on why the composite is not the headline, and in the artifact for trend. No .pyscn.toml: tuning similarity_threshold to 0.86 does take duplication to 100 and the composite to 87, but that value is fitted just above the five known groups and globally blinds the tool to any future clone below it. The number would improve because the tool stopped looking. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DSMyMqQ4uuQ9TAbJLnJ6Yi --- .github/workflows/code-health.yml | 25 +++++- tools/health_signals.py | 121 ++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tools/health_signals.py diff --git a/.github/workflows/code-health.yml b/.github/workflows/code-health.yml index c04b9b59..11ff851c 100644 --- a/.github/workflows/code-health.yml +++ b/.github/workflows/code-health.yml @@ -75,6 +75,16 @@ jobs: PY cat pyscn-sanity.txt + - name: Signals that mean something for this package + # pyscn grades six sub-scores and four of them do not apply here, so the + # composite moves for reasons that are not about this codebase. This + # extracts the three that do -- dead code, a NEW clone group, dependency + # depth -- and adds the interface cost of the public surface, which + # nothing else measures. See tools/health_signals.py for why each. + if: always() + continue-on-error: true + run: python tools/health_signals.py > health-signals.txt && cat health-signals.txt + - name: Maintainability ranking (wily) # Worst-maintained files today, and how the package has moved recently. run: | @@ -92,7 +102,17 @@ jobs: if: always() run: | { - echo '## Structural analysis' + echo '## Signals' + echo '```' + cat health-signals.txt 2>/dev/null || echo 'signals unavailable' + echo '```' + echo + echo '
Full pyscn scores (advisory)' + echo + echo 'The composite averages six sub-scores; four do not apply to a' + echo 'function-oriented package, and Architecture penalises a leaf' + echo 'for being depended upon. Read the signals above instead.' + echo echo '```' if [[ -s pyscn-summary.txt ]]; then cat pyscn-summary.txt @@ -101,6 +121,8 @@ jobs: fi cat pyscn-sanity.txt 2>/dev/null || true echo '```' + echo '
' + echo cat wily-summary.txt 2>/dev/null || true echo echo 'Full reports are attached to this run as the' @@ -118,6 +140,7 @@ jobs: .pyscn/reports/ pyscn-summary.txt pyscn-sanity.txt + health-signals.txt wily-summary.txt retention-days: 90 if-no-files-found: warn diff --git a/tools/health_signals.py b/tools/health_signals.py new file mode 100644 index 00000000..26fed179 --- /dev/null +++ b/tools/health_signals.py @@ -0,0 +1,121 @@ +"""Report the health signals that mean something for this package. + +pyscn grades six sub-scores; four of them do not apply here. Complexity counts +synthetic per-file rows, CBO and LCOM stay pinned at 100 in a function-oriented +package, and Architecture penalises a leaf for being depended upon -- extracting +BaseMetadata to a dependency-free leaf lowered it. The composite averages all +six, so it moves for reasons that are not about this codebase. + +What is left is genuinely useful, and is what this prints: dead code, a NEW +clone group appearing, and the dependency depth. Plus the interface cost of the +public getters, which nothing else measures and which is the closest automatable +proxy for whether a module is deep. +""" + +import ast +import glob +import json +import sys +from pathlib import Path + +BASELINE = Path(".pyscn-known-clones.json") +PACKAGE = Path("dataretrieval") + + +_SPANS: dict[str, dict[int, str]] = {} + + +def enclosing(path, start): + if path not in _SPANS: + _SPANS[path] = { + n.lineno: n.name + for n in ast.walk(ast.parse(Path(path).read_text(encoding="utf-8"))) + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) + } + for line in range(start, start + 3): + if line in _SPANS[path]: + return _SPANS[path][line] + return f"" + + +def clone_groups(report): + groups = [] + for g in report["clone"]["clone_groups"]: + groups.append( + frozenset( + f"{c['location']['file_path']}::" + f"{enclosing(c['location']['file_path'], c['location']['start_line'])}" + for c in g["clones"] + ) + ) + return groups + + +def required_arguments(): + """Public functions carrying required arguments, worst module first.""" + out = [] + for path in sorted(PACKAGE.rglob("*.py")): + tree = ast.parse(path.read_text(encoding="utf-8")) + declared = None + for node in tree.body: + if isinstance(node, ast.Assign) and any( + isinstance(t, ast.Name) and t.id == "__all__" for t in node.targets + ): + declared = set(ast.literal_eval(node.value)) + required = 0 + for node in tree.body: + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + public = ( + node.name in declared + if declared is not None + else not node.name.startswith("_") + ) + if not public: + continue + positional = node.args.posonlyargs + node.args.args + required += max(0, len(positional) - len(node.args.defaults)) + if required: + out.append((str(path), required)) + return sorted(out, key=lambda r: -r[1]) + + +reports = sorted(glob.glob(".pyscn/reports/*.json")) +if not reports: + print("no pyscn JSON report found") + sys.exit(0) +report = json.loads(Path(reports[-1]).read_text()) + +dead = report["dead_code"]["summary"]["total_findings"] +deps = report["system"]["DependencyAnalysis"] +print(f"dead code {dead} findings") +print( + f"dependency depth {deps['MaxDepth']} (max path), " + f"{deps['TotalDependencies']} edges" +) + +current = clone_groups(report) +if BASELINE.exists(): + baseline = json.loads(BASELINE.read_text())["groups"] + known = {frozenset(g["members"]) for g in baseline} + new = [g for g in current if g not in known] + gone = [g for g in known if g not in current] + print( + f"clone groups {len(current)} total, " + f"{len(known) - len(gone)} known, {len(new)} NEW" + ) + for g in new: + print(" NEW GROUP -- not in .pyscn-known-clones.json:") + for member in sorted(g): + print(f" {member}") + for g in gone: + print(f" resolved (baseline is stale by one group): {sorted(g)[0]} ...") +else: + print(f"clone groups {len(current)} (no baseline file to compare against)") + +print("\ninterface cost -- public functions with required arguments:") +rows = required_arguments() +if not rows: + print(" none: every public function is fully keyword-optional") +for path, count in rows[:6]: + print(f" {path:<44} {count} required") From 5234ddc3962c46dc2f7e2ba2928ccb17af4d617f Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 09:22:22 -0500 Subject: [PATCH 2/7] refactor(ogc): make an OGC API one value, not three arguments base_url, dialect and extra_id_cols always travel together and are constant for a service, but were threaded separately through get_ogc_data and _finalize_ogc. OgcApi bundles them, so an adapter declares its API once -- WATERDATA_API, NGWMN_API -- and the engine and the shaper take one value. The invariant was already leaking. get_cql passed extra_id_cols and dialect but not base_url, relying on the ambient it happened to have entered; the value it fell back to was right by luck rather than by construction. Passing two of three is now unrepresentable. Honest about the measurement: the interface-cost signal this PR added barely moves -- ogc/engine.py drops two optional parameters, the package total 581 to 579. That is because _finalize_ogc is private, so the metric does not see it, and the metric therefore under-reports depth won at an internal seam. Worth knowing about the signal as much as about this change: it measures the public surface, which is where interface cost is paid by users, not where it is paid by maintainers. The gain that is not in the number: a third OGC service is now one object rather than three arguments to thread through two layers and keep consistent, and the partial-application bug above cannot recur. 780 tests, mypy clean across 58 files, 7/7 contracts, all gates. --- dataretrieval/ngwmn.py | 9 ++++++--- dataretrieval/ogc/__init__.py | 4 +++- dataretrieval/ogc/engine.py | 14 ++++---------- dataretrieval/ogc/policy.py | 30 ++++++++++++++++++++++++++++++ dataretrieval/ogc/shaping.py | 22 +++++++++------------- dataretrieval/waterdata/cql.py | 6 ++---- dataretrieval/waterdata/utils.py | 16 ++++++++++++---- tests/waterdata_utils_test.py | 7 ++----- 8 files changed, 68 insertions(+), 40 deletions(-) diff --git a/dataretrieval/ngwmn.py b/dataretrieval/ngwmn.py index 154899fa..2aeb1664 100644 --- a/dataretrieval/ngwmn.py +++ b/dataretrieval/ngwmn.py @@ -24,7 +24,7 @@ from dataretrieval.codes.states import apply_state from dataretrieval.credentials import WATERDATA_BASE_URL -from dataretrieval.ogc import OgcDialect, get_ogc_data, prepare_request_args +from dataretrieval.ogc import OgcApi, OgcDialect, get_ogc_data, prepare_request_args if TYPE_CHECKING: from dataretrieval._response_metadata import BaseMetadata @@ -89,6 +89,10 @@ sort_cols=("sample_time", "monitoring_location_id"), ) +#: The NGWMN OGC API. It carries no synthetic id columns, so only the base URL +#: and the dialect differ from the default. +NGWMN_API = OgcApi(base_url=NGWMN_OGC_API_URL, dialect=NGWMN_DIALECT) + def _get(service: str, local_vars: dict[str, Any]) -> tuple[pd.DataFrame, BaseMetadata]: """Marshal a getter's arguments and dispatch to the shared OGC facade. @@ -104,8 +108,7 @@ def _get(service: str, local_vars: dict[str, Any]) -> tuple[pd.DataFrame, BaseMe args, service, output_id=_NGWMN_OUTPUT_ID, - base_url=NGWMN_OGC_API_URL, - dialect=NGWMN_DIALECT, + api=NGWMN_API, ) diff --git a/dataretrieval/ogc/__init__.py b/dataretrieval/ogc/__init__.py index 4a091e32..67cdac1d 100644 --- a/dataretrieval/ogc/__init__.py +++ b/dataretrieval/ogc/__init__.py @@ -2,6 +2,7 @@ The public facade exposes only the minimal collection-adapter seam: +- :class:`OgcApi` — one OGC API's identity: base URL, dialect, id columns. - :class:`OgcDialect` — per-API request/response quirks. - :func:`prepare_request_args` — normalize caller kwargs for the engine. - :func:`get_ogc_data` — full orchestrated OGC fetch (chunking + pagination). @@ -14,10 +15,11 @@ """ from dataretrieval.ogc.engine import fetch_ogc_request, get_ogc_data -from dataretrieval.ogc.policy import OgcDialect +from dataretrieval.ogc.policy import OgcApi, OgcDialect from dataretrieval.ogc.requests import prepare_request_args __all__ = [ + "OgcApi", "OgcDialect", "fetch_ogc_request", "get_ogc_data", diff --git a/dataretrieval/ogc/engine.py b/dataretrieval/ogc/engine.py index 6b9e4a3c..1c7f4689 100644 --- a/dataretrieval/ogc/engine.py +++ b/dataretrieval/ogc/engine.py @@ -40,7 +40,7 @@ from dataretrieval.ogc.errors import _raise_for_non_200 from dataretrieval.ogc.policy import ( DEFAULT_DIALECT, - OgcDialect, + OgcApi, _require_positive_int, ) @@ -219,10 +219,8 @@ def get_ogc_data( collection: str, output_id: str, *, - base_url: str, + api: OgcApi, max_rows: int | None = None, - extra_id_cols: frozenset[str] | set[str] = frozenset(), - dialect: OgcDialect | None = None, ) -> tuple[pd.DataFrame, BaseMetadata]: """ Retrieves OGC (Open Geospatial Consortium) data as a DataFrame with metadata. @@ -283,8 +281,6 @@ def get_ogc_data( if max_rows is not None: _require_positive_int(max_rows, "max_rows") - if dialect is None: - dialect = _DEFAULT_DIALECT args = args.copy() args["collection"] = collection args = _switch_arg_id(args, id_name=output_id, collection=collection) @@ -311,13 +307,11 @@ def get_ogc_data( convert_type=convert_type, collection=collection, max_rows=max_rows, - extra_id_cols=extra_id_cols, - dialect=dialect, - base_url=base_url, + api=api, ) # No progress block here: the executor that emits the events owns the line # (see :meth:`~dataretrieval.transport.fanout.FanOut.resume`). - with _row_cap(max_rows), _ogc_base_url(base_url), _dialect(dialect): + with _row_cap(max_rows), _ogc_base_url(api.base_url), _dialect(api.dialect): return _fetch_once(args, finalize=finalize) diff --git a/dataretrieval/ogc/policy.py b/dataretrieval/ogc/policy.py index 99498ec1..beb23c48 100644 --- a/dataretrieval/ogc/policy.py +++ b/dataretrieval/ogc/policy.py @@ -73,3 +73,33 @@ class OgcDialect: # Default dialect: a plain OGC API with no CQL2-only collections and no # date-only collections (every time argument rendered as a full UTC datetime). DEFAULT_DIALECT = OgcDialect() + + +@dataclass(frozen=True) +class OgcApi: + """One OGC API's identity: where it lives and how it answers. + + These three facts always travel together and are constant for a service, + so they are one value rather than three arguments threaded through the + engine and the shaper. An adapter declares its API once + (``waterdata.utils.WATERDATA_API``, ``ngwmn.NGWMN_API``) and passes that. + + Adding a third OGC service is then one object, not three arguments to + thread through two layers and remember to keep consistent -- the mistake + the previous shape invited, and made: ``get_cql`` passed the dialect and + the id columns but let the base URL fall back to a default that happened + to be right. + + Attributes + ---------- + base_url : str + Root the collections hang off, e.g. ``.../ogcapi/v0``. + dialect : OgcDialect + Per-API quirks the request builder and shaper need. + extra_id_cols : frozenset[str] + Synthetic id columns this API's results carry, ordered to the front. + """ + + base_url: str + dialect: OgcDialect = DEFAULT_DIALECT + extra_id_cols: frozenset[str] = frozenset() diff --git a/dataretrieval/ogc/shaping.py b/dataretrieval/ogc/shaping.py index e9d26b32..68b58806 100644 --- a/dataretrieval/ogc/shaping.py +++ b/dataretrieval/ogc/shaping.py @@ -12,14 +12,16 @@ import logging import re -from typing import Any +from typing import TYPE_CHECKING, Any import httpx import pandas as pd +if TYPE_CHECKING: + from dataretrieval.ogc.policy import OgcApi, OgcDialect + from dataretrieval._response_metadata import BaseMetadata from dataretrieval.ogc.context import _ogc_base_url -from dataretrieval.ogc.policy import DEFAULT_DIALECT, OgcDialect try: import geopandas as gpd @@ -364,10 +366,8 @@ def _finalize_ogc( output_id: str, convert_type: bool, collection: str, + api: OgcApi, max_rows: int | None = None, - extra_id_cols: frozenset[str] | set[str] = frozenset(), - dialect: OgcDialect | None = None, - base_url: str | None = None, ) -> tuple[pd.DataFrame, BaseMetadata]: """Shape a combined OGC result into the user-facing ``(df, md)``. @@ -390,11 +390,7 @@ def _finalize_ogc( ``base_url`` is captured with the finalizer so resumed calls query the same API's schema when their combined result is empty. """ - if dialect is None: - dialect = DEFAULT_DIALECT - if base_url is None: - base_url = _ogc_base_url.get() - frame = _deal_with_empty(frame, properties, collection, base_url=base_url) + frame = _deal_with_empty(frame, properties, collection, base_url=api.base_url) # Normalize to PEP-8 snake_case column names *first*, so the dialect's # ``time_cols``/``numerical_cols``/``sort_cols`` (all snake_case) match # regardless of whether the API returns snake_case (Water Data, where @@ -408,9 +404,9 @@ def _finalize_ogc( if renames: frame = frame.rename(columns=renames) if convert_type: - frame = _type_cols(frame, dialect) - frame = _arrange_cols(frame, properties, output_id, extra_id_cols) - frame = _sort_rows(frame, dialect) + frame = _type_cols(frame, api.dialect) + frame = _arrange_cols(frame, properties, output_id, api.extra_id_cols) + frame = _sort_rows(frame, api.dialect) if max_rows is not None: frame = frame.head(max_rows) return frame, BaseMetadata(response) diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 5139b071..205c9af1 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -23,10 +23,9 @@ ) from dataretrieval.ogc.shaping import _finalize_ogc from dataretrieval.waterdata.utils import ( - _EXTRA_ID_COLS, _OUTPUT_ID_BY_COLLECTION, OGC_API_URL, - WATERDATA_DIALECT, + WATERDATA_API, _accept_legacy_kwargs, ) @@ -186,8 +185,7 @@ def get_cql( output_id=output_id, convert_type=convert_type, collection=collection, - extra_id_cols=_EXTRA_ID_COLS, - dialect=WATERDATA_DIALECT, + api=WATERDATA_API, ) diff --git a/dataretrieval/waterdata/utils.py b/dataretrieval/waterdata/utils.py index c24624d0..7be47a90 100644 --- a/dataretrieval/waterdata/utils.py +++ b/dataretrieval/waterdata/utils.py @@ -24,7 +24,7 @@ import pandas as pd from dataretrieval.codes.states import apply_state -from dataretrieval.ogc import OgcDialect, prepare_request_args +from dataretrieval.ogc import OgcApi, OgcDialect, prepare_request_args from dataretrieval.ogc import get_ogc_data as _facade_get_ogc_data # Endpoint constants live in one place for the whole collection; they are re-bound @@ -96,6 +96,15 @@ sort_cols=("time", "monitoring_location_id"), ) +#: This package's view of the Water Data OGC API: where it lives, how it +#: answers, and the synthetic id columns its results carry. Declared once so +#: the engine and the shaper take one value instead of three arguments. +WATERDATA_API = OgcApi( + base_url=OGC_API_URL, + dialect=WATERDATA_DIALECT, + extra_id_cols=_EXTRA_ID_COLS, +) + # The Water-Data-specific *extras* on top of the engine's own no-normalize set # (which already covers the date-range params and ``bbox``). Scalar non-string # knobs are caught by runtime type, so only iterables with special handling @@ -216,9 +225,7 @@ def get_ogc_data( collection, output_id, max_rows=max_rows, - base_url=OGC_API_URL, - extra_id_cols=_EXTRA_ID_COLS, - dialect=WATERDATA_DIALECT, + api=WATERDATA_API, ) @@ -291,6 +298,7 @@ def wrapper(*args: Any, **kwargs: Any) -> _R: "BASE_URL", "OGC_API_URL", "SAMPLES_URL", + "WATERDATA_API", "WATERDATA_DIALECT", "_EXTRA_ID_COLS", "_NO_NORMALIZE_PARAMS", diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index e805ec92..4b46c711 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -41,17 +41,14 @@ from dataretrieval.waterdata import get_stats_date_range, get_stats_por from dataretrieval.waterdata.stats import _handle_nesting, get_data from dataretrieval.waterdata.utils import ( - _EXTRA_ID_COLS, OGC_API_URL, - WATERDATA_DIALECT, + WATERDATA_API, _get_args, ) # The Water Data injection ``get_cql`` performs at its call site, so these tests # exercise the same result shape the typed getters produce. -_finalize_ogc = functools.partial( - _ogc_finalize, extra_id_cols=_EXTRA_ID_COLS, dialect=WATERDATA_DIALECT -) +_finalize_ogc = functools.partial(_ogc_finalize, api=WATERDATA_API) _LOGGER_NAME = _utils_module.__name__ From 21fc4c0074fcde0c3ace982579637ffd7777bc72 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 09:33:21 -0500 Subject: [PATCH 3/7] refactor(ogc): let the API answer what its own id column is called output_id is a fact about an API, not something a caller should have to look up -- yet three call sites each resolved it a different way. The Water Data wrapper defaulted it from a module-level map, get_cql imported the same map to look it up *and* validate against it, and NGWMN passed a constant because every one of its collections uses the same name. OgcApi now carries that mapping (or the single name, which is what NGWMN's constant always was) and answers output_id(collection). The engine resolves it when the caller does not, so get_ogc_data's third positional argument becomes optional. reference.py still passes one explicitly, which is the case the map does not cover, and that is now the only reason to pass it. get_cql gains from this twice: it validates against WATERDATA_API.output_ids rather than importing the map separately, so the collection it accepts and the id it renames can no longer disagree. The number moves two points -- engine drops one required and one optional parameter, package totals 70 to 69 and 581 to 580. That undersells it, as the previous commit's note predicts: the win is that a caller cannot pass a collection with the wrong id column, because it no longer passes one at all. 780 tests, mypy clean across 58 files, 7/7 contracts, all gates. --- dataretrieval/ngwmn.py | 7 +++++-- dataretrieval/ogc/engine.py | 4 +++- dataretrieval/ogc/policy.py | 16 ++++++++++++++++ dataretrieval/waterdata/cql.py | 7 +++---- dataretrieval/waterdata/utils.py | 15 ++++++++------- 5 files changed, 35 insertions(+), 14 deletions(-) diff --git a/dataretrieval/ngwmn.py b/dataretrieval/ngwmn.py index 2aeb1664..9b51ba3e 100644 --- a/dataretrieval/ngwmn.py +++ b/dataretrieval/ngwmn.py @@ -91,7 +91,11 @@ #: The NGWMN OGC API. It carries no synthetic id columns, so only the base URL #: and the dialect differ from the default. -NGWMN_API = OgcApi(base_url=NGWMN_OGC_API_URL, dialect=NGWMN_DIALECT) +NGWMN_API = OgcApi( + base_url=NGWMN_OGC_API_URL, + dialect=NGWMN_DIALECT, + output_ids=_NGWMN_OUTPUT_ID, +) def _get(service: str, local_vars: dict[str, Any]) -> tuple[pd.DataFrame, BaseMetadata]: @@ -107,7 +111,6 @@ def _get(service: str, local_vars: dict[str, Any]) -> tuple[pd.DataFrame, BaseMe return get_ogc_data( args, service, - output_id=_NGWMN_OUTPUT_ID, api=NGWMN_API, ) diff --git a/dataretrieval/ogc/engine.py b/dataretrieval/ogc/engine.py index 1c7f4689..951ca731 100644 --- a/dataretrieval/ogc/engine.py +++ b/dataretrieval/ogc/engine.py @@ -217,7 +217,7 @@ async def follow_up(cursor: str, sess: httpx.AsyncClient) -> httpx.Response: def get_ogc_data( args: dict[str, Any], collection: str, - output_id: str, + output_id: str | None = None, *, api: OgcApi, max_rows: int | None = None, @@ -281,6 +281,8 @@ def get_ogc_data( if max_rows is not None: _require_positive_int(max_rows, "max_rows") + if output_id is None: + output_id = api.output_id(collection) args = args.copy() args["collection"] = collection args = _switch_arg_id(args, id_name=output_id, collection=collection) diff --git a/dataretrieval/ogc/policy.py b/dataretrieval/ogc/policy.py index beb23c48..0555f72f 100644 --- a/dataretrieval/ogc/policy.py +++ b/dataretrieval/ogc/policy.py @@ -16,6 +16,7 @@ from __future__ import annotations import numbers +from collections.abc import Mapping from dataclasses import dataclass, field @@ -103,3 +104,18 @@ class OgcApi: base_url: str dialect: OgcDialect = DEFAULT_DIALECT extra_id_cols: frozenset[str] = frozenset() + #: How the wire ``id`` is renamed for the caller: a per-collection mapping, + #: or one name every collection uses. Excluded from the hash because a + #: mapping is unhashable and this value is identity, not a key. + output_ids: Mapping[str, str] | str = "id" + + def output_id(self, collection: str) -> str: + """The user-facing name the wire ``id`` takes for ``collection``. + + The caller should not have to know this -- it is a fact about the API, + so the API answers it. Callers with a collection outside the mapping + (a reference table, say) may still pass one explicitly. + """ + if isinstance(self.output_ids, str): + return self.output_ids + return self.output_ids.get(collection, "id") diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 205c9af1..6626e8ae 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -23,7 +23,6 @@ ) from dataretrieval.ogc.shaping import _finalize_ogc from dataretrieval.waterdata.utils import ( - _OUTPUT_ID_BY_COLLECTION, OGC_API_URL, WATERDATA_API, _accept_legacy_kwargs, @@ -144,12 +143,12 @@ def get_cql( ... ' "02070010%"]}', ... ) """ - if collection not in _OUTPUT_ID_BY_COLLECTION: + if collection not in WATERDATA_API.output_ids: raise ValueError( f"Unknown collection {collection!r}. Valid collections: " - f"{sorted(_OUTPUT_ID_BY_COLLECTION)}." + f"{sorted(WATERDATA_API.output_ids)}." ) - output_id = _OUTPUT_ID_BY_COLLECTION[collection] + output_id = WATERDATA_API.output_id(collection) # ``dict`` is the pythonic input — serialize on the way out. ``str`` is sent # verbatim so callers who already have a CQL2 doc (e.g. imported from a diff --git a/dataretrieval/waterdata/utils.py b/dataretrieval/waterdata/utils.py index 7be47a90..ce2ee5a6 100644 --- a/dataretrieval/waterdata/utils.py +++ b/dataretrieval/waterdata/utils.py @@ -103,6 +103,7 @@ base_url=OGC_API_URL, dialect=WATERDATA_DIALECT, extra_id_cols=_EXTRA_ID_COLS, + output_ids=_OUTPUT_ID_BY_COLLECTION, ) # The Water-Data-specific *extras* on top of the engine's own no-normalize set @@ -189,11 +190,13 @@ def get_ogc_data( ) -> tuple[pd.DataFrame, BaseMetadata]: """Water-Data wrapper over :func:`~dataretrieval.ogc.get_ogc_data`. - Defaults ``output_id`` from the Water Data collection map when not given, - and supplies the Water Data extra-id columns and dialect, so the typed - getters in ``api.py`` call this unchanged. (Sibling OGC APIs such as - NGWMN call ``dataretrieval.ogc.get_ogc_data`` directly with their own - base URL and dialect rather than going through this Water Data wrapper.) + Supplies :data:`WATERDATA_API`, so the typed getters in ``api.py`` call + this unchanged. ``output_id`` is resolved by that value and only needs + passing for a collection outside its map (a reference table, say). + + Sibling OGC APIs such as NGWMN call ``dataretrieval.ogc.get_ogc_data`` + directly with their own :class:`~dataretrieval.ogc.OgcApi` rather than + going through this Water Data wrapper. Parameters ---------- @@ -218,8 +221,6 @@ def get_ogc_data( A metadata object with request information, including the URL and query time. """ - if output_id is None: - output_id = _OUTPUT_ID_BY_COLLECTION[collection] return _facade_get_ogc_data( args, collection, From 129eb219d682b2be3ab7336e64134df5f9004229 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 09:57:22 -0500 Subject: [PATCH 4/7] refactor(waterdata): keep the state queryable with the collection Which queryable a collection filters state on, and in which representation, is a fact about the collection. It was a pair of arguments repeated at five getters: to="name", into="state_name" three times in metadata, to="fips_us", into="state_code" twice in the statistics getters. Nothing tied either pair to the collection it described, so a getter could name the wrong one and no check would notice. ngwmn already had the answer. It holds the same fact as data -- {"sites": {"into": "state_name", "to": "name"}, "providers": {"into": "state", "to": "postal"}} -- and makes one call. Water Data now does the same through _STATE_QUERYABLE, and _with_state takes the collection rather than the answer. The generic mechanism stays where it already was, in the codes.states leaf. Deliberately service-side rather than a field on OgcApi. OgcApi describes the protocol -- base URL, dialect, id columns -- and "which queryable holds the state" is USGS domain vocabulary that would leak into the generic OGC leaf. The mapping also spans both the OGC collections and the Statistics API's resources, which OgcApi does not cover. Beyond the lookup: the two statistics getters bound the collection name to a local instead of repeating a literal at the request call, so the collection whose state rule is applied and the collection actually requested are now the same value rather than two strings that agree by inspection. 780 tests, mypy clean across 58 files, 7/7 contracts, all gates. --- dataretrieval/waterdata/metadata.py | 12 +++-------- dataretrieval/waterdata/time_series.py | 12 +++++++---- dataretrieval/waterdata/utils.py | 29 +++++++++++++++++++++----- tests/waterdata_utils_test.py | 19 ++++++++--------- 4 files changed, 44 insertions(+), 28 deletions(-) diff --git a/dataretrieval/waterdata/metadata.py b/dataretrieval/waterdata/metadata.py index 6ab681e2..880bafa8 100644 --- a/dataretrieval/waterdata/metadata.py +++ b/dataretrieval/waterdata/metadata.py @@ -336,9 +336,7 @@ def get_monitoring_locations( # Build argument dictionary, omitting None values (resolving the unified # `state` argument into the OGC `state_name` queryable). - args = _get_args( - _with_state(locals(), to="name", into="state_name"), exclude={"max_rows"} - ) + args = _get_args(_with_state(locals(), collection), exclude={"max_rows"}) return get_ogc_data(args, collection, max_rows=max_rows) @@ -587,9 +585,7 @@ def get_time_series_metadata( # Build argument dictionary, omitting None values (resolving the unified # `state` argument into the OGC `state_name` queryable). - args = _get_args( - _with_state(locals(), to="name", into="state_name"), exclude={"max_rows"} - ) + args = _get_args(_with_state(locals(), collection), exclude={"max_rows"}) return get_ogc_data(args, collection, max_rows=max_rows) @@ -845,9 +841,7 @@ def get_combined_metadata( collection = "combined-metadata" # Resolve the unified `state` argument into the OGC `state_name` queryable. - args = _get_args( - _with_state(locals(), to="name", into="state_name"), exclude={"max_rows"} - ) + args = _get_args(_with_state(locals(), collection), exclude={"max_rows"}) return get_ogc_data(args, collection, max_rows=max_rows) diff --git a/dataretrieval/waterdata/time_series.py b/dataretrieval/waterdata/time_series.py index f3c7b572..6dd55d71 100644 --- a/dataretrieval/waterdata/time_series.py +++ b/dataretrieval/waterdata/time_series.py @@ -1006,14 +1006,16 @@ def get_stats_por( ... end_date="01-31", ... ) """ + collection = "observationNormals" + # Build argument dictionary, omitting None values params = _get_args( - _with_state(locals(), to="fips_us", into="state_code"), + _with_state(locals(), collection), exclude={"expand_percentiles"}, ) return stats.get_data( - args=params, service="observationNormals", expand_percentiles=expand_percentiles + args=params, service=collection, expand_percentiles=expand_percentiles ) @@ -1152,15 +1154,17 @@ def get_stats_date_range( ... computation_type=["minimum", "maximum"], ... ) """ + collection = "observationIntervals" + # Build argument dictionary, omitting None values params = _get_args( - _with_state(locals(), to="fips_us", into="state_code"), + _with_state(locals(), collection), exclude={"expand_percentiles"}, ) return stats.get_data( args=params, - service="observationIntervals", + service=collection, expand_percentiles=expand_percentiles, ) diff --git a/dataretrieval/waterdata/utils.py b/dataretrieval/waterdata/utils.py index ce2ee5a6..32dcc17a 100644 --- a/dataretrieval/waterdata/utils.py +++ b/dataretrieval/waterdata/utils.py @@ -160,17 +160,33 @@ def _get_args( ) -def _with_state(local_vars: dict[str, Any], *, to: str, into: str) -> dict[str, Any]: - """Resolve the unified ``state`` argument into an endpoint's state queryable. +#: Which state queryable each collection filters on, and in which +#: representation. A fact about the collection, so it lives with the collection +#: rather than being repeated at every getter that filters by state -- the +#: shape ``ngwmn`` already uses for the same problem. Spans both the OGC +#: collections and the Statistics API's resources, which differ here: the OGC +#: metadata collections take a full state name, Statistics takes a FIPS code. +_STATE_QUERYABLE: dict[str, dict[str, str]] = { + "monitoring-locations": {"to": "name", "into": "state_name"}, + "time-series-metadata": {"to": "name", "into": "state_name"}, + "combined-metadata": {"to": "name", "into": "state_name"}, + "observationNormals": {"to": "fips_us", "into": "state_code"}, + "observationIntervals": {"to": "fips_us", "into": "state_code"}, +} + + +def _with_state(local_vars: dict[str, Any], collection: str) -> dict[str, Any]: + """Resolve the unified ``state`` argument into a collection's queryable. Returns the (mutated) args mapping. ``state`` is the canonical, format-flexible parameter (full name / postal / FIPS); it is normalized via - :func:`~dataretrieval.codes.states.to_state` to the ``to`` representation - and stored under ``into`` (the queryable this endpoint actually filters on). + :func:`~dataretrieval.codes.states.to_state` into whichever representation + and queryable ``collection`` filters on -- see :data:`_STATE_QUERYABLE`. It is additive sugar over the native ``state_code`` / ``state_name`` parameters, which still accept the API's raw values (e.g. non-US FIPS); passing ``state`` together with either raises ``ValueError``. """ + queryable = _STATE_QUERYABLE[collection] # Flatten ``**queryables`` first so a native state param arriving that way # (e.g. ``get_time_series_metadata``'s ``state_code``, which isn't an # explicit parameter) is visible to apply_state's mutual-exclusion guard. @@ -178,7 +194,10 @@ def _with_state(local_vars: dict[str, Any], *, to: str, into: str) -> dict[str, # check and silently send both. _flatten_queryables(local_vars) return apply_state( - local_vars, to=to, into=into, reject=("state_code", "state_name") + local_vars, + to=queryable["to"], + into=queryable["into"], + reject=("state_code", "state_name"), ) diff --git a/tests/waterdata_utils_test.py b/tests/waterdata_utils_test.py index 4b46c711..a1cc2fc5 100644 --- a/tests/waterdata_utils_test.py +++ b/tests/waterdata_utils_test.py @@ -1067,19 +1067,19 @@ def test_with_state_routes_into_native_queryable(): """``_with_state`` resolves the canonical ``state`` argument into the endpoint's native queryable (any encoding -> the requested representation) and leaves args without ``state`` untouched.""" - assert _utils_module._with_state({"state": "WI"}, to="name", into="state_name") == { + assert _utils_module._with_state({"state": "WI"}, "monitoring-locations") == { "state_name": "Wisconsin" } - assert _utils_module._with_state( - {"state": "Wisconsin"}, to="fips_us", into="state_code" - ) == {"state_code": "US:55"} + assert _utils_module._with_state({"state": "Wisconsin"}, "observationNormals") == { + "state_code": "US:55" + } # Multi-value state fans out element-wise. assert _utils_module._with_state( - {"state": ["WI", "55"]}, to="name", into="state_name" + {"state": ["WI", "55"]}, "monitoring-locations" ) == {"state_name": ["Wisconsin", "Wisconsin"]} # No ``state`` -> mapping returned unchanged. assert _utils_module._with_state( - {"state_name": "Ohio"}, to="name", into="state_name" + {"state_name": "Ohio"}, "monitoring-locations" ) == {"state_name": "Ohio"} @@ -1088,11 +1088,11 @@ def test_with_state_conflict_raises(): is ambiguous and raises.""" with pytest.raises(ValueError, match="not both"): _utils_module._with_state( - {"state": "WI", "state_code": "55"}, to="name", into="state_name" + {"state": "WI", "state_code": "55"}, "monitoring-locations" ) with pytest.raises(ValueError, match="not both"): _utils_module._with_state( - {"state": "WI", "state_name": "Wisconsin"}, to="name", into="state_name" + {"state": "WI", "state_name": "Wisconsin"}, "monitoring-locations" ) @@ -1104,8 +1104,7 @@ def test_with_state_conflict_via_queryables_raises(): with pytest.raises(ValueError, match="not both"): _utils_module._with_state( {"state": "WI", "queryables": {"state_code": "55"}}, - to="name", - into="state_name", + "monitoring-locations", ) From 752252ce4962ff79b104388f2c560c5254b759e8 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 10:07:12 -0500 Subject: [PATCH 5/7] refactor: apply /simplify findings Four agents; efficiency near-clean, the other three found real work. One was a regression I introduced two commits ago. Moving output_id onto OgcApi turned a strict lookup into a silent default. _OUTPUT_ID_BY_COLLECTION[collection] used to raise KeyError for an unregistered Water Data collection; .get(collection, "id") returned "id", which is right for NGWMN (one id for every collection) and wrong for every Water Data entry. A forgotten registration would have mis-shaped the result instead of reporting itself. The mapping form now raises and names the known collections; the string form still answers directly, which is what it means -- this API applies one id everywhere. That also settles whether the Mapping-or-str union earns its keep. It does now: with a strict lookup the string is not a restatement of the default, it is the statement that ids do not vary by collection. The union was leaking regardless. get_cql tested membership against WATERDATA_API.output_ids directly, which works only because Water Data's form is a dict -- against the string form "in" silently becomes a substring test ("i" in "id" is True). OgcApi.knows() and .collections answer for both, and get_cql asks rather than reaching past the method into the field. Vocabulary: the statistics getters were binding a local named collection for observationNormals and observationIntervals, using the same idiom as real collections, but stats.get_data builds its own request against the Statistics API and never touches the OGC engine. By CONTEXT.md's own test -- the reasoning that already declared Samples' tags not collections -- those are resources. They are named resource now, and _STATE_QUERYABLE says why it is keyed by both kinds. Also: dropped _DEFAULT_DIALECT, dead since its only use site went and whose comment claimed tests referenced it (they do not); corrected get_ogc_data's and _finalize_ogc's docstrings, which still described the three-argument shape; gave OgcApi.output_ids field(hash=False) so the comment claiming it is excluded from hashing is true; made health_signals.py parse each file once rather than twice; and folded the workflow's inline project-root check into that script, which was reading the same report through a second copy of the same glob. Skipped: sharing the __all__ AST walk between tools/ and tests/. It is a near-verbatim four-line fragment, but wiring a shared module between a CI script and the test suite costs more coupling than the duplication does. 780 tests, mypy clean across 58 files, 7/7 contracts, all gates. --- .github/workflows/code-health.yml | 26 ---------------- dataretrieval/ogc/engine.py | 34 +++++++------------- dataretrieval/ogc/policy.py | 43 +++++++++++++++++++++++--- dataretrieval/ogc/shaping.py | 2 +- dataretrieval/waterdata/cql.py | 4 +-- dataretrieval/waterdata/time_series.py | 12 +++---- dataretrieval/waterdata/utils.py | 22 +++++++------ tools/health_signals.py | 26 ++++++++++++++-- 8 files changed, 95 insertions(+), 74 deletions(-) diff --git a/.github/workflows/code-health.yml b/.github/workflows/code-health.yml index 11ff851c..601ed47c 100644 --- a/.github/workflows/code-health.yml +++ b/.github/workflows/code-health.yml @@ -51,30 +51,6 @@ jobs: pyscn analyze --json --no-open dataretrieval 2>&1 | tee pyscn-summary.txt pyscn analyze --html --no-open dataretrieval >/dev/null - - name: Check the analysis actually resolved the package - # pyscn infers a project root, and when it guesses wrong it silently - # resolves only a fraction of the imports -- which *raises* the score, - # because most of what it grades is dependency-derived. A degraded run - # therefore looks like an improved one. Record the resolved edge count - # next to the score so that is visible rather than flattering. - if: always() - continue-on-error: true - run: | - python - <<'PY' > pyscn-sanity.txt - import glob, json, os - reports = sorted(glob.glob(".pyscn/reports/*.json")) - if not reports: - print("no pyscn JSON report found"); raise SystemExit - s = json.load(open(reports[-1]))["system"]["Summary"] - root, deps = s["ProjectRoot"], s["TotalDependencies"] - print(f"modules={s['TotalModules']} resolved_dependencies={deps} root={root}") - if os.path.realpath(root) != os.path.realpath(os.getcwd()): - print(f"WARNING: project root {root!r} is not the checkout; " - "import resolution is probably degraded and the scores " - "above are not comparable to previous runs.") - PY - cat pyscn-sanity.txt - - name: Signals that mean something for this package # pyscn grades six sub-scores and four of them do not apply here, so the # composite moves for reasons that are not about this codebase. This @@ -119,7 +95,6 @@ jobs: else echo 'pyscn produced no output' fi - cat pyscn-sanity.txt 2>/dev/null || true echo '```' echo '' echo @@ -139,7 +114,6 @@ jobs: path: | .pyscn/reports/ pyscn-summary.txt - pyscn-sanity.txt health-signals.txt wily-summary.txt retention-days: 90 diff --git a/dataretrieval/ogc/engine.py b/dataretrieval/ogc/engine.py index 951ca731..63c2cd02 100644 --- a/dataretrieval/ogc/engine.py +++ b/dataretrieval/ogc/engine.py @@ -39,7 +39,6 @@ from dataretrieval.ogc.context import _dialect, _ogc_base_url, _row_cap from dataretrieval.ogc.errors import _raise_for_non_200 from dataretrieval.ogc.policy import ( - DEFAULT_DIALECT, OgcApi, _require_positive_int, ) @@ -63,9 +62,6 @@ # Set up logger for this module logger = logging.getLogger(__name__) -# Compatibility alias: the old name used internally and in tests. -_DEFAULT_DIALECT = DEFAULT_DIALECT - def _next_req_url( resp: httpx.Response, *, body: dict[str, Any] | None = None @@ -236,30 +232,22 @@ def get_ogc_data( collection : str The OGC API collection name (e.g., ``"daily"``, ``"monitoring-locations"``, ``"continuous"``). - output_id : str - The user-facing id column the wire ``id`` is renamed to. Required — - the per-API collection-to-id map lives in the caller, not here. + output_id : str, optional + The user-facing id column the wire ``id`` is renamed to. Defaults to + whatever ``api`` registers for this collection; pass one only for a + collection the API does not register, such as a reference table whose + ids follow a rule rather than a list. + api : OgcApi + Which OGC API to target: its base URL, its dialect, the synthetic id + columns its results carry, and the id it renames the wire ``id`` to. + Required, because this package is API-neutral and names no API of its + own -- each adapter declares its own (``waterdata.utils.WATERDATA_API``, + ``ngwmn.NGWMN_API``) and passes that. max_rows : int, optional Stop paginating once this many rows have been collected and truncate the result to exactly ``max_rows``. ``None`` (default) fetches the full result. Intended for cheap previews of large, un-chunked tables (e.g. :func:`get_reference_table`). - base_url : str - OGC API base URL to target. Required: this package is API-neutral and - names no API of its own, so each adapter passes its own base (e.g. - ``waterdata.utils.OGC_API_URL``, ``ngwmn.NGWMN_OGC_API_URL``). It was - once optional, falling back to whatever was in ambient scope -- which - defaults to the empty string, so omitting it built a *relative* - ``/collections/{id}/items`` that planning accepted and only httpx - rejected at send time, surfacing as a NetworkError about an unknown - service. Requiring it moves that mistake to the call site, where mypy - catches it. - extra_id_cols : set or frozenset, optional - Synthetic id columns to push to the end of a result frame (see - :func:`_arrange_cols`). Defaults to an empty set. - dialect : OgcDialect, optional - Per-API request quirks (CQL2-only collections, date-only collections). - Defaults to a plain OGC API with neither. Returns ------- diff --git a/dataretrieval/ogc/policy.py b/dataretrieval/ogc/policy.py index 0555f72f..3735b1f1 100644 --- a/dataretrieval/ogc/policy.py +++ b/dataretrieval/ogc/policy.py @@ -105,17 +105,50 @@ class OgcApi: dialect: OgcDialect = DEFAULT_DIALECT extra_id_cols: frozenset[str] = frozenset() #: How the wire ``id`` is renamed for the caller: a per-collection mapping, - #: or one name every collection uses. Excluded from the hash because a - #: mapping is unhashable and this value is identity, not a key. - output_ids: Mapping[str, str] | str = "id" + #: or a single name when the API applies one to every collection. Excluded + #: from hashing because a mapping is unhashable. + output_ids: Mapping[str, str] | str = field(default="id", hash=False) def output_id(self, collection: str) -> str: """The user-facing name the wire ``id`` takes for ``collection``. The caller should not have to know this -- it is a fact about the API, so the API answers it. Callers with a collection outside the mapping - (a reference table, say) may still pass one explicitly. + (a reference table, whose ids follow a rule rather than a list) pass + one explicitly instead. + + Raises + ------ + KeyError + If this API registers ids per collection and does not know this + one. Deliberately loud: a silent fallback would rename the id + column to something no Water Data collection uses, mis-shaping the + result instead of reporting the unregistered collection. """ if isinstance(self.output_ids, str): return self.output_ids - return self.output_ids.get(collection, "id") + try: + return self.output_ids[collection] + except KeyError: + raise KeyError( + f"{collection!r} has no output id registered for this API. " + f"Known collections: {sorted(self.output_ids)}." + ) from None + + def knows(self, collection: str) -> bool: + """Whether this API recognizes ``collection``. + + Ask this rather than testing ``collection in api.output_ids``: that + works only for the mapping form, and silently becomes a substring test + when an API names one id for every collection. + """ + if isinstance(self.output_ids, str): + return True + return collection in self.output_ids + + @property + def collections(self) -> tuple[str, ...]: + """Registered collection names, or empty when ids are not per-collection.""" + if isinstance(self.output_ids, str): + return () + return tuple(sorted(self.output_ids)) diff --git a/dataretrieval/ogc/shaping.py b/dataretrieval/ogc/shaping.py index 68b58806..48bfd907 100644 --- a/dataretrieval/ogc/shaping.py +++ b/dataretrieval/ogc/shaping.py @@ -387,7 +387,7 @@ def _finalize_ogc( rather than only per-chunk, so a chunked call's total is bounded to exactly ``max_rows`` and a resumed call honors the cap too. The per-``_paginate`` ``_row_cap`` is only an early-stop download bound. - ``base_url`` is captured with the finalizer so resumed calls query the same + ``api`` is captured with the finalizer so resumed calls query the same API's schema when their combined result is empty. """ frame = _deal_with_empty(frame, properties, collection, base_url=api.base_url) diff --git a/dataretrieval/waterdata/cql.py b/dataretrieval/waterdata/cql.py index 6626e8ae..7c0f6863 100644 --- a/dataretrieval/waterdata/cql.py +++ b/dataretrieval/waterdata/cql.py @@ -143,10 +143,10 @@ def get_cql( ... ' "02070010%"]}', ... ) """ - if collection not in WATERDATA_API.output_ids: + if not WATERDATA_API.knows(collection): raise ValueError( f"Unknown collection {collection!r}. Valid collections: " - f"{sorted(WATERDATA_API.output_ids)}." + f"{list(WATERDATA_API.collections)}." ) output_id = WATERDATA_API.output_id(collection) diff --git a/dataretrieval/waterdata/time_series.py b/dataretrieval/waterdata/time_series.py index 6dd55d71..99b179d4 100644 --- a/dataretrieval/waterdata/time_series.py +++ b/dataretrieval/waterdata/time_series.py @@ -1006,16 +1006,16 @@ def get_stats_por( ... end_date="01-31", ... ) """ - collection = "observationNormals" + resource = "observationNormals" # Build argument dictionary, omitting None values params = _get_args( - _with_state(locals(), collection), + _with_state(locals(), resource), exclude={"expand_percentiles"}, ) return stats.get_data( - args=params, service=collection, expand_percentiles=expand_percentiles + args=params, service=resource, expand_percentiles=expand_percentiles ) @@ -1154,17 +1154,17 @@ def get_stats_date_range( ... computation_type=["minimum", "maximum"], ... ) """ - collection = "observationIntervals" + resource = "observationIntervals" # Build argument dictionary, omitting None values params = _get_args( - _with_state(locals(), collection), + _with_state(locals(), resource), exclude={"expand_percentiles"}, ) return stats.get_data( args=params, - service=collection, + service=resource, expand_percentiles=expand_percentiles, ) diff --git a/dataretrieval/waterdata/utils.py b/dataretrieval/waterdata/utils.py index 32dcc17a..fc3f7f69 100644 --- a/dataretrieval/waterdata/utils.py +++ b/dataretrieval/waterdata/utils.py @@ -160,12 +160,16 @@ def _get_args( ) -#: Which state queryable each collection filters on, and in which -#: representation. A fact about the collection, so it lives with the collection +#: Which state queryable each endpoint filters on, and in which +#: representation. A fact about the endpoint, so it lives with the endpoint #: rather than being repeated at every getter that filters by state -- the -#: shape ``ngwmn`` already uses for the same problem. Spans both the OGC -#: collections and the Statistics API's resources, which differ here: the OGC -#: metadata collections take a full state name, Statistics takes a FIPS code. +#: shape ``ngwmn`` already uses for the same problem. +#: +#: Keyed by OGC collection *and* by Statistics API resource, because both +#: accept a state filter and they disagree about it: the OGC metadata +#: collections take a full state name, Statistics takes a FIPS code. Those are +#: different kinds of thing (see ``CONTEXT.md``) and only share this table +#: because the question is the same one. _STATE_QUERYABLE: dict[str, dict[str, str]] = { "monitoring-locations": {"to": "name", "into": "state_name"}, "time-series-metadata": {"to": "name", "into": "state_name"}, @@ -175,18 +179,18 @@ def _get_args( } -def _with_state(local_vars: dict[str, Any], collection: str) -> dict[str, Any]: - """Resolve the unified ``state`` argument into a collection's queryable. +def _with_state(local_vars: dict[str, Any], endpoint: str) -> dict[str, Any]: + """Resolve the unified ``state`` argument into an endpoint's queryable. Returns the (mutated) args mapping. ``state`` is the canonical, format-flexible parameter (full name / postal / FIPS); it is normalized via :func:`~dataretrieval.codes.states.to_state` into whichever representation - and queryable ``collection`` filters on -- see :data:`_STATE_QUERYABLE`. + and queryable ``endpoint`` filters on -- see :data:`_STATE_QUERYABLE`. It is additive sugar over the native ``state_code`` / ``state_name`` parameters, which still accept the API's raw values (e.g. non-US FIPS); passing ``state`` together with either raises ``ValueError``. """ - queryable = _STATE_QUERYABLE[collection] + queryable = _STATE_QUERYABLE[endpoint] # Flatten ``**queryables`` first so a native state param arriving that way # (e.g. ``get_time_series_metadata``'s ``state_code``, which isn't an # explicit parameter) is visible to apply_state's mutual-exclusion guard. diff --git a/tools/health_signals.py b/tools/health_signals.py index 26fed179..360a61c3 100644 --- a/tools/health_signals.py +++ b/tools/health_signals.py @@ -15,6 +15,7 @@ import ast import glob import json +import os import sys from pathlib import Path @@ -22,14 +23,22 @@ PACKAGE = Path("dataretrieval") +_TREES: dict[str, ast.Module] = {} _SPANS: dict[str, dict[int, str]] = {} +def tree_for(path): + """Parse each file once; both walks below want the same trees.""" + if path not in _TREES: + _TREES[path] = ast.parse(Path(path).read_text(encoding="utf-8")) + return _TREES[path] + + def enclosing(path, start): if path not in _SPANS: _SPANS[path] = { n.lineno: n.name - for n in ast.walk(ast.parse(Path(path).read_text(encoding="utf-8"))) + for n in ast.walk(tree_for(path)) if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)) } for line in range(start, start + 3): @@ -55,7 +64,7 @@ def required_arguments(): """Public functions carrying required arguments, worst module first.""" out = [] for path in sorted(PACKAGE.rglob("*.py")): - tree = ast.parse(path.read_text(encoding="utf-8")) + tree = tree_for(str(path)) declared = None for node in tree.body: if isinstance(node, ast.Assign) and any( @@ -86,6 +95,19 @@ def required_arguments(): sys.exit(0) report = json.loads(Path(reports[-1]).read_text()) +# pyscn infers a project root, and when it guesses wrong it resolves only a +# fraction of the imports -- which *raises* its scores, because most of what it +# grades is dependency-derived. A degraded run therefore looks like an improved +# one. Report the root so that is visible rather than flattering. +summary = report["system"]["Summary"] +root = summary["ProjectRoot"] +if os.path.realpath(root) != os.path.realpath(os.getcwd()): + print( + f"WARNING: pyscn resolved against {root!r}, not this checkout -- import\n" + f"resolution is probably degraded and these numbers are not comparable." + ) +print(f"modules {summary['TotalModules']}") + dead = report["dead_code"]["summary"]["total_findings"] deps = report["system"]["DependencyAnalysis"] print(f"dead code {dead} findings") From 0172b0aff8acd7da5c6544168f9364c21ba0500f Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 14:08:16 -0500 Subject: [PATCH 6/7] refactor(ogc): remove chunking dependency hop --- dataretrieval/ogc/chunking.py | 140 +++---------------------------- dataretrieval/ogc/engine.py | 64 +++++++++++++- dataretrieval/ogc/filters.py | 2 +- tests/waterdata_chunking_test.py | 5 +- 4 files changed, 74 insertions(+), 137 deletions(-) diff --git a/dataretrieval/ogc/chunking.py b/dataretrieval/ogc/chunking.py index c958b49d..8e57d380 100644 --- a/dataretrieval/ogc/chunking.py +++ b/dataretrieval/ogc/chunking.py @@ -8,14 +8,10 @@ chunk URL under the budget. Requests that already fit get a trivial single-step plan — the executor has one code path either way. -This module owns the OGC-specific half: the byte budget, the -``parallel_chunks`` dial, and the ``multi_value_chunked`` decorator that -ties a plan to a fetcher. Driving the resulting chunks to -completion — bounded concurrency, retry, failure precedence, resume — is -API-neutral and belongs to -:class:`dataretrieval.transport.fanout.FanOut`, which this module hands -its plan to. :class:`~dataretrieval.ogc.planning.ChunkPlan` satisfies -:class:`~dataretrieval.transport.fanout.FanOutPlan` structurally. +This module owns the public ``parallel_chunks`` dial and preserves compatibility +names from the former combined planner/executor. The OGC engine composes +planning with API-neutral fan-out directly, avoiding an extra dependency hop in +every getter call. Parallel chunks: the planner is conservative by default — it splits only as far as the byte limit forces. A caller who knows their result is large can opt @@ -36,26 +32,17 @@ from __future__ import annotations -import functools -from collections.abc import Callable, Iterator +from collections.abc import Iterator from contextlib import contextmanager -from typing import Any -import httpx -import pandas as pd - -from dataretrieval._ambient import Ambient -from dataretrieval.transport.fanout import ( - FanOut, - _active_client, - _Fetch, - _Finalize, - _passthrough_result, - active_client, +from dataretrieval.ogc.engine import ( + _parallel_chunks, +) +from dataretrieval.ogc.engine import ( + multi_value_chunked as multi_value_chunked, ) -from dataretrieval.transport.retry import RetryPolicy +from dataretrieval.transport.fanout import FanOut, _active_client, active_client -from .planning import ChunkPlan from .policy import _require_positive_int # Compatibility aliases. ``ChunkedCall`` was this module's executor before it @@ -70,22 +57,6 @@ get_active_client = active_client _chunked_client = _active_client -# Empirically the API replies HTTP 414 above ~8200 bytes of full URL — -# matches nginx's default ``large_client_header_buffers`` of 8 KB. 8000 -# leaves ~200 bytes for request-line framing and proxy variance. The decorator -# resolves this module-level default at call time when ``url_limit`` is None, -# so a test can ``monkeypatch.setattr`` it on this module. -_OGC_URL_BYTE_LIMIT = 8000 - - -# Parallel-chunks dial: opt-in to fan a query out *more finely* than the byte -# limit alone requires. Scoped to a ``with parallel_chunks(...):`` block (a -# ContextVar), deliberately NOT an env var (see :func:`parallel_chunks` for -# why). The ambient holds ``n`` — the requested cap on the plan's total -# chunk count; ``1`` (the default, outside any block) means "off — chunk -# only as much as the byte limit needs, no extra fan-out". -_parallel_chunks: Ambient[int] = Ambient("ogc_parallel_chunks", 1) - @contextmanager def parallel_chunks(n: int) -> Iterator[None]: @@ -188,92 +159,3 @@ def parallel_chunks(n: int) -> Iterator[None]: _require_positive_int(n, "parallel_chunks(n)", examples="2, 8, 32") with _parallel_chunks(n): yield - - -def multi_value_chunked( - *, - build_request: Callable[..., httpx.Request], - url_limit: int | None = None, -) -> Callable[[_Fetch[dict[str, Any]]], Callable[..., tuple[pd.DataFrame, Any]]]: - """ - Decorate an async fetcher to transparently chunk over-budget requests. - - Returns a callable that builds a :class:`ChunkPlan` from ``args``, - constructs a :class:`ChunkedCall` over the decorated - ``async def fetch(args) -> (df, response)``, and drives it to - completion via :meth:`ChunkedCall.resume`. The plan splits multi-value - list params and the cql-text filter so each chunk URL fits the - byte limit. An already-fitting request is a one-step plan, unless an - active :func:`parallel_chunks` block asks the plan to fan out more - finely. See the module docstring for the concurrency model. - - Parameters - ---------- - build_request : Callable[..., httpx.Request] - Factory that turns a kwargs dict into a sized httpx request, - e.g. ``_construct_api_requests``. Called during planning to - measure each candidate plan. - url_limit : int, optional - Byte budget for the request (URL + body). When ``None`` - (default), the module-level ``_OGC_URL_BYTE_LIMIT`` is - resolved at call time so test patches via - ``monkeypatch.setattr`` take effect. - - Returns - ------- - Callable - A *synchronous* wrapper ``wrapper(args, *, finalize=...) -> - (df, response)`` that executes the underlying plan transparently - over the decorated async fetcher. - - Raises - ------ - Unchunkable - If no plan can fit ``url_limit``. - ChunkInterrupted - On a mid-execution transient — 429, 5xx, or a bare transport - error: :class:`QuotaExhausted` for 429, :class:`ServiceInterrupted` - for the rest. See :class:`ChunkedCall` for the resume semantics. - - See Also - -------- - ChunkPlan : Planning shape (axes, partitioning, passthrough). - ChunkedCall : Per-chunk execution and resume semantics. - """ - - def decorator( - fetch: _Fetch[dict[str, Any]], - ) -> Callable[..., tuple[pd.DataFrame, Any]]: - @functools.wraps(fetch) - def wrapper( - args: dict[str, Any], - *, - finalize: _Finalize = _passthrough_result, - ) -> tuple[pd.DataFrame, Any]: - limit = _OGC_URL_BYTE_LIMIT if url_limit is None else url_limit - # Read the parallel_chunks dial ``n`` from the ambient set by - # ``parallel_chunks`` (1 = off outside any such block; otherwise the - # requested total chunk cap). It only affects *planning*, done - # here up front, so a later resume — which re-issues the - # already-planned chunks — needs no snapshot. - plan = ChunkPlan( - args, build_request, limit, max_chunks=_parallel_chunks.get() - ) - retry_policy = RetryPolicy.from_env() - # The concurrency cap is resolved inside ``resume()`` from - # ``API_USGS_CONCURRENT``; ``1`` is a sequential gather, - # ``total <= 1`` a one-element gather — no special branch. - return ChunkedCall( - plan, - fetch, - retry_policy, - finalize, - canonical_url=plan.canonical_url, - # The collection name, for the progress line the executor - # opens. ``get_ogc_data`` puts it in ``args``. - service=args.get("collection"), - ).resume() - - return wrapper - - return decorator diff --git a/dataretrieval/ogc/engine.py b/dataretrieval/ogc/engine.py index 63c2cd02..04cecec9 100644 --- a/dataretrieval/ogc/engine.py +++ b/dataretrieval/ogc/engine.py @@ -35,9 +35,10 @@ import httpx import pandas as pd -import dataretrieval.ogc.chunking as chunking +from dataretrieval._ambient import Ambient from dataretrieval.ogc.context import _dialect, _ogc_base_url, _row_cap from dataretrieval.ogc.errors import _raise_for_non_200 +from dataretrieval.ogc.planning import ChunkPlan from dataretrieval.ogc.policy import ( OgcApi, _require_positive_int, @@ -51,7 +52,13 @@ _switch_properties_id, ) from dataretrieval.ogc.shaping import GEOPANDAS, _finalize_ogc, _get_resp_data -from dataretrieval.transport.fanout import FanOut, active_client +from dataretrieval.transport.fanout import ( + FanOut, + _Fetch, + _Finalize, + _passthrough_result, + active_client, +) from dataretrieval.transport.links import resolve_next_url from dataretrieval.transport.pagination import paginate from dataretrieval.transport.retry import RetryPolicy @@ -63,6 +70,15 @@ logger = logging.getLogger(__name__) +# Empirically the API replies HTTP 414 above ~8200 bytes of full URL. Resolve +# this default at call time so tests and alternate deployments can patch it. +_OGC_URL_BYTE_LIMIT = 8000 + +# The public ``parallel_chunks`` context manager lives in ``ogc.chunking``; +# execution reads the shared ambient here without depending on that facade. +_parallel_chunks: Ambient[int] = Ambient("ogc_parallel_chunks", 1) + + def _next_req_url( resp: httpx.Response, *, body: dict[str, Any] | None = None ) -> str | None: @@ -305,13 +321,53 @@ def get_ogc_data( return _fetch_once(args, finalize=finalize) -@chunking.multi_value_chunked(build_request=_construct_api_requests) +def multi_value_chunked( + *, + build_request: Callable[..., httpx.Request], + url_limit: int | None = None, +) -> Callable[[_Fetch[dict[str, Any]]], Callable[..., tuple[pd.DataFrame, Any]]]: + """Decorate an async fetcher to transparently plan and execute chunks. + + OGC-specific planning is composed here with the API-neutral + :class:`~dataretrieval.transport.fanout.FanOut` executor. The public + :func:`dataretrieval.ogc.chunking.parallel_chunks` context manager controls + optional finer fan-out through the shared ``_parallel_chunks`` ambient. + """ + + def decorator( + fetch: _Fetch[dict[str, Any]], + ) -> Callable[..., tuple[pd.DataFrame, Any]]: + @functools.wraps(fetch) + def wrapper( + args: dict[str, Any], + *, + finalize: _Finalize = _passthrough_result, + ) -> tuple[pd.DataFrame, Any]: + limit = _OGC_URL_BYTE_LIMIT if url_limit is None else url_limit + plan = ChunkPlan( + args, build_request, limit, max_chunks=_parallel_chunks.get() + ) + return FanOut( + plan, + fetch, + RetryPolicy.from_env(), + finalize, + canonical_url=plan.canonical_url, + service=args.get("collection"), + ).resume() + + return wrapper + + return decorator + + +@multi_value_chunked(build_request=_construct_api_requests) async def _fetch_once( args: dict[str, Any], ) -> tuple[pd.DataFrame, httpx.Response]: """Send one prepared-args OGC request asynchronously; return (frame, response). - ``@chunking.multi_value_chunked`` models every multi-value list + ``@multi_value_chunked`` models every multi-value list parameter and the cql-text filter as a chunkable axis, greedy-halves the biggest chunk across all axes until each chunk URL fits, and iterates the cartesian product. With no chunkable inputs the diff --git a/dataretrieval/ogc/filters.py b/dataretrieval/ogc/filters.py index 50875409..a476d9ac 100644 --- a/dataretrieval/ogc/filters.py +++ b/dataretrieval/ogc/filters.py @@ -4,7 +4,7 @@ - ``FILTER_LANG``: the type alias used for the ``filter_lang`` kwarg. -Internal helpers used by ``chunking.multi_value_chunked``'s joint +Internal helpers used by ``engine.multi_value_chunked``'s joint planner: ``_split_top_level_or`` (clause partitioning), ``_is_chunkable`` (filter-language gate), and ``_check_numeric_filter_pitfall`` (the lexicographic-comparison guard). diff --git a/tests/waterdata_chunking_test.py b/tests/waterdata_chunking_test.py index 19687869..cf20699f 100644 --- a/tests/waterdata_chunking_test.py +++ b/tests/waterdata_chunking_test.py @@ -45,7 +45,6 @@ TransientError, Unchunkable, ) -from dataretrieval.ogc import chunking as _chunking from dataretrieval.ogc import engine as _engine from dataretrieval.ogc.chunking import ( ChunkedCall, @@ -367,7 +366,7 @@ async def fetch(args): def test_multi_value_chunked_lazy_url_limit(monkeypatch): - """``url_limit=None`` → resolve chunking._OGC_URL_BYTE_LIMIT at call + """``url_limit=None`` → resolve engine._OGC_URL_BYTE_LIMIT at call time, so tests that patch the constant affect this decorator too.""" calls = [] @@ -378,7 +377,7 @@ async def fetch(args): elapsed=datetime.timedelta(seconds=0.1), headers={} ) - monkeypatch.setattr(_chunking, "_OGC_URL_BYTE_LIMIT", 240) + monkeypatch.setattr(_engine, "_OGC_URL_BYTE_LIMIT", 240) # 4 sites of 10 chars → exceeds 240 → planner splits. fetch({"sites": ["S" * 10 + str(i) for i in range(4)]}) assert len(calls) > 1, "patched constant should drive chunking" From 26cfca9619f0604481bb0e9a3b9ff7ee266052c7 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Mon, 10 Aug 2026 14:19:16 -0500 Subject: [PATCH 7/7] docs(architecture): record health score convergence --- .../source/architecture/health-iterations.rst | 104 ++++++++++++++++++ docs/source/architecture/index.rst | 14 ++- 2 files changed, 112 insertions(+), 6 deletions(-) create mode 100644 docs/source/architecture/health-iterations.rst diff --git a/docs/source/architecture/health-iterations.rst b/docs/source/architecture/health-iterations.rst new file mode 100644 index 00000000..34d3c4bd --- /dev/null +++ b/docs/source/architecture/health-iterations.rst @@ -0,0 +1,104 @@ +Architecture health iteration log +================================= + +Purpose +------- + +This log records small architecture experiments evaluated with ``pyscn +1.29.0``. It keeps rejected ideas available for future work without leaving +non-improving code in the branch. PySCN is treated as an architectural fitness +function, not as a substitute for design review: public compatibility, +dependency direction, and the package's function-oriented API remain higher +priority than optimizing a composite metric. + +Method +------ + +Each iteration starts from the latest accepted commit, runs ``pyscn analyze +--json --no-open dataretrieval``, and compares the rounded health score with the +accepted score. A code change is retained only when the target health score +increases and the test, type, lint, and dependency-contract checks pass. +Experiments that improve an unrounded or subsidiary signal but leave the target +score unchanged are reverted and recorded below. Iteration stops at 95 or after +three consecutive experiments leave the accepted score unchanged. + +Results +------- + +.. list-table:: PySCN architecture iterations + :header-rows: 1 + :widths: 8 20 12 12 48 + + * - Iteration + - Experiment + - Health score + - Decision + - Evidence and rationale + * - Baseline + - Existing ``ci/health-report-signals`` branch + - 82 + - Starting point + - Complexity 95, Dead Code 100, Duplication 70, Coupling 100, + Cohesion 100, Dependencies 80, Architecture 87; maximum dependency + depth 8 with 139 internal edges. + * - 1 + - Remove ``ogc.engine -> ogc.chunking`` from the execution path + - 83 + - **Accepted** (``0172b0af``) + - The chunk planner/executor composition moved to the OGC engine while + ``ogc.chunking`` retained the public ``parallel_chunks`` dial and + compatibility imports. Maximum depth fell from 8 to 7, dependency score + rose from 80 to 85, and edges fell from 139 to 138. The full test suite, + Ruff, mypy, and all import-linter contracts passed. + * - 2 + - Inline private ``transport.env`` into ``transport.retry`` + - 83 + - Rejected + - Modules fell from 58 to 57 and edges from 138 to 135, but every rounded + PySCN sub-score and the composite were unchanged. This remains a + reasonable cleanup if module count or edge count becomes a separately + adopted fitness function. + * - 3 + - Extract ``FanOut._run`` failure precedence + - 83 + - Rejected + - The high-risk-function count fell from 20 to 19 and 175 focused tests + passed, but Complexity remained 95 and the composite remained 83. Revisit + if the project adopts the raw high-risk count as a ratchet. + * - 4 + - Remove historical transport aliases from ``ogc.chunking`` + - 83 + - Rejected + - Architecture rose from 87 to 88 and edges fell from 138 to 137, but the + target composite did not move. The experiment also removed deliberately + retained compatibility names for a marginal metric gain, conflicting + with the project's higher-priority compatibility characteristic. + +Convergence +----------- + +The accepted score is **83**. Iterations 2, 3, and 4 each returned 83, so the +three-straight-iterations convergence condition was reached. Further attempts +were stopped rather than collapsing typed public getter families or widely +used policy leaves merely to satisfy tool heuristics. + +The remaining 70 Duplication score is dominated by intentional symmetry: +collection-specific Water Data and NGWMN getters preserve explicit typed +signatures and documentation, WQP wrappers preserve discoverable service +entry points, and deprecated NWIS signatures are frozen. The Architecture +score also treats dependency-free leaves and service-neutral orchestrators as +single-responsibility violations because they are used across several module +communities. Those findings remain useful review prompts, but changing those +boundaries solely for the score would weaken the documented architecture. + +Future experiments +------------------ + +- Reconsider the ``transport.env`` consolidation if dependency edge count is + promoted from an informational signal to an explicit fitness function. +- Reconsider the ``FanOut._run`` extraction if raw high-risk findings, rather + than the rounded Complexity score, become a ratchet. +- Remove ``ogc.chunking`` compatibility aliases only through an intentional + compatibility decision, not as an incidental score optimization. +- Prefer changes that reduce maximum dependency depth, introduce no new clone + groups, preserve typed public APIs, and keep all import-linter contracts. diff --git a/docs/source/architecture/index.rst b/docs/source/architecture/index.rst index a92f19c4..31190732 100644 --- a/docs/source/architecture/index.rst +++ b/docs/source/architecture/index.rst @@ -105,12 +105,13 @@ Shared components state; ``requests`` owns argument normalization and HTTP request construction; ``schema`` executes queryables/schema requests; ``engine`` supplies OGC cursor and response strategies to transport pagination; - ``planning`` determines chunk boundaries; ``chunking`` connects those plans - to the shared fan-out executor and retains compatibility aliases; - ``interruptions`` and ``retry`` re-export their moved compatibility - surfaces; and ``shaping``, ``dates``, ``filters``, and ``errors`` isolate - their named protocol concerns. The full runtime OGC graph, including the - facade, is acyclic -- enforced by the package-wide fitness function in + ``planning`` determines chunk boundaries; ``engine`` composes those plans + with the shared fan-out executor; ``chunking`` owns the public + ``parallel_chunks`` dial and retains compatibility aliases; ``interruptions`` + and ``retry`` re-export their moved compatibility surfaces; and ``shaping``, + ``dates``, ``filters``, and ``errors`` isolate their named protocol + concerns. The full runtime OGC graph, including the facade, is acyclic -- + enforced by the package-wide fitness function in ``tests/architecture_test.py``. ``dataretrieval.transport`` @@ -343,4 +344,5 @@ Architecturally significant changes should: .. toctree:: :maxdepth: 1 + health-iterations decisions/index