feature/srwgs - #18
Conversation
rernst
commented
Aug 14, 2026
rernst
commented
- Add dragen and dragenutils
BasMonkey
left a comment
BasMonkey
left a comment
There was a problem hiding this comment.
Great work overall! I've left a few comments below with some suggestions that I think could further improve the codebase.
| "nextflow": "25.10.2" | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing newline at the end of this file, see this StackOverflow thread on why it's important.
There was a problem hiding this comment.
Snap files are automatically generated by nf-test. I don't think we should update/change these because they are compared using diff when running (automated) tests.
| } | ||
|
|
||
| """ | ||
| mkdir -p /scratch/reference |
There was a problem hiding this comment.
I can imagine that hardcoding /scratch/reference could become problematic when multiple DRAGEN tasks run concurrently on the same node. This may increase the risk of race conditions or file collisions. I would suggest using task-specific scratch directories (for example /scratch/${task.workDir.name}) to priovide better isolation between tasks.
There was a problem hiding this comment.
@Jorisvansteenbrugge is 'fixing' this by creating a separate process for untarring the reference genome, see: https://github.com/UMCUGenetics/NF-Modules/blob/feature/dragen_genome_untar/modules/UMCUGenetics/untar/genome/main.nf
This also makes sure that we only perform the untar once per pipeline excecution.
There was a problem hiding this comment.
Nice solution @Jorisvansteenbrugge ! I think it's safe to keep this as-is too.
| --fastq-list-sample-id ${meta.id} \\ | ||
| --output-file-prefix ${prefix} \\ | ||
| --output-directory ./ \\ | ||
| --intermediate-results-dir /scratch \\ |
There was a problem hiding this comment.
Same as the comment above about hardcoded /scratch.
There was a problem hiding this comment.
I think for this module it is fine to just use /scratch, because this process can only run on ICA, where this /scratch is a special folder (see https://help.ica.illumina.com/project/p-flow/f-pipelines#scratch-space-notes). On ICA this scratch folder is unique to the job.
There was a problem hiding this comment.
Makes sense. You may ignore my comment.
| tuple val(meta), path("${prefix}.mapping_metrics.csv"), optional: true, emit: mapping_metrics | ||
| tuple val(meta), path("${prefix}.ploidy_estimation_metrics.csv"), optional: true, emit: ploidy_estimation_metrics | ||
| tuple val(meta), path("${prefix}.gvcf_metrics.csv"), optional: true, emit: gvcf_metrics | ||
| tuple val("${task.process}"), val('dragen'), eval("dragen --version 2>&1 | sed 's/^dragen Version //'"), topic: versions, emit: versions_dragen |
There was a problem hiding this comment.
dragen --version in the eval block fails because the dragen binary seems to be at /opt/edico/bin/dragen and is not in $PATH. I would suggest to update it to use the full path: /opt/edico/bin/dragen --version 2>&1 | sed 's/^dragen Version //'.
There was a problem hiding this comment.
The problem here is that we can't run this test using the dragen container -> this proprietary software and we can't access it, furthermore we don't have FPGA nodes available within GitHub Actions. In order to make the stub test work (just testing in/output logic) we overwrite the container in https://github.com/UMCUGenetics/NF-Modules/blob/feature/srwgs/tests/config/modules.config with a simple container containing only bash.
See also nf-core example: https://github.com/nf-core/modules/blob/master/modules/nf-core/dragen/germline/tests/main.nf.test.snap
There was a problem hiding this comment.
This makes sense now. Thanks for the clarification!
BasMonkey
left a comment
BasMonkey
left a comment
There was a problem hiding this comment.
Thanks for the clarifications! Looks all good now!
2 participants
