Skip to content

Replace bwa with bwa-mem2 and align samtools versions - #124

Open
hoelzer wants to merge 12 commits into
devfrom
bwa-mem2
Open

Replace bwa with bwa-mem2 and align samtools versions#124
hoelzer wants to merge 12 commits into
devfrom
bwa-mem2

Conversation

@hoelzer

@hoelzer hoelzer commented Jul 31, 2026

Copy link
Copy Markdown
Member
  • --bwa now runs bwa-mem2 instead of bwa — mainly for the faster index build on large indices combining several eukaryotic genomes. The flag and all output file names are unchanged.
  • Separate bwamem2_index label: the index build is single-threaded, so it no longer reserves 24 CPUs, but gets more memory than the mapping step. A bwa-mem2 index is ~4x larger than a bwa index and is held in memory, so node.config memory requests went up (32 GB mapping / 48 GB index, escalating over 4 retries).
  • All containers and conda envs now ship samtools/htslib 1.24. This also bumps minimap2 2.26→2.31, bedtools 2.30.0→2.31.1, seqkit 2.6.1→2.13.0, pigz 2.3.4→2.8.
  • Fixes: the short-read mapper had no conda env under conda/mamba and no cpus/memory under local/standard; samclipy now has an explicit container instead of implicitly using smallTask's, and git was added to its env.

⚠️ minimap2 2.26→2.31 may shift alignments slightly, so decontamination output is not guaranteed bit-identical to previous runs.

Hopefully also fixing #110

hoelzer and others added 3 commits July 31, 2026 14:58
Swap the short-read mapper alternative behind --bwa from bwa to bwa-mem2,
mainly for the faster index build of large indices combining several
eukaryotic genomes. The --bwa parameter and all result files/names are
unchanged.

- rename modules/bwa.nf -> modules/bwamem2.nf (bwa_index/bwa ->
  bwamem2_index/bwamem2, labels bwamem2_index/bwamem2)
- new container nanozoo/bwa-mem2:2.3--4533398
- envs/bwamem2.yaml with bwa-mem2 2.3 and samtools/htslib 1.24
- separate label for the index step: it is single-threaded, so it no longer
  reserves 24 CPUs, but it gets more memory than the mapping step
- a bwa-mem2 index is ~4x larger than a bwa index and is held in memory,
  so the memory requests in node.config went up
- add the mapper to conda.config (it had no environment with the
  conda/mamba profiles) and to local.config (it had no cpus/memory with
  the local/standard profiles)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Update all containers and conda environments to the same samtools/htslib
version, and pin htslib explicitly so bgzip/tabix are no longer pulled in
only as a transitive samtools dependency.

- containers: samtools 1.24, minimap2 2.31, bed_samtools 2.31.1,
  bwa-mem2 2.3 (now with samtools), seqkit 2.13.0
- envs follow the container versions: minimap2 2.26 -> 2.31,
  bedtools 2.30.0 -> 2.31.1, seqkit 2.6.1 -> 2.13.0, samtools -> 1.24
- envs/minimap2.yaml: pigz 2.3.4 -> 2.8, the old pin cannot be solved
  next to samtools 1.24 (zlib 1.2 vs libzlib 1.3.2)
- envs/seqkit.yaml: tabix 1.11 -> htslib 1.24, which ships tabix and bgzip
- samclipy gets an explicit container instead of implicitly taking the one
  of the smallTask label; the samtools image has the python and git it
  needs. Its env now pins python 3.14 (as in the container) and adds git,
  which the process calls but the env never provided.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CI matrix uses NXF_VER "latest", which now resolves to Nextflow 26.04,
where the strict syntax is the default. That broke all "latest" jobs, on
this branch as well as on main.

Fixed here:
- nextflow.config: `def trace_timestamp` is a variable declaration, which
  cannot be mixed with config statements -> moved into the params scope
- clean.nf: typed `for` loops are not part of the syntax -> use `each`
- clean.nf: `addParams()` on include statements was removed. `tool` was
  never read anywhere, `lib_pairedness` is now set in the params scope,
  which is where the modules read it from anyway
- modules: `if` blocks around publishDir/storeDir are not valid process
  directives -> use the `enabled` option of publishDir, and a ternary for
  the storeDir of download_host
- modules/utils.nf: `env(TOTALRECORDS)` refers to a shell variable, the
  strict parser wants the string form `env('TOTALRECORDS')`

Note that `enabled` needs a real boolean: `enabled: params.keep` silently
publishes nothing, because Nextflow runs a String through
Boolean.parseBoolean().

This is not enough for Nextflow >=26.04 -- the ~180 top-level statements in
clean.nf have to move into the entry workflow first. Until then the CI
matrix is pinned to 25.10.0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hoelzer and others added 3 commits August 1, 2026 13:34
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The illumina conda job fails reproducibly while creating environments, with
either `LockError: Failed to acquire lock` or a package archive that another
process removed mid-extraction. Both come from several `conda env create`
calls running at the same time against the shared package cache.

- executor.queueSize = 1 for the test config, so environments are built one
  after another. The tasks are stubs, there is nothing to parallelize.
- point conda.cacheDir at one shared directory. It defaults to a path
  relative to the launch dir, and nf-test gives every test its own launch
  dir, so each of the six illumina tests rebuilt every environment from
  scratch. Now they are built once and reused.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hoelzer and others added 3 commits August 3, 2026 09:37
check_own always wrote checked.fa.gz, so more than one --own (or --keep)
FASTA made concat_contamination fail with an input file name collision.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An --input glob that matches files but forms no read pairs left every
process without a task, so the run reported success without cleaning
anything. checkIfExists does not catch this.

Also give bwamem2_index 8 cpus: the build is not as serial as assumed and
uses ~10 cores, so a cgroup-pinned single core would slow it down badly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
minimap2 was handed the reference FASTA directly, so every sample rebuilt
the index; for a multi-genome host that dominates the runtime. Add a
minimap2_index process mirroring bwamem2_index.

-k and -w are baked into the index, so index and mapping have to agree on
the preset: minimap2_preset() is now the single source for both, which
also collapses the four near-identical mapping commands into one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hoelzer and others added 2 commits August 4, 2026 09:14
Nothing in the config declared a time, so every task ran under the
default limit of the queue. Mapping a large sample against a multi-genome
host exceeded that and SLURM killed the job with exit 140; the retry
ladder then only raised the memory, which does not buy more wall time.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
index_bam, idxstats_from_bam and flagstats_from_bam ran under the
minimap2 label, so each of them asked for 24 cpus, 24 GB and now also a
12 h wall time for a few minutes of work. On a backfill scheduler that
delays them behind the mapping jobs they follow.

They get the samtools container that the smallTask label already uses
and a matching small Conda environment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant