Add WDL implementation for Salmon by PriyankaaXD · Pull Request #326 · stjudecloud/workflows

PriyankaaXD · 2026-08-13T00:19:59Z

Adds a WDL implementation for Salmon (mapping-mode only), per the "tool wishlist" issue #228.

Tasks added in tools/salmon.wdl:

build_salmon_index — wraps salmon index
quant — wraps salmon quant

All "important options" from Salmon's docs are exposed as inputs, with defaults verified against salmon quant --help-reads output 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.yml using the new Sprocket test framework, covering both tasks with real output assertions. Verified locally: sprocket lint passes cleanly, sprocket dev test passes 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:

You have added a few sentences describing the PR here.
The code passes all CI tests without any errors or warnings.
You have added tests (when appropriate).
You have added an entry in any relevant CHANGELOGs (when appropriate).
If you have made any changes to the scripts/ or docker/ directories, please ensure any image versions have been incremented accordingly!
You have updated the README or other documentation to account for these changes.

stjudecloud-cloudy · 2026-08-13T00:20:17Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

adthrasher · 2026-08-13T13:16:57Z

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.

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 )

I removed these customs files entirely and switched to reusing the existing shared fixtures (fastqs/test_R1.fq.gz/test_R2.fq.gz) instead and rebuilt the test transcriptome from real sequences within those files, so both build_salmon_index and quant tests now run against existing shared data rather than anything new.

adthrasher · 2026-08-13T13:18:57Z

Can you rename this to use yaml as the extension to match our repository convention?

Also this should go under tools/test/.

adthrasher · 2026-08-13T13:20:54Z

+
+    runtime {
+        cpu: ncpu
+        memory: "16 GB"


Does salmon use a consistent amount of RAM or is it dependent on the input and/or transcriptome?

Sorry, I originally hardcoded 16 GB without thinking much about how the memory usage would actually vary.

Salmon's memory usage mainly depends on the index size, which depends on the transcriptome size and whether decoys are included. The input reads are streamed, so their size doesn't have as much impact on RAM usage.

I've now updated both build_salmon_index and quant to calculate memory_gb dynamically based on the input size, similar to how disk_size_gb is already handled. There's also a modify_memory_gb option if the estimate needs to be adjusted for specific data.

adthrasher · 2026-08-13T13:22:38Z

+        cpu: ncpu
+        memory: "16 GB"
+        disks: "~{disk_size_gb} GB"
+        container: "quay.io/biocontainers/salmon:1.9.0--h7e5ed60_0"


Is there a reason we're using such an old version of salmon?

I'd picked 1.9.0 somewhat arbitrarily. I did try updating to 1.12.1 (the latest release still on the original C++ codebase — 2.0+ is a full Rust rewrite with a different index format, so I avoided that for now), but that specific container build (quay.io/biocontainers/salmon:1.12.1--h017bda4_0) hits a locale::facet::_S_create_c_locale crash during indexing in my test environment — a known class of bug in minimal Docker images missing locale data, unrelated to our WDL logic itself. Reverting to 1.9.0, which runs cleanly and passes both tests. Happy to revisit if you know of a working newer tag, or if this is worth filing upstream with BioContainers.

according to the official migration doc - https://github.com/COMBINE-lab/salmon/blob/master/MIGRATION.md

I'm not seeing any reason we wouldn't want the latest version (rewrite and all). @adthrasher any reason you see not to use a >=v2 version?

adthrasher · 2026-08-13T13:24:06Z

+            "~{if length(read_twos) == 0 then "--fldMean " + fld_mean else ""}" \
+            "~{if length(read_twos) == 0 then "--fldSD " + fld_sd else ""}" \


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?

adthrasher · 2026-08-13T13:28:16Z

@@ -0,0 +1,255 @@
+version 1.1
+
+task build_salmon_index {


The build task likely needs the decoys mode exposed as that is the recommended way to run with mapping mode.

The information on running with decoys is scattered and not mentioned on the 2.0 doc site - https://combine-lab.github.io/salmon/

I was able to find this on the old docs site - https://salmon.readthedocs.io/en/latest/salmon.html#preparing-transcriptome-indices-mapping-based-mode
and this SC guide that seems to be where @PriyankaaXD pulled the current Bash from - https://combine-lab.github.io/alevin-tutorial/2019/selective-alignment/

I think decoy-aware indexing will be a must for any production workflow, but it seems like a barrel of worms that we can address in a follow up PR. I think we'll probably need a separate WDL task for doing the decay-aware ref building, and for this PR we can merge without touching decoys at all.

Unless @adthrasher is there a straightforward solution I'm missing?

I'm OK either way. I think the proposed version in this PR is a straightforward start.

adthrasher · 2026-08-13T13:29:17Z

+            -l "~{lib_type}" \
+            -1 ~{sep(" ", squote(read_one_fastqs_gz))} \
+            ~{if length(read_twos) > 0 then "-2 " + sep(" ", squote(read_twos)) else ""} \
+            --validateMappings \


This is the default right? This probably needs to be a Boolean input with a true default.

adthrasher · 2026-08-13T13:29:55Z

+          - Name: salmon_index.tar.gz
+
+quant:
+  - name: quantifies_paired_end_reads


Since SE mode is implemented, it should get a test.

I added a single-end test — and it actually caught a real bug: quant was always using -1/-2 regardless of read type, which Salmon rejects for genuine single-end input (it requires -r instead). Fixed the command logic to switch based on whether read_two_fastqs_gz is provided.

adthrasher · 2026-08-13T13:30:30Z

The index here also needs documentation on how it was generated.

Added test/fixtures/salmon/README.md explaining how transcripts.fasta and salmon_index.tar.gz were generated from the shared FASTQ fixtures.

adthrasher · 2026-08-13T13:31:21Z

+
+    input {
+        File salmon_index_tar_gz
+        Array[File] read_one_fastqs_gz


Suggested change

Array[File] read_one_fastqs_gz

Array[File]+ read_one_fastqs_gz

This needs to be non-empty.

@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 🤷‍♀️

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).

I'll investigate 🫡
in the mean time, let's follow through with Andrew's original recommendation here and make it non-empty

The CHANGELOG entry points to #240

adthrasher · 2026-08-13T13:32:06Z

This also needs a CHANGELOG entry.

…d FASTQ data, add SE test

a-frantz

This is looking really great! Left some comments to address, but this is close to the finish line 🚀

a-frantz · 2026-08-23T20:07:55Z

@@ -0,0 +1,5 @@
+# Salmon test fixtures
+
+`transcripts.fasta` — synthetic transcripts built by concatenating real R1 + reverse-complemented R2 sequences from the shared `fastqs/test_R1.fq.gz`/`fastqs/test_R2.fq.gz` fixtures, ensuring genuine alignment for testing purposes.


the input to build_salmon_index can be just any FASTAs, so what you have here isn't invalid for any technical reason, however to closer replicate the real data (mostly for posterity rather than correctness) we can drop this transcripts.fasta file and instead use one of the existing .fa files from fixtures/reference/. Those aren't quite right either, as really the input to Salmon indexing should be from the transcriptome, and the fasta sequences we've already committed to the repo are from the genome.

Ideally, we should really be committing a new subsetted transcriptome reference and using one of the existing genome references as decoy sequences; but, this repo is already ginormous with all our reference test files and I'd rather not add a new FASTA when existing FASTAs will work fine.

Please uncommit this transcripts.fasta file, and regenerate salmon_index.tar.gz using one of the already committed .fa files (dealer's choice for which)

a-frantz · 2026-08-23T20:10:53Z

@@ -0,0 +1,33 @@
+build_salmon_index:
+  - name: builds_index_successfully


can you add a test verifying the decoy argument works as expected? Per my prior comment, any fasta file should work for validating the processing. It's a bit nonsensical data-wise, but you could use GRCh38.chr1_chr19.fa as the reference and the chrY_chrM fasta as the decoy.

a-frantz · 2026-08-23T20:13:08Z

+        transcripts_fasta: "FASTA format file containing the reference transcriptome to index"
+        decoys_fasta: {
+            description: "Optional FASTA file containing decoy genome sequences to improve mapping specificity.",
+            help: "Per Salmon's recommended decoy-aware indexing workflow.",


Suggested change

help: "Per Salmon's recommended decoy-aware indexing workflow.",

I'd rather not include any "recommended" language here

a-frantz · 2026-08-23T20:15:57Z

+            n_cores=$(nproc)
+        fi
+
+        gentrome="~{transcripts_fasta}"


"gentrome" is not a term I've seen elsewhere (portmanteau of "genome" and "transcriptome"), and the decoy workflow has other appropriate uses besides a transcriptome+genome, so I would rename this var more generically for clarity:

Suggested change

gentrome="~{transcripts_fasta}"

fasta="~{transcripts_fasta}"

a-frantz · 2026-08-23T20:17:08Z

+        cpu: ncpu
+        memory: "~{ceil(transcripts_fasta_size * 4) + 4 + modify_memory_gb} GB"
+        disks: "~{disk_size_gb} GB"
+        container: "quay.io/biocontainers/salmon:1.9.0--h7e5ed60_0"


can we update to a Salmon-v2 container?

a-frantz · 2026-08-23T20:21:10Z

+        tar -czf "~{prefix}.tar.gz" "~{prefix}"
+    >>>
+
+    output {


can we also output the "raw" quant.sf file? - https://combine-lab.github.io/salmon/reference/output-formats/

this should be in addition the tarballed output. Some users will likely only care about the quant file, and for running a workflow the quant file may be the only one that's needed downstream so having to extract it from a tarball is a cumbersome intermediate step.

…decoys help text

PriyankaaXD mentioned this pull request Aug 13, 2026

tool wishlist #228

Open

adthrasher requested review from DelaramR, a-frantz and adthrasher August 13, 2026 13:14

adthrasher assigned PriyankaaXD Aug 13, 2026

adthrasher requested changes Aug 13, 2026

View reviewed changes

PriyankaaXD added 10 commits August 19, 2026 10:29

Update Salmon container image to a more current version

1332a54

Add Sprocket unit tests for Salmon WDL tasks

81f8293

Fix fldMean/fldSD argument quoting and suppress SC2086 false-positive

5282101

Move test file to tools/test/ and rename to .yaml extension

c07cb93

Fix single-end quant mode (-r flag), rebuild test fixtures from share…

3d2e4a5

…d FASTQ data, add SE test

Require non-empty array for read_one_fastqs_gz

d3c3c24

Expose validateMappings as a Boolean input

581ac93

Expose decoy-aware indexing as an optional input on build_salmon_index

fa9d808

Make memory allocation dynamic for both tasks, based on input size

449001d

Add CHANGELOG entry for Salmon WDL implementation

4f222d5

PriyankaaXD force-pushed the add-salmon-wdl branch from c2d9dde to 4f222d5 Compare August 19, 2026 05:17

Document how Salmon test fixtures were generated

cc2177c

PriyankaaXD requested a review from adthrasher August 19, 2026 05:53

a-frantz requested changes Aug 23, 2026

View reviewed changes

Rename gentrome variable to fasta, remove 'recommended' wording from …

6020a4e

…decoys help text

		"~{if length(read_twos) == 0 then "--fldMean " + fld_mean else ""}" \
		"~{if length(read_twos) == 0 then "--fldSD " + fld_sd else ""}" \

	Array[File] read_one_fastqs_gz
	Array[File]+ read_one_fastqs_gz

		@@ -0,0 +1,5 @@
		# Salmon test fixtures

		`transcripts.fasta` — synthetic transcripts built by concatenating real R1 + reverse-complemented R2 sequences from the shared `fastqs/test_R1.fq.gz`/`fastqs/test_R2.fq.gz` fixtures, ensuring genuine alignment for testing purposes.

		@@ -0,0 +1,33 @@
		build_salmon_index:
		- name: builds_index_successfully

Uh oh!

Conversation

PriyankaaXD commented Aug 13, 2026

Uh oh!

stjudecloud-cloudy commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

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!

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!

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!

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!

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!

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!

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!

adthrasher commented Aug 13, 2026

Uh oh!

a-frantz left a comment

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!

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!

Reviewers

stjudecloud-cloudy commented Aug 13, 2026 •

edited

Loading