perf: prevent bin QC block on all binner jobs finishing by dialvarezs · Pull Request #1062 · nf-core/mag

dialvarezs · 2026-06-27T06:18:03Z

Performance / QoL improvement

In the current implementation, bins are grouped by sample before being fed to SEQKIT_STATS, which computes their sizes for downstream filtering. Because groupTuple has no size, it can't emit until every binner has finished, blocking bin QC for bins that already finished.

This PR removes that grouping and feeds each binner's bins to SEQKIT_STATS as they complete. Functionality is preserved: downstream re-keys the stats by bin filename (splitCsv -> join), so the result is identical regardless of how the seqkit jobs are batched.

The only drawback is more seqkit jobs (from 1 per sample to 1 per sample+assembler+binner), but that's negligible compared to removing the global pipeline lock.

PR checklist

piplus2

I see the advantage of splitting the stat into many processes but still there's the risk of having thousands of them.

piplus2 · 2026-06-27T17:14:48Z

        ch_binning_results_gzipped_final = ch_binning_results_gzipped_final.mix(ch_semibin_bins)
    }

-    // group bins into per-sample process and not flood clusters with thousands of seqkit jobs


what about the possible creation of thousands of jobs as reported here?

Yes, that's the drawback. In practice, the worst case (all binners and all assemblers enabled) works out to about samples * 30 jobs, and most realistic configurations should be around samples * 10 or less.
I think that's an acceptable tradeoff, and arguably better than the hard lock we have today: because some binners are way slower than others, the current behavior can force you to wait 10+ hours for the last binner to finish before the pipeline can continue. Grouping per-sample avoids that serial bottleneck.

That's still going to generate hundreds of jobs... On a standard HPC most of them will go on queue. Since it's a big change, I'd suggest to perform some tests on real scenarios. Also, I'm not sure that the way the stats results are currently processed in the subsequent steps of the workflow are consistent with these changes. From my understanding they expect a single tsv per sample.

For reference, ch_seqkitstats_results stores one item per bin (it splits the seqkit CSV per row), so the downstream result ends up exactly the same regardless of how many tsv files seqkit produces, nothing expects a single tsv per sample.

I think the numbers of jobs is a valid concern, but most HPC handle small jobs without issues, and also they will not be spawned at the same time, as the binning jobs finish instead.

As an additional note, the whole post-binning workflow already runs at the same level of granularity (sample * assembler * binner): BUSCO, CheckM(2), GUNC, GTDB-Tk, CATPACK, Tiara, etc. So in practice, seqkit will not dispatch a lot of jobs compared to other tools in the pipeline.

I'm running some mag benchmarks now and hit this: my mag run has been about ~15h just waiting for COMEbin to finish, after it failed once.

So @piplus2 is sort of fair in his worry of flooding a HPC with lots of jobs - while you are right @dialvarezs that it is sort of the same for all the others, the problem is the seqkitstats is a very short/fast job which is something HPC admins do not like (batch them! Use an array!) as it adds a bunch of unnecessary strain on the scheduler database etc (apparently anyway).

Even if I think that would probably not toooo terrible if Diego's estimate is correct (samples * 30), as a mid-ground we could either:

Just document/make people aware of maxForks

Have a dedicated parameter than controls maxForks for this one process? (this would also arguably be more visible than just document)

What do you think?

I understand your point. Since this is a major change in the workflow, I'd like to leave it to @jfy133, as from my understanding, he wrote the original part.

…-from-binners

piplus2

@jfy133 I feel you may know this better than me, I don't feel confident enough about the possible consequences of these changes.

piplus2 · 2026-06-29T08:42:30Z

        ch_binning_results_gzipped_final = ch_binning_results_gzipped_final.mix(ch_semibin_bins)
    }

-    // group bins into per-sample process and not flood clusters with thousands of seqkit jobs


I understand your point. Since this is a major change in the workflow, I'd like to leave it to @jfy133, as from my understanding, he wrote the original part.

