fix: deduplicate bclconvert FASTQ outputs - #12883
Conversation
matthdsm
left a comment
There was a problem hiding this comment.
Nice change! I encountered the same issue in our demux workflow
erikrikarddaniel
left a comment
There was a problem hiding this comment.
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'snft-utilsbump 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'srecursiveParseresolving an empty string viaPaths.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
left a comment
There was a problem hiding this comment.
Approving — the findings above are nits/food-for-thought, not blockers.
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 throughsanitizeOutput(). That output includes read-group metadata where the library value can be empty:In
nft-utils1.1.1, every string encountered recursively is tested as a possible filesystem path. An empty string behaves as follows: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 directoriesThis 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.