Add WDL implementation for Salmon - #326
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
I didn't check out the commit, but can you comment on where these reads were sourced? We want to track the origin our our test data.
There was a problem hiding this comment.
I'd prefer if we didn't add these files. We already have FASTQ fixtures - https://github.com/stjudecloud/workflows/blob/main/test/fixtures/fastqs/README.md
The existing test fixtures should be reused (re: #280 , I don't want more LFS files hitting the history )
There was a problem hiding this comment.
Can you rename this to use yaml as the extension to match our repository convention?
There was a problem hiding this comment.
Also this should go under tools/test/.
|
|
||
| runtime { | ||
| cpu: ncpu | ||
| memory: "16 GB" |
There was a problem hiding this comment.
Does salmon use a consistent amount of RAM or is it dependent on the input and/or transcriptome?
| cpu: ncpu | ||
| memory: "16 GB" | ||
| disks: "~{disk_size_gb} GB" | ||
| container: "quay.io/biocontainers/salmon:1.9.0--h7e5ed60_0" |
There was a problem hiding this comment.
Is there a reason we're using such an old version of salmon?
| "~{if length(read_twos) == 0 then "--fldMean " + fld_mean else ""}" \ | ||
| "~{if length(read_twos) == 0 then "--fldSD " + fld_sd else ""}" \ |
There was a problem hiding this comment.
I don't think this will work as the arguments end up quoted in bash. Was this an attempt to address a sprocket lint warning?
| @@ -0,0 +1,255 @@ | |||
| version 1.1 | |||
|
|
|||
| task build_salmon_index { | |||
There was a problem hiding this comment.
The build task likely needs the decoys mode exposed as that is the recommended way to run with mapping mode.
| -l "~{lib_type}" \ | ||
| -1 ~{sep(" ", squote(read_one_fastqs_gz))} \ | ||
| ~{if length(read_twos) > 0 then "-2 " + sep(" ", squote(read_twos)) else ""} \ | ||
| --validateMappings \ |
There was a problem hiding this comment.
This is the default right? This probably needs to be a Boolean input with a true default.
| - Name: salmon_index.tar.gz | ||
|
|
||
| quant: | ||
| - name: quantifies_paired_end_reads |
There was a problem hiding this comment.
Since SE mode is implemented, it should get a test.
There was a problem hiding this comment.
The index here also needs documentation on how it was generated.
|
|
||
| input { | ||
| File salmon_index_tar_gz | ||
| Array[File] read_one_fastqs_gz |
There was a problem hiding this comment.
| Array[File] read_one_fastqs_gz | |
| Array[File]+ read_one_fastqs_gz |
This needs to be non-empty.
There was a problem hiding this comment.
@adthrasher I think we stopped using non-empty arrays as the resulting WDL is unwieldy ? Or we had a commit adding them and then removing them? I can't remember where we landed on it, but I'm fine without this. If the user doesn't supply any FASTQs, salmon will blow up with an informative error, so 🤷♀️
There was a problem hiding this comment.
I can't remember the details now. It's something we should probably revisit, though. I'd much rather the WDL fail upfront at analysis because of an empty array than the underlying tool erroring. If WDL doesn't do non-empty arrays well, then we should push for updates to the spec and to the engine(s).
There was a problem hiding this comment.
I'll investigate 🫡
in the mean time, let's follow through with Andrew's original recommendation here and make it non-empty
|
This also needs a |
4 participants
Adds a WDL implementation for Salmon (mapping-mode only), per the "tool wishlist" issue #228.
Tasks added in
tools/salmon.wdl:build_salmon_index— wrapssalmon indexquant— wrapssalmon quantAll "important options" from Salmon's docs are exposed as inputs, with defaults verified against
salmon quant --help-readsoutput on Salmon 1.9.0. Parameter documentation is copied from Salmon's official docs, per guidance in the issue. Scoped to mapping-mode only (FASTQ input) — no BAM/alignment-mode support, as requested.Tests added in
tools/salmon.ymlusing the new Sprocket test framework, covering both tasks with real output assertions. Verified locally:sprocket lintpasses cleanly,sprocket dev testpasses both tests.This is my first contribution to this project — happy to make any adjustments you'd like!
Before submitting this PR, please make sure:
scripts/ordocker/directories, please ensure any image versions have been incremented accordingly!