Add RuBR under rapid - #8
Merged
Merged
Conversation
ryanlauastro
self-requested a review
May 29, 2026 21:36
ryanlauastro
approved these changes
May 29, 2026
ryanlauastro
left a comment
ryanlauastro
left a comment
Collaborator
There was a problem hiding this comment.
Looks good after preliminary lookthrough
rusholme
added a commit
that referenced
this pull request
Aug 6, 2026
…defects The external implementation review sampled the extracted stage bodies against the deleted monolith and found five fidelity defects. It sampled; this sweeps. The authority throughout is `5664024^:pipeline/awsBatchSubmitJobs_runSingle*`. #7 SFFT could not be invoked at all. The argv carried six positional inputs and `--sci_star_list`/`--ref_star_list`/`--crossconv_flag`; the tool's own parser takes two positionals and `--scicat`/`--refcat`/`--crossconv` (store_true). Every SFFT-enabled science job died in argparse with status 2. The branch was also inverted: the monolith passes catalogues in the non-rimtimsim case, with 50.0/100.0 bright-source masking, and none in the rimtimsim case, with 20000.0/30.0. Restored, with the crossconv-dependent output filenames the tool actually writes. #8 Statistics lookups named keys that do not exist. `clippedmed` (x4), `datascale`, `gmin`, `gmax`, `npixsat`, `npixnan` — the helper returns `gmed`, `gsigma`, `gdatamin`, `gdatamax`, `satcount`, `nancount`. Every dedicated reference-image job died here. #19 SFFT and naive silently used ZOGY's science. Both detected on ZOGY's Scorr image, weighted with ZOGY's uncertainty image and fitted with ZOGY's difference PSF. Each variant now builds its own uncertainty image, uses its own PSF (the reference PSF for naive, per the monolith's TODO), and detects on the right image. The naive branch had also dropped its coverage-map masking entirely. #20 Positive and negative PSF catalogues overwrote each other — one configured filename for both signs, so both product names carried negative bytes. The schema was also reduced: no RA/Dec columns, no finder catalogue, and the parquet written from the unjoined table. Restored, join included. #21 The clipped science average was computed after fake-source injection, so injected pixels entered the uncertainty model. The monolith computed it at line 798 and injection opened at 806. Split into its own stage, sequenced ahead of injection — a sequence position is visible where an inline computation's is not. #22 The inline reference build dropped `generatePhotUtilsReferenceImageCatalog`. A science job building its own reference produced no reference PSF/finder catalogues. Also restored its saturation-rate divisor: the science monolith divides by the science exposure time, where the dedicated reference-image pipeline divides by 60.0. Both reproduced against their own authority. #23 The difference image was stamped with the reference pipeline's PPID. The monolith read one science ppid (15) and stamped it into both headers. `ppid_for(JOB_TYPE_SCIENCE)` is that value and needs no manifest fact. Two defects the sweep found beyond the review's sample: * Thirteen release-content keys the W4B config migration dropped while stage code still read them by name — the psfcat filename triples for all three variants and the reference image, ZOGY's three outputs, awaicgen's three, bkgest's two, and the naive difference's. Each a KeyError in a stage that had already done real work. * `prepare_zogy_inputs` clipped ZOGY's input reference statistics at the saturation *rate* where the monolith used the undivided level, and fell back to a config key that does not exist when the reference was downloaded rather than built. The regression tests assert against real artefacts, not restated expectations: the SFFT argv is parsed by the tool's own parser, lifted from its source; the statistics key names are read out of the helper's own source; every release key a stage reads is checked against the real TOML. Both were wrong together before, which is what a test of the code against itself cannot catch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TdQSan34f2B5fg4J8hauKi
rusholme
added a commit
that referenced
this pull request
Aug 6, 2026
… wrong Nine findings, three P0. Two of the nine summaries were materially wrong about the mechanism while being right that a defect existed, and one was wrong about half its claim -- so the rows record what re-verification actually found rather than restating the report: #8's headline claim is FALSE: dispatch_registration does use the approved connection helper. The real defect is worse -- two concurrent connections on contradictory contracts with the transaction boundary between them. #5's "FixA staged it" is FALSE: no bundle-reconstruction code existed anywhere. retention.py reconstructs TAGS, not bundles, which is the likeliest source of the confusion. #5's read-failure half does not hold: store faults and genuine absence ARE distinguished and the record-store path already defers. Only the bundle half is real. Also recorded: the mechanism #1's summary missed (the CLASS_MATERIALIZED branch has no return, so both writers run and the second strands the first's checksum); that hp6/hp9 existed nowhere and had to be derived rather than copied; that scalefacref is the reciprocal of what the stage records; and why rid='null' works at all -- it is a PostgreSQL type predicate, not a comparison. Two findings are recorded rather than fixed, both instances of the same pattern the payload proposal's as-is finding 7 names: rapid_db.py exits 64 from library code, and virtualPipelineOperator reads four env vars at module scope with the same treatment, so importing the operator terminates the interpreter when any is unset. Moving those checks is a change to the operator's startup contract, which the operations design owns.
rusholme
added a commit
that referenced
this pull request
Aug 6, 2026
…wn connection Round-4 findings #1 and #2, both in the production VPO. #1 — `submission_env(job_type)` took the job type and ignored it, returning one singular RAPID_JOB_QUEUE/RAPID_JOB_DEFINITION pair to all three phases. The route matrix does not allow that: reference-image runs on the bulk class, science and post-process on prompt. Whichever pair was configured, at least one phase reached a queue whose job definition names the other class, and `validate_route` rejects it at the entrypoint before any processing. The queue and definition now come from the route matrix, resolved through the parameter tree the entrypoint validates against — one fact, one home. The four keys are live (`/rapid/pipeline/batch/{queue,job-definition}-*`, verified 2026-08-06). The image and release facts stay in the environment, where they belong: they change with every build. #2 — `production_registrar()` returned a callback built over `registrar(rapid_db.RAPIDDB, store)`, so the registrar opened a second, autocommitting connection while `run_registration(regconn, ...)` advanced the watermark on the first. Two connections cannot be one transaction: product rows became durable before the watermark was attempted, and a crash between them left rows written with the attempt still a candidate. That is round-3 finding #8, fixed in the registration job and reintroduced here. It is now a factory taking the pass's connection, matching `entrypoints.job.registrar_for(context, conn)` deliberately rather than by coincidence. A factory because the three phases each open their own registration connection; one callback built once could borrow only one. Also fixed, found while writing the routing tests: `submission_env` built an `ExecutionBinding` with `manifest_checksum=None`, which raises `ValueError` unconditionally — the operator could not submit anything at all. The checksum is a property of a batch and is not knowable when the operator resolves its binding, so this carries the four facts it does know as `SubmissionBinding` and `submit_gathered` builds the validated binding once the manifest exists. Tests: routing asserted per phase at the submit-call boundary, so no submission is needed to know which queue a phase would reach; the registrar test fails on two connections by inspecting the handle it would build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fwWUXXLB9sD46eN8Ctfog
rusholme
added a commit
that referenced
this pull request
Aug 9, 2026
Seen on the first successful width-2 probe, in its own output:
registration pass: {'registered': 0, ..., 'would_register': 1087}
The operator passed `registrar_factory=None`, and
`run_registration` turns a missing registrar into `dry_run=True`. So a
LIVE pass decided 1,087 registrations, reported success, and wrote no
rows -- the "registration reported success while writing nothing" defect
the consumer already fixed once (review finding #5), reintroduced one
layer up by the operator that replaced its caller.
A live pass now builds the real registrar via
`virtualPipelineOperator.production_registrar`, which owns it: the S3
record store is built once and the registrar binds to each pass's OWN
connection, so product rows and the registration watermark commit in a
single transaction. Two connections cannot be one transaction (round-3
finding #8), and rebuilding that here would be the third place to get it
wrong.
A rehearsal keeps None deliberately -- it holds no connection either, and
a rehearsal that wrote registration rows would be a rehearsal with
effects, which is the whole thing this restructure exists to prevent.
47 tests, exit 0.
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
2 participants
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.
No description provided.