Repository navigation
Test suite, CI, and a pipeline that runs off our cluster - #2
Merged
Merged
Conversation
Nextflow 25.10 and later default to a stricter parser that refuses an assignment used as an expression and drops while loops entirely, and both appear in the samplesheet parsing. Splitting one and replacing the other with list padding is behaviour preserving. main.nf still needs its script-level code moved inside the workflow before that parser accepts the file outright.
QC_REPORT takes three inputs that fall back to a placeholder when their feature is off, and all three are off by default, so every default run handed it the same "NO_FILE" path three times and Nextflow refused to stage them. The placeholders are now distinct real files under assets/, referenced through projectDir so a stray NO_FILE in the launch directory is never staged in their place.
A stub block lets `-stub-run` execute the whole DAG in seconds with no container and no reference data, which is what makes a pipeline-level check cheap enough to run on every change. Each stub creates exactly the outputs its process declares.
tests/data is a 170 kB cohort (5 kb reference, three genes, three samples) built from a fixed seed, covering paired-end, single-end and a sample split over two runs. `-profile test` runs it with the default feature set; `-profile test_full` turns on every optional branch so one run instantiates every process.
990 tests over the consensus decision tree, the QC verdict engine, every parser, the k-mer liftover and the read filter. The ruff configuration is deliberately narrow, selecting the rules that catch real defects rather than imposing a restyle, and it excludes the vendored KrakenTools script.
The report computes Spearman, Kruskal-Wallis, chi-square and Benjamini-Hochberg in hand-written ES5, because nothing else fits inside a self-contained HTML file. These 77 tests assert that arithmetic against SciPy and statsmodels reference values, and check the asset bundle parses and stays ES5. The harness reads the module list out of qc_report.py, so the two cannot drift.
The validator gets one deliberately broken samplesheet per failure mode, plus a check that it still reports every problem in a single pass. The stub-run tests assert the DAG completes on both profiles, that test_full reaches every declared process, and that a multi-run sample maps twice but merges once.
Lint, unit tests on two Python versions, the front-end tests, a stub run against both the minimum and the latest Nextflow, and a strict docs build. A fixtures job regenerates the test cohort and fails on any diff, so the committed data stays something anyone can rebuild. tests/run_tests.sh runs the same checks locally.
Two more samplesheet cases and blank lines in two fixtures. That takes bin/qc_report.py to 99% statement coverage outside main().
docs.yml already runs mkdocs build --strict with the system libraries the social plugin needs, so it just gains a pull_request trigger; only the build job runs there, since deploy stays gated on the Pages variable. Drops the stale indel-mask branch trigger now that the branch is merged.
The guard tested `not line`, but iterating a file yields "\n" for a blank line and never "", so it never fired and the row unpacking raised ValueError instead. Flips the test that pinned the crash.
CONTRIBUTING covers setting up, running the suite locally, what CI gates on, the ruff policy and the commit convention this repository actually uses, plus the two known debt items a newcomer would otherwise trip over. Contributor Covenant 2.1, a security policy, issue forms and a pull-request template alongside it.
The base config hardcoded one machine: a SLURM executor with --qos=short, `module load singularity`, /scr and /storage bind paths, a Kraken database under /storage, and a leftover samplesheet name. All of that now lives in a `garnatxa` profile, so `-profile garnatxa` replaces the -c file a user needed before and a clone runs correctly anywhere without being edited. Adds a generic `slurm` profile, makes `standard` run on the current host, pins the container by digest instead of the mutable 1.0.1 tag, requires --tsv with a message that says what to pass, and prints the resolved profile, executor and container in the startup banner.
Nextflow 25.10 and later default to a parser that rejects statements at the top level of a script and requires a dynamic process directive to be a closure, so main.nf only ran through the deprecated v1 fallback. The samplesheet parsing is now a function, every statement lives inside the workflow, and each dynamic publishDir is a closure. Verified on 24.04.2 (the declared minimum) and 26.04.6; the test suite and CI no longer set NXF_SYNTAX_PARSER.
Nextflow puts every --flag into params whether the pipeline declares it or not, so with roughly ninety of them a typo like --treads was accepted, ignored, and the run finished on the default. The declared set is read straight out of nextflow.config, so the two cannot drift, and an unrecognised name now fails with a suggestion. --help lists every parameter with its default, grouped by the config's own sections. Also drops process.publishDirMode, which is not a Nextflow directive and only produced a warning; the publish mode already reaches each process via params.publish_mode. A new test flags any params.x referenced but never declared.
Each process asks for what its tool needs and Kraken2 asks for 80 GB, so on a laptop the local executor simply refuses to schedule it: "Process requirement exceeds available memory". --max_cpus, --max_memory and --max_time now feed Nextflow's resourceLimits, so one flag makes the whole pipeline fit instead of editing nine modules. Left unset, every process keeps the size it asked for. The assignment sits after the profiles on purpose: it reads params.max_*, and a profile that sets them is only merged when its own block is parsed.
The container is pinned by digest, so it has to be published before the tag that references it, and the version lives in three files that have to agree. RELEASING records that order. .zenodo.json is the archive metadata, ready for the DOI; the minting itself is blocked until the repository is public.
main.nf now satisfies the strict parser, so NXF_SYNTAX_PARSER is no longer needed and the note becomes a description of what that parser actually forbids. Drops the "-profile standard on a cluster" hint from the bug template, which stopped being the common failure once standard became the portable default.
Flags the profile change as breaking: -profile standard used to submit to SLURM and now runs on the current host.
A GFF attribute was matched as a substring, so `locus_tag=` also matched inside `old_locus_tag=` and `gene=` inside `pseudogene=`, both routine in RefSeq and Prokka output: genes were labelled with an obsolete tag, and build_gene_map could override a curated H37Rv locus. mask_profile summed raw interval lengths for its headline total, so an unmerged BED could report above 100% masked. Four parsers in stats_to_legacy wrapped a whole read loop in one try/except, so a single bad line silently discarded every line after it; a partial genome size then propagated into depth, breadth, evenness and every density bin. Genuinely-zero metrics were erased to NA, which made a failed sample look unmeasured. Genotype classification assumed diploid spellings, so a haploid no-call counted as heterozygous. parse_dr dropped a float-formatted depth. The tests that pinned each of these now assert the corrected behaviour.
Every command that meant "on a cluster" said -profile standard, which now runs on the current host, and several pages carried a warning about standard failing off a cluster that no longer applies. The HPC section of the configuring tutorial is rewritten around -profile garnatxa instead of a hand-written -c file, keeping the explanation of why binds are needed. Also corrects the container being described as pinned by tag, --tsv having a default, and --kraken2_db pointing at storage nobody else has, and documents --help and the resource ceilings.
Around 500 lines of bash and awk lived inside modules/*.nf, and none of it was reachable by any test: a stub run replaces the script block and nothing else can import it. Three pieces move to bin/, where the suite can exercise them. format_snps_for_backbone.awk is the genotype re-validation, verified byte-identical to the version it replaces and previously copied into both FreeBayes processes, where the copy had already lost the comment explaining why it exists. vcf_filter_rules.py builds the hom and het bcftools expressions, which are the definition of a call and were also written out twice. safe_tabix had drifted: the copy in annotation.nf still used the older zgrep check that cannot tell a truncated VCF from an empty one, and it now gets the hardened version for free. variants.nf drops from 485 lines to 393. 35 new tests cover the extracted code, including a run against a real bcftools to confirm the expressions select what they claim to.
FORMAT/DP can be ".", and the awk assigned it without coercing to a number, so the three numeric fallbacks were skipped by a string comparison and ADP=. was written out. The consensus reads that as zero depth and turns a called SNP into a gap. The upstream filter requires a real depth, so this was probably unreachable, but every fallback in that block exists precisely to guarantee a number.
CALL_BACKBONE carried two copies of count_bases, one per branch, differing only in whether a BAQ-adjusted depth was available to compare against. That function reads an mpileup base string, where a read start is a caret plus a mapping-quality character that must not be counted and an indel is a sign, a length and that many bases belonging to the previous position. Miscount any of it and the allelic depth the consensus relies on is quietly wrong, and nothing was checking. One program now, parameterised by a BAQ flag, with 27 tests over the pileup grammar and the coverage call. variants.nf drops from 393 lines to 342.
1697 lines and 53 functions in one file, covering file parsing, the QC verdict engine, the panel builders, the HTML assembly and the CLI. It is now a 319-line CLI over bin/qcreport/, split along the seams that were already there: parsers, metrics, panels and render. The move is mechanical. 75 of the 76 top-level definitions are byte-identical in exactly one module; the exception is _ASSET_DIR, which needed one more dirname() to keep resolving to bin/report_assets from a directory deeper. With the clock frozen the old and new versions produce byte-identical qc_flags.tsv and HTML on the demo cohort, so behaviour is unchanged rather than assumed unchanged. Two tests pin what this refactor risks: that the module imports cleanly, and that the CLI still runs when invoked by absolute path from an unrelated directory, which is how modules/report.nf calls it.
main.nf was 556 lines with about 390 of them a single block of channel plumbing. Each numbered stage is now its own sub-workflow with an explicit take/emit, and the body of main.nf is config, channel construction and eleven one-line calls. Every conditional moved inside the sub-workflow it belongs to, so a stage that is switched off returns its empty-channel placeholder instead of main.nf branching around it. Emit names had to diverge from the local names they carry: `emit: x = y` is a real assignment, and a local `def x` in scope shadows the binding property Nextflow then looks for. Equivalence was checked by running the previous main.nf side by side and diffing execution traces: identical per-process and per-task counts on both profiles, and identical published output trees. Note that process names are now qualified, so the first run after this will not resume from an older work directory.
Six defects the new tests turned up, plus two more the extraction exposed, and the three monoliths that came apart: the bash inside the process scripts, qc_report.py and the workflow body.
Six of these were assigned to a run that lost its work and never got picked back up, so they stayed pinned as current behaviour while everything around them was fixed. get_ro_ao assigned the reference count before the alternate sum could raise, so a malformed AD returned a half-initialized result. IntervalMasker recognised a header row by its first field, which silently discarded every interval on a contig named chrom-anything; it now looks at whether the coordinate columns are numbers. FastaReader raised a bare IndexError on a header with no name. The SAM filter indexed fields on a blank line. A BED coordinate could be negative and expand to positions no reference has. build_snp_matrix threw away a depth that a single-element AD had stated, emitting an empty cell for a known value. build_min_unique_len keeps its behaviour but stops being silent about it: a position genmap never scored is written as "placeable at any read length", which is right at a contig end and wrong in the middle, so an interior hole now warns.
The smallest-K coverage rule stays as it is, because deriving coverage from every K would change masking in a way no fixture can validate against real genmap output. Points the reader at the warning instead of reading as an unfixed defect.
The directory tree in outputs.md still showed one flat bin/ and no sub-workflows. It now covers subworkflows/, the qcreport package and the scripts the process blocks call, with a note on why the science lives in bin/ at all: a script: block is unreachable from any test, so anything with a decision in it has to be a file the process calls. CONTRIBUTING gains a table saying which layer a change belongs in, and the emit-name collision that otherwise costs an afternoon. tests/README lists what the extracted scripts decide and corrects the task counts and the module that builds the report bundle.
The number this work is heading for, in the three places RELEASING says have to agree. Nothing is tagged and no release is published: the changelog section stays Unreleased until one is. The container stays at the 1.0.1 image and its digest. No bundled tool changed, and the scripts added under bin/ are staged by Nextflow from the pipeline directory rather than baked into the image, so there is nothing to rebuild.
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.
Gives BAMpiro a test suite and CI, makes it run correctly on a machine that is not our cluster, and takes apart the three parts of the codebase that had grown untestable.
Warning
-profile standardno longer submits to SLURM. It is now the portable default and runs on the current host. Use-profile slurmfor a generic cluster, or-profile garnatxafor ours, which carries the bind paths, QoS,module loadand Kraken database and replaces the-c cluster.configfile we used to write by hand.Process names are now qualified (
CALL_VARIANTS:CALL_FREEBAYES), so task hashes change and the first run after this will not-resumefrom an older work directory.Tests and CI
Around 1,200 tests in three legs, all runnable with
tests/run_tests.sh. None of them needs a container, a reference genome, real reads or a network connection.tests/unit/bin/: the consensus decision tree, the QC verdict engine, every parser, the k-mer liftover, the read filter, and the awk and filter expressions the process scripts calltests/js/tests/pipeline/-stub-runof the DAG on both test profilesEvery process gained a
stub:block, so-stub-runwalks the whole workflow in seconds.-profile testruns the bundled 170 kB fixture cohort (a 5 kb reference, three samples covering paired-end, single-end and a sample split over two runs);-profile test_fullturns on every optional branch so a single run instantiates every process, and a test asserts exactly that. The cohort derives from one seed and CI regenerates it and fails on any diff.CI gates every pull request with lint, unit tests on two Python versions, the front-end tests, and a stub run against both the minimum supported Nextflow and the current release.
Fixed
The suite paid for itself before it was finished.
QC_REPORTthe sameNO_FILEpath three times and Nextflow refused to stage them. 66 tasks would complete and then the run died, right before the headline output.main.nfdid not compile under the strict Nextflow parser, the default since 25.10, and only ran through the deprecated v1 fallback. Now verified on 24.04.2 and 26.04.6.locus_tag=also matched insideold_locus_tag=andgene=insidepseudogene=, both routine in RefSeq and Prokka output. Genes were labelled with an obsolete tag and the curated H37Rv map could be silently overridden.try/except, so one malformed line silently discarded every line after it; a partial genome size then propagated into depth, breadth, evenness and every density bin.NA, making a failed sample indistinguishable from an unmeasured one.-profile generic, which is haploid.safe_tabixhad drifted between its two copies: the one inannotation.nfstill used a check that cannot tell a truncated VCF from an empty one, and wrote a fake index without saying so.FORMAT/DPreached the consensus asADP=., which reads as zero depth and turns a called SNP into a gap.mask_profilereporting above 100% masked,get_ro_aoreturning a half-initialized result,IntervalMaskersilently dropping every interval on a contig namedchrom*, two stream parsers crashing on a blank line, a BED coordinate allowed to be negative, and a known depth thrown away.Portable by default
The base config used to be one machine: a SLURM executor with
--qos=short,module load singularity,/scrand/storagebind paths, a Kraken database under/storage, and a leftover samplesheet name. All of it moved intoconf/garnatxa.config.--tsvis required and says what to pass.--kraken2_dbhas no default, and without one the contamination screen is skipped.1.0.1tag, so two runs a month apart cannot silently use different tool versions.--parameterstops the run with a suggestion (--treads-> "did you mean--threads?") instead of being accepted and ignored. The declared set is read fromnextflow.config, so the two cannot drift.--helplists every parameter with its default.--max_cpus/--max_memory/--max_timecap every request to what the machine can give. Without them Kraken2 asks for 80 GB and the local executor simply refuses to schedule it.Refactored
Behaviour-preserving throughout, and each step verified rather than assumed.
main.nfbin/qc_report.pybin/qcreport/packagemodules/variants.nfThe science came out of the process scripts. Around 500 lines of bash and awk lived inside
modules/*.nf, where no test could reach it:-stub-runreplaces the script block and nothing can import it. The genotype re-validation, the backbone pileup parser, the hom/het bcftools expressions andsafe_tabixnow live inbin/, with 62 tests over them including a run against a real bcftools. Two of those pieces were duplicated across processes, and one copy had already lost the comment explaining why it exists.Each refactor was checked, not asserted: the report produces byte-identical output on the demo cohort with the clock frozen; the workflow was run side by side with the previous
main.nfand the execution traces diffed to identical task counts and identical published outputs; the extracted awk was compared character by character with the version it replaced before being swapped in.Also
Community files (CONTRIBUTING, a Code of Conduct, a security policy, issue forms, a pull-request template),
RELEASING.mdrecording the order a release has to happen in, and.zenodo.jsonready for a DOI. Minting it needs the repository public, so that step is still blocked, along with the docs site and the two README badges pointing at it.Not a release.
manifest.versionreads1.1.0, which is the number this work is heading for, but nothing is tagged and no release is published; the changelog section stays[Unreleased]until one is. The container also stays at the 1.0.1 image and its digest: no bundled tool changed, and the scripts added underbin/are staged by Nextflow from the pipeline directory rather than baked into the image.