Skip to content

fix: deduplicate bclconvert FASTQ outputs - #12883

Merged
maxulysse merged 3 commits into
nf-core:masterfrom
atrigila:fix/bcl-demultiplex-duplicate-fastq-clean
Sep 11, 2026
Merged

fix: deduplicate bclconvert FASTQ outputs#12883
maxulysse merged 3 commits into
nf-core:masterfrom
atrigila:fix/bcl-demultiplex-duplicate-fastq-clean

Conversation

@atrigila

@atrigila atrigila commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

This PR is missing its nf-test snapshot due to an issue with the latest version (v1.1.1) of sanitizeOutput().

In main.nf.test, the test passes the entire workflow output through sanitizeOutput(). That output includes read-group metadata where the library value can be empty:

rg.LB = row.RGLB ? row.RGLB : ""

In nft-utils 1.1.1, every string encountered recursively is tested as a possible filesystem path. An empty string behaves as follows:

"" -> Paths.get("") -> current working directory

sanitizeOutput() consequently walked and hashed the modules repository instead of retaining an empty string. The generated snapshot currently contains unrelated files such as thousands of repository files and directories

This is the same behaviour documented in [nf-core/nft-utils issue #84](nf-core/nft-utils#84).


This PR will then require nft-utils to be released first to its latest dev version with the fix, incorporate that new plugin release into nf-core/modules and only after that update this PR' snapshot.

@maxulysse
maxulysse marked this pull request as ready for review September 11, 2026 09:21
@maxulysse
maxulysse requested review from a team as code owners September 11, 2026 09:21

@matthdsm matthdsm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change! I encountered the same issue in our demux workflow

@erikrikarddaniel erikrikarddaniel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with some help from Claude Code.

The core fix looks correct: createReadgroupBCLCONVERT now dedupes by a filename-based fastq_key before building each readgroup tuple, using the same file(fq).name matching convention the function already relied on for fastq1/fastq2 lookup, so it's not introducing a new matching assumption. Traced the rest of the diff:

  • nf-test.config's nft-utils bump to 1.2.0 is real and does contain the fix referenced in the PR description (checked the GitHub release notes and nf-core/nft-utils#84/#85 -- sanitizeOutput's recursiveParse resolving an empty string via Paths.get("") to cwd and recursively hashing it).
  • Most of tests/main.nf.test.snap's diff is explained by that version bump (directory-content snapshots expanding from bare names like "Logs"/"multiqc_data" into full recursive file listings) plus the Nextflow version bump embedded by nf-test (26.04.1 -> 26.08.0) and a fresh timestamp -- not by this PR's code change.

One gap worth flagging: the bcl_demultiplex readgroup count in the snapshot is unchanged before/after (4 entries, same samples) -- so as far as I can tell the existing fixture's fastq_list.csv doesn't actually contain a duplicate row that exercises the new emitted_fastq dedup path. CI passing here doesn't seem to be evidence the fix works, since nothing in the suite currently produces a duplicate to dedupe. Might be worth a small addition to the test CSV (a second row pointing at an already-used fastq pair) to actually assert the output count drops, since right now the fix is only exercised in production data, not in CI.

Separately, I couldn't tell from the PR title/description what BCL Convert scenario produces duplicate fastq_list.csv rows for the same output file pair in the first place -- my own best guess is multi-lane samples combined with --no-lane-splitting (one CSV row per lane, but a single merged fastq per sample), but that's inference on my part, not something I could confirm from BCL Convert's own docs. If that's right, a one-line comment or PR description addition explaining the trigger would help the next person who touches this function understand why the dedup key is filename-based rather than, say, RGID-based.

The unrelated bcl2fastq MD5 changes further down the snapshot (ConversionStats.xml, DemuxSummaryF1L1.txt, Stats.json, laneBarcode.html) sit under a function this PR doesn't touch (generateReadgroupBCL2FASTQ) -- looks like normal snapshot churn from re-running against the new nextflow/nft-utils versions rather than a side effect of this fix, but flagging in case it's not.

Non-blocking either way -- @maxulysse's already approved and I don't have anything that should hold this up, just leaving these as food for thought.

@erikrikarddaniel erikrikarddaniel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the findings above are nits/food-for-thought, not blockers.

@maxulysse
maxulysse added this pull request to the merge queue Sep 11, 2026
Merged via the queue into nf-core:master with commit c5d82c0 Sep 11, 2026
31 of 35 checks passed
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.

4 participants