Merge new alert code into SMDC - #16
Conversation
(cherry picked from commit bac7bc8)
(cherry picked from commit 3beed41)
(cherry picked from commit 9f90540)
(cherry picked from commit e5d4bd5)
(cherry picked from commit da08cc3)
…entiation from PSF fit (cherry picked from commit 46f8c8c)
(cherry picked from commit b492b03)
(cherry picked from commit 70a78d3)
(cherry picked from commit e5dc51b)
(cherry picked from commit 053d0c6)
(cherry picked from commit eb9145f)
(cherry picked from commit 0bdfb18)
(cherry picked from commit 07548ff)
(cherry picked from commit 3e1d3fe)
rusholme
left a comment
rusholme
left a comment
There was a problem hiding this comment.
Claude-drafted review (Ben's session; findings verified against smdc-alerts by hand before posting). Comment-type rather than "request changes" — please address 1–3 before merging, the rest can follow.
Summary of the inline comments, in priority order:
AssociationErroris now raised for a source with nomerges_<field>row, but nothing catches it: one bad row aborts the whole chip'sbatch_producerun (and the CLI invocation).- Shipped docs and the committed
sample_alert.avrostill say/embed schema 00.02;latest.txtandparam_registry.VERSIONare 00.03. alerts/schema/00/02/is a dead intermediate created and superseded inside this PR — drop it.- kete pinned to
>=1.1.0,<2(PyPI is at 3.2.2) with no stated reason. CLASS_STAR0.5/0.5 split — "need to fix" commit, still needs team sign-off (not a code defect).sample_data/alert.jsonis missingtimeProcessedMjd; nothing tests sample JSON against the.avsc.- Deploy dependency: schema 00.03 must be registered in the Glue Schema Registry before this runs, or producers fail at first use — worth a line in the PR/runbook.
Not inline-able: docs/source/prod/sample_alert.avro is a ~218 KB binary in git. Defensible (the regen needs a live DB, so a docs-build step can't make it) — just note the exception where the no-binaries convention is stated.
Checked and sound: the retry/abort logic in _stage/get_cutouts, the memory-leak fix and timestamp handling in efa89a0, the MPC-orbits → ssMatch/refMatch rename (consistent across avsc, providers, docs), the KONA wiring, live-DB test isolation, no credentials or account IDs. Merge shape is clean: all 15 commits yours, nothing outside alerts/, docs/, modules/solarsystem/, requirements*.
Separately (outside this PR's diff): pipeline/stages/alert_production.py mentions batch_produce only in a stale past-tense docstring and never calls it — drift or unfinished integration? Worth reconciling when you fix item 1.
| row = self._chip_objects.get(detection.sid) | ||
| if row is None: | ||
| return None | ||
| raise AssociationError( |
There was a problem hiding this comment.
1 (fix before merge). AssociationError replaces the old return-None behaviour here and at the second raise site below — a deliberate contract change, fine — but no caller catches it. batch_produce (alerts/produce.py:662) loops provider.iter_sources(pid) with no try/except, and alerts/cli.py doesn't catch around the call either, so one source with a missing merges_<field> row (stale partition, cross-matching race) kills every other source on that chip. Same gap for CutoutStagingError on the required=True path (failing loud there is legitimate, but it's still uncaught).
Suggested fix: wrap the loop body in batch_produce with try/except AssociationError — log, count the drop, continue the chip.
| versioning with Confluent schema in order to use. This file's schema | ||
| may become out of date from the current alert code. | ||
|
|
||
| :download:`sample_alert.avro <sample_alert.avro>` (schema version ``00.02``). |
There was a problem hiding this comment.
2 (fix before merge). Hardcoded 00.02, but param_registry.VERSION and alerts/schema/latest.txt are 00.03 — and the committed sample_alert.avro embeds writer schema rapid.v00_02.alert (checked with fastavro). This PR adds both the guard against exactly this — docs/update_sample_alert.sh warns on a products.rst/VERSION mismatch — and the |alert_schema_version| substitution in conf.py; neither was applied here.
Fix: use |alert_schema_version| on this line, rerun docs/update_sample_alert.sh against a 00.03 record, commit the regenerated .avro.
| @@ -0,0 +1,127 @@ | |||
| { | |||
There was a problem hiding this comment.
3 (fix before merge). 00/02 didn't exist on smdc before this PR: created in a8b9097, superseded by 00/03 in e949bb2, never referenced by code, docs, or latest.txt afterwards. A full never-released schema version shipped as the PR's own scratch step — and it's what made item 2 easy to get wrong. Drop the whole alerts/schema/00/02/ directory.
| @@ -1,6 +1,3 @@ | |||
| kete | |||
| kete>=1.1.0,<2 | |||
There was a problem hiding this comment.
4. This is a two-major-version downgrade (unconstrained kete resolves to 3.2.2 on PyPI today), and rapid_kona.py is correctly moved to the 1.x API in lockstep — but no commit message or comment says why 3.x was rejected. One line here (bug? incompatibility? convenience?) so the pin isn't blindly bumped back later.
(Dropping sys/logging/os from this file is correct — stdlib never belonged here.)
There was a problem hiding this comment.
We need kete 1.x due to the fact that kete 3.x is maintained outside of IPAC and may not be stable. It didn't end up being a big change, but I'll add a comment about this!
| # tree. Equal thresholds make the split exhaustive; moving them apart | ||
| # excludes an unclassifiable middle band from both trees. | ||
| # TODO: settle the threshold(s) with the team (possible three-way split); | ||
| # CLASS_STAR is unreliable at faint mags, so each match carries its score. |
There was a problem hiding this comment.
5. bbbb856 ("Changed CLASS_STAR threshold, but need to fix") moved these from 0.9/0.9 to 0.5/0.5 and nothing revisits it. The split is still exhaustive and each match carries its own classStar, so this isn't a functional defect — but the TODO above needs a team decision. Please get sign-off on the threshold before or soon after merge, and say in the commit which it is.
| @@ -0,0 +1,277 @@ | |||
| { | |||
There was a problem hiding this comment.
6. The diaSource object here is missing timeProcessedMjd, which rapid.v00_03.diaSource.avsc declares and test_schema.py confirms is stamped at assembly. Nothing currently checks sample JSON against its .avsc (the schema tests only compare .avsc vs param_registry.py). Regenerate via alerts/test/gen_sample_alert.py and consider a test asserting each sample_data/*.json field set matches its schema.
| @@ -1 +1 @@ | |||
| 00.01 | |||
| 00.03 | |||
There was a problem hiding this comment.
7 (deploy note, not a code fix). GlueSchemaRegistry.schema_version_id raises KeyError for an unregistered schema name — correct, fail-closed. But nothing in the PR body or docs says ops must register 00.03 in the Glue Schema Registry before this deploys, or producers hard-fail at first use. One line in the PR description or the deploy runbook.
2 participants
Automatically and manually merged commits dealing with the alert code from dev (with alerts branch merged in and removed) to SMDC.