Repository navigation
Template update for nf-core/tools v4.2.1 - #224
Open
ljwharbers wants to merge 8 commits into
Open
ljwharbers wants to merge 8 commits into
ljwharbers wants to merge 8 commits into
Conversation
Conflict resolutions: - linting.yml: keep dev's plain pre-commit step (51d1d20) instead of the template's prek-action; take the template's other pin bumps. - nf-test.config: nft-utils 0.0.3 -> 1.2.1, keep nft-bam. - .gitignore / .nf-core.yml: keep dev's extra entries and comment. - ro-crate-metadata.json: regenerated with nf-core pipelines rocrate. Follow-on: the template drops ext.use_gpu from process_gpu. Drop it from the custom GPU labels too and have CLAIR3 read task.accelerator, which was the only consumer of task.ext.use_gpu. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
clair_only, consensus, deep_only and union run only on release or dispatch, so they were last regenerated on 6 Oct and have drifted since: the haplotagged tumour BAM/BAI md5s and Severus breakpoints_double.csv. Not caused by the template update: the same two tests regenerated on unmodified dev (2b1d510) give byte-identical snapshots apart from timestamps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 29, 2026
Collaborator
Author
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def args = task.ext.args ?: '' | ||
| prefix = task.ext.prefix ?: "${meta.id}" | ||
| def use_gpu = task.ext.use_gpu as boolean | ||
| def use_gpu = task.accelerator?.request as boolean |
There was a problem hiding this comment.
GPU mode is now decided by task.accelerator, but the label (line 3) and the container (lines 7-9) are still chosen from params.use_gpu. Any config that sets accelerator separately from --use_gpu can now pair the two inconsistently. Two examples:
- An institutional or user config sets
acceleratorfor CLAIR3 (or for all processes) without--use_gpu. Clair3 then runs on the CPU biocontainer with--use_gpu --device=cuda:0and fails. - A config clears
acceleratorwith--use_gpu, e.g. on executors where GPUs are passed through and not requested. Clair3 then silently runs in CPU mode on the GPU image.
Before this PR, ext.use_gpu was equal to params.use_gpu, so all three decisions matched. DeepVariant, DeepSomatic and fibertools still branch on params.use_gpu. Reading the param here keeps one source of truth for label, image and flags:
Suggested change
| def use_gpu = task.accelerator?.request as boolean | |
| def use_gpu = params.use_gpu as boolean |
No CI test runs the GPU branch. The small tests (default.nf.test) only run Clair3 on CPU, so please confirm a --use_gpu Clair3 run on the Slurm/GPU test array before merging.
Collaborator
Author
There was a problem hiding this comment.
Fixed in bd5dace: def use_gpu = params.use_gpu as boolean, as suggested, so the label, image and flags all come from one param.
I ran the GPU branch as you asked, and it showed that GPU Clair3 was broken before this PR, for two separate reasons:
hkubal/clair3:v2.0.2_gpucouldn't find the reference index. Itsrun_clair3.pycallsresolve()on a relative--ref_fn, which follows Nextflow's symlink into the FASTA's own task directory, but the.faiis staged from a different task. It aborted at start with "Reference index fai file not found". 2.0.3+ usesabsolute()instead. f1066e4 moves the GPU image tohkubal/clair3:v2.0.5_gpu, which has the fix and bundles the same 21 models as before. CPU runs keep the 2.0.3 biocontainer.- HKU's GPU image can't run on B200s. Its PyTorch is built for sm_50 to sm_90. On a Mindwell B200 (sm_100), it fails in
m.to(device)with "no kernel image is available for execution on the device". Nothing in the pipeline can change that; A100 (sm_80), H100 (sm_90) and Ada cards are supported.
Verification: run -profile test,apptainer,gpu --use_gpu from an export of f1066e4, on Mindwell interactive (NVIDIA RTX 5000 Ada, sm_89), Slurm job 11765791.
- Both CLAIR3 tasks used
clair3-v2.0.5_gpu, ran with--use_gpu --device=cuda:0(log:USE GPU: True,GPU DEVICE: cuda:0), and exited 0. - On sample2-normal, all 8512 GPU calls are identical to the CPU run (2.0.3 biocontainer, nf-test
default) in CHROM/POS/REF/ALT/FILTER/GT. The CPU run has 2 extra PASS calls (8248 vs 8246). - The pipeline completed. Its only failures were the SAVANA CNA/TO tasks, which fail on the test data on CPU too and are ignored by design.
|
|
||
| jobs: | ||
| fix-linting: | ||
| uses: nf-core/actions/.github/workflows/fix-linting.yml@v1 |
There was a problem hiding this comment.
This calls a reusable workflow by the mutable tag @v1 and passes it BOT_TOKEN (nf_core_bot_auth_token), a token that can push. The old fix_linting.yml pinned every action by SHA and also had a github.repository == 'IntGenomicsLab/lrsomatic' guard, which is gone here.
The same PR also unpins nf-core/setup-nextflow@v3 and nf-core/setup-nf-test@v2 from SHAs to tags. That happens in .github/actions/nf-test/action.yml, .github/actions/get-shards/action.yml, linting.yml and download_pipeline.yml, while every other action stays SHA-pinned. The PR description calls these changes "pin bumps".
If unpinning the nf-core-owned actions is intended, please say so in the PR. Otherwise, pin them (and this reusable workflow) to commit SHAs with a # vX comment, as the rest of the workflows do.
Collaborator
Author
There was a problem hiding this comment.
Kept as is on purpose. These lines are exactly what the nf-core/tools 4.2.1 template ships (origin/TEMPLATE has nf-core/setup-nextflow@v3, nf-core/setup-nf-test@v2 and fix-linting.yml@v1). Re-pinning them here would make every future template sync conflict on these lines. Third-party actions stay SHA-pinned, as in the template.
On the token:
- This repository has no
nf_core_bot_auth_tokensecret (onlyCLAUDE_CODE_OAUTH_TOKENis set), soBOT_TOKENresolves to an empty string. Nothing with push rights is passed to the reusable workflow. nf-core/actionsfix-linting.yml@v1only runs on PR comments that contain@nf-core-bot fix linting, and it checks that the commenter has repository permission before doing anything.issue_commentworkflows from a fork run in the fork's own context, so the droppedgithub.repositoryguard didn't protect anything here.
You're right that "pin bumps" in the PR description was misleading. I've corrected it to say the nf-core-owned actions now follow the template's tags.
|
Automated review: 2 findings, posted inline.
To check by running the pipeline: CI never runs the Clair3 GPU branch; |
task.accelerator could disagree with params.use_gpu, which still picks CLAIR3's label and container, whenever a config set accelerator on its own: GPU flags on the CPU image, or CPU mode on the GPU image. Read the param, as DeepVariant, DeepSomatic and fibertools do. Addresses the #224 review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With --use_gpu, Clair3 2.0.2 aborted at start with "Reference index fai file not found": run_clair3.py resolve()s a relative --ref_fn, following Nextflow's symlink into the FASTA's own task dir, and the .fai is staged from a different task. 2.0.3+ uses absolute() instead. v2.0.5_gpu has that fix and bundles the same models as before; CPU runs keep the 2.0.3 biocontainer. Found by a --use_gpu test run for the #224 review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
Review follow-upBoth automated review findings are addressed (replies are on the threads):
The GPU test found a bug that predates this PR (f1066e4): GPU Clair3 never got past start-up on GPU test: Slurm job 11765791,
CPU checks: the |
Collaborator
Author
|
@claude review |
| "breakpoint_clusters.tsv:md5,d36a70de292ee130ef30da4a58bced18", | ||
| "breakpoint_clusters_list.tsv:md5,0c0ce62e329f8de492487e8414c30a50", | ||
| "breakpoints_double.csv:md5,003e4e80ec4233315762233e5f1ec0d8", | ||
| "breakpoints_double.csv:md5,e307fa52370d89e72ea09f84d4eff2f7", |
There was a problem hiding this comment.
The snapshot drift has a pattern the PR doesn't explain. Before this refresh, each sample's Severus breakpoints_double.csv had the same md5 in all four extended tests. For example, sample1 was 003e4e80… in clair_only, consensus, deep_only and union. After the refresh it depends on the small-variant caller:
| sample | clair_only | consensus | deep_only | union |
|---|---|---|---|---|
| sample1 | e307fa52 |
c76eef6f |
3dcda68b |
e307fa52 |
| sample2 | fa0c1072 |
d5d056ce |
d5d056ce |
fa0c1072 |
| sample3 | 0c3021e3 |
0c3021e3 |
ff3bd14c |
ff3bd14c |
So Severus output now changes with the phasing/haplotagging from Clair3 vs DeepVariant, and before it didn't. That fits a change to which BAM or phased VCF reaches Severus (maybe #223, "Fixed the Severus input channel"), not random noise. The PR shows the drift is already on dev, but REVIEW.md asks for unexplained md5 drift to be explained. Please name the commit/PR that causes it, and confirm that Severus now depending on the caller is intended. Otherwise this refresh could lock in a regression from dev.
Collaborator
Author
There was a problem hiding this comment.
Good catch. The cause is #223 ("Severus fix", merge commit 5208521, 14 Oct), and the change is intended.
Why: #223 builds severus_input from PHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch, the Longphase-haplotagged BAMs, instead of the untagged minimap2 BAMs, so that Severus gets HP tags for haplotype-aware SV calling. Before that, severus.log reported H1/H2 coverage of 0.0, and every hVAF was x,0,0. HP tags come from phasing the small variants of the selected germline caller(s), so Severus now depends on the caller by design. #223's test plan expected "no snapshot changes" because PR CI only runs small, so nobody ran the extended tests until now.
A/B check: I ran clair_only and deep_only on dev just before #223 (91b8c1e) and at its merge (5208521); #223 changes only workflows/lrsomatic.nf. Snapshots weren't updated. These are the breakpoints_double.csv md5s from each run's output (Slurm array 11765924):
| sample | 91b8c1e clair_only | 91b8c1e deep_only | 5208521 clair_only | 5208521 deep_only |
|---|---|---|---|---|
| sample1 | 003e4e80 |
003e4e80 |
e307fa52 |
3dcda68b |
| sample2 | ab7ccd32 |
ab7ccd32 |
fa0c1072 |
d5d056ce |
| sample3 | c18b66ee |
c18b66ee |
0c3021e3 |
ff3bd14c |
- Before Severus fix #223, both callers give identical Severus output, matching the old snapshots.
- From Severus fix #223 on, the output splits by caller and matches the refreshed values in this PR exactly.
- So the refresh records what dev produces after Severus fix #223 and doesn't lock in a regression.
unionandconsensusphase a union or intersection of both callers' sets, so they match one caller or the other depending on which heterozygous sites support each SV read.
I'll add this to the snapshot section of the PR description.
| container "${ workflow.containerEngine in ['singularity', 'apptainer'] && !task.ext.singularity_pull_docker_container ? | ||
| (params.use_gpu ? 'docker://docker.io/hkubal/clair3:v2.0.2_gpu' : 'https://depot.galaxyproject.org/singularity/clair3:2.0.3--py311hbc58adc_0') : | ||
| (params.use_gpu ? 'docker.io/hkubal/clair3:v2.0.2_gpu' : 'quay.io/biocontainers/clair3:2.0.3--py311hbc58adc_0') }" | ||
| (params.use_gpu ? 'docker://docker.io/hkubal/clair3:v2.0.5_gpu' : 'https://depot.galaxyproject.org/singularity/clair3:2.0.3--py311hbc58adc_0') : |
There was a problem hiding this comment.
The GPU limitation of the new image isn't documented for users. The PR thread says hkubal/clair3:v2.0.5_gpu doesn't run on B200 (its PyTorch is built for sm ≤ 90), so users on B200 nodes should leave --use_gpu off. Right now that caveat is only in a PR comment. docs/usage.md:585 still says --use_gpu enables GPU "for DeepVariant and DeepSomatic". It doesn't mention Clair3 or fibertools, even though both switch on the same param. A B200 user who sets --use_gpu for DeepVariant will have Clair3 fail too, and the docs give no warning. Please update the --use_gpu row in docs/usage.md (and the schema description if useful) to list all the tools it affects and the Clair3 GPU architecture limit.
Collaborator
Author
There was a problem hiding this comment.
Fixed in 0c1cf3f. The --use_gpu row in docs/usage.md now lists every tool it switches: DeepVariant, DeepSomatic, Clair3 and the fibertools GPU steps (m6A prediction, nucleosomes, FIRE). It also says to add the gpu profile, and that hkubal/clair3:v2.0.5_gpu supports compute capability ≤ 9.0 (A100, H100, RTX Ada) but not newer cards such as the B200, where --use_gpu should stay off. The schema description lists the same tools, and the limit is in a new help_text.
|
Automated re-review: 2 new findings, posted inline. Both earlier findings (CLAIR3 reading
Needs a test run to check: PR CI ( |
docs/usage.md named only DeepVariant and DeepSomatic, but Clair3 and the fibertools GPU steps switch on the same param. Also note the gpu profile and that hkubal/clair3:v2.0.5_gpu supports compute capability <= 9.0, so it fails on B200s. Addresses the #224 re-review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
Template update for nf-core/tools 4.2.1
Manual template sync (
nf-core pipelines sync) from tools 4.1.0 → 4.2.1, following the 4.2.0 release notes. This pipeline isis_nfcore: false, so no automated sync PR is opened.TEMPLATEwas clean at 4.1.0 (be762da), so this is a straight fast-forward to 0ab3233.Template changes pulled in
nf-test.config, matching nf-core/modules.nft-bamis kept.manifest.diagram: set to the existing README metro map,assets/lrsomatic_1.0.png. The new lint check passes.fix_linting.yml→fix-linting.yml: now uses the reusable workflow from nf-core/actions.ext.use_gpuremoved fromprocess_gpu:acceleratoralone decides GPU use (see below).PIPELINE_COMPLETIONno longer blocks on the MultiQC report (multiqc_reports.isBound() ? … : [])..vscode/extensions.jsonadded with recommended extensions; the admonition CSS link is fixed.nf-core/setup-nextflow@v3,nf-core/setup-nf-test@v2, thenf-core/actionsfix-linting workflow@v1) are now referenced by tag, not SHA, exactly as in the 4.2.1 template. They are kept that way on purpose, so future syncs don't conflict. Also thetrace_report_suffixicon fix, and RO-Crate 1.3 metadata (regenerated withnf-core pipelines rocrate).The blog's two "common conflicts" did not come up.
awsfulltest.ymldoesn't exist here. Innextflow.config,doimerged cleanly, anddiagramnow points at our metro map.Conflict resolutions
.github/workflows/linting.yml: kept dev's plainpre-commitstep instead of the template'sprek-action(we left prek-action on purpose in 51d1d20). Took the template's other pin bumps.nf-test.config: nft-utils bumped,nft-bam@0.7.0kept..gitignore/.nf-core.yml: kept dev's extra entries and thenf_test_contentcomment.ro-crate-metadata.json: regenerated.The template drops
ext.use_gpu.CLAIR3was its only consumer (task.ext.use_gpu), so without a fix GPU Clair3 would silently fall back to CPU. Changes:ext.use_gpufrom our customprocess_gpu_high,process_gpu_very_highandprocess_gpu_very_high_memorylabels too;CLAIR3now readsparams.use_gpu, the same param that already picks its label and container. An earlier commit readtask.accelerator; that changed after review, because a config settingacceleratoron its own could split the flags from the image.No other module reads
ext.use_gpu; deepvariant/deepsomatic/fibertools branch onparams.use_gpu.Stale extended-test snapshots (separate commit)
clair_only,consensus,deep_onlyandunionrun only on release or dispatch. They had drifted since their last regeneration (6 Oct) in the haplotagged tumour BAM/BAI md5s and Severusbreakpoints_double.csv. This drift isn't from the template.clair_onlyanddeep_onlyregenerated on unmodified dev (2b1d510) give byte-identical snapshots, ignoring timestamps. The refresh is in its own commit, so it can be dropped if you'd rather handle it separately.Severus drift comes from #223 (5208521) and is intended. #223 runs Severus on the Longphase-haplotagged BAMs, so
breakpoints_double.csvnow depends on the germline caller whose phased variants set the HP tags. I checked with an A/B run: on 91b8c1e (just before #223),clair_onlyanddeep_onlygive identical Severus md5s, which match the old snapshots. On 5208521 they split by caller and match the refreshed values exactly (Slurm array 11765924; table in the review thread).Testing (Mindwell, apptainer, Nextflow 26.04.3, nf-test 0.9.4)
tests/default.nf.test: PASSED, snapshot unchangedclair_only,consensus,deep_only,union: PASSED with--update-snapshot(drift as above)nf-test test --tag small(what PR CI runs): 140/140 PASSEDnf-core pipelines lint(4.2.1): 0 failed, no new warnings.prek run --all-files: all hooks pass, including Nextflow lint🤖 Generated with Claude Code