Improve architecture health signals and dependency depth - #366
Draft
thodson-usgs wants to merge 7 commits into
Draft
Improve architecture health signals and dependency depth#366thodson-usgs wants to merge 7 commits into
thodson-usgs wants to merge 7 commits into
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DSMyMqQ4uuQ9TAbJLnJ6Yi
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.
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.
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.
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.
thodson-usgs
force-pushed
the
ci/health-report-signals
branch
from
August 10, 2026 15:07
939fa71 to
752252c
Compare
thodson-usgs
changed the title
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.
Summary
Architecture health result
PySCN 1.29.0 improved from 82 to 83 during the iterative pass:
Three subsequent experiments retained a composite score of 83 and were reverted, satisfying the requested convergence condition. The experiment log explains their subsidiary effects and why they were not retained.
Validation
pytest -q tests/— 780 passed, 12 deselectedruff check .ruff format --check .mypy dataretrievallint-imports— 7 contracts keptpyscn analyze --json --no-open dataretrievalThe new architecture pages parse successfully. A warning-as-error Sphinx build still reports two pre-existing global warnings: duplicate indexing for
WQP_Metadata.site_infoand unpicklablenbsphinx_custom_formatsconfiguration.