Contamination removal, fastq preprocessing - #20
Conversation
|
@coderabbitai please review this PR |
|
✅ Actions performedReview triggered.
|
|
Warning Review limit reached
More reviews will be available in 26 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe pull request integrates fastp-based read trimming with optional rRNA removal via SortMeRNA, adds MultiQC quality reporting, establishes comprehensive test infrastructure, and standardizes CPU resource allocation to 1 across Snakemake rules. A new CI-aware reads processing pipeline replaces static trimming, and salmon quantification is refined with decoy-aware indexing and transcript-level analysis. ChangesWorkflow Enhancement: QC, Reporting, and rRNA Removal
Comprehensive Testing Framework
Sequence Diagram(s)sequenceDiagram
participant InputFastq as Input Fastq Files
participant TrimRule as Trimming Rule
participant FastpTool as Fastp Tool
participant TrimmedReads as Trimmed Reads & Reports
participant RemoveRRNA as Remove rRNA Rule
participant SortMERNATool as SortMeRNA Tool
participant FinalReads as Final Reads
participant QuantRule as Quantification Rules
participant MultiQCRule as MultiQC Rule
participant QCReport as QC Report Output
InputFastq->>TrimRule: r1, r2 fastq
TrimRule->>FastpTool: execute fastp (fastp container)
FastpTool->>TrimmedReads: trimmed r1/r2 (temp), HTML, JSON
alt execution_mode != CI
TrimmedReads->>RemoveRRNA: trimmed reads
RemoveRRNA->>SortMERNATool: execute sortmerna
SortMERNATool->>FinalReads: rRNA-filtered r1/r2, stats
else CI mode
TrimmedReads->>FinalReads: use trimmed reads as final
end
FinalReads->>QuantRule: proceed to quantification
QuantRule->>MultiQCRule: quantification outputs
TrimmedReads->>MultiQCRule: fastp HTML/JSON
alt SortMeRNA executed
RemoveRRNA->>MultiQCRule: sortmerna logs
end
MultiQCRule->>QCReport: generate multiqc_report.html
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
workflow/rules/bam_cleaning.smk (1)
99-104:⚠️ Potential issue | 🟠 MajorAlign
ncpuswith thread configuration for GATK rules.Both
mark_duplicatesandSplitNCigarReadsexplicitly pass{threads}to GATK's-XX:ParallelGCThreadsparameter (lines 108, 227), butncpusremains fixed at 1. Withconfig["params"]["MarkDuplicates"]["threads"]andconfig["params"]["SplitNCigarReads"]["threads"]both set to 4, the scheduler will under-allocate CPU resources. Updatencpusto match the thread values:Fix ncpus resource allocation
rule mark_duplicates: resources: mem="32G", runtime="240m", - ncpus=1, + ncpus=config["params"]["MarkDuplicates"]["threads"], rule SplitNCigarReads: resources: mem="32G", runtime="720m", - ncpus=1, + ncpus=config["params"]["SplitNCigarReads"]["threads"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/bam_cleaning.smk` around lines 99 - 104, The rule's CPU allocation is fixed at ncpus=1 while GATK is passed {threads}; update the rules (e.g., the mark_duplicates and SplitNCigarReads rules) to set resources ncpus to the matching thread value from the config (use config["params"]["MarkDuplicates"]["threads"] and config["params"]["SplitNCigarReads"]["threads"] or the local threads variable) so the scheduler allocates the same number of CPUs as the GATK -XX:ParallelGCThreads setting.workflow/rules/pMHC.smk (1)
18-31:⚠️ Potential issue | 🔴 CriticalThe
-nparameter is ignored by the launcher script; fixPool()to use thencpusargument.The launcher script parses
-n {params.threads}(which is 4) but createsPool()without passing thencpusargument (line 269 ofnetmhcpan_launcher.py). This causes the pool to spawn processes equal to the system's total CPU count, while Snakemake allocates only 1 CPU. Change line 269 fromwith Pool() as pool:towith Pool(processes=args.ncpus) as pool:to respect the configured thread count and avoid resource contention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/pMHC.smk` around lines 18 - 31, The netmhcpan launcher ignores the -n/threads allocation because it constructs a multiprocessing Pool without passing the configured CPU count; update the pool creation in netmhcpan_launcher.py (the context around with Pool() as pool:) to pass the parsed ncpus argument (use args.ncpus or the variable holding the parsed -n value) so it becomes with Pool(processes=args.ncpus) as pool: ensuring the launcher respects Snakemake's threads parameter and avoids spawning the system CPU count.workflow/rules/alignment.smk (2)
39-43:⚠️ Potential issue | 🟠 Major
ncpus=1contradictsthreads: 12— SLURM will under-allocate CPUs for STAR.The
alignrule requests 12 threads (line 39) but only 1 CPU from the cluster scheduler (line 43). Ifncpusis what gets passed to SLURM's--cpus-per-task, the job will oversubscribe its allocation, leading to contention and possible cluster policy violations.
ncpusshould matchthreads, or be derived from it:Proposed fix
resources: mem="60G", runtime="960m", - ncpus=1, + ncpus=config["params"]["STAR"]["threads"],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/alignment.smk` around lines 39 - 43, In the align rule, ncpus in resources is hard-coded to 1 while threads is set via config["params"]["STAR"]["threads"], causing SLURM CPU under-allocation; update resources.ncpus to derive from the same threads value (e.g., set ncpus = config["params"]["STAR"]["threads"] or reference the rule's threads variable) so that resources.ncpus matches threads for the STAR job.
75-89:⚠️ Potential issue | 🟠 Major
sortAlignlost itsthreads:directive — Snakemake scheduling and SLURM allocation will be wrong.Moving the thread count to
params.threadsmeans Snakemake's--coresaccounting treats this rule as single-threaded, yetsamtools sortwill actually use 4 threads. This causes over-subscription both locally and on the cluster (wherencpus=1).Restore the
threads:directive and keepncpusin sync:Proposed fix
params: threads=config["params"]["samtools"]["threads"] + threads: config["params"]["samtools"]["threads"] resources: mem="10G", runtime="120m", - ncpus=1, + ncpus=config["params"]["samtools"]["threads"], ... shell: """ - samtools sort -@ {params.threads} -o {output} {input} + samtools sort -@ {threads} -o {output} {input} """The same issue applies to
indexSortAligned(lines 110–124).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/alignment.smk` around lines 75 - 89, Restore a proper threads directive for the rules that actually use multithreading: add a top-level threads: X (e.g., threads=config["params"]["samtools"]["threads"]) to the sortAlign rule (and similarly to indexSortAligned) instead of only putting the count into params.threads, and update resources.ncpus to match that threads value so Snakemake/SLURM scheduling aligns with the samtools sort invocation; reference the rule names sortAlign and indexSortAligned, the params.threads symbol, the threads directive, and resources.ncpus when making the change.
🧹 Nitpick comments (2)
workflow/rules/reporting.smk (2)
38-45: Shell command passes all inputs positionally — verify MultiQC handles this correctly.
multiqc {input}will expand to a space-separated list of all input files. MultiQC typically expects directories to scan, but it does accept individual file paths. With many samples, this command line could become very long. If you hit argument length limits, consider passing a file list via--file-list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/reporting.smk` around lines 38 - 45, The shell invocation in the rule uses a positional expansion "multiqc {input}" which can generate a very long command line for many inputs; update the reporting rule to detect large input lists and instead write the input paths to a temporary file and call MultiQC with "--file-list <path>" (preserving {params.outdir}, {params.title} and --force and piping to {log}); ensure the rule that contains the shell stanza (the reporting.smk rule using multiqc {input}) creates the temp file from {input} and passes that file to MultiQC to avoid argument length limits.
24-25: Pin the MultiQC container version for reproducibility.Using
:latestmeans different pipeline runs may silently use different MultiQC versions, which can change report format or supported modules. Pin to a specific version (e.g.,v1.33, the current stable release).Proposed fix
container: - "docker://ewels/multiqc:latest" + "docker://ewels/multiqc:v1.33"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/rules/reporting.smk` around lines 24 - 25, The MultiQC container reference in the rule (the container string "docker://ewels/multiqc:latest") should be pinned to a specific version for reproducibility; update that value to a fixed tag (for example "docker://ewels/multiqc:v1.33" or whichever tested stable release you use) in the reporting rule so runs don't depend on the floating :latest image.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup/download_res.py`:
- Around line 232-255: The download_sortmerna_db helper currently ignores
dry-run and returns dest_file even if extraction failed; update its signature to
accept a dry_run flag (e.g., add parameter dry_run or pass args.dry_run into
download_sortmerna_db), branch early to skip wget/tar when dry_run is True, and
before returning ensure the expected file (dest_file) actually exists on disk—if
extraction failed, raise or return None/raise an exception so callers don’t
update config; also apply the same dry-run propagation/validation to the other
call site mentioned (lines ~314-315) that invokes this helper.
- Around line 248-250: The lint error F541 is caused by an unnecessary f-string
in the run_command call; update the arguments passed to run_command (the call
that includes ["tar", "-xzf", tar_path, "-C", outfolder, f"--wildcards",
f"*/{keep_file}", "--strip-components=1"]) by replacing f"--wildcards" with a
plain string "--wildcards" while keeping f"*/{keep_file}" unchanged; this change
should be made where the run_command invocation is defined/used in
download_res.py.
In `@workflow/rules/deepvariant.smk`:
- Around line 47-50: The rules DeepVariant and SelectDeepVariantCalls request 1
CPU in resources but actually use 4 threads from config; update the resources
block in each rule to set ncpus to the configured thread count (e.g., ncpus=4)
so ncpus matches threads=config["params"]["deepvariant"]["threads"] for
DeepVariant and threads=config["params"]["samtools"]["threads"] for
SelectDeepVariantCalls, ensuring the scheduler allocation aligns with
--num_shards and bcftools --threads usage.
In `@workflow/rules/HLA_typing.smk`:
- Around line 62-66: The genotype rule declares threads:
config["params"]["t1k"]["threads"] and passes -t {threads} to run-t1k but
resources sets ncpus=1; update the resources block in the genotype rule so ncpus
matches the thread allocation (e.g., set ncpus to
config["params"]["t1k"]["threads"] or to the explicit value 8), or alternatively
cap both threads and ncpus to 1 if the tool must be single-threaded; ensure you
modify the resources:ncpus entry (referencing the genotype rule, resources,
ncpus, threads and the run-t1k call) accordingly.
In `@workflow/rules/index.smk`:
- Around line 17-19: The CPU allocation is mismatched: update the resource
declarations in the star_index and salmon_idx rules so ncpus matches the
configured threads (replace ncpus=1 with
ncpus=config["params"]["STAR"]["threads"] for the star_index rule and
ncpus=config["params"]["salmon"]["threads"] for the salmon_idx rule), ensuring
the rules still set threads: config["params"]["STAR"]["threads"] and threads:
config["params"]["salmon"]["threads"] respectively so the scheduler allocates
one CPU per thread used by the tools.
In `@workflow/rules/quantification.smk`:
- Around line 33-35: The resource allocation in the Salmon quantification rule
is incorrect: the rule sets ncpus=1 while Salmon is run with -p {threads} and
config specifies threads: 8; update the rule's ncpus to match the configured
thread count (e.g., set ncpus=8) so that the ncpus parameter aligns with threads
used by Salmon (refer to the runtime, ncpus, mem entries and the Salmon command
that uses -p {threads}).
In `@workflow/rules/reads_trimming.smk`:
- Around line 51-54: The trimming rule's log path uses
config["datadirs"]["logs"]["trimming"] directly, missing the common output
prefix; update the log path construction in the trimming rule so it prefixes the
path with config["OUTPUT_FOLDER"] (i.e., join config["OUTPUT_FOLDER"] with
config["datadirs"]["logs"]["trimming"] and the "{patient}.log" filename) to
match other rules like align/remove_rrna.
- Around line 107-111: The rule remove_rrna has a threads vs resources.ncpus
mismatch: threads is set (via threads or
config["params"]["sortmerna"]["threads"]) but resources.ncpus remains 1; update
resources.ncpus to match the threads value (e.g., set
ncpus=config["params"]["sortmerna"]["threads"] or the literal 8) so the
resources block aligns with the threads declaration in the remove_rrna rule.
- Around line 40-62: The trimming rule currently sets params.threads but lacks a
Snakemake threads: directive and hardcodes resources.ncpus=1, causing CPU
oversubscription when fastp is invoked; add a threads: params.threads line to
the trimming rule, update the fastp -w argument to use the Snakemake {threads}
placeholder (instead of {params.threads}) and set resources.ncpus to the same
value as params.threads so the scheduler allocates the correct number of CPUs
(reference symbols: trimming rule, params.threads, resources.ncpus, and the
fastp invocation).
- Around line 122-138: The remove_rrna rule currently writes sortmerna output to
{output.stats} while declaring a separate log directive and assumes fixed
filenames "_fwd.fq.gz"/"_rev.fq.gz"; update the shell block to start with "set
-euo pipefail", redirect sortmerna stdout/stderr via tee to the declared {log}
(or remove the log directive), and replace the hardcoded mv lines that reference
{params.out_prefix}_fwd.fq.gz / _rev.fq.gz with a robust detection that checks
for actual sortmerna-produced names (e.g., glob or test for
{params.out_prefix}_*fwd* and {params.out_prefix}_*rev* including both .fq.gz
and .fastq.gz variants) and only mv when the detected files exist so outputs
match Snakemake's {output.r1}/{output.r2}.
In `@workflow/rules/reporting.smk`:
- Around line 32-37: The multiqc log path currently uses
config["datadirs"]["logs"]["trimming"] which places the log in the trimming logs
directory; change the path so "multiqc.log" is written alongside QC/reporting
outputs instead (e.g. replace config["datadirs"]["logs"]["trimming"] with
config["datadirs"]["qc_reports"] or a dedicated reporting log dir such as
config["datadirs"]["logs"]["reporting"]), keeping the same os.path.join pattern
with config["OUTPUT_FOLDER"] and the filename "multiqc.log".
In `@workflow/rules/strelka.smk`:
- Around line 96-98: Strelka2 and SelectStrelka2Calls request only ncpus=1 while
launching multi-threaded jobs; update each rule to request ncpus equal to the
thread count used by the tool (use ncpus=params.threads for Strelka2 which
passes {params.threads}, and ncpus=threads for SelectStrelka2Calls which uses
{threads}) so the scheduler allocates sufficient CPUs and avoids contention.
---
Outside diff comments:
In `@workflow/rules/alignment.smk`:
- Around line 39-43: In the align rule, ncpus in resources is hard-coded to 1
while threads is set via config["params"]["STAR"]["threads"], causing SLURM CPU
under-allocation; update resources.ncpus to derive from the same threads value
(e.g., set ncpus = config["params"]["STAR"]["threads"] or reference the rule's
threads variable) so that resources.ncpus matches threads for the STAR job.
- Around line 75-89: Restore a proper threads directive for the rules that
actually use multithreading: add a top-level threads: X (e.g.,
threads=config["params"]["samtools"]["threads"]) to the sortAlign rule (and
similarly to indexSortAligned) instead of only putting the count into
params.threads, and update resources.ncpus to match that threads value so
Snakemake/SLURM scheduling aligns with the samtools sort invocation; reference
the rule names sortAlign and indexSortAligned, the params.threads symbol, the
threads directive, and resources.ncpus when making the change.
In `@workflow/rules/bam_cleaning.smk`:
- Around line 99-104: The rule's CPU allocation is fixed at ncpus=1 while GATK
is passed {threads}; update the rules (e.g., the mark_duplicates and
SplitNCigarReads rules) to set resources ncpus to the matching thread value from
the config (use config["params"]["MarkDuplicates"]["threads"] and
config["params"]["SplitNCigarReads"]["threads"] or the local threads variable)
so the scheduler allocates the same number of CPUs as the GATK
-XX:ParallelGCThreads setting.
In `@workflow/rules/pMHC.smk`:
- Around line 18-31: The netmhcpan launcher ignores the -n/threads allocation
because it constructs a multiprocessing Pool without passing the configured CPU
count; update the pool creation in netmhcpan_launcher.py (the context around
with Pool() as pool:) to pass the parsed ncpus argument (use args.ncpus or the
variable holding the parsed -n value) so it becomes with
Pool(processes=args.ncpus) as pool: ensuring the launcher respects Snakemake's
threads parameter and avoids spawning the system CPU count.
---
Nitpick comments:
In `@workflow/rules/reporting.smk`:
- Around line 38-45: The shell invocation in the rule uses a positional
expansion "multiqc {input}" which can generate a very long command line for many
inputs; update the reporting rule to detect large input lists and instead write
the input paths to a temporary file and call MultiQC with "--file-list <path>"
(preserving {params.outdir}, {params.title} and --force and piping to {log});
ensure the rule that contains the shell stanza (the reporting.smk rule using
multiqc {input}) creates the temp file from {input} and passes that file to
MultiQC to avoid argument length limits.
- Around line 24-25: The MultiQC container reference in the rule (the container
string "docker://ewels/multiqc:latest") should be pinned to a specific version
for reproducibility; update that value to a fixed tag (for example
"docker://ewels/multiqc:v1.33" or whichever tested stable release you use) in
the reporting rule so runs don't depend on the floating :latest image.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.test/common.smkDockerfileconfig/config_main.yamlsetup/download_res.pysetup/resources.jsonworkflow/Snakefileworkflow/rules/HLA_typing.smkworkflow/rules/alignment.smkworkflow/rules/annotate_variants.smkworkflow/rules/bam_cleaning.smkworkflow/rules/base_recalibration.smkworkflow/rules/common.smkworkflow/rules/deepvariant.smkworkflow/rules/filter_calls.smkworkflow/rules/index.smkworkflow/rules/pMHC.smkworkflow/rules/quantification.smkworkflow/rules/reads_trimming.smkworkflow/rules/reporting.smkworkflow/rules/strelka.smk
| def download_sortmerna_db(url, keep_file, outfolder): | ||
| """ | ||
| Download SortMeRNA database, extract only the needed file, and cleanup. | ||
| """ | ||
| dest_file = os.path.join(outfolder, keep_file) | ||
| if os.path.isfile(dest_file): | ||
| logging.info(f"{keep_file} already exists. Skipping.") | ||
| return dest_file | ||
|
|
||
| tar_filename = url.split("/")[-1] | ||
| tar_path = os.path.join(outfolder, tar_filename) | ||
|
|
||
| logging.info(f"Downloading SortMeRNA database from {url}") | ||
| if not os.path.isfile(tar_path): | ||
| run_command(["wget", "-c", url, "-P", outfolder]) | ||
|
|
||
| logging.info(f"Extracting {keep_file} from archive") | ||
| run_command(["tar", "-xzf", tar_path, "-C", outfolder, f"--wildcards", f"*/{keep_file}", "--strip-components=1"]) | ||
|
|
||
| logging.info("Cleaning up archive") | ||
| if os.path.isfile(tar_path): | ||
| os.remove(tar_path) | ||
|
|
||
| return dest_file |
There was a problem hiding this comment.
Honor --dry-run and validate extraction output.
The new SortMeRNA downloader ignores dry-run and may update config even if extraction didn’t actually produce keep_file. Please pass args.dry_run into the helper, skip download/extract when dry, and verify the extracted file exists before returning.
🛠️ Suggested fix
-def download_sortmerna_db(url, keep_file, outfolder):
+def download_sortmerna_db(url, keep_file, outfolder, dry=False):
@@
- logging.info(f"Downloading SortMeRNA database from {url}")
- if not os.path.isfile(tar_path):
- run_command(["wget", "-c", url, "-P", outfolder])
+ logging.info(f"Downloading SortMeRNA database from {url}")
+ if dry:
+ logging.info("Dry-run enabled; skipping download/extraction.")
+ return dest_file
+ if not os.path.isfile(tar_path):
+ run_command(["wget", "-c", url, "-P", outfolder])
@@
- run_command(["tar", "-xzf", tar_path, "-C", outfolder, f"--wildcards", f"*/{keep_file}", "--strip-components=1"])
+ run_command(["tar", "-xzf", tar_path, "-C", outfolder, "--wildcards", f"*/{keep_file}", "--strip-components=1"])
+ if not os.path.isfile(dest_file):
+ raise FileNotFoundError(f"Expected {dest_file} after extraction")
@@
- elif ftype == "sortmerna":
- path = download_sortmerna_db(res_entry['url'], res_entry['keep_file'], outfolder)
+ elif ftype == "sortmerna":
+ path = download_sortmerna_db(res_entry['url'], res_entry['keep_file'], outfolder, args.dry_run)Also applies to: 314-315
🧰 Tools
🪛 Ruff (0.15.2)
[error] 249-249: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setup/download_res.py` around lines 232 - 255, The download_sortmerna_db
helper currently ignores dry-run and returns dest_file even if extraction
failed; update its signature to accept a dry_run flag (e.g., add parameter
dry_run or pass args.dry_run into download_sortmerna_db), branch early to skip
wget/tar when dry_run is True, and before returning ensure the expected file
(dest_file) actually exists on disk—if extraction failed, raise or return
None/raise an exception so callers don’t update config; also apply the same
dry-run propagation/validation to the other call site mentioned (lines ~314-315)
that invokes this helper.
| logging.info(f"Extracting {keep_file} from archive") | ||
| run_command(["tar", "-xzf", tar_path, "-C", outfolder, f"--wildcards", f"*/{keep_file}", "--strip-components=1"]) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the offending f-string
rg -n 'f"--wildcards"' setup/download_res.pyRepository: ctglab/ENEO
Length of output: 176
🏁 Script executed:
#!/bin/bash
# Let's also check the actual line to see the full context
if [ -f setup/download_res.py ]; then
sed -n '248,250p' setup/download_res.py
fiRepository: ctglab/ENEO
Length of output: 230
Remove the unnecessary f-string to satisfy Ruff F541.
f"--wildcards" on line 249 has no placeholders and triggers a lint error. The adjacent f"*/{keep_file}" should remain since it contains a placeholder.
🧹 Minimal fix
- run_command(["tar", "-xzf", tar_path, "-C", outfolder, f"--wildcards", f"*/{keep_file}", "--strip-components=1"])
+ run_command(["tar", "-xzf", tar_path, "-C", outfolder, "--wildcards", f"*/{keep_file}", "--strip-components=1"])🧰 Tools
🪛 Ruff (0.15.2)
[error] 249-249: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setup/download_res.py` around lines 248 - 250, The lint error F541 is caused
by an unnecessary f-string in the run_command call; update the arguments passed
to run_command (the call that includes ["tar", "-xzf", tar_path, "-C",
outfolder, f"--wildcards", f"*/{keep_file}", "--strip-components=1"]) by
replacing f"--wildcards" with a plain string "--wildcards" while keeping
f"*/{keep_file}" unchanged; this change should be made where the run_command
invocation is defined/used in download_res.py.
| resources: | ||
| runtime="480m", | ||
| ncpus=config["params"]["deepvariant"]["threads"], | ||
| ncpus=1, | ||
| mem="16G", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n workflow/rules/deepvariant.smk | head -60Repository: ctglab/ENEO
Length of output: 2313
🏁 Script executed:
cat -n workflow/rules/deepvariant.smk | sed -n '95,110p'Repository: ctglab/ENEO
Length of output: 762
🏁 Script executed:
find . -name "config*.yaml" -o -name "config*.yml" | head -20Repository: ctglab/ENEO
Length of output: 242
🏁 Script executed:
cat config/config_main.yaml | grep -A 5 "deepvariant\|samtools"Repository: ctglab/ENEO
Length of output: 729
🏁 Script executed:
# Also check test config
cat .test/config/config.yaml | grep -A 5 "deepvariant\|samtools"Repository: ctglab/ENEO
Length of output: 589
Align ncpus with thread usage in these rules.
Both rules are configured to use 4 threads but request only 1 CPU core:
- DeepVariant (line 37):
threads=config["params"]["deepvariant"]["threads"](set to 4) → passes to--num_shards={params.threads}but resources specifyncpus=1 - SelectDeepVariantCalls (line 95):
threads=config["params"]["samtools"]["threads"](set to 4) → passes tobcftools ... --threads {threads}but resources specifyncpus=1
This under-allocation can cause job failures or scheduler violations. Set ncpus to match the configured thread counts:
resources:
runtime="480m",
- ncpus=1,
+ ncpus=config["params"]["deepvariant"]["threads"],
mem="16G",
@@
resources:
runtime="20m",
- ncpus=1,
+ ncpus=config["params"]["samtools"]["threads"],
mem="8G",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/deepvariant.smk` around lines 47 - 50, The rules DeepVariant
and SelectDeepVariantCalls request 1 CPU in resources but actually use 4 threads
from config; update the resources block in each rule to set ncpus to the
configured thread count (e.g., ncpus=4) so ncpus matches
threads=config["params"]["deepvariant"]["threads"] for DeepVariant and
threads=config["params"]["samtools"]["threads"] for SelectDeepVariantCalls,
ensuring the scheduler allocation aligns with --num_shards and bcftools
--threads usage.
| threads: config["params"]["t1k"]["threads"] | ||
| resources: | ||
| runtime="240m", | ||
| ncpus=4, | ||
| ncpus=1, | ||
| mem="32G", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "config_main.yaml" -o -name "HLA_typing.smk" | head -20Repository: ctglab/ENEO
Length of output: 112
🏁 Script executed:
rg -n "t1k" --type yamlRepository: ctglab/ENEO
Length of output: 389
🏁 Script executed:
# Locate and read the HLA_typing.smk file
find . -name "HLA_typing.smk" -exec wc -l {} +Repository: ctglab/ENEO
Length of output: 90
🏁 Script executed:
# Extract t1k configuration from config_main.yaml
rg -A 10 "t1k:" config/config_main.yamlRepository: ctglab/ENEO
Length of output: 619
🏁 Script executed:
# Read the HLA_typing.smk file to see context around lines 62-66
cat -n workflow/rules/HLA_typing.smk | sed -n '50,80p'Repository: ctglab/ENEO
Length of output: 1117
🏁 Script executed:
# Check the extract_hla rule to see how it handles resources
cat -n workflow/rules/HLA_typing.smk | sed -n '79,108p'Repository: ctglab/ENEO
Length of output: 1082
Align ncpus with thread allocation in the genotype rule.
The genotype rule declares threads: 8 (from config) and uses -t {threads} in the run-t1k command, but ncpus is hardcoded to 1. This will cause resource contention: the tool will attempt to spawn 8 threads with only 1 CPU allocated. Change ncpus=8 to match the thread count, or if threads should be capped, set both to 1.
The extract_hla rule is correctly configured with ncpus=1 for its single-threaded Python execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/HLA_typing.smk` around lines 62 - 66, The genotype rule
declares threads: config["params"]["t1k"]["threads"] and passes -t {threads} to
run-t1k but resources sets ncpus=1; update the resources block in the genotype
rule so ncpus matches the thread allocation (e.g., set ncpus to
config["params"]["t1k"]["threads"] or to the explicit value 8), or alternatively
cap both threads and ncpus to 1 if the tool must be single-threaded; ensure you
modify the resources:ncpus entry (referencing the genotype rule, resources,
ncpus, threads and the run-t1k call) accordingly.
| mem="60G", | ||
| ncpus=8, | ||
| ncpus=1, | ||
| runtime="360m", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the workflow file to see context around lines 17-19 and 57-59
cat -n workflow/rules/index.smk | head -70Repository: ctglab/ENEO
Length of output: 2572
🏁 Script executed:
#!/bin/bash
# Search for configuration files that define STAR and salmon thread counts
fd -e yaml -e yml -e json | head -20Repository: ctglab/ENEO
Length of output: 542
🏁 Script executed:
#!/bin/bash
# Look for config parameter definitions
find . -type f \( -name "config*" -o -name "params*" -o -name "*.yaml" -o -name "*.yml" \) | grep -v ".git" | head -20Repository: ctglab/ENEO
Length of output: 571
🏁 Script executed:
#!/bin/bash
# Read the main config file to see STAR and salmon thread settings
cat config/config_main.yamlRepository: ctglab/ENEO
Length of output: 3761
🏁 Script executed:
#!/bin/bash
# Also check the test config
cat .test/config/config.yamlRepository: ctglab/ENEO
Length of output: 3623
Align ncpus with configured threads to avoid CPU under-allocation.
The star_index rule has threads: config["params"]["STAR"]["threads"] (12 threads per config) but ncpus=1, and the salmon_idx rule has threads: config["params"]["salmon"]["threads"] (8 threads per config) but ncpus=1. Since STAR explicitly uses --runThreadN {threads}, it will spawn 12 threads within a single allocated CPU, causing contention and degraded performance. Align ncpus with the thread configuration:
Suggested fix
resources:
mem="60G",
- ncpus=1,
+ ncpus=config["params"]["STAR"]["threads"],
runtime="360m", resources:
mem="40G",
- ncpus=1,
+ ncpus=config["params"]["salmon"]["threads"],
runtime="240m",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mem="60G", | |
| ncpus=8, | |
| ncpus=1, | |
| runtime="360m", | |
| mem="60G", | |
| ncpus=config["params"]["STAR"]["threads"], | |
| runtime="360m", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/index.smk` around lines 17 - 19, The CPU allocation is
mismatched: update the resource declarations in the star_index and salmon_idx
rules so ncpus matches the configured threads (replace ncpus=1 with
ncpus=config["params"]["STAR"]["threads"] for the star_index rule and
ncpus=config["params"]["salmon"]["threads"] for the salmon_idx rule), ensuring
the rules still set threads: config["params"]["STAR"]["threads"] and threads:
config["params"]["salmon"]["threads"] respectively so the scheduler allocates
one CPU per thread used by the tools.
| log: | ||
| os.path.join(config["datadirs"]["logs"]["trimming"], "{patient}.log"), | ||
| wrapper: | ||
| "v1.0.0/bio/fastp" | ||
| os.path.join( | ||
| config["datadirs"]["logs"]["trimming"], | ||
| "{patient}.log"), |
There was a problem hiding this comment.
Log path is missing config["OUTPUT_FOLDER"] prefix.
Unlike every other rule in this PR (e.g., align, remove_rrna), the trimming log path doesn't include the output folder prefix. This will write the log relative to the working directory instead of the designated output tree.
Proposed fix
log:
os.path.join(
+ config["OUTPUT_FOLDER"],
config["datadirs"]["logs"]["trimming"],
"{patient}.log"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log: | |
| os.path.join(config["datadirs"]["logs"]["trimming"], "{patient}.log"), | |
| wrapper: | |
| "v1.0.0/bio/fastp" | |
| os.path.join( | |
| config["datadirs"]["logs"]["trimming"], | |
| "{patient}.log"), | |
| log: | |
| os.path.join( | |
| config["OUTPUT_FOLDER"], | |
| config["datadirs"]["logs"]["trimming"], | |
| "{patient}.log"), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/reads_trimming.smk` around lines 51 - 54, The trimming rule's
log path uses config["datadirs"]["logs"]["trimming"] directly, missing the
common output prefix; update the log path construction in the trimming rule so
it prefixes the path with config["OUTPUT_FOLDER"] (i.e., join
config["OUTPUT_FOLDER"] with config["datadirs"]["logs"]["trimming"] and the
"{patient}.log" filename) to match other rules like align/remove_rrna.
| threads: config["params"]["sortmerna"]["threads"] | ||
| resources: | ||
| mem="32G", | ||
| runtime="240m", | ||
| ncpus=1, |
There was a problem hiding this comment.
remove_rrna: same ncpus vs threads mismatch.
threads: 8 but ncpus=1. Align ncpus with threads.
Proposed fix
threads: config["params"]["sortmerna"]["threads"]
resources:
mem="32G",
runtime="240m",
- ncpus=1,
+ ncpus=config["params"]["sortmerna"]["threads"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| threads: config["params"]["sortmerna"]["threads"] | |
| resources: | |
| mem="32G", | |
| runtime="240m", | |
| ncpus=1, | |
| threads: config["params"]["sortmerna"]["threads"] | |
| resources: | |
| mem="32G", | |
| runtime="240m", | |
| ncpus=config["params"]["sortmerna"]["threads"], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/reads_trimming.smk` around lines 107 - 111, The rule
remove_rrna has a threads vs resources.ncpus mismatch: threads is set (via
threads or config["params"]["sortmerna"]["threads"]) but resources.ncpus remains
1; update resources.ncpus to match the threads value (e.g., set
ncpus=config["params"]["sortmerna"]["threads"] or the literal 8) so the
resources block aligns with the threads declaration in the remove_rrna rule.
| shell: | ||
| """ | ||
| sortmerna \ | ||
| --ref {input.rrna_db} \ | ||
| --reads {input.r1} \ | ||
| --reads {input.r2} \ | ||
| --workdir {params.workdir} \ | ||
| --aligned {params.workdir}/rrna \ | ||
| --other {params.out_prefix} \ | ||
| --paired_in \ | ||
| --fastx \ | ||
| --threads {threads} \ | ||
| --out2 2>&1 | tee {output.stats} | ||
| mv {params.out_prefix}_fwd.fq.gz {output.r1} | ||
| mv {params.out_prefix}_rev.fq.gz {output.r2} | ||
| rm -rf {params.workdir} | ||
| """ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n workflow/rules/reads_trimming.smk | sed -n '110,145p'Repository: ctglab/ENEO
Length of output: 1238
🌐 Web query:
sortmerna output file naming convention --out2 extension
💡 Result:
In SortMeRNA, --out2 is not an output file extension or a special “.out2” file type—it’s a boolean flag that changes how paired-end reads are written when you also use --fastx. (hpc.nih.gov)
What --out2 does
- With paired-end input (
--reads R1 --reads R2),--out2makes SortMeRNA write forward and reverse reads to separate files (instead of one combined/interleaved file). (hpc.nih.gov)
Output filenames
The base output prefix still comes from --aligned [dir/][pfx] and/or --other [dir/][pfx], and SortMeRNA automatically appends the correct format extension (.fastq, .fasta, and possibly .gz depending on input / --zip-out). (hpc.nih.gov)
When --out2 is used together with --sout, SortMeRNA explicitly documents these aligned output name stems (then the usual .fastq/.fasta[.gz] is added):
aligned-paired-fwdaligned-paired-revaligned-singleton-fwdaligned-singleton-rev(hpc.nih.gov)
And if --other is also enabled, you’ll get the analogous additional files (8 total). (hpc.nih.gov)
Citations:
- 1: https://hpc.nih.gov/apps/sortmerna.html?utm_source=openai
- 2: https://hpc.nih.gov/apps/sortmerna.html?utm_source=openai
- 3: https://hpc.nih.gov/apps/sortmerna.html?utm_source=openai
- 4: https://hpc.nih.gov/apps/sortmerna.html?utm_source=openai
- 5: https://hpc.nih.gov/apps/sortmerna.html?utm_source=openai
🌐 Web query:
sortmerna --other output filename naming --paired
💡 Result:
With SortMeRNA v4.x, --other (and --aligned) take a directory/prefix ([dir/][pfx]), and SortMeRNA adds the proper extension automatically (fasta/fastq, plus .gz depending on input / --zip-out). [dir] and [pfx] are optional; if omitted, outputs go to WORKDIR/out/ and use the default prefixes other / aligned. [1]
For paired-end input (two --reads files), SortMeRNA writes two FASTX files per prefix, typically by appending:
*_fwd.*and*_rev.*for forward/reverse mates (e.g.,--other other_reads→other_reads_fwd.fastq+other_reads_rev.fastq). [2]
If you additionally use --out2 together with --sout, SortMeRNA further splits into paired vs singleton, producing 4 aligned files (and 8 total if --other is also enabled): aligned-paired-fwd, aligned-paired-rev, aligned-singleton-fwd, aligned-singleton-rev (and the analogous other-* set). [1]
Sources:
[1] SortMeRNA 4.3.6 help text (NIH HPC page) describing --other/--aligned prefix rules and --out2/--sout file sets.
[2] Snakemake wrapper demonstrating paired-output filenames as *_fwd.* / *_rev.* for --aligned and --other.
🏁 Script executed:
# Search for any sortmerna documentation or examples in the repo
rg -i "sortmerna" --type md --type txt -A 3 -B 1Repository: ctglab/ENEO
Length of output: 37
🌐 Web query:
sortmerna fastq input output extension .fq .fastq naming
💡 Result:
-
Input FASTQ naming/extension: SortMeRNA does not require specific extensions like
.fqvs.fastq(or gzipped variants). For--reads, the file extension is optional and the program auto-detects whether it’s FASTA/FASTQ and compressed/uncompressed (e.g.,.fastq,.fq,.fastq.gz,.fq.gz, or even no extension). [1][2] -
Output naming/extension: For outputs like
--alignedand--other, you provide a path/prefix (optionally including directories). SortMeRNA then automatically appends the appropriate extension based on the output type/format (e.g.,fasta,fastq,sam,blast, etc.). If you omit arguments, defaults are used (e.g.,WORKDIR/out/aligned.fasta). Examples shown in the docs include-aligned some/dir/prefixproducingsome/dir/prefix.fasta. [2]
Sources:
[1] SortMeRNA ReadTheDocs user guide (stable)
[2] NIH HPC SortMeRNA help/docs page
🏁 Script executed:
# Check if there are any usage examples or test files in the repo that show sortmerna output
fd -t f "sortmerna" | head -20Repository: ctglab/ENEO
Length of output: 37
Use {log} and verify output filenames to handle sortmerna's automatic extension appending.
The remove_rrna rule has two issues:
-
Unused log directive: The rule declares
log:(lines 116–121) but the shell block writes to{output.stats}instead. Either redirectteeto{log}or remove the declaration. -
Fragile output filename assumptions: Lines 135–136 hardcode
_fwd.fq.gz/_rev.fq.gz, but sortmerna automatically appends extensions based on the input format. With.fastq.gzinputs, sortmerna produces_fwd.fastq.gz(not.fq.gz), causing themvcommands to fail silently sinceset -eis not enabled.
Fix: Add set -euo pipefail at the top of the shell block, use {log}, and verify (or glob) the actual output filenames that sortmerna produces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/reads_trimming.smk` around lines 122 - 138, The remove_rrna
rule currently writes sortmerna output to {output.stats} while declaring a
separate log directive and assumes fixed filenames "_fwd.fq.gz"/"_rev.fq.gz";
update the shell block to start with "set -euo pipefail", redirect sortmerna
stdout/stderr via tee to the declared {log} (or remove the log directive), and
replace the hardcoded mv lines that reference {params.out_prefix}_fwd.fq.gz /
_rev.fq.gz with a robust detection that checks for actual sortmerna-produced
names (e.g., glob or test for {params.out_prefix}_*fwd* and
{params.out_prefix}_*rev* including both .fq.gz and .fastq.gz variants) and only
mv when the detected files exist so outputs match Snakemake's
{output.r1}/{output.r2}.
| log: | ||
| os.path.join( | ||
| config["OUTPUT_FOLDER"], | ||
| config["datadirs"]["logs"]["trimming"], | ||
| "multiqc.log" | ||
| ), |
There was a problem hiding this comment.
Log path is under logs.trimming — should reflect reporting/QC.
The multiqc log is written to the trimming log directory, which is misleading. Consider adding a dedicated log directory for reporting, or at minimum placing this log alongside the QC reports output.
Proposed fix (using the qc_reports output dir)
log:
os.path.join(
config["OUTPUT_FOLDER"],
- config["datadirs"]["logs"]["trimming"],
+ config["datadirs"]["qc_reports"],
"multiqc.log"
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log: | |
| os.path.join( | |
| config["OUTPUT_FOLDER"], | |
| config["datadirs"]["logs"]["trimming"], | |
| "multiqc.log" | |
| ), | |
| log: | |
| os.path.join( | |
| config["OUTPUT_FOLDER"], | |
| config["datadirs"]["qc_reports"], | |
| "multiqc.log" | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/reporting.smk` around lines 32 - 37, The multiqc log path
currently uses config["datadirs"]["logs"]["trimming"] which places the log in
the trimming logs directory; change the path so "multiqc.log" is written
alongside QC/reporting outputs instead (e.g. replace
config["datadirs"]["logs"]["trimming"] with config["datadirs"]["qc_reports"] or
a dedicated reporting log dir such as config["datadirs"]["logs"]["reporting"]),
keeping the same os.path.join pattern with config["OUTPUT_FOLDER"] and the
filename "multiqc.log".
| runtime="240m", | ||
| ncpus=2, | ||
| ncpus=1, | ||
| mem="16G", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the strelka.smk file at the specific lines
head -150 workflow/rules/strelka.smk | tail -60 | cat -nRepository: ctglab/ENEO
Length of output: 2265
🏁 Script executed:
#!/bin/bash
# Get the exact content around lines 96-98 and 142-144
sed -n '90,100p' workflow/rules/strelka.smk | cat -nRepository: ctglab/ENEO
Length of output: 391
🏁 Script executed:
#!/bin/bash
# Check lines around 142-144
sed -n '138,148p' workflow/rules/strelka.smk | cat -nRepository: ctglab/ENEO
Length of output: 495
🏁 Script executed:
#!/bin/bash
# Search for thread configuration in the codebase
fd -t f '\.(yaml|yml|json|py)$' | xargs rg -l "strelka2.*thread|samtools.*thread" | head -20Repository: ctglab/ENEO
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Look for config files with thread parameters
find . -type f \( -name "*.yaml" -o -name "*.yml" -o -name "*.json" \) | xargs rg "threads|ncpus" -B 2 -A 2 | head -100Repository: ctglab/ENEO
Length of output: 4851
🏁 Script executed:
#!/bin/bash
# Search for the complete Strelka2 rule definition including params
sed -n '80,110p' workflow/rules/strelka.smk | cat -nRepository: ctglab/ENEO
Length of output: 1063
🏁 Script executed:
#!/bin/bash
# Check if there's a params section in the Strelka2 rule
rg -n "rule.*Strelka2|params.*threads" workflow/rules/strelka.smk -A 20 -B 2 | head -80Repository: ctglab/ENEO
Length of output: 2436
🏁 Script executed:
#!/bin/bash
# Look for where params.threads might be set for strelka2
rg "params\.threads" workflow/rules/strelka.smk -B 5 -A 1Repository: ctglab/ENEO
Length of output: 228
Align ncpus with thread counts to prevent scheduler resource under-allocation.
Both rules spawn multi-threaded processes but request only ncpus=1 from the scheduler, causing CPU contention:
- Strelka2 (line 97): passes
{params.threads}(configured as 8 threads) but allocates only 1 CPU - SelectStrelka2Calls (line 143): uses
--threads {threads}(configured as 4 threads) but allocates only 1 CPU
Suggested adjustment
resources:
runtime="240m",
- ncpus=1,
+ ncpus=config["params"]["strelka2"]["threads"],
mem="16G",
@@
resources:
runtime="20m",
- ncpus=1,
+ ncpus=config["params"]["samtools"]["threads"],
mem="8G",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workflow/rules/strelka.smk` around lines 96 - 98, Strelka2 and
SelectStrelka2Calls request only ncpus=1 while launching multi-threaded jobs;
update each rule to request ncpus equal to the thread count used by the tool
(use ncpus=params.threads for Strelka2 which passes {params.threads}, and
ncpus=threads for SelectStrelka2Calls which uses {threads}) so the scheduler
allocates sufficient CPUs and avoids contention.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workflow/rules/deepvariant.smk (2)
53-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCleanup should run on failure too (use shell trap).
rmruns only on the success path. Ifrun_deepvariantfails, temp files/directories are left behind and can accumulate across retries.Suggested fix
shell: """ mkdir -p {params.tmp_dir} + cleanup() {{ + rm -f {params.uncompressed_regions} + rm -rf {params.tmp_dir} + }} + trap cleanup EXIT + if [[ "{input.regions}" == *.gz ]]; then zcat {input.regions} > {params.uncompressed_regions} else cat {input.regions} > {params.uncompressed_regions} fi @@ - rm -f {params.uncompressed_regions} - rm -rf {params.tmp_dir} """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/rules/deepvariant.smk` around lines 53 - 67, The cleanup commands (rm -f and rm -rf) at the end of the deepvariant rule's shell block only execute if run_deepvariant succeeds, causing temporary files to accumulate when the tool fails and the step is retried. Add a shell trap directive at the beginning of the shell block that ensures the rm commands for uncompressed_regions and tmp_dir execute on EXIT, regardless of whether run_deepvariant succeeds or fails.
29-30:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse a job-scoped temporary regions file; current path can race and corrupt shared inputs.
params.uncompressed_regionsis derived from the shared input path (replace('.gz','')), so parallel samples can overwrite/delete the same file. Also, ifinput.regionsis already uncompressed, this can resolve to the same path and redirect output onto the source file.Suggested fix
params: @@ - uncompressed_regions=lambda wc, input: - input.regions.replace('.gz',''), + uncompressed_regions=lambda wc: os.path.join( + config["OUTPUT_FOLDER"], + "tmp", + f"{wc.patient}_deepvariant_tmp", + "regions.bed", + ), @@ shell: """ mkdir -p {params.tmp_dir} - zcat {input.regions} > {params.uncompressed_regions} + if [[ "{input.regions}" == *.gz ]]; then + zcat {input.regions} > {params.uncompressed_regions} + else + cat {input.regions} > {params.uncompressed_regions} + fi run_deepvariant \ @@ --regions={params.uncompressed_regions} \ @@ rm -f {params.uncompressed_regions} rm -rf {params.tmp_dir} """Also applies to: 54-55, 62-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workflow/rules/deepvariant.smk` around lines 29 - 30, The uncompressed_regions parameter derives its path directly from the input.regions path by removing the .gz extension, which causes a race condition when multiple samples run in parallel—they can overwrite the same file. Additionally, if input.regions is already uncompressed, the replace operation returns the original path, causing output redirection to the source file itself. Replace the simple string replacement with a job-scoped temporary file path that includes sample-specific identifiers (wildcards) to ensure each job writes to a unique file. Use Snakemake's temp() directive or construct a path that incorporates the sample identifier to prevent collisions and self-overwriting of source files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@workflow/rules/deepvariant.smk`:
- Around line 53-67: The cleanup commands (rm -f and rm -rf) at the end of the
deepvariant rule's shell block only execute if run_deepvariant succeeds, causing
temporary files to accumulate when the tool fails and the step is retried. Add a
shell trap directive at the beginning of the shell block that ensures the rm
commands for uncompressed_regions and tmp_dir execute on EXIT, regardless of
whether run_deepvariant succeeds or fails.
- Around line 29-30: The uncompressed_regions parameter derives its path
directly from the input.regions path by removing the .gz extension, which causes
a race condition when multiple samples run in parallel—they can overwrite the
same file. Additionally, if input.regions is already uncompressed, the replace
operation returns the original path, causing output redirection to the source
file itself. Replace the simple string replacement with a job-scoped temporary
file path that includes sample-specific identifiers (wildcards) to ensure each
job writes to a unique file. Use Snakemake's temp() directive or construct a
path that incorporates the sample identifier to prevent collisions and
self-overwriting of source files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a31a8c1-9b9f-4e18-b1c6-684886d9506d
⛔ Files ignored due to path filters (1)
workflow/supplementary_res/intervals_coding.BED.gzis excluded by!**/*.gz
📒 Files selected for processing (2)
workflow/rules/deepvariant.smkworkflow/supplementary_res/intervals_coding.BED.gz.tbi
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
setup/download_res.py (1)
285-290:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the missing directory check in the “missing resource” guard.
Line [286] only checks
os.path.isfile(existing_path). For directory resources, this logs a false error before Line [289] correctly skips existing directories.Suggested fix
- if name not in resources and not os.path.isfile(existing_path): + if name not in resources and not (os.path.isfile(existing_path) or os.path.isdir(existing_path)): logging.error(f"{name} missing in resources and not in repo.") continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@setup/download_res.py` around lines 285 - 290, The condition on line 286 that guards the "missing resource" error log only checks os.path.isfile(existing_path) but does not check os.path.isdir(existing_path), which causes false error logs for directory resources that actually exist. Update the condition to also verify that existing_path is not a directory by adding an os.path.isdir(existing_path) check to the guard condition, so it matches the logic used on line 289 where both file and directory existence are properly checked.
🧹 Nitpick comments (1)
tests/test_workflow_conformity.py (1)
28-30: ⚡ Quick winMake directive extraction robust to single- and double-quoted paths.
_extractcurrently captures only double-quoted values, so valid single-quotedinclude/conda/scriptdirectives are ignored.🔧 Suggested tweak
def _extract(text, keyword): - pattern = re.compile(rf'{keyword}:\s*\n?\s*"([^"]+)"') + pattern = re.compile(rf"{re.escape(keyword)}:\s*\n?\s*['\"]([^'\"]+)['\"]") return pattern.findall(text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workflow_conformity.py` around lines 28 - 30, The _extract function's regex pattern currently only matches double-quoted values using the pattern with explicit double quotes. Modify the regex pattern to accept both single and double quotes around the captured values. Update the pattern to match either a single quote or double quote at the start, capture the content between the quotes, and then match the corresponding closing quote type. This will make the function robust to both single-quoted and double-quoted directives for include, conda, and script keywords.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/conformity-tests.yml:
- Around line 14-18: Replace the floating version tags in the GitHub Actions
with pinned full commit SHAs to prevent tag drift and injection attacks. For the
actions/checkout@v4 action, pin it to its full commit SHA and add a
configuration to disable credential persistence by setting the token parameter
appropriately. For the actions/setup-python@v5 action, pin it to its full commit
SHA. This ensures the exact code is executed every time and reduces the attack
surface for the workflow.
In `@setup/download_res.py`:
- Line 207: The dict_file assignment removes all dots from the fasta filename
instead of just the final extension, causing files like GRCh38.primary.fa to
become GRCh38primary.dict instead of GRCh38.primary.dict. Replace the
problematic string split logic with a suffix-safe approach using either
pathlib.Path().stem or by splitting only the rightmost dot using rsplit('.', 1),
then append the .dict extension properly to preserve the full path with all
intermediate dots intact.
In `@tests/test_setup_download.py`:
- Around line 63-89: The test functions test_cli_args_are_declared,
test_dry_run_uses_store_true, test_outfolder_is_required, and
test_filetype_handlers_cover_resources_json all use permissive substring
searches that can false-pass. Replace each substring check with proper AST-level
validation: for test_cli_args_are_declared, test_dry_run_uses_store_true, and
test_outfolder_is_required, parse the script_src using the ast module to find
ArgumentParser.add_argument() calls and validate that each expected argument is
actually present with the correct parameters (e.g., action='store_true' for
--dry-run, required=True for --outfolder); for
test_filetype_handlers_cover_resources_json, instead of checking if the filetype
string exists anywhere, validate that there is an actual handler branch (such as
a case statement or conditional that explicitly handles that filetype) in the
handler logic.
In `@workflow/rules/index.smk`:
- Around line 51-52: The output file gentrome.fa.gz is being written as
uncompressed plain text despite the .gz extension. In the shell command at line
51, pipe the output of the cat command through gzip compression before
redirecting to {output.gentrome} to ensure the file is actually compressed and
matches its file extension.
In `@workflow/scripts/merge_salmon_quantification.R`:
- Around line 6-13: Before assigning names(files) <- patients, add validation
checks to ensure that the length of files equals the length of patients, and
that the patients vector contains unique values. If either validation fails,
stop execution with a descriptive error message. This prevents silent
mislabeling of sample columns when the input data structure is inconsistent with
assumptions.
---
Outside diff comments:
In `@setup/download_res.py`:
- Around line 285-290: The condition on line 286 that guards the "missing
resource" error log only checks os.path.isfile(existing_path) but does not check
os.path.isdir(existing_path), which causes false error logs for directory
resources that actually exist. Update the condition to also verify that
existing_path is not a directory by adding an os.path.isdir(existing_path) check
to the guard condition, so it matches the logic used on line 289 where both file
and directory existence are properly checked.
---
Nitpick comments:
In `@tests/test_workflow_conformity.py`:
- Around line 28-30: The _extract function's regex pattern currently only
matches double-quoted values using the pattern with explicit double quotes.
Modify the regex pattern to accept both single and double quotes around the
captured values. Update the pattern to match either a single quote or double
quote at the start, capture the content between the quotes, and then match the
corresponding closing quote type. This will make the function robust to both
single-quoted and double-quoted directives for include, conda, and script
keywords.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c47215d9-d492-411c-b86f-e31c4cb13537
📒 Files selected for processing (16)
.github/workflows/conformity-tests.ymlpytest.inisetup/download_res.pytests/__init__.pytests/conftest.pytests/requirements.txttests/test_config_conformity.pytests/test_resources_manifest.pytests/test_setup_download.pytests/test_url_reachability.pytests/test_workflow_conformity.pyworkflow/envs/merge_salmon_quant.ymlworkflow/envs/multiqc.ymlworkflow/envs/sortmerna.ymlworkflow/rules/index.smkworkflow/scripts/merge_salmon_quantification.R
✅ Files skipped from review due to trivial changes (4)
- pytest.ini
- tests/requirements.txt
- workflow/envs/multiqc.yml
- workflow/envs/sortmerna.yml
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.11" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find workflow actions that are not pinned to a 40-char SHA
rg -nP '^\s*-\s*uses:\s*[^@]+@(?!(?:[0-9a-f]{40})\b).+' .github/workflowsRepository: ctglab/ENEO
Length of output: 345
🏁 Script executed:
cat -n .github/workflows/conformity-tests.ymlRepository: ctglab/ENEO
Length of output: 708
🌐 Web query:
GitHub Actions security best practices pinning commit SHA vs tags
💡 Result:
Pinning GitHub Actions to a full-length commit SHA is a critical security best practice, as it is the only way to ensure that an action is used as an immutable release [1][2]. Why Commit SHA Pinning is Recommended: - Immutability: Git tags and branches are mutable references that can be deleted, moved, or recreated by a repository owner or an attacker to point to different code [3][4][5]. Pinning to a full-length commit SHA guarantees that your workflow executes the exact code you expect every time, preventing "retargeting" attacks where malicious code is silently injected into a previously trusted version [3][6][5]. - Security Policy: GitHub now supports organization-level policies that can mandate full commit SHA pinning [1][7]. When enabled, any workflow referencing an action by a tag or branch will fail, forcing compliance across all repositories in an organization [3][6][7]. - Protection Against Compromise: If an action repository is compromised, pinning to a SHA ensures your pipeline continues to run the last known-good, reviewed code, rather than automatically adopting the attacker's latest malicious commit [4][6]. Best Practices for Implementation: - Use Inline Comments: When pinning to a SHA, include the original version tag as an inline comment (e.g., uses: actions/checkout@3df4ab1 # v4.0.0) [4][6][8]. This maintains human readability and allows automated dependency management tools to correlate the pin with semantic versions [4][9]. - Automate Updates: Manually maintaining SHAs is inefficient. Tools like Renovate (using presets such as helpers:pinGitHubActionDigestsToSemver) or Dependabot can be configured to manage these pins automatically, opening pull requests to update SHAs as new releases are published [4][6][8][10]. - Audit Existing Workflows: Use static analysis tools like CodeQL or Zizmor to identify unpinned actions in your repositories and audit them for migration to SHA pins [6][2]. While pinning to a tag is more convenient, it should only be done if you fully trust the creator and are aware of the risk that the tag's underlying code can change [1]. In professional or secure environments, moving away from tag-based references to SHA-pinned references is the standard for mitigating supply chain risks [7][5].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://codeql.github.com/codeql-query-help/actions/actions-unpinned-tag/
- 3: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 4: https://safeguard.sh/resources/blog/github-actions-pinning-sha-vs-tag
- 5: https://samsond.github.io/posts/why-tags-fail/
- 6: https://architecture.lullabot.com/adr/20260505-pin-github-actions-to-hash/
- 7: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 8: https://tenthirtyam.org/dispatches/2026/03/27/why-you-should-pin-github-actions-to-commit-hashes/
- 9: https://nearform.com/insights/why-you-should-pin-your-github-actions-by-commit-hash/
- 10: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
Pin third-party actions to full commit SHAs and disable credential persistence.
Using floating tags (@v4, @v5) exposes the workflow to upstream tag drift and retargeting attacks where malicious code can be silently injected into a previously trusted version. Full commit SHA pinning is the only way to ensure the exact code is executed every time. Additionally, this job does not need persisted checkout credentials.
🔧 Suggested hardening patch
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<full-commit-sha> # v4
+ with:
+ persist-credentials: false
- - uses: actions/setup-python@v5
+ - uses: actions/setup-python@<full-commit-sha> # v5🧰 Tools
🪛 zizmor (1.25.2)
[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/conformity-tests.yml around lines 14 - 18, Replace the
floating version tags in the GitHub Actions with pinned full commit SHAs to
prevent tag drift and injection attacks. For the actions/checkout@v4 action, pin
it to its full commit SHA and add a configuration to disable credential
persistence by setting the token parameter appropriately. For the
actions/setup-python@v5 action, pin it to its full commit SHA. This ensures the
exact code is executed every time and reduces the attack surface for the
workflow.
Source: Linters/SAST tools
| """ | ||
| dict_file = f"{''.join(fasta_file.split('.')[:-1])}.dict" | ||
| fasta_file.replace(".fa", ".dict").replace(".fasta", ".dict") | ||
| dict_file = f"{''.join(fasta_file.split('.')[:-1])}.dict" |
There was a problem hiding this comment.
Use a suffix-safe path builder for the .dict filename.
Line [207] removes all dots from the full path (''.join(fasta_file.split('.')[:-1])), so names like GRCh38.primary.fa become GRCh38primary.dict. This can produce the wrong dictionary path.
Suggested fix
- dict_file = f"{''.join(fasta_file.split('.')[:-1])}.dict"
+ dict_file = str(Path(fasta_file).with_suffix(".dict"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@setup/download_res.py` at line 207, The dict_file assignment removes all dots
from the fasta filename instead of just the final extension, causing files like
GRCh38.primary.fa to become GRCh38primary.dict instead of GRCh38.primary.dict.
Replace the problematic string split logic with a suffix-safe approach using
either pathlib.Path().stem or by splitting only the rightmost dot using
rsplit('.', 1), then append the .dict extension properly to preserve the full
path with all intermediate dots intact.
| def test_cli_args_are_declared(script_src): | ||
| for arg in EXPECTED_CLI_ARGS: | ||
| assert arg in script_src, ( | ||
| f"CLI argument '{arg}' not found in parse_arguments" | ||
| ) | ||
|
|
||
|
|
||
| def test_dry_run_uses_store_true(script_src): | ||
| assert "store_true" in script_src, ( | ||
| "--dry-run should use action='store_true'" | ||
| ) | ||
|
|
||
|
|
||
| def test_outfolder_is_required(script_src): | ||
| assert "required=True" in script_src, ( | ||
| "--outfolder should be a required argument" | ||
| ) | ||
|
|
||
|
|
||
| def test_filetype_handlers_cover_resources_json(script_src): | ||
| """Every filetype in resources.json must have a corresponding handler branch.""" | ||
| resources = json.loads((ROOT / "setup" / "resources.json").read_text()) | ||
| filetypes = {entry["filetype"].lower() for entry in resources.values()} | ||
| for ft in filetypes: | ||
| assert ft in script_src, ( | ||
| f"Filetype '{ft}' from resources.json has no handler in download_res.py" | ||
| ) |
There was a problem hiding this comment.
CLI and handler contract checks are overly permissive and can false-pass.
Current checks rely on global substring matches (arg in script_src, "store_true" in script_src, "required=True" in script_src, ft in script_src), so unrelated text/comments can satisfy assertions without real parser/handler wiring.
Use AST-level checks on ArgumentParser.add_argument(...) calls and explicit handler-branch matching to validate the actual contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_setup_download.py` around lines 63 - 89, The test functions
test_cli_args_are_declared, test_dry_run_uses_store_true,
test_outfolder_is_required, and test_filetype_handlers_cover_resources_json all
use permissive substring searches that can false-pass. Replace each substring
check with proper AST-level validation: for test_cli_args_are_declared,
test_dry_run_uses_store_true, and test_outfolder_is_required, parse the
script_src using the ast module to find ArgumentParser.add_argument() calls and
validate that each expected argument is actually present with the correct
parameters (e.g., action='store_true' for --dry-run, required=True for
--outfolder); for test_filetype_handlers_cover_resources_json, instead of
checking if the filetype string exists anywhere, validate that there is an
actual handler branch (such as a case statement or conditional that explicitly
handles that filetype) in the handler logic.
| cat <(sed -E 's/^>([^|]+).*/>\\1/' {input.cdna}) {input.genome} > {output.gentrome} | ||
| """ |
There was a problem hiding this comment.
gentrome.fa.gz is written as plain text.
Line [51] writes uncompressed data to a .gz path. This content/extension mismatch can break downstream consumers.
Suggested fix
- cat <(sed -E 's/^>([^|]+).*/>\\1/' {input.cdna}) {input.genome} > {output.gentrome}
+ cat <(sed -E 's/^>([^|]+).*/>\\1/' {input.cdna}) {input.genome} | gzip -c > {output.gentrome}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cat <(sed -E 's/^>([^|]+).*/>\\1/' {input.cdna}) {input.genome} > {output.gentrome} | |
| """ | |
| cat <(sed -E 's/^>([^|]+).*/>\\1/' {input.cdna}) {input.genome} | gzip -c > {output.gentrome} | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workflow/rules/index.smk` around lines 51 - 52, The output file
gentrome.fa.gz is being written as uncompressed plain text despite the .gz
extension. In the shell command at line 51, pipe the output of the cat command
through gzip compression before redirecting to {output.gentrome} to ensure the
file is actually compressed and matches its file extension.
| files <- file.path(snakemake@input[["quant"]]) | ||
| patients <- snakemake@params[["patients"]] | ||
| gtf_file <- file.path(snakemake@input[["annotation"]]) | ||
| coldata <- data.frame(files = files, names = patients, stringsAsFactors = FALSE) | ||
|
|
||
| # Label each quantification file with its patient so the output columns carry | ||
| # the sample identity instead of anonymous defaults. | ||
| names(files) <- patients | ||
|
|
There was a problem hiding this comment.
Validate sample labels before assigning names(files).
Line [12] assumes length(files) == length(patients) and unique patient IDs. If this is violated, sample columns can be mislabeled silently.
Suggested fix
files <- file.path(snakemake@input[["quant"]])
patients <- snakemake@params[["patients"]]
gtf_file <- file.path(snakemake@input[["annotation"]])
+if (length(files) != length(patients)) {
+ stop("quant files and patients must have the same length")
+}
+if (anyDuplicated(patients)) {
+ stop("patient identifiers must be unique")
+}
+
# Label each quantification file with its patient so the output columns carry
# the sample identity instead of anonymous defaults.
names(files) <- patients📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| files <- file.path(snakemake@input[["quant"]]) | |
| patients <- snakemake@params[["patients"]] | |
| gtf_file <- file.path(snakemake@input[["annotation"]]) | |
| coldata <- data.frame(files = files, names = patients, stringsAsFactors = FALSE) | |
| # Label each quantification file with its patient so the output columns carry | |
| # the sample identity instead of anonymous defaults. | |
| names(files) <- patients | |
| files <- file.path(snakemake@input[["quant"]]) | |
| patients <- snakemake@params[["patients"]] | |
| gtf_file <- file.path(snakemake@input[["annotation"]]) | |
| if (length(files) != length(patients)) { | |
| stop("quant files and patients must have the same length") | |
| } | |
| if (anyDuplicated(patients)) { | |
| stop("patient identifiers must be unique") | |
| } | |
| # Label each quantification file with its patient so the output columns carry | |
| # the sample identity instead of anonymous defaults. | |
| names(files) <- patients |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workflow/scripts/merge_salmon_quantification.R` around lines 6 - 13, Before
assigning names(files) <- patients, add validation checks to ensure that the
length of files equals the length of patients, and that the patients vector
contains unique values. If either validation fails, stop execution with a
descriptive error message. This prevents silent mislabeling of sample columns
when the input data structure is inconsistent with assumptions.
…le, add fastp config, robust ref_dict)
1 participant
Summary by CodeRabbit