Add LRSOMATICREPORT as the final pipeline step - #176
Open
ljwharbers wants to merge 23 commits into
Open
Conversation
Wraps the lrsomatic_report R/Quarto tool (added as a git submodule) in a new local module that renders a self-contained per-sample HTML report from the pipeline's key outputs (VEP-annotated somatic SNVs, Severus SVs, ASCAT copy number, QC). Runs last, gated by --skip_report, so future dependents (e.g. a real Wakhan integration) can hook in without restructuring. Every report input is optional and joined by plain sample-id keys (not full meta maps) so missing/skipped upstream steps degrade gracefully instead of breaking the join. The module's environment.yml was verified against the actual R code (not just the tool's README) and trimmed to what's really used; a Wave container was built and validated with a real end-to-end render. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This PR is against the
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new final pipeline step (LRSOMATICREPORT) to render a per-sample, self-contained HTML report from the pipeline’s main outputs, controlled via --skip_report and report-related parameters.
Changes:
- Integrates
LRSOMATICREPORTas the final workflow step, joining optional upstream outputs to degrade gracefully when steps are skipped. - Introduces a new local module (
modules/local/lrsomaticreport) with conda/container support and nf-test coverage. - Adds new report parameters to
nextflow.config,nextflow_schema.json, and updates documentation and module configuration/publishing.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workflows/lrsomatic.nf | Wires LRSOMATICREPORT into the main workflow and assembles/join report inputs. |
| nextflow.config | Adds skip_report, report_src, and report_gene_panel defaults. |
| nextflow_schema.json | Exposes report parameters in the schema (report_options) and adds skip_report. |
| conf/modules.config | Adds publishing configuration and parameter-to-args wiring for LRSOMATICREPORT. |
| modules/local/lrsomaticreport/main.nf | Implements the report-rendering process (staging inputs, running render_report.R). |
| modules/local/lrsomaticreport/environment.yml | Defines the conda environment used for the module. |
| modules/local/lrsomaticreport/meta.yml | Adds nf-core style metadata for the new module. |
| modules/local/lrsomaticreport/tests/main.nf.test | Adds nf-test coverage (stub + real render) for the module. |
| modules/local/lrsomaticreport/tests/main.nf.test.snap | Snapshot output for the stub test. |
| docs/usage.md | Documents --skip_report, --report_src, and --report_gene_panel. |
| docs/output.md | Documents the new per-sample report/ output directory and HTML artifact. |
| .gitmodules | Adds the assets/lrsomatic_report submodule definition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CI's docker/singularity 25.04.0 jobs regressed vs. dev after adding the
report step. Chasing the failures end-to-end (real render, not just
config) surfaced five distinct bugs:
1. workflows/lrsomatic.nf: normal-sample QC was keyed by the boolean
meta.paired_data instead of meta.id, so the join produced a
malformed remainder tuple and crashed the pipeline for any matched
tumor/normal pair reaching the report step. Also fixed misleading
comments (paired_data is not a sample id).
2. modules/local/lrsomaticreport/main.nf: qc_tumor_files/qc_normal_files
were staged flat. mosdepth/samtools default to a meta.id-only
prefix, so a matched pair's tumor and normal QC files share a name
and collided in the task work dir. Fixed via stageAs subdirectories
(qc_tumor/*, qc_normal/*) with basename-based destination linking.
3. modules/local/lrsomaticreport/environment.yml: missing r-r.utils,
needed by data.table::fread() to read a gzipped VCF directly --
only surfaced once a real Severus VCF reached the render step.
Required rebuilding the Wave container (new frozen tag
4506737a6b63b769); the container directive now follows this
codebase's existing dual-engine pattern (singularity blob URL +
docker tag, e.g. modules/local/bcftools/view/main.nf) since the
frozen singularity artifact is SIF/ORAS-native, not a portable OCI
image.
4. assets/lrsomatic_report submodule (re-pinned to fdf2a0a):
parse_severus_vcf's fread() errored instead of returning zero rows
when a sample's Severus VCF has no variant records at all (skip
landing exactly on the last line). Fixed upstream with tryCatch.
5. modules/local/lrsomaticreport/main.nf: report_src was staged via a
shared symlink (same fixed path for every sample), and Quarto
renders in-place next to the .qmd. Concurrent per-sample tasks
raced on that single physical directory ("cannot open file
per_sample.qmd"). Fixed with stageInMode 'copy' for an isolated
copy per task.
Snapshot regenerated (additive: LRSOMATICREPORT versions entry +
sample*/report/*_report.html); tests/.nftignore excludes the
Quarto-rendered HTML's unstable content, matching multiqc/nanoplot.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previously pinned tag (4506737a6b63b769) was built during a
singularity.enabled=true Wave session, which only produces a
Singularity-native SIF artifact -- Docker CI's docker|25.04.0 job
failed to pull it ("Encountered remote
application/vnd.sylabs.sif.config.v1+json (unknown) when fetching").
A second Wave freeze build under a docker-context session
(docker.enabled=true, wave.strategy=['conda']) produced tag
9d12b9297c3c4d38, a genuine OCI image (verified via
`skopeo inspect --raw`: application/vnd.oci.image.manifest.v1+json
with real tar+gzip layers). The dual-engine container directive now
uses this new tag for the docker branch; the singularity branch's
blob URL is unchanged (already confirmed working).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
stageInMode 'copy' (added to fix a race condition where concurrent per-sample Quarto renders collided on a shared symlinked report_src directory) has a real bug in Nextflow 25.04.0 for directory-type path inputs under the docker executor: docker|latest-everything passed but docker|25.04.0 failed with "cannot open file lrsomatic_report/bin/render_report.R: No such file or directory" -- the copied directory came out incomplete. Replaced the process-level directive with a plain `cp -rL` in the script body itself. This is pure shell with no Nextflow-version dependency, and fixes the same underlying problem: each task now dereferences report_src into its own private, task-local copy before Quarto renders in-place next to the .qmd. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Wakhan The `assets/lrsomatic_report` submodule could not reach anyone. `nextflow run IntGenomicsLab/lrsomatic` clones the pipeline repo but not its submodules, and CI checks out without `submodules: recursive` -- so the gitlink resolved to an empty directory for end users and for every CI run, which is what has been failing PR #176. Replace it with the upstream tree as real tracked files (bin/, R/, templates/, assets/, LICENSE, README.md; ~565 KB), recorded in assets/lrsomatic_report/VENDORED.md. `--report_src` stays, now as an override for a local checkout rather than a required setup step. Dependencies stay in the Wave multi-package container, rebuilt from the module's environment.yml after adding r-base64enc (used by R/utils.R embed_png(), listed in the upstream recipe, missing here). The tool is at v1.1.0, several releases past the pin. Rewire accordingly: - Drop the symlink tree that faked variants/clairs vs variants/clairsto so the old CLI could infer run mode. v1.1.0 derives the mode from whether normal-side QC is present and discovers files recursively by base name, so staging is now flat plus three fixed locations (qc/tumor, qc/normal, wakhan). - Drop `cp -rL` of report_src: render_report.R copies templates/ and assets/ into a task-local ._render itself, so the shared source dir is never written. - Feed the phased somatic VCF rather than the pre-phasing caller VCF, at the path the tool looks for it. The VAF/depth/phase-set columns now come from the same file VEP annotated instead of a possibly-consensus VCF. - Add SV_VEP.out.vcf, the tool's primary SV annotation source. - Add the Wakhan outputs it renders. Its per-solution plots all share one base name, so WAKHAN gains a `solution_dirs` output and the directories are staged whole rather than the files individually. - Export TMPDIR into the task work dir alongside HOME. Quarto's Deno runtime creates a session dir under TMPDIR and dies with "Read-only file system (os error 30): tmpdir" wherever the container's /tmp is not writable. The module test suite previously passed `checkIfExists` on a directory that existed but was empty, which is why it never caught any of this. It now has a stub test and a real-render test with a VEP somatic VCF, so a broken container, an incomplete tool tree or CLI drift all fail loudly. Refs #133 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The vendored tool tree needs a `linguist-vendored` entry so GitHub does not count 565 KB of upstream R and SCSS as pipeline source, but .gitattributes is template-managed and any edit fails files_unchanged. Opt it out the way the repo already opts out CODE_OF_CONDUCT.md and the workflow files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 57 changed files in this pull request and generated no new comments.
Suppressed comments (3)
modules/local/lrsomaticreport/main.nf:16
- The PR description says
container:points atcommunity.wave.seqera.io/library/r-base_quarto_r-data.table_r-dplyr_pruned:f1d36670d940c971, but this module hard-codes different Wave image tags/digests. Please align the PR description with the actual container references in code (or vice versa) so users know which image is expected to work.
workflows/lrsomatic.nf:961 ASCAT.out.pngcan emit a list of PNGs for a sample. After.mix(...).groupTuple()that can produce nested lists (e.g.[file, [png1,png2]]), which then gets passed downstream asascat_files. Flattening here (as you already do for Wakhan) keeps the contract consistently[meta, [file, ...]]and avoids surprising staging behavior inLRSOMATICREPORT.
workflows/lrsomatic.nf:545- The updated comment describes
meta.paired_dataas a boolean, but elsewhere it’s treated as a truthy/falsy value (it may be an id string for the paired sample). To avoid misleading future changes, reword the comment to describe the truthiness contract rather than a strict boolean.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 54 out of 57 changed files in this pull request and generated no new comments.
Suppressed comments (3)
conf/modules.config:563
ext.argspasses--gene-panelwithout quoting the parameter value. If--report_gene_panelis a file path containing spaces (common on shared filesystems) or shell metacharacters, the rendered command line will break or be mis-parsed. Quote the value so it is passed as a single argument.
modules/local/lrsomaticreport/main.nf:16- The PR description states the report module uses a specific Wave image tag (
...r-data.table_r-dplyr_pruned:f1d36670d940c971), but the module is pinned to different tags here (...r-base64enc_r-data.table_pruned:dc62d809aa6fd497/...:c1049dbaf31bf178). This kind of drift can lead to hard-to-reproduce render failures if the pinned container doesn't matchenvironment.yml. Please align the PR documentation and the pinned image(s), and double-check the pinned image actually contains all R/Quarto deps listed inenvironment.yml.
assets/lrsomatic_report/VENDORED.md:10 - The PR description says
assets/lrsomatic_reportwas added as a git submodule pinned to a specific commit, but this file explicitly documents that the directory is vendored (not a submodule) and records different upstream/vendored SHAs. Please update the PR description to match the actual approach used in the diff so reviewers/users aren't misled about how the report source is managed.
This directory is a **vendored copy** of the standalone report tool, not a git submodule.
Do not edit it here — fix upstream, tag a release, and re-sync.
| | |
|---|---|
| Upstream | <https://github.com/ljwharbers/lrsomatic_report> |
| Release | `v1.1.0` (`9d660a77d5f23f92e1f7ff34f85da7956f445009`) |
| Vendored commit | `d17a636aeb3f79462b7f58db9102f4030941195b` (`main`) |
…ort-module # Conflicts: # CHANGELOG.md
Addresses the two review comments still live on PR #176. --report_gene_panel is documented as accepting a path to a TSV, but the raw param was interpolated straight into the command line: the file was never staged, so it was not bound into the docker/singularity container and render_report.R aborted with "--gene-panel not found". Add an optional `gene_panel` path input, wired from the workflow only when the param resolves to an existing file (a builtin panel name still travels via ext.args alone), and have conf/modules.config pass the quoted *base* name -- unchanged for a builtin, and the staged name for a TSV. Quoting also fixes panel paths containing spaces. The activation-hook loop hard-coded CONDA_PREFIX=/opt/conda, which is right for the Wave image but wrong under -profile conda, where it already points at the task's own env. Only fall back to /opt/conda when unset, and glob the hooks from $CONDA_PREFIX. Also pass checkIfExists to the report_src lookup so a bad --report_src fails fast, and cover the panel path with a real (non-stub) nf-test that renders with a user-supplied TSV -- it only passes if the file is genuinely staged into the container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iner-and-ascat-list Fix/lrsomaticreport container and ascat list
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 66 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
README.md:107
- Grammar issue: “a
multiqcreport from that combines …” should be “amultiqcreport that combines …”.
This pipeline produces a series of different output files. The main output is an aligned and phased tumour bam file. This bam file can be used by any typical downstream tool that uses bam files as input. Furthermore, we have sample-specific QC outputs from `cramino` (fastq), `cramino` (bam), `mosdepth`, `samtools` (stats/flagstat/idxstats), and optionally `fibertools`. Finally, we have a `multiqc` report from that combines the output from `mosdepth` and `samtools` into one html report, and a self-contained per-sample HTML report (`<sample>/report/<sample>_report.html`) covering small variants, structural variants, copy number and QC in one place — disable it with `--skip_report`.
The report tool's --gene-panel became repeatable in lrsomatic_report v1.3.0 (upstream PR #16), so any number of panels can be active in one report and a variant or SV is kept if it hits any of them. Nothing pipeline-side could deliver that while the vendored copy was v1.2.1, where is_no_gene_panel() reads a vector of panels as "no panel", so this both re-syncs the tool and widens --report_gene_panel to a comma-separated list of builtin names and/or TSV paths. A single value behaves exactly as before. Only real files are staged, now into gene_panels/ rather than the task root: there are N of them and their names come from the user, while the root also holds sample_dir/, versions.yml and the output HTML. Two panel files sharing a base name would collide there whatever directories they came from, so that combination is rejected up front. conf/modules.config builds the repeated --gene-panel flags, and does so with string operations only. `file()` is not in scope inside an ext.args closure -- the delegate is the config script binding -- and the failure is a task-time MissingMethodException, not a parse error. The pre-existing `file(params.report_gene_panel).name` had the same defect and was simply never reached: every pipeline-level test left report_gene_panel null, and the module tests hard-code ext.args. tests/default.nf.test now passes a builtin and a panel file, which is what catches it. So "is this entry a panel file?" is a textual test (a path separator, or a .tsv suffix) shared by the staging decision and the flag builder, rather than a filesystem probe on one side and a guess on the other. validateReportGenePanels() then rejects at launch anything where the textual test and the filesystem would disagree -- along with a missing panel file, a name that is not a builtin, `none` mixed with a real panel, and duplicate base names. Previously a typo'd path staged nothing, raised nothing, and killed the run inside the report task after alignment, calling and annotation had all run. The vendored re-sync needed no dependency change: a library()/require() grep over v1.3.0's R/, bin/ and templates/ resolves to packages already pinned in environment.yml, and upstream recipe/meta.yaml only moved its version string. Both Wave container digests therefore stand. Version strings carried by hand were bumped together per VENDORED.md: the module's version topic, meta.yml, the module snapshot and the five pipeline snapshots. Verified under -profile test,singularity on Mindwell: all four module tests pass, including a new three-panel real render that asserts DEFAULT_PANELS names every panel -- had only the last --gene-panel survived, it would name one. The rendered report shows four panel checkboxes with the three requested ones ticked. default, deep_only, consensus and union pass at pipeline level. clair_only fails on md5s this branch cannot affect (sample{1,2} normal samtools stats, severus breakpoints_double.csv and read_qual.txt, and the merged sample4_tumor.bam, whose digest differs between two runs of identical code); its snapshot is otherwise untouched here apart from the version string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sample4 is the only sample in test_sheet_2.csv with two tumour replicates, so the only one that goes through SAMTOOLS_MERGE. Both replicate BAMs carry `@PG ID:minimap2` and `@PG ID:samtools` from identical align/sort commands, and merge disambiguates the collision with a random hex suffix -- minimap2-6CC32AF6 in one run, minimap2-39EB7FA0 in the next. So the digest in tests/clair_only.nf.test.snap could never match: not flaky-in-one-environment but unpinnable in principle. That suffix is the only difference between runs. Header length is identical at 1712 bytes and all 186,714,518 bytes of alignment records are byte-identical. The index moves with it because those bytes change the header's compressed BGZF block size, shifting every virtual offset -- which is why the .bai has to go too. Both files are now in tests/.nftignore, and the reads are asserted instead with bam().getReadsMD5(), the approach modules/nf-core/samtools/merge's own test already takes for the same reason. That needs nft-bam loaded in nf-test.config; the vendored nf-core minimap2/align, longphase/haplotag and samtools/merge tests already assume it, but nf-test.config ignores modules/nf-core/**/tests/* so it was never loaded. stable_name keeps both filenames -- their existence is still snapshotted, alongside the .exists() assertion. Regenerating touched only what it should: the two entries dropped, sample4_merged_reads added, and every other digest reproduced its committed value byte for byte. This does not make clair_only pass. Germline phasing is nondeterministic -- three runs produced three distinct phased-VCF digests -- and that propagates into the haplotagged BAMs (HP tags) and the whatshap stats, none of which are ignored. sample1's BAMs and whatshap stats matched in three of four runs and not in the fourth. Since the same files are snapshotted by all five pipeline tests, deciding what to do about that is a wider call than this commit, which fixes only the entry that failed every single time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…iner-and-ascat-list Apply several report gene panels at once (lrsomatic_report v1.3.0)
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a substantial new final pipeline stage plus a large vendored tool subtree, so it warrants a final human pass despite only minor identified issues.
Review details
Suppressed comments (1)
README.md:107
- The sentence has a grammatical error: "a
multiqcreport from that combines" should be "amultiqcreport that combines".
This pipeline produces a series of different output files. The main output is an aligned and phased tumour bam file. This bam file can be used by any typical downstream tool that uses bam files as input. Furthermore, we have sample-specific QC outputs from `cramino` (fastq), `cramino` (bam), `mosdepth`, `samtools` (stats/flagstat/idxstats), and optionally `fibertools`. Finally, we have a `multiqc` report from that combines the output from `mosdepth` and `samtools` into one html report, and a self-contained per-sample HTML report (`<sample>/report/<sample>_report.html`) covering small variants, structural variants, copy number and QC in one place — disable it with `--skip_report`.
- Files reviewed: 71/74 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+9
to
+10
| | Release | `v1.3.0` (`236ab35d5d7c018df1c6b477ecc867c60efd2ff9`) | | ||
| | Vendored commit | `236ab35d5d7c018df1c6b477ecc867c60efd2ff9` (the tag itself) | |
Two nits from the Copilot review threads on #176 that were still real: - README: "a `multiqc` report from that combines" dropped the stray "from", and "one html report" -> "one HTML report". - LRSOMATICREPORT: quote the interpolated paths in the script block. The `Rscript` invocation passed report_src, the sample id, the sex and the output filename unquoted, so a projectDir or sample id containing a space would word-split; the flat-input symlink loop had the same shape. ${args} stays unquoted -- it is a pre-built argument string that already quotes each `--gene-panel` value in conf/modules.config. No output or version changes, so no snapshots move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 tasks
fix: address remaining PR #176 review comments
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new report step currently doesn’t stage WhatsHap stats into the report task (so the Phasing section will always be missing), and there are a couple of robustness/convention issues that should be fixed before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
modules/local/lrsomaticreport/main.nf:16
- This module’s container selection ignores
task.ext.singularity_pull_docker_container, whereas other modules in this repo use it to decide whether to use a Singularity-native image vs a Docker/OCI reference. With-profile singularityplussingularity_pull_docker_container=true, this process would still try to use theoras://SIF reference, diverging from the rest of the pipeline’s behavior.
Consider matching the established pattern (workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ? ... : ...).
subworkflows/local/utils_nfcore_lrsomatic_pipeline/main.nf:289
gene_lists_dir.list()can returnnull(e.g. ifparams.report_src/assets/gene_listsexists but is not a directory, or on an I/O error). In that case the subsequent.findAll { ... }will throw a NullPointerException during parameter validation, which is a confusing failure mode for a user typo in--report_src.
Guard both isDirectory() and the return value of .list() so validation fails cleanly and predictably.
- Files reviewed: 71/74 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+1059
to
+1065
| // Tumor-side QC: keyed by the sample's own id, which for tumor rows is already the report id | ||
| ch_mosdepth_summary | ||
| .mix(ch_mosdepth_global, ch_cramino_post_txt, ch_bam_stats, ch_bam_flagstat) | ||
| .filter { meta, _f -> meta.type == 'tumor' } | ||
| .map { meta, f -> [meta.id, f] } | ||
| .groupTuple() | ||
| .set { report_qc_tumor_ch } |
The comments this PR added were far denser than the surrounding code and in places longer than the lines they described. Cut ~84 lines of commentary without touching any code: - lrsomaticreport/main.nf: the 8-line Wave container note, the 7-line input block prose, the 7-line gene-panel note and the 4-5 line blocks in the script section. - conf/modules.config: the 16-line block explaining the ext.args closure was longer than the closure itself, in a file with no other explanatory comments. - utils_nfcore_lrsomatic_pipeline: the four new helpers had multi-paragraph doc blocks; the file's existing helpers use a one-line header. - workflows/lrsomatic.nf: the LRSOMATICREPORT section header is now the same shape as the other MODULE: headers, and two trailing comments that only restated `meta.paired_data` are gone. - tests/.nftignore, clair_only.nf.test, default.nf.test, module test: the same rationale was spelled out at length in two places; one keeps it, the other points at it. Kept the non-obvious reasons: the wave rebuild command, CONDA_PREFIX falling back only when unset (hard-coding it broke -profile conda), R's list.files() not descending into symlinked directories, why the panel test in conf/modules.config has to be textual, and why sample4's merged BAM md5 is unstable. Verified: `nextflow config .` resolves the LRSOMATICREPORT block unchanged, `nextflow lint` clean, pre-commit prettier/whitespace/nf-lint pass, and `git diff -U0` confirms only comment lines moved. No snapshots affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Launch-time gene panel validation can throw an NPE when --report_src exists but does not contain a readable assets/gene_lists directory, so the builtin panel discovery needs to be made null-safe.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
subworkflows/local/utils_nfcore_lrsomatic_pipeline/main.nf:281
reportBuiltinGenePanels()assumes${params.report_src}/assets/gene_listsis a directory and calls.list()unguarded. If--report_srcpoints at the wrong existing path (e.g. a file, or a tool tree missingassets/gene_lists/),gene_lists_dir.exists()can still be true but.list()returns null, causing an NPE during parameter validation instead of a clear error / empty builtin list.
- Files reviewed: 71/74 changed files
- Comments generated: 0 new
- Review effort level: Lite
Vendored assets/lrsomatic_report is untouched here; its comments are trimmed upstream in ljwharbers/lrsomatic_report#20 and land with the next re-sync. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Parameter validation likely fails at launch because reportBuiltinGenePanels() calls .list() on a file(...) result that is typically a Path, which can throw at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 71/74 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+266
to
+277
| def reportBuiltinGenePanels() { | ||
| def gene_lists_dir = file("${params.report_src}/assets/gene_lists") | ||
| if (!gene_lists_dir.exists()) { | ||
| return [] | ||
| } | ||
| return gene_lists_dir | ||
| .list() | ||
| .findAll { it.endsWith('.tsv') } | ||
| .collect { it.replaceFirst(/(\.(hg38|t2t))?\.tsv$/, '') } | ||
| .unique() | ||
| .sort() | ||
| } |
Follows assets/lrsomatic_report/VENDORED.md: replaces bin/, R/, templates/, assets/, LICENSE and README.md from the v1.3.2 tag (75c65b2), updates the VENDORED.md table, the version topic in the module and meta.yml, the snapshot version lines and the CHANGELOG. environment.yml is unchanged: the recipe's dependency list still matches, so the container digests stay. v1.3.2 brings the v1.3.1 changes (live facet counts, facet menu keeps the table's horizontal scroll, flatter theme) plus one-line inline comments (lrsomatic_report#20). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The report module can fail when linking inputs that include path components (e.g. SEVERUS outputs) and the rendered report footer currently advertises an inconsistent tool version.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 71/74 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+31
to
+35
| // Discovery is recursive and matches on base name, so suffix-distinct files can be linked flat | ||
| def flat_inputs = [vep_somatic, sv_vep, severus_vcf, ascat_files].flatten().findAll { f -> f } | ||
| def link_flat = flat_inputs ? """ | ||
| for f in ${flat_inputs.collect { f -> "\"${f}\"" }.join(' ')}; do ln -s "\$PWD/\$f" "sample_dir/\$f"; done | ||
| """ : '' |
|
|
||
| {{< include sections/_qc.qmd >}} | ||
|
|
||
| *Report generated `r format(Sys.time(), "%Y-%m-%d %H:%M")` · LRSomatic report v1.3.1* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wraps the lrsomatic_report R/Quarto tool as a new local module
LRSOMATICREPORTthat renders a self-contained per-sample HTML report, and runs it as the final pipeline step.assets/lrsomatic_reportis a vendored copy ofv1.3.0(236ab35); seeassets/lrsomatic_report/VENDORED.mdfor the rationale, the exact provenance, and the re-sync procedure.nextflow run IntGenomicsLab/lrsomaticclones the pipeline but does not initialise submodules, so a submodule would leave the default--report_srcpointing at an empty directory.--skip_report(defaultfalse), consistent withskip_ascat/skip_wakhan.remainder: true, so any skipped or missing upstream step degrades gracefully to a "not available" section instead of breaking the join.--skip_report,--report_src(default: the vendored path),--report_gene_panel(a comma-separated list of panels — see below).solution_dirsoutput to the WAKHAN module so its per-solution copy-number plots can be staged into the report.What the vendored v1.3.0 brings over the originally vendored v1.1.0
The vendored tree was re-synced
v1.1.0→v1.2.1→v1.3.0over the life of this PR. Nothing on the pipeline side had to change for the report features themselves — no new module inputs, no staging changes, no container rebuild (see below); the only pipeline-side change was making--report_gene_panelaccept a list.From v1.2.x:
lymphoid.{hg38,t2t}.tsv, and a newsarcomapanel) and are selected by their bare name, resolved against the detected reference. A coordinate-carrying panel must declare its reference; a mismatch is a hard error rather than a silently wrong filter. Coordinate matching is what makes breakend filtering reliable — whether a breakend carries a VEP gene symbol at all depends on the VEP invocation.New in v1.3.0:
--gene-panelis now repeatable. On the pipeline side--report_gene_paneltherefore takes a comma-separated list — builtin names and custom paths mixed freely — and the panels are unioned: a variant or SV is kept if it hits any of them. Panel values are validated at launch (validateReportGenePanels()) instead of failing inside the report task after alignment, calling and annotation have already run:nonecannot be combined with a real panel, an unknown builtin name or a missing panel file is an error, and two panel files sharing a base name are rejected because they would collide in the task's staging directory.Containers
The module pins two Wave builds from its own
environment.yml(dependencies only — R, Quarto and the tool's R packages; the tool itself is vendored):oras://community.wave.seqera.io/library/r-base_quarto_r-base64enc_r-data.table_pruned:dc62d809aa6fd497community.wave.seqera.io/library/r-base_quarto_r-base64enc_r-data.table_pruned:c1049dbaf31bf178Two separate builds are needed:
wave --singularityproduces a Singularity-native SIF (theoras://reference), the default build a genuine OCI image. Rebuild both wheneverenvironment.ymlchanges.Neither re-sync changed
environment.yml: alibrary()/require()grep over the upstreamR/,bin/andtemplates/resolves to circlize, data.table, dplyr, DT, ggplot2, htmltools, optparse, quarto and yaml, all already pinned. Both images are unchanged.Notable fixes made along the way
environment.ymlwas originally derived from the tool's README package list; grepping the actual R source showedGenomicRanges/ComplexHeatmap/tidyrare unused, so they were dropped. This also sidesteps a real bioconda/micromamba incompatibility —GenomeInfoDbData's post-link data-fetch script never runs under micromamba.activate.dhooks (Quarto needsQUARTO_SHARE_PATH), so the module sources them explicitly.CONDA_PREFIXis exported first because one hook references it unguarded underset -u, but it now defaults to the existing value and only falls back to/opt/condawhen unset — hard-coding it broke-profile conda.--report_gene_panelentry is either a builtin panel name or a path to a TSV. Panel paths are declared as an optionalpath(gene_panels, stageAs: 'gene_panels/*')module input so Nextflow stages them and they are bound into the container; previously a path was interpolated into the command line as a bare string and the tool aborted with "--gene-panel not found". Each--gene-panelargument is also quoted, so panel paths containing spaces work.HOMEandTMPDIRare pointed at the task work dir so Quarto/Deno's cache and session directories can't depend on the container's$HOMEor a read-only/tmp.Known limitations
somatic_vcffed to the report is the final (possibly consensus) somatic VCF, staged undervariants/phased/purely to drive the report's run-mode detection and VAF column; if multiple somatic callers were combined via consensus, that VAF column won't reflect a single real caller. As of v1.2.0 the report makes this visible — the variant-table footnote names the source file and flags multi-caller rows — rather than hiding it. Joining per caller instead would require staging each caller's VCF, i.e. a change to this module's input tuple, so it was deliberately not done. The VEP-based variant table (the primary source) is unaffected.Test plan
nextflow config .validates;--report_gene_panelresolves to the expected--gene-panelargument(s)nextflow lintclean — no new warnings or errors introducedpre-commit run --all-filescleannf-test, stubnf-test, real render under-profile singularity— renders a valid HTML report from a VEP somatic VCFnf-test, real render with a user-supplied gene panel TSV — passes only if the panel file is genuinely staged into the container, which is the regression this PR fixesnf-test, real render with several gene panels at once — one repeated--gene-panelflag per entry, builtin and file mixed-profile singularity; the only snapshot movement across the re-syncs was the version stringnf-teston CI (docker | 25.04.0anddocker | latest-everything)🤖 Generated with Claude Code