jfy133 · 2026-06-29T07:11:45Z

    "-profile assembly_input": {
        "content": [
-            285,
+            291,


Are the additional files variable?

Yes, these are the additonal processes from "flattening" seqkit.

jfy133 · 2026-06-29T07:17:07Z

        ch_binning_results_gzipped_final = ch_binning_results_gzipped_final.mix(ch_semibin_bins)
    }

-    // group bins into per-sample process and not flood clusters with thousands of seqkit jobs


So @piplus2 is sort of fair in his worry of flooding a HPC with lots of jobs - while you are right @dialvarezs that it is sort of the same for all the others, the problem is the seqkitstats is a very short/fast job which is something HPC admins do not like (batch them! Use an array!) as it adds a bunch of unnecessary strain on the scheduler database etc (apparently anyway).

Even if I think that would probably not toooo terrible if Diego's estimate is correct (samples * 30), as a mid-ground we could either:

Just document/make people aware of maxForks

Have a dedicated parameter than controls maxForks for this one process? (this would also arguably be more visible than just document)

What do you think?

piplus2 · 2026-06-30T09:25:23Z

@jfy133 maxForks makes sense to me. At least we have full control of what's gonna happen.

dialvarezs · 2026-06-30T14:40:28Z

maxForks makes sense to me.
I'm not sure how we should handle the parameter, though. A single one for seqkit and also gunzip and prokka (these ones are the heavy offenders, the run per bin)?

jfy133 · 2026-07-01T07:57:31Z

maxForks makes sense to me.
I'm not sure how we should handle the parameter, though. A single one for seqkit and also gunzip and prokka (these ones are the heavy offenders, the run per bin)?

In principle yes. Or if they are are all running per bin, you could share across all because the number of bins won't change much at each stage

…-from-binners # Conflicts: # tests/test_assembly_input.nf.test.snap # tests/test_hybrid.nf.test.snap # tests/test_longreadonly.nf.test.snap # tests/test_single_end.nf.test.snap

dialvarezs · 2026-07-02T12:20:29Z

I added a new parameter to control seqkit concurrency, --bin_seqkit_stats_max_forks. By deafult is disabled (no cap).
I'll add one for prokka too, but in another PR.

jfy133 · 2026-07-02T12:22:43Z


    withName: SEQKIT_STATS {
        ext.args   = ""
+        maxForks   = params.bin_seqkit_stats_max_forks ?: null


The default should be set in nextflow.config, no?

Yep, we can remove the ?: null

perf: avoid seqkit groupTuple that blocks bin QC on slowest binner

9a92603

dialvarezs requested review from d4straub, jfy133, muabnezor and prototaxites as code owners June 27, 2026 06:18

dialvarezs changed the title ~~perf: prevent bin QC block on slowest binner~~ perf: prevent bin QC block on all binner jobs finishing Jun 27, 2026

dialvarezs added 3 commits June 27, 2026 03:20

chore: update changelog

72a1c5d

chore: update snapshots

1f034e9

Merge branch 'dev' into perf/decouple-seqkit-from-binners

8ae9382

piplus2 reviewed Jun 27, 2026

View reviewed changes

dialvarezs added 2 commits June 27, 2026 14:30

Merge remote-tracking branch 'upstream/dev' into perf/decouple-seqkit…

dabe12f

…-from-binners

chore: update snapshot

083f3ce

piplus2 reviewed Jun 29, 2026

View reviewed changes

jfy133 reviewed Jun 30, 2026

View reviewed changes

Merge branch 'dev' into perf/decouple-seqkit-from-binners

06cd652

dialvarezs added 4 commits July 2, 2026 05:33

Merge branch 'dev' into perf/decouple-seqkit-from-binners

37e86b9

Merge remote-tracking branch 'upstream/dev' into perf/decouple-seqkit…

5a14101

…-from-binners # Conflicts: # tests/test_assembly_input.nf.test.snap # tests/test_hybrid.nf.test.snap # tests/test_longreadonly.nf.test.snap # tests/test_single_end.nf.test.snap

chore: update snapshots

6187b4f

feat: add bin_seqkit_stats_max_forks

04f98bf

dialvarezs requested a review from jfy133 July 2, 2026 12:20

jfy133 approved these changes Jul 2, 2026

View reviewed changes

dialvarezs added 2 commits July 2, 2026 09:25

chore: remove unneded defensive default

fac0f67

fix: typo

5ac718e

dialvarezs merged commit 03de338 into nf-core:dev Jul 2, 2026
23 checks passed

Uh oh!

Conversation

dialvarezs commented Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

piplus2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dialvarezs Jun 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piplus2 Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piplus2 left a comment

Choose a reason for hiding this comment

Uh oh!

piplus2 Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piplus2 commented Jun 30, 2026

Uh oh!

dialvarezs commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfy133 commented Jul 1, 2026

Uh oh!

dialvarezs commented Jul 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dialvarezs commented Jun 27, 2026 •

edited

Loading

dialvarezs Jun 27, 2026 •

edited

Loading

piplus2 Jun 29, 2026 •

edited

Loading

piplus2 Jun 29, 2026 •

edited

Loading

dialvarezs commented Jun 30, 2026 •

edited

Loading