P3-15′-e-ii: bound each mlocarna alignment so a pathological candidate is spared, not fatal - #117
Merged
Merged
Conversation
…e is spared, not fatal
SLURM job 1205 (the full-corpus covariation-(a) supply run) lost shard 016 to the
12 h wall having completed 1 of its 8 alignable candidates: a single mlocarna ran
6h11m of continuous CPU without finishing. A longer --time is not the fix — seven
alignments remained, and the align-wall distribution has no useful tail bound.
Measured over the 272 alignments that succeeded in the 47 surviving shards
(align_wall_s, read off the run's own status tables): median 8.5 s, p95 35.4 s,
p99 165.2 s, second-largest 352.7 s (depth 232), largest 8500.2 s (depth 844).
The 24x gap between the two slowest means any bound in [353 s, 8500 s) admits the
same 271 alignments and rejects the same one. That single alignment is 69 % of all
align wall-clock in the corpus (3.41 h across 47 shards). Runtime tracks MSA depth
— ADR-0005 A10 Pin 2's own "LocARNA scaling caveat", made quantitative.
Inputs: $ROUND_DIR/status/status_{000..047}.json (job 1205, 47 x 20 rows)
Outputs: homolog_db.ToolTimeoutError + _run(timeout_s=) + _run_bounded +
_kill_process_group; homolog_msa.align_candidate(timeout_s=);
covariation_producer.align_shard(align_timeout_s=) + _discard_msa +
--align-timeout-s; ${ALIGN_TIMEOUT_S:?} in mine_round_producer.sbatch,
mine_round.sbatch (leg (c) export AND forward) and
mine_round_measure.sbatch; tests/unit/test_align_timeout.py (33 tests)
The bound kills the whole PROCESS GROUP, not just the child: subprocess.run's own
timeout reaps only the process it spawned, and mlocarna is a Perl driver that
dispatches locarna workers, so a plain timeout would orphan them and leave the
node's cores burning — bounded on paper, unbounded in fact. _kill_process_group
refuses to signal its own group, so a future edit dropping start_new_session
degrades to killing the child instead of SIGKILLing the SLURM array task.
A timeout resolves to reason="align_timeout" with no MSA written, hence
unavailable => SPARED (ADR-0005 D14) — never a mined negative. _discard_msa makes
that absence true by construction: the promote block tests [ -s .../msa.sto ], not
this run's verdict, so a stale consensus would otherwise be promoted and scored as
de-novo output this run never produced (CLAUDE.md §10.3). The bound is
keyword-required-no-default at every seam (the ADR-0006 A4 rule-parameter shape)
so a caller that forgets it fails loudly rather than re-running 1205 unbounded.
Disclosed, not hidden: the shard-016 re-run scores under a stricter producibility
bar than the other 47 shards; a bound can only turn passed/failed into unavailable
=> spared, and mining is a conjunction, so it costs yield and never correctness.
mine_round_measure.sbatch is now right-censored at the bound — the job that exists
to measure the wall distribution cannot observe a tail beyond it, so envelopes
derived from it are lower bounds. Amending A10's envelope is a §7 item-2 sign-off,
not taken here.
Validation: tests/unit + tests/golden under deterministic ordering — ZERO new
failures vs a matched main baseline at 0a57017 (worktree 13 FAILED, main 26,
worktree a strict subset; the delta is coverage_sim + selection_val SKIPPING on
unmaterialised DVC inputs). 18 sabotages all RED against their named test — after
S14 exposed one of my own tests as unable to distinguish a threaded value from a
hardcoded constant (it asserted 600.0, the value the sabotage hardcodes); it now
asserts two distinct non-round values. bash -n, ruff and black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds required per-candidate alignment timeouts. Timed-out subprocess groups are terminated and classified separately. Stale MSA files are removed, candidates become unavailable, and SLURM scripts forward the timeout configuration. ChangesAlignment timeout enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SLURM
participant align_shard
participant align_candidate
participant mlocarna
SLURM->>align_shard: Pass --align-timeout-s
align_shard->>align_candidate: Pass align_timeout_s
align_candidate->>mlocarna: Run with timeout_s
mlocarna-->>align_candidate: Alignment or timeout
align_candidate-->>align_shard: Result or ToolTimeoutError
align_shard->>align_shard: Remove stale MSA and mark unavailable
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ll real 1. (major) A nan bound could masquerade as a bound. `type=float` accepts nan/inf, and every comparison against nan is False, so `nan <= 0` was False and the guard waved it through; `communicate(timeout=nan)` then never fires. The bound would be declared, recorded in provenance, and completely inert — the unbounded behaviour that lost shard 016, wearing the fix's clothes. Added `assert_usable_timeout` (positive AND finite), an argparse type that applies it, and an up-front check in `align_shard` so an unusable bound dies before the ~37 min search stage rather than on the first sufficient candidate. 2. (major) The `insufficient_homologs` branch did not discard a stale MSA — I had fixed two of three spare branches. That branch `continue`s before the try/except, so a leftover msa.sto under a re-used workdir survived and would be promoted and scored as a de-novo consensus the run never produced. 3. (major) The post-kill pipe drain was unbounded. `_kill_process_group` kills the group, but a grandchild that escaped it (e.g. by calling setsid) still holds the inherited stdout/stderr write ends, so `communicate()` reads toward an EOF that never arrives — an unbounded wait inside the code path whose job is to bound one. Bounded at DRAIN_TIMEOUT_S = 5.0. 4. (minor) The `ALIGN_TIMEOUT_S:-` negative assertion searched comments too — the same comment-filter trap I had already fixed for the invocation extractor and then did not apply to its sibling in the same file. 5-6. (minor) The documented SUBMIT lines in mine_round.sbatch and mine_round_measure.sbatch did not export ALIGN_TIMEOUT_S, which those files now declare required. An operator following the header would abort the job it documents — and for mine_round.sbatch only after leg (a) had spent 8 GPUs. Validation: 47 tests (14 new), all green; 7 sabotages of these six fixes, each RED against its named test; zero new failures vs the matched main baseline at 0a57017 under deterministic ordering; ruff, black and bash -n clean. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
1. The `:?` positive assertion still searched the RAW file text while the `:-`
negative assertion three lines below searched the comment-stripped body — the
asymmetry introduced by r1's own fix, so "fixed one of two identical things"
twice over on the same pair. Verified by an isolated counterfactual rather than
asserted: an sbatch whose body hardcodes ALIGN_TIMEOUT_S="600" while its header
merely QUOTES the `:?` form passed all four assertions GREEN before this change
and goes RED after it. Both assertions now read the code.
2. The reaping test wrote its pidfile with open().write(), which creates the file
and then fills it; a timeout landing between those steps left an empty pid.txt
and int("") failed the test for a reason unrelated to reaping. Written via
os.replace, with an explicit non-empty assertion before parsing.
Validation: 47 tests green; the r2-F1 fix sabotage-checked with its counterfactual;
zero new failures vs the matched main baseline at 0a57017 under deterministic
ordering; ruff and black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slurm/p2/mine_round.sbatch (1)
132-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
ALIGN_TIMEOUT_Sis checked for presence, not for value, in all three sbatch scripts. The${ALIGN_TIMEOUT_S:?…}pattern rejects an absent or empty value only. A malformed value such as60opasses every script and is rejected only by--align-timeout-sargparse inside each producer task, after that task has already spent its search stage. A numeric guard at declaration time moves the failure before the expensive legs.
slurm/p2/mine_round.sbatch#L132-L136: add the numeric guard after the:?assignment, so the orchestrator aborts before the 8-GPU scan legs and before it forwards the value at Line 195.slurm/p2/mine_round_measure.sbatch#L119-L127: add the same guard after Line 127, before the scan leg.slurm/p2/mine_round_producer.sbatch#L69-L72: add the same guard after Line 72, before STAGE 1.♻️ Proposed guard (identical in all three files)
ALIGN_TIMEOUT_S="${ALIGN_TIMEOUT_S:?set ALIGN_TIMEOUT_S (per-candidate mlocarna bound, s) via --export}" +case "$ALIGN_TIMEOUT_S" in + ''|*[!0-9.]*|.|*.*.*) echo "FATAL: ALIGN_TIMEOUT_S must be a positive number of seconds (got '$ALIGN_TIMEOUT_S')" >&2; exit 2 ;; +esac🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slurm/p2/mine_round.sbatch` around lines 132 - 136, The ALIGN_TIMEOUT_S declarations in slurm/p2/mine_round.sbatch lines 132-136, slurm/p2/mine_round_measure.sbatch lines 119-127, and slurm/p2/mine_round_producer.sbatch lines 69-72 only reject missing or empty values. Add the identical numeric validation guard immediately after each existing required-value assignment, before scan or stage execution and before forwarding the value, so malformed values fail in the submitting script.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@slurm/p2/mine_round.sbatch`:
- Around line 132-136: The ALIGN_TIMEOUT_S declarations in
slurm/p2/mine_round.sbatch lines 132-136, slurm/p2/mine_round_measure.sbatch
lines 119-127, and slurm/p2/mine_round_producer.sbatch lines 69-72 only reject
missing or empty values. Add the identical numeric validation guard immediately
after each existing required-value assignment, before scan or stage execution
and before forwarding the value, so malformed values fail in the submitting
script.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0aba8fa-4d65-45f5-99f0-e4d24157eaca
📒 Files selected for processing (8)
slurm/p2/mine_round.sbatchslurm/p2/mine_round_measure.sbatchslurm/p2/mine_round_producer.sbatchsrc/tbox_finder/mining/covariation_producer.pysrc/tbox_finder/mining/homolog_db.pysrc/tbox_finder/mining/homolog_msa.pytests/unit/test_align_timeout.pytests/unit/test_covariation_producer.py
… fix for one was wrong
1. The documented-submit check asserted against the CONCATENATION of every comment
block containing "sbatch", so a header sentence mentioning `sbatch` in one place
and `ALIGN_TIMEOUT_S=` in another satisfied it while the real submit command
omitted the export. That is the third instance in this PR of a test that could
pass on prose. Now asserted per command.
⚠ The reviewer's proposed fix — keep every block containing ".sbatch" and require
the export in all of them — would have FAILED on the correct tree: 9 of the 12
such blocks across these three files are prose ABOUT another script
("mine_round.sbatch's leg (c) submits WITHOUT ..."). The extractor instead
requires a real `sbatch` INVOCATION (a command boundary before the token, a flag
or script path after it — which excludes the `sbatch` inside `mine_round.sbatch`,
preceded by a dot) AND that the block names THIS file. It selects exactly one
command per file, verified by enumeration.
Sabotage-checked with its counterfactual: dropping the export from the real
submit line while leaving prose that mentions it is RED now and was GREEN under
the concatenation check.
2. The reaping test read the pidfile without checking it exists, so a timeout landing
before `os.replace` raised FileNotFoundError and the assertion message never ran.
Demonstrated: with the child not recording its pid, the failure is now
"the child never recorded its pid — the test cannot say anything about reaping"
where it was previously a bare FileNotFoundError.
Validation: 47 tests green; zero new failures vs the matched main baseline at
0a57017 under deterministic ordering; ruff and black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
… its presence
CodeRabbit app r4 (1 nitpick, real): `${ALIGN_TIMEOUT_S:?…}` rejects an absent or
empty value only. A malformed one like `60o` passed all three scripts and was caught
by argparse inside the producer task — after that task had already spent its ~37 min
search stage, and from the orchestrator after the 8-GPU scan legs had run.
All three sbatch files now check shape (digits, at most one dot) and then reject a
numeric zero, immediately after the `:?` assignment. The zero case is the one that
matters most: because a timeout SPARES rather than fails, `ALIGN_TIMEOUT_S=0` would
mark every candidate `unavailable` and read as a clean, producible-nothing round.
Tested by EXECUTING the shipped bytes — the declaration plus guard are lifted out of
each sbatch and run under bash, not retyped, so the test exercises the line that runs
on the cluster: 14 values x 3 files. Plus a position test asserting the guard precedes
each file's first expensive leg.
⚠ That position test failed twice on correct code before it was right, both times for
the same reason this PR keeps hitting: it matched a string instead of a command.
`conda activate` appears in a header comment AND in a trailing comment
(`set -eo pipefail # NOT -u: conda activate hooks read unset vars.`), and it is not
the expensive leg anyway — mine_round.sbatch activates its env to run a cheap no-GPU
readiness preflight. The test now names each file's first expensive leg explicitly
(the search stage; LEG a's 8-GPU scan).
Validation: 92 tests green; 3 sabotages (drop the shape guard, drop the zero guard,
move the guard after the expensive leg) each RED against its named test; zero new
failures vs the matched main baseline at 0a57017 under deterministic ordering;
bash -n, ruff and black clean.
Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
… class they exposed Four rounds, 11 findings, all real (6/2/2/1). Three changed shipped behaviour: the nan bound that slipped past a `<= 0` guard, the third spare branch that kept a stale MSA, and the unbounded post-kill drain. One review-suggested fix was wrong and is recorded as such — it would have failed on the correct tree. Also records the class this PR hit four times: a byte-level check that matches PROSE, because these sbatch files document their own commands at length. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
bioedca
added a commit
that referenced
this pull request
Aug 8, 2026
… off P3-15'-e (#118) Two dev-log defects from one cause: a `cd` left the shell in the MAIN checkout, so `cat >> analyses/phase3_log.qmd` appended the P3-15'-e-ii stanza there instead of in the worktree. It never reached PR #117 (that commit reads "8 files changed" with no dev-log entry), and the later review-round addendum — appended correctly in the worktree, where the file's last stanza was still P3-15'-e — merged attached to the WRONG step. This lands the stanza CLAUDE.md §2.4 requires for the step, with the addendum inside it, and removes the addendum from P3-15'-e's stanza, which ends cleanly again. Nothing executable changes; the diff is one Quarto file. Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM Co-authored-by: bioedca <bioedca@gmail.com>
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.
Why
SLURM job 1205 (the full-corpus covariation-(a) supply run) lost shard 016 to the 12 h wall having completed 1 of its 8 alignable candidates — one
mlocarnaran 6h11m of continuous CPU without finishing. CLAUDE.md §9.3 step 8 recorded that as a stop-and-ask; the chosen remedy is a per-alignment bound, not a longer wall (seven alignments remained).The bound is measured, not guessed
Read off the run's own status tables (
align_wall_sfor the 272 alignments that succeeded in the 47 surviving shards):There is a 24× gap between the two slowest, so any bound in [353 s, 8500 s) admits exactly the same 271 alignments and rejects exactly the same one. 600 s is 1.7× the slowest alignment that ever finished cleanly and caps a 20-candidate shard at 3.3 h against a 12 h wall. Runtime tracks MSA depth — ADR-0005 A10 Pin 2's own "LocARNA scaling caveat", made quantitative. That single 8500 s alignment is 69 % of all align wall-clock in the corpus (3.41 h across 47 shards).
What changed
homolog_db:ToolTimeoutError,_run(timeout_s=)(None= the unchanged unbounded path, so the certified job-741/766 search and index calls run byte-identically),_run_bounded,_kill_process_group.homolog_msa.align_candidate(timeout_s=)→covariation_producer.align_shard(align_timeout_s=)keyword-required, no default (the ADR-0006 A4 rule-parameter shape) → CLI--align-timeout-s required=True→${ALIGN_TIMEOUT_S:?}in all three sbatch files that align, includingmine_round.sbatchleg (c), which both declares and forwards it._discard_msaon every non-success branch.The kill takes the whole process group.
subprocess.run(timeout=…)reaps only the process it spawned;mlocarnais a Perl driver that dispatcheslocarnaworkers (--threadsis a real parallelism knob), so a plain timeout would orphan them and leave the node's cores burning — bounded on paper, unbounded in fact._kill_process_groupalso refuses to signal its own group, so a future edit droppingstart_new_sessiondegrades to killing the child rather thanSIGKILLing the SLURM array task itself.A timeout spares.
reason="align_timeout", no MSA written ⇒unavailable⇒ spared (ADR-0005 D14), never a mined negative — and distinct fromalign_failed/insufficient_homologs, because all three spare but only this one follows from a knob the run chose._discard_msamakes that absence true by construction: the promote block tests[ -s …/msa.sto ], not this run's verdict, so a stale consensus under a re-used workdir would otherwise be promoted and scored as de-novo output this run never produced (§10.3).Disclosed, not hidden
passed/failedintounavailable⇒ spared, and mining is a conjunction, so it costs yield and never correctness.mine_round_measure.sbatchis now right-censored at the bound — the job that exists to measure the wall distribution cannot observe a tail beyond it, so any envelope derived from it is a lower bound. This is exactly how A10 Pin 2 came to price a round from a K=50 sample whose max align was 47.5 s. Amending A10's envelope is a §7 item-2 sign-off, not taken.Validation
tests/unit+tests/goldenunder deterministic ordering: zero new failures vs a matchedmainbaseline at0a57017(worktree 13 FAILED, main 26, worktree a strict subset). The 13-test delta istest_coverage_sim+test_selection_valskipping on unmaterialised DVC inputs in the worktree — the same delta P3-15′-e recorded.align_failed.600.0, the same value the sabotage hardcodes into_cmd_align_shard, so it could not distinguish a threaded value from a constant. Re-written to assert two distinct non-round values (137.5,911).bash -nclean on all three sbatch files; ruff + black clean.Not in this PR
The re-run itself — that is a §9.3 submit-ack, still open.
https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
Summary by CodeRabbit
New Features
Bug Fixes
Tests