From e650d92d7d1f1572c3ed7312979c0b79edbd465d Mon Sep 17 00:00:00 2001 From: Felix Krueger Date: Sun, 19 Jul 2026 16:54:41 +0100 Subject: [PATCH 1/3] bismark/align: intra-instance -p threading (Bowtie2 + HISAT2) Replace the --multicore fork resource model with an intra-instance -p divisor (-p = task.cpus / n_instances; n = 2 directional/pbat, 4 non-directional, 1 combined-index, 2 combined_index_parallel+non_dir) for Bowtie 2 and HISAT2. minimap2/rammap keep the legacy --multicore auto-compute unchanged. Lower peak RAM (one index load per strand instance instead of M forks each re-loading the index), fuller CPU use, and byte-identical Bowtie 2 output (-p N --reorder is thread-invariant). Emits -p only when task.cpus/n >= 2, else omits it (faithful single-thread-per-instance). Adds 4 nf-tests asserting -p is emitted (bowtie2 dir/non-dir, hisat2, combined-index) via a dedicated cpus=8 config; existing tests unchanged (cpus=2 emits no -p -> byte-identical, snapshots unchanged). --- modules/nf-core/bismark/align/main.nf | 76 +++++++--- .../nf-core/bismark/align/tests/main.nf.test | 138 ++++++++++++++++++ .../align/tests/nextflow_pthreads.config | 16 ++ 3 files changed, 208 insertions(+), 22 deletions(-) create mode 100644 modules/nf-core/bismark/align/tests/nextflow_pthreads.config diff --git a/modules/nf-core/bismark/align/main.nf b/modules/nf-core/bismark/align/main.nf index ff7af322352f..bab4717ab2e3 100644 --- a/modules/nf-core/bismark/align/main.nf +++ b/modules/nf-core/bismark/align/main.nf @@ -28,32 +28,64 @@ process BISMARK_ALIGN { } def fastq = meta.single_end ? reads : "-1 ${reads[0]} -2 ${reads[1]}" - // Try to assign sensible bismark --multicore if not already set - if(!args.contains('--multicore') && task.cpus){ + // Aligner thread model: + // - Bowtie 2 + HISAT2 run 2 (directional/pbat) or 4 (non-directional) *concurrent* + // strand instances, each of which honours `-p` (threads per instance). Dividing the + // task's cpus across the instances (`-p = task.cpus / n_instances`) uses all cpus with + // a single index load per instance, instead of `--multicore N` forking N chunks that + // each re-load the index (Bowtie 2: N x 2/4 concurrent index copies -> high peak RAM). + // Bowtie 2 `-p N --reorder` is thread-invariant (byte-identical); HISAT2 is near- + // invariant (a handful of reads re-baseline). See the resource-model benchmark. + // - minimap2/rammap ignore `-p` (bismark hard-codes minimap2 `-t 2`; rammap auto-scales), + // so they keep the legacy `--multicore` auto-compute untouched. + def isMinimapLike = args.contains('--minimap2') || args.contains('--mm2') || args.contains('--rammap') || args.contains('--ram') + if(isMinimapLike){ - // Numbers based on recommendation by Felix for a typical mouse genome - def ccore = 1 - def cpu_per_multicore = 3 - def mem_per_multicore = (13.GB).toBytes() - if(args.contains('--non_directional')){ - cpu_per_multicore = 5 - mem_per_multicore = (18.GB).toBytes() - } + // Legacy --multicore auto-compute (unchanged) — minimap2/rammap only. + if(!args.contains('--multicore') && task.cpus){ + + // Numbers based on recommendation by Felix for a typical mouse genome + def ccore = 1 + def cpu_per_multicore = 3 + def mem_per_multicore = (13.GB).toBytes() + if(args.contains('--non_directional')){ + cpu_per_multicore = 5 + mem_per_multicore = (18.GB).toBytes() + } - // How many multicore splits can we afford with the cpus we have? - ccore = ((task.cpus as int) / cpu_per_multicore) as int + // How many multicore splits can we afford with the cpus we have? + ccore = ((task.cpus as int) / cpu_per_multicore) as int - // Check that we have enough memory - try { - def tmem = (task.memory as MemoryUnit).toBytes() - def mcore = (tmem / mem_per_multicore) as int - ccore = Math.min(ccore, mcore) - } catch (Exception e) { - log.warn "Error catched: ${e}" - log.warn "Not able to define bismark align multicore based on available memory" + // Check that we have enough memory + try { + def tmem = (task.memory as MemoryUnit).toBytes() + def mcore = (tmem / mem_per_multicore) as int + ccore = Math.min(ccore, mcore) + } catch (Exception e) { + log.warn "Error catched: ${e}" + log.warn "Not able to define bismark align multicore based on available memory" + } + if(ccore > 1){ + args += " --multicore ${ccore}" + } } - if(ccore > 1){ - args += " --multicore ${ccore}" + } else if(!args.contains('--multicore') && !args.contains('--parallel') && !args.contains('-p ') && task.cpus){ + + // Bowtie 2 / HISAT2: split the cpus across the concurrent strand instances. + def n_instances = 2 + if(args.contains('--combined_index')){ + // combined = one both-strands pass (n=1); only the opt-in parallel non-directional + // path runs two concurrent passes (n=2). + n_instances = (args.contains('--non_directional') && args.contains('--combined_index_parallel')) ? 2 : 1 + } else if(args.contains('--non_directional')){ + n_instances = 4 + } + + // -p threads per instance; emit only when it lands >= 2 (bismark requires -p >= 2), + // otherwise omit it (faithful single-thread-per-instance default). + def pthreads = ((task.cpus as int) / n_instances) as int + if(pthreads >= 2){ + args += " -p ${pthreads}" } } """ diff --git a/modules/nf-core/bismark/align/tests/main.nf.test b/modules/nf-core/bismark/align/tests/main.nf.test index b2fe71b6ffa1..145d7b8b9e41 100644 --- a/modules/nf-core/bismark/align/tests/main.nf.test +++ b/modules/nf-core/bismark/align/tests/main.nf.test @@ -205,4 +205,142 @@ nextflow_process { ) } } + + test("bowtie2 | intra-instance -p threading | paired-end | 8 cpus") { + + config './nextflow_pthreads.config' + + when { + params { + bismark_args = '--bowtie2' + genomeprep_args = '--bowtie2' + } + process { + """ + input[0] = Channel.of([ + [ id:'test', single_end:false ], + [ + file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test.methylated_1.fastq.gz', checkIfExists: true), + file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test.methylated_2.fastq.gz', checkIfExists: true) + ] + ]) + input[1] = Channel.of([ + [ id:'sarscov2'], + file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true) + ]) + input[2] = BISMARK_GENOMEPREPARATION.out.index + """ + } + } + + then { + assertAll( + { assert process.success }, + // directional Bowtie 2 = 2 instances; 8 cpus / 2 = -p 4 + { assert file(process.out.report[0][1]).text.contains('-p 4 --reorder') } + ) + } + } + + test("bowtie2 | intra-instance -p threading | non_directional | 8 cpus") { + + config './nextflow_pthreads.config' + + when { + params { + bismark_args = '--bowtie2 --non_directional' + genomeprep_args = '--bowtie2' + } + process { + """ + input[0] = Channel.of([ + [ id:'test', single_end:true ], + file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test.methylated_1.fastq.gz', checkIfExists: true) + ]) + input[1] = Channel.of([ + [ id:'sarscov2'], + file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true) + ]) + input[2] = BISMARK_GENOMEPREPARATION.out.index + """ + } + } + + then { + assertAll( + { assert process.success }, + // non-directional = 4 instances; 8 cpus / 4 = -p 2 + { assert file(process.out.report[0][1]).text.contains('-p 2 --reorder') } + ) + } + } + + test("hisat2 | intra-instance -p threading | paired-end | 8 cpus") { + + config './nextflow_pthreads.config' + + when { + params { + bismark_args = '--hisat2' + genomeprep_args = '--hisat2' + } + process { + """ + input[0] = Channel.of([ + [ id:'test', single_end:false ], + [ + file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test.methylated_1.fastq.gz', checkIfExists: true), + file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test.methylated_2.fastq.gz', checkIfExists: true) + ] + ]) + input[1] = Channel.of([ + [ id:'sarscov2'], + file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true) + ]) + input[2] = BISMARK_GENOMEPREPARATION.out.index + """ + } + } + + then { + assertAll( + { assert process.success }, + // HISAT2 shares the 2/4 instance model + honours -p; directional = 2 instances, 8/2 = -p 4 + { assert file(process.out.report[0][1]).text.contains('-p 4 --reorder') } + ) + } + } + + test("bowtie2 | combined_index | intra-instance -p threading | single-end | 8 cpus") { + + config './nextflow_pthreads.config' + + when { + params { + bismark_args = '--bowtie2 --combined_index' + genomeprep_args = '--bowtie2 --combined_genome' + } + process { + """ + input[0] = Channel.of([ + [ id:'test', single_end:true ], + file(params.modules_testdata_base_path + 'genomics/sarscov2/illumina/fastq/test.methylated_1.fastq.gz', checkIfExists: true) + ]) + input[1] = Channel.of([ + [ id:'sarscov2'], + file(params.modules_testdata_base_path + 'genomics/sarscov2/genome/genome.fasta', checkIfExists: true) + ]) + input[2] = BISMARK_GENOMEPREPARATION.out.index + """ + } + } + + then { + assertAll( + { assert process.success }, + // combined index = 1 both-strands pass (n=1); 8 cpus / 1 = -p 8 + { assert file(process.out.report[0][1]).text.contains('-p 8 --reorder') } + ) + } + } } diff --git a/modules/nf-core/bismark/align/tests/nextflow_pthreads.config b/modules/nf-core/bismark/align/tests/nextflow_pthreads.config new file mode 100644 index 000000000000..63080a7c19d4 --- /dev/null +++ b/modules/nf-core/bismark/align/tests/nextflow_pthreads.config @@ -0,0 +1,16 @@ +process { + // Force enough cpus that the intra-instance `-p` divisor emits `-p` (>= 2 threads + // per strand instance): at 8 cpus, directional -> -p 4, non-directional -> -p 2, + // combined (n=1) -> -p 8. + withName: BISMARK_ALIGN { + cpus = 8 + ext.args = params.bismark_args + } + // Genome preparation takes ONLY the aligner-selection (and, for the combined test, + // --combined_genome) flags — NOT --non_directional / --combined_index, which the + // bismark_genome_preparation parser rejects. Kept as a separate param so alignment + // flags never leak into the prep command. + withName: BISMARK_GENOMEPREPARATION { + ext.args = params.genomeprep_args + } +} From 4159ac7944c71dbd36645442a64018831a572950 Mon Sep 17 00:00:00 2001 From: Felix Krueger Date: Sun, 19 Jul 2026 17:24:04 +0100 Subject: [PATCH 2/3] test: cap bismark/align -p threading tests at 4 cpus (CI runner ceiling) The -p-threading tests requested cpus=8 to force the divisor to emit -p, but nf-core CI runners have 4 cpus and nextflow.enable.strict=true rejects a request exceeding available cores (req 8 > avail 4) before the process runs. Drop to cpus=4 (the runner ceiling): directional -> -p 2, HISAT2 directional -> -p 2, combined (n=1) -> -p 4. The non-directional case at 4/4=1 now asserts -p is OMITTED, exercising the omit-below-2 boundary. Verified 9/9 nf-test pass on native amd64 with the 3.1.0 container. --- .../nf-core/bismark/align/tests/main.nf.test | 24 +++++++++---------- .../align/tests/nextflow_pthreads.config | 9 +++---- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/modules/nf-core/bismark/align/tests/main.nf.test b/modules/nf-core/bismark/align/tests/main.nf.test index 145d7b8b9e41..77bd260abfff 100644 --- a/modules/nf-core/bismark/align/tests/main.nf.test +++ b/modules/nf-core/bismark/align/tests/main.nf.test @@ -206,7 +206,7 @@ nextflow_process { } } - test("bowtie2 | intra-instance -p threading | paired-end | 8 cpus") { + test("bowtie2 | intra-instance -p threading | paired-end | 4 cpus") { config './nextflow_pthreads.config' @@ -236,13 +236,13 @@ nextflow_process { then { assertAll( { assert process.success }, - // directional Bowtie 2 = 2 instances; 8 cpus / 2 = -p 4 - { assert file(process.out.report[0][1]).text.contains('-p 4 --reorder') } + // directional Bowtie 2 = 2 instances; 4 cpus / 2 = -p 2 + { assert file(process.out.report[0][1]).text.contains('-p 2 --reorder') } ) } } - test("bowtie2 | intra-instance -p threading | non_directional | 8 cpus") { + test("bowtie2 | non_directional | low-cpu omits -p (4 cpus / 4 instances < 2)") { config './nextflow_pthreads.config' @@ -269,13 +269,13 @@ nextflow_process { then { assertAll( { assert process.success }, - // non-directional = 4 instances; 8 cpus / 4 = -p 2 - { assert file(process.out.report[0][1]).text.contains('-p 2 --reorder') } + // non-directional = 4 instances; 4 cpus / 4 = 1 (< 2) -> -p omitted (I2 boundary) + { assert !file(process.out.report[0][1]).text.contains('-p ') } ) } } - test("hisat2 | intra-instance -p threading | paired-end | 8 cpus") { + test("hisat2 | intra-instance -p threading | paired-end | 4 cpus") { config './nextflow_pthreads.config' @@ -305,13 +305,13 @@ nextflow_process { then { assertAll( { assert process.success }, - // HISAT2 shares the 2/4 instance model + honours -p; directional = 2 instances, 8/2 = -p 4 - { assert file(process.out.report[0][1]).text.contains('-p 4 --reorder') } + // HISAT2 shares the 2/4 instance model + honours -p; directional = 2 instances, 4/2 = -p 2 + { assert file(process.out.report[0][1]).text.contains('-p 2 --reorder') } ) } } - test("bowtie2 | combined_index | intra-instance -p threading | single-end | 8 cpus") { + test("bowtie2 | combined_index | intra-instance -p threading | single-end | 4 cpus") { config './nextflow_pthreads.config' @@ -338,8 +338,8 @@ nextflow_process { then { assertAll( { assert process.success }, - // combined index = 1 both-strands pass (n=1); 8 cpus / 1 = -p 8 - { assert file(process.out.report[0][1]).text.contains('-p 8 --reorder') } + // combined index = 1 both-strands pass (n=1); 4 cpus / 1 = -p 4 + { assert file(process.out.report[0][1]).text.contains('-p 4 --reorder') } ) } } diff --git a/modules/nf-core/bismark/align/tests/nextflow_pthreads.config b/modules/nf-core/bismark/align/tests/nextflow_pthreads.config index 63080a7c19d4..d0f66ec3558b 100644 --- a/modules/nf-core/bismark/align/tests/nextflow_pthreads.config +++ b/modules/nf-core/bismark/align/tests/nextflow_pthreads.config @@ -1,9 +1,10 @@ process { - // Force enough cpus that the intra-instance `-p` divisor emits `-p` (>= 2 threads - // per strand instance): at 8 cpus, directional -> -p 4, non-directional -> -p 2, - // combined (n=1) -> -p 8. + // 4 cpus = the nf-core CI runner ceiling (strict mode hard-errors if a process + // requests more than is available). At 4 cpus the intra-instance `-p` divisor gives: + // directional -> -p 2, HISAT2 directional -> -p 2, combined (n=1) -> -p 4, and + // non-directional (n=4) -> 4/4=1 -> NO -p (exercises the I2 omit-below-2 boundary). withName: BISMARK_ALIGN { - cpus = 8 + cpus = 4 ext.args = params.bismark_args } // Genome preparation takes ONLY the aligner-selection (and, for the combined test, From bf33c7a02da8d33413f1d251ed1abb68e34e3a85 Mon Sep 17 00:00:00 2001 From: Felix Krueger Date: Fri, 24 Jul 2026 12:14:57 +0100 Subject: [PATCH 3/3] bismark/align: trim verbose comments in -p threading module Presentation-only pass on the intra-instance -p threading change: condense the aligner thread-model comment block and drop internal plan jargon in the test config/assertions. No logic, test, or snapshot changes. --- modules/nf-core/bismark/align/main.nf | 20 +++++-------------- .../nf-core/bismark/align/tests/main.nf.test | 8 ++++---- .../align/tests/nextflow_pthreads.config | 11 +++------- 3 files changed, 12 insertions(+), 27 deletions(-) diff --git a/modules/nf-core/bismark/align/main.nf b/modules/nf-core/bismark/align/main.nf index bab4717ab2e3..0ad81a42522d 100644 --- a/modules/nf-core/bismark/align/main.nf +++ b/modules/nf-core/bismark/align/main.nf @@ -28,16 +28,9 @@ process BISMARK_ALIGN { } def fastq = meta.single_end ? reads : "-1 ${reads[0]} -2 ${reads[1]}" - // Aligner thread model: - // - Bowtie 2 + HISAT2 run 2 (directional/pbat) or 4 (non-directional) *concurrent* - // strand instances, each of which honours `-p` (threads per instance). Dividing the - // task's cpus across the instances (`-p = task.cpus / n_instances`) uses all cpus with - // a single index load per instance, instead of `--multicore N` forking N chunks that - // each re-load the index (Bowtie 2: N x 2/4 concurrent index copies -> high peak RAM). - // Bowtie 2 `-p N --reorder` is thread-invariant (byte-identical); HISAT2 is near- - // invariant (a handful of reads re-baseline). See the resource-model benchmark. - // - minimap2/rammap ignore `-p` (bismark hard-codes minimap2 `-t 2`; rammap auto-scales), - // so they keep the legacy `--multicore` auto-compute untouched. + // Give each strand instance `-p = cpus / n_instances` threads (one index load per strand, + // byte-identical) instead of forking N --multicore chunks that each re-load the index. + // minimap2/rammap can't take -p from bismark yet, so they keep the legacy --multicore path. def isMinimapLike = args.contains('--minimap2') || args.contains('--mm2') || args.contains('--rammap') || args.contains('--ram') if(isMinimapLike){ @@ -71,18 +64,15 @@ process BISMARK_ALIGN { } } else if(!args.contains('--multicore') && !args.contains('--parallel') && !args.contains('-p ') && task.cpus){ - // Bowtie 2 / HISAT2: split the cpus across the concurrent strand instances. def n_instances = 2 if(args.contains('--combined_index')){ - // combined = one both-strands pass (n=1); only the opt-in parallel non-directional - // path runs two concurrent passes (n=2). + // combined index = one both-strands pass; parallel non-directional runs two. n_instances = (args.contains('--non_directional') && args.contains('--combined_index_parallel')) ? 2 : 1 } else if(args.contains('--non_directional')){ n_instances = 4 } - // -p threads per instance; emit only when it lands >= 2 (bismark requires -p >= 2), - // otherwise omit it (faithful single-thread-per-instance default). + // bismark requires -p >= 2, so omit it below that (one thread per instance). def pthreads = ((task.cpus as int) / n_instances) as int if(pthreads >= 2){ args += " -p ${pthreads}" diff --git a/modules/nf-core/bismark/align/tests/main.nf.test b/modules/nf-core/bismark/align/tests/main.nf.test index 77bd260abfff..60b34eb602a3 100644 --- a/modules/nf-core/bismark/align/tests/main.nf.test +++ b/modules/nf-core/bismark/align/tests/main.nf.test @@ -236,7 +236,7 @@ nextflow_process { then { assertAll( { assert process.success }, - // directional Bowtie 2 = 2 instances; 4 cpus / 2 = -p 2 + // directional = 2 instances; 4 cpus / 2 -> -p 2 { assert file(process.out.report[0][1]).text.contains('-p 2 --reorder') } ) } @@ -269,7 +269,7 @@ nextflow_process { then { assertAll( { assert process.success }, - // non-directional = 4 instances; 4 cpus / 4 = 1 (< 2) -> -p omitted (I2 boundary) + // non-directional = 4 instances; 4 cpus / 4 = 1 (< 2) -> no -p { assert !file(process.out.report[0][1]).text.contains('-p ') } ) } @@ -305,7 +305,7 @@ nextflow_process { then { assertAll( { assert process.success }, - // HISAT2 shares the 2/4 instance model + honours -p; directional = 2 instances, 4/2 = -p 2 + // HISAT2 uses the same 2/4-instance model; directional 4/2 -> -p 2 { assert file(process.out.report[0][1]).text.contains('-p 2 --reorder') } ) } @@ -338,7 +338,7 @@ nextflow_process { then { assertAll( { assert process.success }, - // combined index = 1 both-strands pass (n=1); 4 cpus / 1 = -p 4 + // combined index = 1 pass; 4 cpus / 1 -> -p 4 { assert file(process.out.report[0][1]).text.contains('-p 4 --reorder') } ) } diff --git a/modules/nf-core/bismark/align/tests/nextflow_pthreads.config b/modules/nf-core/bismark/align/tests/nextflow_pthreads.config index d0f66ec3558b..595274efdab3 100644 --- a/modules/nf-core/bismark/align/tests/nextflow_pthreads.config +++ b/modules/nf-core/bismark/align/tests/nextflow_pthreads.config @@ -1,16 +1,11 @@ process { - // 4 cpus = the nf-core CI runner ceiling (strict mode hard-errors if a process - // requests more than is available). At 4 cpus the intra-instance `-p` divisor gives: - // directional -> -p 2, HISAT2 directional -> -p 2, combined (n=1) -> -p 4, and - // non-directional (n=4) -> 4/4=1 -> NO -p (exercises the I2 omit-below-2 boundary). + // 4 cpus = nf-core CI ceiling; enough to exercise the -p divisor across paths. withName: BISMARK_ALIGN { cpus = 4 ext.args = params.bismark_args } - // Genome preparation takes ONLY the aligner-selection (and, for the combined test, - // --combined_genome) flags — NOT --non_directional / --combined_index, which the - // bismark_genome_preparation parser rejects. Kept as a separate param so alignment - // flags never leak into the prep command. + // Prep gets only the aligner flag (+ --combined_genome), not the alignment-only flags + // (--non_directional/--combined_index) that its parser rejects. withName: BISMARK_GENOMEPREPARATION { ext.args = params.genomeprep_args }