Skip to content

Fix/clinvar prepare - #206

Merged
AmberVerhasselt merged 4 commits into
IntGenomicsLab:devfrom
AmberVerhasselt:fix/clinvar-prepare
Sep 25, 2026
Merged

AmberVerhasselt merged 4 commits into
IntGenomicsLab:devfrom
AmberVerhasselt:fix/clinvar-prepare

Conversation

@AmberVerhasselt

@AmberVerhasselt AmberVerhasselt commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Multi-sample GRCh38 runs fail in SOMATIC_VEP with:

Can't stage file https://ftp.ncbi.nlm.nih.gov/pub/clinvar/vcf_GRCh38/archive_2.0/2026/clinvar_20260829.vcf.gz -- reason: Unable to access path

ClinVar was staged as a Nextflow foreign file. Every VEP task that uses it (germline, somatic and SV, per sample) re-checks it on NCBI with a HEAD request, and NCBI answers the resulting burst with HTTP 503. Twenty parallel HEAD requests got about half 503s. When a check fails, the staging cache key changes, so Nextflow downloads the file again, and all 3 retries fail within a second. -resume cannot recover. A 2-sample run gets through; an 8-sample run fails every time.

Fix

  • New local module VEPPLUGIN_CLINVAR, called from PREPARE_VEP_PLUGINS:
    • downloads the ClinVar VCF and its .tbi once per run, retrying 429/5xx responses;
    • keeps the release's own basename, so the --custom file= argument is unchanged;
    • publishes both files to <outdir>/vep_plugins/.
  • The VCF is checked against a new --vep_clinvar_md5:
    • the GRCh38 and CHM13 defaults carry the MD5 their hosts publish (NCBI .md5, Ensembl CHECKSUMS), so the pinned release cannot change silently;
    • overriding --vep_clinvar drops the default MD5, the same rule as the default index;
    • a remote ClinVar without an MD5 logs a warning;
    • --vep_clinvar_md5 with a local file is an error.
  • A local --vep_clinvar is still staged as given.
  • Only ClinVar changes. REVEL and EVE keep their existing path.

Two implementation notes:

  • The module reports its version through versions.yml, not an eval() topic. Nextflow numbers eval() outputs across the whole pipeline and hashes that number, so one new eval() output changed the cache key of every task that has one, and -resume of an existing run re-ran everything from METAEXTRACT on. Confirmed with a two-process reproduction.
  • The wget container has no CA bundle, so conf/modules.config passes --no-check-certificate, as the existing WGET config does. The MD5 check covers integrity.

Testing

  • nf-test (singularity): 34 function tests in utils_nfcore_lrsomatic_pipeline/tests/main.function.nf.test, 9 of them new, plus a new stub test for VEPPLUGIN_CLINVAR. All pass.
  • Module shell logic run inside its container against the real NCBI file: the correct MD5 passes and a wrong MD5 exits 1.
  • A real 8-sample GRCh38 run on the VSC (Mindwell), resumed from the failing run:
    • 198 tasks came from the cache, and VEPPLUGIN_CLINVAR ran once with clinvar_20260829.vcf.gz: OK;
    • SOMATIC, GERMLINE and SV VEP for the first 4 samples completed (8 of those tasks started within 2 minutes), with 0 staging warnings;
    • the annotated VCFs carry ClinVar_CLNSIG, ClinVar_CLNREVSTAT and ClinVar_CLNDN.
  • nextflow lint reports no new errors or warnings, and pre-commit passes.
  • The pipeline-level tests run with skip_vep_plugins = true, so their snapshots are unchanged.

Also corrects the ClinVar download sizes in docs/usage.md (193 MB for GRCh38, 99 MB for CHM13).

🤖 Generated with Claude Code

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

AmberVerhasselt and others added 2 commits September 25, 2026 11:06
A remote ClinVar was staged as a Nextflow foreign file, which every VEP
task re-checks on its host with a HEAD request. On a multi-sample GRCh38
run NCBI answers that burst with HTTP 503, so SOMATIC_VEP failed with
"Can't stage file ...clinvar_20260829.vcf.gz", and -resume could not
recover because the failed check changed the staging cache key.

The new VEPPLUGIN_CLINVAR step downloads the VCF and its index once per
run, retrying 503s, and checks the VCF against --vep_clinvar_md5. The
GRCh38 and CHM13 defaults carry the MD5 their hosts publish, so the
pinned release cannot change silently; a user URL without an MD5 warns,
and an MD5 with a local file is rejected.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Nextflow numbers eval() outputs across the whole pipeline and hashes
that number, so one new eval() output shifts the cache key of every
task that has one: a -resume of an existing run re-ran everything from
METAEXTRACT on. A versions.yml path output leaves other tasks' hashes
untouched.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 25, 2026 11:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@ljwharbers ljwharbers left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The approach is sound: one download per run, the release's own basename kept, an MD5 pinned per genome, and versions.yml instead of eval() to protect the pipeline-wide eval counter. The new function tests are good. Two things need fixing before this merges (inline). The rest are nits.

