Conversation
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>
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>
This was referenced Aug 1, 2026
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>
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>
matthuska
reviewed
Aug 14, 2026
| dependencies: | ||
| - bedtools=2.30.0 | ||
| - samtools=1.14 | ||
| - bedtools=2.31.1 |
Collaborator
There was a problem hiding this comment.
Just a general comment about conda package specifications (see https://docs.conda.io/projects/conda-build/en/latest/resources/package-spec.html#package-match-specifications for full details):
I'm pretty sure that using a single '=' sign is equivalent to a fuzzy search for any version starting with the version you give. So for samtools=1.24, it is going to allow samtools=1.24.*. If that's what you want, then good. If you want to specify an exact version, you need to use double '=='.
Member
Author
There was a problem hiding this comment.
Good catch — you're right, a single = is a prefix match, so samtools=1.24 was accepting 1.24.*. That matters here because the containers are pinned to an exact tag, so the conda and container profiles could drift onto different versions of the same tool — exactly the skew this PR set out to remove.
Switched every pinned package in all 12 env files to == (46b979d).
One exception, which is a nice illustration of your point in the other direction: python in samclipy stays on a single =. conda-forge has no package versioned exactly 3.14 — only 3.14.0rc1, 3.14.0, 3.14.1, … — so python==3.14 fails to solve outright. It's fuzzy on purpose and the file now says so.
All 12 environments re-checked with a mamba env create --dry-run solve, and the conda CI jobs pass.
Collaborator
|
Overall looks fine to me. Amazing how many changes are needed to (mainly) just swap between two very similar tools, but there were also a few other bugs fixed and cleanups made. |
In a conda specification a single '=' is a prefix match: samtools=1.24 resolves to samtools 1.24.*, so a later patch release changes what the conda/mamba profiles install. The containers are pinned to an exact tag, which means the two profiles can end up on different versions of the same tool - the kind of skew this branch set out to remove. Use '==' for every pinned package. The exception is python in samclipy, which stays fuzzy because conda-forge has no package versioned exactly "3.14", only 3.14.0, 3.14.1, ...; the file says so. All 12 environments were checked to still solve with mamba. 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
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.
Hopefully also fixing #110