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: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:32:06Z

This also needs a CHANGELOG entry.

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: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?

Quick note on the v2 migration: I updated the container and confirmed all our command-line flags remain compatible (both index and quant), and all 4 tests pass. But one thing worth flagging — the new SSHash-based index format is significantly larger than v1's for the same reference: our GRCh38.chrY_chrM.fa-based fixture went from 224MB (v1/pufferfish) to 1.15GB (v2/SSHash). Given the earlier concern about repo/LFS bloat, wanted to surface this before it lands — let me know if you'd still like to proceed with committing this larger fixture, or if you'd prefer a different approach (e.g., a smaller subset reference, or building the index fresh in CI rather than committing it).

wow, that size explosion is shocking! Pinging @adthrasher as he might find this interesting.

I did some digging this morning, and I think the reason a tiny ref FASTA is creating such a huge index is because we are using subsetted chromosomes from the genome as the reference sequences, where Salmon is expecting individual transcripts from the transcriptome. There is likely an exponential relationship between ref seq length and disk size.

Interestingly, when I tried passing the chr1_chr19 fasta as a decoy to the chrY_chrM build, the resulting index shrank by more than half.

Ultimately though, I think we should commit an appropriate transcriptome reference and use that here, instead of taking the lazy shortcut I suggested earlier 😅 I will create a usable ref FASTA from the human reference transcriptome and commit it to main, then you can rebase this PR and use the file I upload as an input to salmon index for the tests.

creating- gencode.v50.BCR_ABL1.transcripts.fa.gz — confirmed the index is now genuinely tiny (311KB vs. 1.1GB before), so that fully resolves the size issue!

One thing I've hit: our existing quant tests use the shared fastqs/test_R1.fq.gz/test_R2.fq.gz fixtures, but those were simulated from chrY_chrM — Salmon (correctly) reports 0 fragments mapping against the new BCR/ABL1 transcriptome, since they're from completely different genes. Do you have or know of any existing fixture reads that actually originate from BCR/ABL1, or would it make sense for me to generate a small synthetic FASTQ pair from within gencode.v50.BCR_ABL1.transcripts.fa.gz itself (similar to how I originally built a synthetic transcript, but this time going the other direction — deriving reads from a real transcript)?

I don't think we need to care that Salmon reports 0 mappings for the purposes of this test. Its a good thought, and maybe something we want to incorporate in the future, but IMO the purpose of these tests is to ensure our WDL API is fully working with all the bells and whistles; we don't actually care about the data coming in or out (aside from whether it is well-formed or not)

_Describe the problem or feature in addition to a link to the issues._ This is needed for writing decent tests in #326 BCR and ABL1 were chosen somewhat arbitrarily. We do have other test fixtures referencing these genes specifically (tests are for Arriba), but this doesn't interact with those tests v50 release chosen as its latest. Other fixtures use v31; I could use that release if we want Before submitting this PR, please make sure: - [x] You have added a few sentences describing the PR here. - [x] 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! - [x] You have updated the README or other documentation to account for these changes (when appropriate).

…d FASTQ data, add SE test

…decoys help text

…add decoy-aware indexing test

…size

a-frantz

looking really good 🚂

a-frantz · 2026-09-03T14:02:15Z

+        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 decoy-aware indexing workflow.",
+            group: "Common",
+        }


Both of these should support using a GZIPPED fasta as input

Fixed — both inputs now go through gzip -dcf with a fallback to plain cp if the file isn't actually gzipped, so either format works transparently.

… up intermediate files

PriyankaaXD · 2026-09-04T05:48:30Z

since I iterated on the index fixture several times (custom → v1 → v2 → reverted → BCR/ABL1), my branch's LFS history now carries a few superseded large blobs (~1.3GB total across old versions) even though the current file is tiny. If you squash-merge, this shouldn't carry into main's history — let me know if you'd prefer I try to clean this up

a-frantz · 2026-09-04T11:56:38Z

If you squash-merge, this shouldn't carry into main's history — let me know if you'd prefer I try to clean this up

we'll do a squash-merge 👍 no action needed on your end

a-frantz

Good news: we're at the nitpicking stage of the review 🤣 Left a bunch of comments, but all of them are about very minor things. This looks great 🚀

a-frantz · 2026-09-04T11:58:01Z

@@ -0,0 +1,3 @@
+# Salmon test fixtures
+
+`salmon_index.tar.gz` — built with Salmon 2.6.0 by running the `build_salmon_index` task against the existing `reference/GRCh38.chrY_chrM.fa` fixture (chosen because the shared `fastqs/test_R1.fq.gz`/`fastqs/test_R2.fq.gz` reads were simulated from this reference, per their FASTQ headers).


this needs to reflect the latest build

Done — updated to reflect the current build using gencode.v50.BCR_ABL1.transcripts.fa.gz and Salmon 2.6.0.

a-frantz · 2026-09-04T12:00:01Z

+### Added
+- Added WDL implementation for Salmon (`build_salmon_index` and `quant` tasks) [#326](https://github.com/stjudecloud/workflows/pull/326)


nit: can we bump this up to a new ## 2026 September header? And make sure there are the empty lines on either side of each header

a-frantz · 2026-09-04T12:01:26Z

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


nit:

Suggested change

task build_salmon_index {

task index {

and make sure this name change gets reflected in the tests and changelog etc

Done — renamed throughout, including tests and CHANGELOG.

a-frantz · 2026-09-04T12:03:04Z

+    meta {
+        description: "Builds a Salmon index from a transcriptome FASTA file, for use in quantification"
+        outputs: {
+            salmon_index_tar_gz: "A gzipped TAR file containing the Salmon index files."


Suggested change

salmon_index_tar_gz: "A gzipped TAR file containing the Salmon index files."

index_tar_gz: "A gzipped TAR file containing the Salmon index files."

Done — updated to use the same regex pattern from star.wdl for stripping the FASTQ read-number/extension suffix, matching the convention used elsewhere for handling non-standardized FASTQ naming.

a-frantz · 2026-09-04T12:07:12Z

+            n_cores=$(nproc)
+        fi
+
+        (gzip -dcf "~{transcripts_fasta}" > transcripts.fasta 2>/dev/null) || cp "~{transcripts_fasta}" transcripts.fasta


let's keep this consistent with how we do this operation in other tasks. Example pulled from the STAR file

gtf_name=~{basename(gtf, ".gz")} gunzip -c "~{gtf}" > "$gtf_name" || ln -sf "~{gtf}" "$gtf_name"

the unzip calls are equivalent here, and no real reason to prefer one over the other, but we definitely want to create a symlink instead of copying the whole file in the second case.

Done — switched to this exact pattern, matching STAR's style.

a-frantz · 2026-09-04T12:12:07Z

+        (gzip -dcf "~{transcripts_fasta}" > transcripts.fasta 2>/dev/null) || cp "~{transcripts_fasta}" transcripts.fasta
+        fasta="transcripts.fasta"
+
+        ~{if defined(decoys_fasta) then "(gzip -dcf " + select_first([decoys_fasta]) + " > decoys.fasta 2>/dev/null) || cp " + select_first([decoys_fasta]) + " decoys.fasta" else ""}


this can be rewritten thanks to a quirk of the WDL language. If an expression within a string interpolation evals to None, the whole string is replaced with the empty string. So we can chop off a lot of this wrapping:

Suggested change

~{if defined(decoys_fasta) then "(gzip -dcf " + select_first([decoys_fasta]) + " > decoys.fasta 2>/dev/null) || cp " + select_first([decoys_fasta]) + " decoys.fasta" else ""}

~{"(gzip -dcf " + decoys_fasta + " > decoys.fasta 2>/dev/null) || cp " + decoys_fasta + " decoys.fasta"}

if decoys_fasta == None the whole thing turns into an empty string no-op.

Please update all the following lines to use this less verbose form, and also use symlinks instead of copying

Used this simplified form for the lines that reference decoys_fasta directly (the gunzip/ln -sf line). The grep, cat, and fasta=combined.fasta lines needed to stay as explicit if defined(decoys_fasta) then ... else "", since they only reference the derived $decoys_name bash variable rather than decoys_fasta itself — there's nothing for WDL to collapse on there. Confirmed by testing: without the explicit wrapping, those lines ran unconditionally even when no decoys were supplied, which broke the plain (non-decoy) index build. Also switched to symlinks as requested.

a-frantz · 2026-09-04T12:15:18Z

+
+    runtime {
+        cpu: ncpu
+        memory: "~{ceil(transcripts_fasta_size * 4) + 4 + modify_memory_gb} GB"


I don't think we need the ceil() call here. WDL engines/backends should all handle fractional/float memory requests fine

Tried removing this, but Sprocket rejected the resulting fractional value (4.000064 GB) as an invalid runtime.memory requirement — reverted, keeping ceil()

a-frantz · 2026-09-04T12:16:27Z

+        description: "Runs Salmon quant in mapping-based mode to quantify transcript-level expression from RNA-Seq reads, using a pre-built Salmon index"
+        outputs: {
+            quant_results_tar_gz: "A gzipped TAR file containing the Salmon quantification output directory, including `quant.sf`.",
+            quant_sf: "The raw `quant.sf` transcript quantification file, provided directly in addition to the tarballed output for convenience."


let's clarify here that the quant.sf file gets renamed to <prefix>.quant.sf

a-frantz · 2026-09-04T12:22:03Z

+        Array[File]+ read_one_fastqs_gz
+        Array[File]? read_two_fastqs_gz
+        String lib_type = "A"
+        String prefix = basename(read_one_fastqs_gz[0], ".fastq.gz")


this should be calculated the same way we do for other FASTQs (whose naming conventions in the wild are frustratingly non-standardized 🤣 ). See the README for an explanation, and just copy+paste the RE from another task.

…upport, misc review fixes

PriyankaaXD · 2026-09-09T05:25:08Z

Just a quick note - builds_index_with_decoys fails locally for me on Windows with Unsupported reparse point type when reading back a symlink created inside the container. Manually confirmed the symlink itself is created correctly on the Linux side (via docker run ... ln -sf ...), so this looks like a Windows-host/Docker-volume limitation on my end, not an actual bug — should be fine in your CI, but flagging in case you want to verify.

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 force-pushed the add-salmon-wdl branch from c2d9dde to 4f222d5 Compare August 19, 2026 05:17

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

a-frantz requested changes Aug 23, 2026

View reviewed changes

a-frantz mentioned this pull request Aug 25, 2026

test: transcript sequences fixture #330

Merged

6 tasks

PriyankaaXD added 15 commits August 27, 2026 08:33

Update Salmon container image to a more current version

8a69a0e

Add Sprocket unit tests for Salmon WDL tasks

d89ca54

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

58108ff

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

23aee50

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

61ca77b

…d FASTQ data, add SE test

Require non-empty array for read_one_fastqs_gz

211befd

Expose validateMappings as a Boolean input

7764bc0

Expose decoy-aware indexing as an optional input on build_salmon_index

b58e2f7

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

13b1847

Add CHANGELOG entry for Salmon WDL implementation

882635b

Document how Salmon test fixtures were generated

fd6f872

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

4b73fad

…decoys help text

Use existing repo reference FASTAs instead of synthetic transcripts, …

6d7bc7d

…add decoy-aware indexing test

Update to Salmon v2.6.0, rebuild index fixture with new format

8aa822a

Revert fixture to v1 index pending maintainer decision on v2 fixture …

a5e46e8

…size

PriyankaaXD requested a review from a-frantz August 31, 2026 07:26

a-frantz reviewed Sep 3, 2026

View reviewed changes

Use BCR_ABL1 transcriptome fixture for index tests

6bc4b66

PriyankaaXD force-pushed the add-salmon-wdl branch from 71e9c9a to 6bc4b66 Compare September 4, 2026 04:21

PriyankaaXD added 2 commits September 4, 2026 10:22

Support gzipped FASTA input for transcripts_fasta/decoys_fasta, clean…

59d07bc

… up intermediate files

Output raw quant.sf file in addition to tarballed output

ce0a847

PriyankaaXD requested a review from a-frantz September 4, 2026 05:49

a-frantz requested changes Sep 4, 2026

View reviewed changes

Rename task to index, fix decoy conditional logic, add gzip symlink s…

1d2910e

…upport, misc review fixes

PriyankaaXD requested a review from a-frantz September 9, 2026 05:26

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

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

		`salmon_index.tar.gz` — built with Salmon 2.6.0 by running the `build_salmon_index` task against the existing `reference/GRCh38.chrY_chrM.fa` fixture (chosen because the shared `fastqs/test_R1.fq.gz`/`fastqs/test_R2.fq.gz` reads were simulated from this reference, per their FASTQ headers).

		### Added
		- Added WDL implementation for Salmon (`build_salmon_index` and `quant` tasks) [#326](https://github.com/stjudecloud/workflows/pull/326)

	salmon_index_tar_gz: "A gzipped TAR file containing the Salmon index files."
	index_tar_gz: "A gzipped TAR file containing the Salmon index files."

	~{if defined(decoys_fasta) then "(gzip -dcf " + select_first([decoys_fasta]) + " > decoys.fasta 2>/dev/null) \|\| cp " + select_first([decoys_fasta]) + " decoys.fasta" else ""}
	~{"(gzip -dcf " + decoys_fasta + " > decoys.fasta 2>/dev/null) \|\| cp " + decoys_fasta + " decoys.fasta"}

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adthrasher commented Aug 13, 2026

Uh oh!

a-frantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

PriyankaaXD commented Sep 4, 2026

Uh oh!

a-frantz commented Sep 4, 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!

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!

stjudecloud-cloudy commented Aug 13, 2026 •

edited

Loading