What I checked

  • The pinned MD5s match what the hosts publish: dffae274… is in NCBI's clinvar_20260829.vcf.gz.md5, and 98bde679… is in Ensembl's CHECKSUMS (MD5 format) for the CHM13 VCF.
  • The corrected sizes match Content-Length: 193,427,450 bytes and 99,024,078 bytes.
  • The resolve → prepare → VEP wiring works: --custom file= gets the basename, and ch_extra_files flattens [vcf, tbi]. The existing _tbi-required check still covers the fetch path, so tbi_url is never null.
  • The 9 new function tests carry tag "small", so PR CI runs them.

Should fix (inline)

  1. vep_plugins/versions.yml is published to the outdir, because the publishDir override has no saveAs.
  2. s3://, gs://, az:// and file:// ClinVar paths now go to wget and fail. Before this PR they were staged by Nextflow.

Nits

  • -resume of an already completed run re-runs GERMLINE_VEP and SOMATIC_VEP once. ClinVar's input path moves from work/stage-<session>/… to the VEPPLUGIN_CLINVAR work dir, so both tasks get a new hash. That is expected and fine, but the PR stresses resume safety, so it deserves a line in the CHANGELOG.
  • The PR description says SV VEP also re-checked ClinVar. It doesn't: SV_VEP gets [] and '' for plugins (workflows/lrsomatic.nf:1009-1010). Only GERMLINE_VEP and SOMATIC_VEP were affected.
  • A remote --vep_clinvar with a local --vep_clinvar_tbi passes the local path to wget and fails with an unclear error. Either file()-stage a local tbi or reject that combination in validateVepPluginParams. Optional: the tbi is never checksum-checked. NCBI publishes no .tbi.md5, but Ensembl's CHECKSUMS lists the CHM13 tbi (50b634a69c36e2479cc2c8f71a8eafd6).
  • docs/output.md never documents vep_plugins/. That predates this PR, but the directory now holds ClinVar too.

CI: everything is green except docker | * | 4/7, which was still running after about 16 minutes (the other shards took 2–3). Worth checking before merge.

Happy to approve once 1 and 2 are addressed.

Comment thread conf/modules.config
// --vep_revel / --vep_eve (and their _tbi) at these files
withName : '.*:VEPPLUGIN_(REVEL|EVE)' {
// --vep_revel / --vep_eve / --vep_clinvar (and their _tbi) at these files
withName : '.*:VEPPLUGIN_(REVEL|EVE|CLINVAR)' {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block replaces the default publishDir, and the default is where the saveAs that drops versions.yml lives. REVEL and EVE report their versions through topics, so this never mattered for them. VEPPLUGIN_CLINVAR emits a versions.yml, so every run now publishes <outdir>/vep_plugins/versions.yml.

Suggested change
withName : '.*:VEPPLUGIN_(REVEL|EVE|CLINVAR)' {
withName : '.*:VEPPLUGIN_(REVEL|EVE|CLINVAR)' {
publishDir = [
mode: params.publish_dir_mode,
path: { "${params.outdir}/vep_plugins" },
saveAs: { filename -> filename.equals('versions.yml') ? null : filename }
]
}

def value = vepPluginResource(data_param)
return value &&
data_param == 'vep_clinvar' &&
value.toString().contains('://')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contains('://') also catches s3://, gs://, az:// and file://. Before this PR, file() staged those through Nextflow's filesystem providers. Now they reach wget, which fails with Unsupported scheme. A cloud run pointing --vep_clinvar at a bucket copy would break. REVEL and EVE use the same test, but this is the only one whose behaviour changes.

Suggest restricting it to what wget can fetch:

        value.toString() ==~ /(?i)^(https?|ftp):\/\/.*/

Plus a function test where an s3:// ClinVar gives false.


tag "modules"
tag "modules_local"
tag "vepplugin_clinvar"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: PR CI runs only tag "small" tests (.github/workflows/nf-test.yml:53,102). The nf-test-changes job lists this test, but no shard runs it. It is a stub test, so adding tag "small" costs nothing.

Comment thread modules/local/vepplugin/clinvar/main.nf Outdated
wget \\
--no-verbose \\
--tries=5 \\
--waitretry=30 \\

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: --waitretry sets the maximum of a linear backoff (1 s, 2 s, … up to 30 s), so with --tries=5 the total wait is about 1+2+3+4 = 10 s. That is probably enough now that there's only one request, but the comment reads as if it covers more. If NCBI stays unavailable for longer, --tries=10 (about 45 s in total) would be a cheap margin.

… CHM13 index

- Don't publish VEPPLUGIN_CLINVAR's versions.yml to vep_plugins/: the
  publishDir override lacked the default saveAs.
- Fetch ClinVar only for http(s)/ftp URLs; s3://, gs://, az:// and
  file:// are staged by Nextflow again, as before this PR.
- Reject a downloaded ClinVar paired with a local --vep_clinvar_tbi,
  which wget could not fetch.
- Add --vep_clinvar_tbi_md5, pinned by default to the MD5 Ensembl
  publishes for the CHM13 index; NCBI publishes none for GRCh38.
- wget --tries=10 (about 45 s of linear backoff), tag the module test
  "small" so PR CI runs it, and document vep_plugins/ in output.md.
- Correct the wording: only GERMLINE_VEP and SOMATIC_VEP staged
  ClinVar, never SV_VEP; note the one-off VEP re-run on resume.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@AmberVerhasselt
AmberVerhasselt merged commit 6c553bc into IntGenomicsLab:dev Sep 25, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants