feat(transducer): publish and range-delete for corrected hydrographs - #864
Merged
Conversation
The hydrograph corrector in OcotilloUI could only download its corrected series as CSV -- there was no POST for transducer observations at all. This adds the two write endpoints its upload contract specifies. POST /observation/transducer-groundwater-level/block publishes one corrected logger file as one block plus all of its readings, in one transaction. The block's span is derived from the measurements rather than sent: nothing links the observation table to the block table, so the reader pairs them by time and a client-supplied span wider than the data would make the block claim readings it does not contain. `deployment_id` is optional and resolved from the deployments covering that span; zero or more than one match is a 422 rather than a guess, because guessing attributes readings to hardware that did not record them. An existing block sharing any instant with the new one is a 409 listing the collisions. `?replace_overlapping=true` deletes those blocks and their readings. The readings have to go with the block -- keeping them would leave rows the reader cannot show that still occupy the deployment/parameter/instant the new series is about to claim, so a "replace" that kept them would fail on the very insert it was asked to make room for. Readings orphaned by a hand-deleted block are caught separately and reported with the earliest colliding timestamp, rather than letting the insert abort on a constraint name. Overlap is inclusive on both bounds, unlike TransducerObservationBlock.overlaps, which is half-open. The reader matches with `start <= t <= end`, so two blocks sharing an endpoint both claim a reading at that instant -- exactly the ambiguity the check exists to prevent. DELETE /observation/transducer-groundwater-level removes every reading for a well inside a closed range and reconciles the blocks that covered them: one left empty is deleted, one left partial has its span narrowed to the survivors. All three parameters are required; there is deliberately no unbounded form. Scope matches the GET on the same path, so the set a client previews is the set this removes. Schema changes: provenance on the block (source_file, source_kind, and an ordered corrections list), and a per-reading `note` set only where a correction moved the value, so NULL reads as "as measured" rather than "unknown". A corrected block is derived data, and a reviewer who cannot see that a series was snapped to a manual measurement cannot review it. The block time-order check is relaxed to `end >= start`: a block covering a single instant is legitimate, either published that way or narrowed to it by a delete. Both write routes are gated on AMP.Staging, a standalone group -- AMPAdmin does not satisfy it and it satisfies nothing else -- so they ship dark while the workbench is validated against real logger files. Two bugs fixed in passing: - The read route called get_transducer_observations positionally, and the helper's fourth positional parameter is `sensor_id`. `start_time` landed in `sensor_id` (unused, dropped), `end_time` landed in `start_time`, and `end_time` was never set, so a requested upper bound was ignored and the lower bound came from the wrong argument. - `sort`/`order` on that route were accepted and ignored. They now work over a whitelist; an unrecognised field is a 422 rather than a silently differently ordered page. Wellntel support from the contract is deferred: both the readings proxy and the sensor_type filter on /thing are blocked on where the API key and the wellname/PointID mapping should live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Coverage✅ 79.02% total — gate is 75%. Coverage for the Python files changed in this PR
|
1 similar comment
Contributor
Coverage✅ 79.02% total — gate is 75%. Coverage for the Python files changed in this PR
|
The check was created as `check_transuder_block_time_order` -- no `c` -- in the initial migration, so that is the name in every deployed database. Postgres cannot alter a check in place, so the drop-and-recreate that relaxing it to `end_datetime >= start_datetime` already required is the free moment to fix the spelling: dropped under the old name, created under the new one, no separate RENAME and no window where the table is unconstrained beyond the one the relaxation already opens. The two spellings are separate constants because they are not interchangeable. `op.drop_constraint` matches on the name in the live database, so anything reaching for the constraint to *find* it has to use the spelling that matches the revision it is running against -- the old one on the way down, the new one on the way up. Verified by round-tripping the migration against the test database: downgrade restores `check_transuder_...` with `>`, upgrade restores `check_transducer_...` with `>=`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Picks up the Dagster PEX branch-deploy work from #863. The branch-deploy CI step that materializes `ingestion_heartbeat_check` came from staging via the pull_request merge ref, but the job it launches is defined in automated_ingestion/defs/jobs/heartbeat.py, which this branch predated -- so the deployed code location had no such job and the launch failed with PipelineNotFoundError. Merging brings the definition along with the step.
Contributor
Coverage✅ 79.02% total — gate is 75%. Coverage for the Python files changed in this PR
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds API support for publishing corrected transducer hydrographs and deleting bounded observation ranges.
Changes:
- Adds authenticated publish and range-delete endpoints with provenance handling.
- Adds domain/service logic for deployment resolution, overlap handling, and block reconciliation.
- Adds schema migration, documentation, and comprehensive tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
api/observation.py |
Adds publish/delete routes and fixes read parameters. |
services/transducer_helper.py |
Implements publishing and range deletion. |
services/observation_helper.py |
Adds transducer result ordering. |
domain/hydrograph.py |
Defines hydrograph validation rules. |
schemas/transducer.py |
Adds publish and response schemas. |
db/transducer.py |
Adds provenance and reading-note fields. |
core/dependencies.py |
Adds the AMP.Staging authorization group. |
alembic/versions/c3d4e5f6a7b8_hydrograph_correction_publish.py |
Migrates provenance fields and block constraints. |
tests/test_transducer_publish.py |
Tests publish, delete, and ordering endpoints. |
tests/test_domain_hydrograph.py |
Tests domain rules. |
tests/test_authorization.py |
Registers the staging dependency. |
docs/hydrograph-correction-publish.md |
Documents the API contract. |
CLAUDE.md |
Documents staging authorization conventions. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Four review findings from the Copilot pass, all real. The publish and range-delete paths each read state, decide, and then write based on what they read, with nothing stopping another writer in between. Two publishes with different timestamps but overlapping spans each saw no existing block and both committed -- the unique constraints only catch identical spans and identical readings -- leaving the inclusive reader with two blocks claiming the same instants. Two range deletes each computed survivors from a snapshot the other was invalidating, so the later update could widen a block back over readings the earlier one had removed. Both now take a transaction-scoped advisory lock on (thing_id, parameter_id) before reading. An advisory lock rather than row locks because on publish there is no row to lock: the conflict is with a block that does not exist yet, so what needs guarding is the series itself. Both paths take the same key in the same order, so they serialize against each other and cannot deadlock against one another. Covered by a test that holds the lock from a second connection and asserts the publish waits for it rather than proceeding on a stale snapshot. `parameter_id` was accepted from the client unchecked while the read and delete routes on the same path resolve groundwater level themselves, so publishing under any other parameter returned 201 for data neither of them could ever list or remove. The field stays -- the contract has the client state it explicitly rather than inherit a server-side default -- but it is now validated against the parameter the route is scoped to. `order` accepted anything and silently fell through to descending, so the near miss `order=ascending` returned 200 with the rows in exactly the opposite order to the one requested. It is now a 422 in the same shape as an unknown sort field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Coverage✅ 79.03% total — gate is 75%. Coverage for the Python files changed in this PR
|
Contributor
|
Your pull request is automatically being deployed to Dagster Cloud.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the API side of the Hydrograph Correction upload contract
(
OcotilloUI/docs/hydrograph-correction-upload-contract.md). The correctorcould only download its corrected series as CSV — there was no
POSTfortransducer observations at all.
API-side notes live in
docs/hydrograph-correction-publish.md.What changed
POST /observation/transducer-groundwater-level/blockOne corrected logger file becomes one block plus all of its readings, in one
transaction.
block table — the reader pairs them by time — so a client-supplied span wider
than the data would make the block claim readings it does not contain.
deployment_idis optional, resolved from the deployments whoseinstallation period covers the span. A NULL installation date reads as
"always installed", a NULL removal date as "still installed". Zero or more
than one match is a 422 telling the client to send it explicitly, because
guessing attributes readings to hardware that did not record them.
data_maturityis derived fromreview_status, not sent — a blockpublished as
not reviewedisprovisionalon USGS terms, and sending bothseparately would let a client store a contradiction.
DELETE /observation/transducer-groundwater-levelthing_id,start_time,end_timeall required; there is deliberately nounbounded form. Scope matches the
GETon the same path, so the set a clientpreviews is the set this removes. Blocks are reconciled after: one left empty
is deleted, one left partial has its span narrowed to the survivors.
Schema
Provenance on the block (
source_file,source_kind, orderedcorrections;provenance.notesreuses the existingcomment), and a per-readingnotesetonly where a correction moved the value — so NULL reads as "as measured" rather
than "unknown". A corrected block is derived data, and a reviewer who cannot see
that a series was snapped to a manual measurement cannot review it.
The block time-order check relaxes from
>to>=. A block covering a singleinstant is legitimate — published that way, or narrowed to it by a delete — and
the reader matches inclusively, so a zero-width block still covers its reading.
Loosening a check constraint cannot invalidate existing rows.
Reviewer notes
Overlap is inclusive on both bounds here, unlike
TransducerObservationBlock.overlaps, which is half-open. The reader matcheswith
start <= t <= end, so two blocks sharing an endpoint both claim anyreading at that instant — exactly the ambiguity the publish conflict check
exists to prevent. This divergence is deliberate and documented in
domain/hydrograph.py.replace_overlapping=truedeletes the superseded readings too. Keeping themwould leave rows the reader cannot show (no block covers them) that still occupy
the deployment/parameter/instant the new series is about to claim — so a
"replace" that kept them would fail on the very insert it was asked to make room
for. Readings orphaned by a block deleted some other way are caught separately
and reported as a 409 naming the earliest colliding timestamp, rather than
letting the insert abort the transaction with a constraint name.
Range delete leaves
transducer_daily_datastale until its next scheduledrefresh. Nothing here refreshes it — a full refresh per delete would cost far
more than the correctness it buys between nightly runs.
Authorization — needs an Authentik grant before anyone can use this
Both write routes are gated on
AMP.Staging, a standalone group:AMPAdmindoes not satisfy it, and it satisfies nothing else. Nobody holds it until it is
granted, so the routes ship dark and are reachable only by whoever is validating
the workbench against real logger files. Deliberately not a fourth AMP tier —
when the workbench is trusted these move to
amp_admin_dependencyand the groupgoes away, which stays a one-line edit this way.
The read route stays on
amp_viewer_dependency; publishing does not change whomay look.
Two bugs fixed in passing
get_transducer_observationspositionally, and thehelper's fourth positional parameter is
sensor_id.start_timelanded insensor_id(unused, silently dropped),end_timelanded instart_time, andend_timewas never set — a requested upper bound was ignored and the lowerbound came from the wrong argument. Now keyword-only.
sort/orderon that route were accepted and ignored. They now work over awhitelist (
observation_datetime,value,id); an unrecognised field is a422 rather than a silently differently ordered page, since that reads as the
data changing, not as a bad request.
Deployment
Migration
c3d4e5f6a7b8adds three nullable columns totransducer_observation_block, one totransducer_observation, and relaxes theblock time-order check. Applied to the local test database only — not yet run
against staging or production. Per
docs/, CD runs alembic, so this goes outwith the deploy.
Testing
Full suite: 978 passed, 81 skipped, 6 xpassed.
49 new tests across two files:
tests/test_domain_hydrograph.py— 22 DB-free rule tests (span derivation,duplicate/out-of-order indices, the inclusive-vs-half-open overlap case,
deployment resolution, block narrowing, inverted range).
tests/test_transducer_publish.py— 27 endpoint tests (publish, provenanceround-trip, per-reading notes, 409 overlap and replace, the orphan-readings
409 with rollback asserted, explicit vs resolved deployment, 404s, six 422
paths, delete whole/partial/inverted/unbounded/unknown-well, ordering).
Coverage:
domain/hydrograph.pyandservices/transducer_helper.pyat 100%,core/dependencies.py100%,schemas/transducer.py99% (the one miss is araiseinside anexceptthat provably executes — asserted on its uniquemessage; coverage misattributes the line). The new route bodies in
api/observation.pyare fully covered.Unrelated tooling note found while measuring:
uv run pytest --covasdocumented in
CLAUDE.mdis broken repo-wide — any--cov=<dotted.module>thattransitively imports numpy dies with
ImportError: cannot load module more than once per process, reproducible on untouched code.--cov=.works.Not built
Everything in the contract's Wellntel section is deferred: the
GET /wellntel/readingsproxy and thesensor_typefilter onGET /thing.Both are blocked on open questions the contract itself raises — where the
Wellntel API key lives and where the wellname→PointID mapping belongs. The UI
already falls back to demo data when they are absent.
🤖 Generated with Claude Code