Docker - #90
Merged
Merged
Conversation
…to fetch resources if needed (don't require S3 locally); add download_hash script in resource_helpers
…3 sync calls for hash fetching
… test volume var for process worker
…o controls going in to overlap script
…/logs; exec instead of run - just use single container; not yet combining or error checking
…invoc file so as to not overwrite
…sing; remove escape parens + white space in vcf header
Fix/bwa index validation
RUFUS could not actually run from FASTQ: the -s/-c parser added the raw fastq to the sample list (which the generator-builder then rejected) while writing an orphaned generator, and FastqToSam.pl emitted invalid SAM (MAPQ='*', a trailing tab, no header) that samtools fastq rejected. FASTQ is unaligned, so it cannot be region-scoped; this wires it up as whole-genome input only (a clear error if -R is given): - FastqToSam.pl now emits valid unmapped SAM records (flag 4, MAPQ 0) with a single @hd header on the first call. It feeds k-mer COUNTING only, where pairing is irrelevant. - The -s/-c parser routes fastq to separate lists; the generator is built from FastqToSam.pl for counting. - Paired FASTQ filtering reuses the existing, tested -q1/-q2 direct path: the two -s files are auto-wired into _arg_fastqA/_arg_fastqB, so no pairing flags or collate/mate-split are needed. An explicit -q1/-q2 still wins. Single-end (-se) is left to the RUFUS.Filter.single path. - Subject/control emptiness checks now count the fastq lists; FASTQ input requires -r. Also fix a whole-genome-mode crash this exposed: post-processing ran `bcftools view -r "$_arg_region"` with an empty region, which segfaults. It now passes calls through unchanged when there is no region. This affected any whole-genome run, not just FASTQ. Validated on the functional fixtures (whole-genome paired FASTQ): all 5 planted somatic variants recovered, identical to the BAM and CRAM paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The single-end filter branch opened the mutant-read FIFO with `exec 6<>"$FIFO_MAIN"` and only closed it (`exec 6>&-`) after `wait`. That read+write fd kept a writer on the pipe open, so RUFUS.Filter.single never saw EOF once `samtools fastq` finished — it blocked forever in its read loop and the `wait` never returned. Any single-end (-se) run hung indefinitely (observed: a tiny fixture timed out at 30m; the paired path does the same work in ~35s). Remove the fd trick; the backgrounded writer and reader rendezvous on the FIFO on their own and EOF is delivered normally. With this, single-end runs to completion and interpret recovers all planted variants. (The final VCF is separately affected by the known strand-bias filter; that is tracked independently.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
F5 paired-end FASTQ (whole-genome): asserts the final VCF recovers all five planted somatic variants with no spurious calls — the paired path works end to end. F6 single-end FASTQ (-se, whole-genome): asserts the run completes (a bounded walltime turns a FIFO-hang regression into a failure) and that interpret recovers all five somatics, checked at the RAW interpret VCF rather than the final VCF. Single-end calls are currently tagged StrandBias and dropped by the post-filter, so the final VCF is empty; that strand-bias behaviour is a separate known issue. The test asserts upstream of it and notes to tighten to the final VCF once it is fixed. Both take an optional EXTRA_BIND to overlay uncommitted code before the image is rebuilt; it is a no-op once FASTQ support ships in the image. Validated against the current code: F5 and F6 both pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Runs the somatic scenario from BAM and from CRAM (the -cr decode path) and asserts the two call sets are byte-identical and both recover the planted somatics. The fixture CRAM is a lossless re-encode of the BAM, so the calls must match exactly; this guards the CRAM path the COLO829 accuracy gate depends on. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The launch scripts now check for BWA indexes, but runRufus.sh is also invoked directly, and its own handling failed open: it resolves _arg_ref_bwa (the extension-stripped prefix when <prefix>.sa exists, otherwise the reference path) without confirming any index files are actually there. A missing index therefore only surfaced at the first bwa mem call, far into the run after the jellyfish, model, hashlist and filter stages. Check the five bwa suffixes on the resolved _arg_ref_bwa prefix plus the .fai on the reference, right after the prefix is chosen and before any expensive stage. On failure, list the missing files and point at build_bwa_indexes.sh, setting _region_exit_reason so the region-status log records it like the other early exits. Update the now-stale comment that claimed the check lived only in the launch script. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents what the file actually does -- the column-aligned SamRead data model and its size/sentinel invariants, the six processing passes in main(), the clipPattern vocabulary the SV code branches on, the parseMutations emission conditions, and the three unrelated AO/RO estimators -- then inventories the defects found against it. Findings are marked [V] (verified against source, compiled tests, or real run output) or [R] (reported by review, not independently re-checked). Headline: in -min and exome runs the .7.7.dist model file is never created, ProcessDist treats the failed open as non-fatal, and the SNV/indel genotyper goes inert -- which is the shared root cause of missing genotypes, every variant being labelled Mosaic, and FILTER=PASS being unreachable on that path. Fixing it alone makes output worse: it activates the shadowing bug at :1586, out-of-bounds depth-clamp reads, and unconstrained GT ploidy. Also documents the upstream boundary: which weaknesses are fixable inside interpret versus which need an upstream change, ranked by value per cost. Parsing SA:Z: and aligning with bwa mem -Y are the two highest-value items and neither needs a new file format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RUFUS.interpret is the last pipeline stage and every input it takes is a file earlier stages leave in the work directory, so it can be re-run on its own. tests/replay/replay.sh reconstructs the invocation from a preserved run directory and replays interpret in ~3 minutes, touching neither the 206GB CRAMs nor the multi-hour assembly pipeline. Subcommands: run, freeze, check, twice. Exit 0/1/2 so check drops into CI. The shadow work directory is built from symlinks, so the reg-test data -- which is the oracle -- is never written to. Four artifacts are compared. calls.vcf strips only ##fileDate and ##RUFUSCommandLine, the sole lines that differ between identical runs. signals.txt scrapes error/warning lines from stdout, deduped with counts; this is the load-bearing one, because interpret returns 0 even on fatal errors, so neither the raw log nor the exit code is a usable signal alone. exit_code and a human-readable summary round it out. baselines/chr20_m5 is frozen from bin/RUFUS.interpret as built 2026-06-08 and verified deterministic (two consecutive runs byte-identical). It reproduces the QUAL overflow: max 108, 4 records above 100. Note the baseline exercises only the model-absent path, so BayseanGenotyper is untested by it; a baseline from a run with a real .7.7.dist is needed before touching the genotyper. Building this corrected the audit: raw interpret output has 3 PASS and 5 real genotypes, so the "GT and FILTER are always ." finding applies to the SNV/indel path only -- the SV/BND paths use ShittyGenotyper and need no model file. Post-processing removes those few, which is why the final VCF appeared empty of both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings db90cf4 (the only commit on master not already in docker) onto the branch so the docker->master promotion stays a clean fast-forward. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # README.md
Adds docs/RUFUS.interpret.audit.md (functional documentation + defect inventory for RUFUS.interpret) and tests/replay/ (standalone replay harness with a frozen, determinism-verified chr20_m5 baseline). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The repo's default branch was renamed master -> main on GitHub, but the workflow still referenced master in four places: the tag-mapping comment, the push trigger list, the GitHub Environment selector, and the image-tag resolver. Consequence: a push to main matched no trigger at all, so no workflow ran, no rufus:stage image was built, and no deployment was recorded against the staging environment. That is why the staging and production legs of the promotion pipeline were never exercised -- the staging trigger has been dead wiring since the rename. Found while attempting the first docker -> default-branch promotion: the push landed on a stale local origin/master ref and created a new remote branch instead of fast-forwarding the default. That stray branch has been deleted; the remote now has only docker and main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RunJellyForRUFUS.sh decided its exit status purely from whether the k-mer histogram came out empty, and never inspected jellyfish's own status. An OOM-killed jellyfish and a region that genuinely contains no k-mers both leave an empty histogram, so both exited 1. runRufus.sh's check_empty_hashes then collapsed every non-zero code into "no kmers found", set the reason to no_control_kmers and called exit 0 -- which the on_exit trap records as NO_VARIANTS. The result: a shard whose k-mer counting died was logged as legitimately variant-free, and the job exited 0 so SLURM reported COMPLETED. In a sharded whole-genome run that is a silently lost shard and a recall number that looks plausible and is wrong. This matters most for exactly the run it would corrupt -- whole-genome at -hs 64G is where an OOM is plausible rather than theoretical. RunJellyForRUFUS.sh now publishes an explicit exit-code contract: 0 - counted OK, k-mers found 1 - ran successfully, region genuinely contains no k-mers 2 - the counting tool itself failed; says nothing about coverage It captures jellyfish's status instead of letting `set -e` abort with it, and also treats "reported success but produced no .Jhash" and a failed histo as tool failures. check_empty_hashes treats anything that is not a clean 0 or 1 as a tool failure -- so an unexpected signal-derived code fails loudly too -- and exits non-zero so the region is recorded as ERROR rather than NO_VARIANTS. Behaviour for healthy (0) and genuinely empty (1) regions is unchanged. On the failure path the FIFO feeder must be torn down before reaping: with jellyfish gone nothing drains the FIFO, so a bare `wait` blocks forever. Caught by testing -- the first version of this fix hung. Verified with a stubbed jellyfish across the contract (OOM with the FIFO left blocked, generic failure, success-without-output, empty histogram, healthy) and against check_empty_hashes for all seven classification cases. Follow-up: this narrows but does not close the window -- a jellyfish that dies after writing a partial non-empty histogram still slips through. The durable version is the F8 robustness case asserting a killed jellyfish yields ERROR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… text
Documentation only. No executable line changes -- `git diff -w` over
non-comment, non-printf lines is empty; the sole non-comment hunk is three
stripped trailing spaces in the exome block.
The model phase is gated on `[ -z "$_arg_min" ] && [ $_arg_exome == "FALSE" ]`,
but _arg_min is initialised to 5 in the defaults block and never cleared, so the
first test is never true and the else branch always runs. ModelDist therefore
never executes in any run mode, the .7.7.dist that Overlap.shorter.sh passes to
RUFUS.interpret via -mod is never produced, ProcessDist fails to open it
non-fatally, and the Bayesian genotyper is inert. That is the shared cause of
GT and FILTER being '.' on essentially every SNV/indel record, FILTER=PASS being
unreachable on that path, and Dist1XCutoff falling back to 100000.
Confirmed across every run in resources/reg_test_files/runs/: zero *.7.7.dist,
fifteen *.7.7.model placeholders, zero logs containing "Starting model phase",
six containing "min was provided". The commented-out condition one line above
shows the earlier behaviour -- every non-exome run built the model until the
`-z "$_arg_min"` conjunct was added, which the default silently defeated.
Two help-text claims were wrong and are corrected:
- "-m,--min ... (no default)" -- there is a default, 5. The wrong text is
likely why this survived: the gate reads as correct if you believe it.
- "-ex, --exome: ... -m = 20" -- that override is gated on the same emptiness
test, so it never fires; an exome run uses -m 5 unless -m is passed.
Both help blocks gain a NOTE, and comments are added at the default, at the
exome block, and at the model phase itself.
Deliberately NOT fixed. Restoring the condition would enable the model for the
first time and simultaneously activate several latent defects in
RUFUS.interpret -- the uninitialised read in BayseanGenotyper, out-of-bounds
depth-clamp reads, and unconstrained GT ploidy in ParseGenotype. Those have to
land in the same change or output gets worse. See docs/RUFUS.interpret.audit.md
section 2.1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On a rerun into a preserved WORK_DIR (RUFUS keeps it on crash for debugging), 'bgzip $DEDUPED_VCF' and 'bgzip $GX_VCF' collided with the prior run's leftover .gz files -- bgzip refuses to overwrite without -f, exits 1, and set -e killed the whole run at the last VCF step. Add -f to both bgzip calls (and bcftools index -f) so a post-crash resume finishes instead of dying on a stale output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Overlap.shorter.sh basenamed the control $parent (intended only for the shortened on-disk intermediate filename) then passed that relative basename to CheckJellyHashList.sh as the hash to query. jellyfish query ran from rufus_wg/ where the relative name did not resolve, giving "Failed to parse header" -- silently swallowed (timeout, no set -e), so control k-mer counts reached interpret empty. Capture the full path in parent_path before the basename clobber and query that; on-disk filenames keep the basename so the -c/-cR paths interpret reads stay consistent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a WG run Add [V] confirmation from the SMHT004 300x whole-genome run: undeclared FILTER=fail reaches bcftools (13 of 2662 records dropped), and the inert-genotyper / FILTER=. failure reproduces on whole-genome, not just the -min/exome path 2.1 first framed. Shared cause: the .7.7.dist model not reaching interpret. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ust -min/exome The root cause is not the -min/exome branch. _arg_min is initialised to 5 in the defaults block and never cleared, so the model gate can never be true and ModelDist has never run in any mode. Passing -min changes nothing. Brings the audit into agreement with the inline documentation added to runRufus.sh, and drops the now-incorrect parenthetical claiming the SMHT004 run did not use -min. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
remove_coinheriteds pileups each control at the variant sites, so it needs an alignable BAM/CRAM. A control supplied as a pre-built hash (a .generator stub) or fastq has no BAM: bwa aligns an empty file and mpileup then fails on the empty bam (exit 1). Filter the control list to its BAM/CRAM entries and run the co-inherited filter over only those; skip entirely if none, since the HashList subtraction has already removed the k-mers of those controls. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three fixes plus a guard. The release runbook was stale on the branch rename: step 1 of "Cutting a production release" said to merge to `master`, and the trigger table mapped `push to master` to the stage image. Both now say `main`. That table is the thing someone reads to understand the pipeline, and step 1 is an instruction about to be followed, so both were actively misleading. The runbook no longer states version numbers, it derives them. `git tag`, `git push` and the pull_staged_image.sh example now read RUFUS_VERSION out of globals.txt inline, using the same extraction as the workflow's release guard. Copy-pasteable, and it cannot drift because there is no longer a second copy of the value. README still has to carry a literal, because it is read outside a checkout. scripts/ci/check_doc_versions.sh asserts those spots match globals.txt and repairs them with --write. Each rule is an explicit anchor plus expected rendering rather than a loose version-shaped regex, so it cannot false-positive on a deliberate reference to an older release, and it fails if a doc is restructured out from under it rather than silently passing. Wired into the build job ahead of the image build: it runs on every push, so drift is caught on a dev push to `docker` rather than at the moment of tagging, and it fails in seconds instead of after the build. Verified: passes clean, fails with a readable diff when either README spot is reverted to an older version, --write repairs both, and the repaired file is byte-identical to the original. Also recorded the durable follow-up in the runbook: the README needs a version at all only because the Zenodo asset is uploaded as rufus_<version>.sif against a per-release record. Publishing under a stable name against the concept record makes the URL permanent and retires this guard entirely. Deliberately deferred until after the first v* tag, since it modifies the release job -- the single irreversible, least-exercised part of the pipeline, and not something to change on the run that first proves it works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The calling stage writes the whole-genome final as temp.RUFUS.Final.<subject>.wg.vcf.gz (region postfix .wg), but post_process.sh -w 0 looked for temp.RUFUS.Final.<subject>.vcf.gz without it, so bcftools sort failed on a nonexistent file and no RUFUS.Final.<subject>.vcf.gz was produced. Match the .wg name in the whole-genome branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two routes, since the image is published to both and the redundancy is deliberate. Docker Hub is now the recommended path on HPC: `apptainer pull` builds the SIF directly, no sudo and no .def. Shows :latest alongside a pinned version, since pinning is what a reproducible analysis wants. The Zenodo instructions no longer hand-build a download URL, because that cannot work. Zenodo redirects the concept record at the record endpoint -- /api/records/<concept> and /records/<concept>/latest both 302 to the newest version -- but NOT at paths beneath it: /records/<concept>/files/<name> returns 404. So there is no appendable stable file URL, and a stable asset filename would not have fixed it either, since the version-specific record id remains in the path. Verified against the live API. Instead the snippet asks the API for the newest record's file list and takes the .sif from it. That is indifferent to both the record id and the filename, so it needs no edits between releases and survives a future asset rename. Hardened against a failure hit while testing: Zenodo's API intermittently returns 504 (observed, 30s gateway timeout). With plain `curl -s` and no `-f` that silently yields an empty URL and then a confusing or truncated download, so the lookup uses -fsSL with retries and asserts the URL is non-empty before fetching. Both commands verified end-to-end against the live record. The guard gains a rule for the pinned apptainer example and loses the one for the old VERSION= line, which no longer exists. Its deferred-work note is updated: the stable-asset-name follow-up is now a tidiness item rather than a prerequisite, since the API lookup already removes the drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e TODO Proofing the refreshed input docs against the argument parser turned up three things that did not match the code. Multiple -s does not mean multiple subjects. Every -s input is folded into one sample: ProbandFileName comes from _arg_subjects[0] and a single ProbandGenerator drives the run. The internal help already said "for split files from the same sample". Documenting it as "-s subject1 -s subject2" would have invited passing two unrelated individuals, which produces no error and a silently merged k-mer set. Reworded in three places to say split files of one sample. -c is not optional on its own. runRufus.sh exits unless at least one of -c or -e is supplied, so "if not provided, RUFUS uses internal control" would leave a reader with a run that dies. Now states the -c/-e requirement explicitly and describes -e-only as single-sample mode. Controls had dropped out of the Input Data list entirely while still being required; restored as its own item. Also added `generator` to the accepted formats, which both help blocks list, and noted that FASTQ is whole-genome only and rejects -R. Fixed `Apptainer exec` -> `apptainer exec` in the two invocation examples; the command is lowercase and the capitalised form does not run. The remaining "singularity" strings in the README are the real /opt/RUFUS/singularity/ path and are correct as-is. setup_slurm.sh gains a TODO for detecting the container runtime rather than hard-coding `singularity`. The generated SLURM scripts work on Apptainer only via its compatibility shim, and flipping the literal to `apptainer` would break Singularity CE sites instead -- so the fix is detection, not picking a side. Deferred on purpose: it rewrites every generated SLURM script, which is the machinery the sharded whole-genome release gate depends on. Comment-only, no executable lines touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s bugs
setup_slurm.sh never required -c. Its required set (arg_parser.sh:166) is
subjects, -b, -r, -a, -p and -l; controls are absent from it. The README listed
-c as Required, so single-sample mode was undocumented despite being supported.
Moved -c to Optional and added a "Single-sample mode" section: omit -c and
supply a hash source instead (-x, -K/-G, or -D/-V), which setup_slurm passes
through to the calling stage as -e/--exclude. Two worked example invocations,
one S3-downloaded and one from local hash directories, matching the style of
the existing examples. Notes that at least one of a control or a hash source is
required, since the calling stage rejects neither, and that specificity is lower
than a matched-control run because population hashes cannot subtract variation
private to the subject.
The Required Arguments block now matches arg_parser.sh exactly.
Also recorded two miswired flags in the optstring, both confirmed by running it
through getopts directly:
-C is declared without a colon, so `-C 36` leaves OPTARG unset, never sets
CPUS_PER_JOB, and drops "36" as an unread positional -- the documented
cpus_per_call option does nothing.
-h is declared with a colon, so bare `-h` misses the h) case and prints
"Option -h requires an argument" ahead of the usage text. Cosmetic.
Neither is fixed here. Repairing -C makes it start taking effect, which changes
SLURM CPU allocation on runs that currently ignore it, and this file feeds the
SLURM-script generator the sharded whole-genome gate depends on. Comment-only.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The getopts optstring declared C without a trailing colon ("M:CK:"), so -C
consumed no argument while its handler still assigned CPUS_PER_JOB=$OPTARG.
Running `-C 36` left OPTARG unset, never set CPUS_PER_JOB, and dropped "36" as
an unread positional. The documented cpus_per_call option silently did nothing,
and every job took the default -- 40 whole-genome, 12 for 1MB windows -- no
matter what the user asked for.
One character: C -> C:.
Behaviour when -C is omitted is unchanged. Defaults are applied at
arg_parser.sh:279/291 via ${CPUS_PER_JOB:-...} before the thread-limit check at
296, so an absent -C still lands on the same value it did before.
Behaviour when -C IS supplied changes, which is the point, and it has one
user-visible consequence worth stating: the check at 296 requires the RUFUS
thread count to be less than cpus per job. A -C smaller than -z was previously
ignored and silently replaced by the default; it now fails setup with an
explicit error. That is the validation doing its job on input it never
previously saw, but anyone who has been passing a too-small -C will notice.
Nothing in the repository passes -C to setup_slurm, so no in-tree script or
test changes behaviour; the blast radius is external launch commands only.
Verified by running the real optstring through getopts: -C 36 now yields
OPTARG=36 with no leftover positional, omitting -C still yields an empty value
for the default to fill, and -C parses correctly adjacent to other flags.
README documents the defaults and the -z constraint now that the flag works.
The sibling defect is left alone and still noted in the source: -h carries a
colon, so bare -h misses the h) case and prints a spurious "requires an
argument" ahead of the usage text. Cosmetic, and not what was asked for here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three usage blocks -- runRufus.sh, post_process.sh and setup_slurm.sh -- had ad-hoc spacing, with descriptions landing anywhere from column 13 to 32 within the same block and long entries running past 250 characters on one line. In a fenced code block that renders as a ragged wall with no visual column. Each block now aligns its descriptions to a single column sized to its own longest flag (20, 21 and 32 respectively), wraps at 100 characters, and indents continuation lines to the description column so multi-line entries read as one item. Verified mechanically rather than by eye: every description in a block starts at the same column, every continuation line matches that column, no line exceeds 100 characters, and all 33 flag entries present before the reflow are still present after it. Also normalised the sentence case of descriptions, which was mixed, and fixed "kilabases" -> "kilobases" and "able to be ran" -> "able to be run" while in there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
No description provided.