fix(modkit): patched modkit 0.6.4 for PacBio 5mC+5hmC reads, 5mC-CpG pileup defaults and --modkit_phased option - #188
fix(modkit): patched modkit 0.6.4 for PacBio 5mC+5hmC reads, 5mC-CpG pileup defaults and --modkit_phased option#188ljwharbers wants to merge 9 commits into
Conversation
… with 5mC+5hmC > 1 Stock modkit 0.4.3-0.6.4 rejects every read whose per-base 5mC and 5hmC probabilities sum above 1, which happens at ~2 CpGs per read in recent PacBio HiFi BAMs (Jasmine >= 26.1.3) and cost 32-65 % of reads in pileup (nanoporetech/modkit#612). Two further 0.6.1+ pileup bugs made --cpg pileups on those files return 0 rows. MODKIT_PILEUP now uses ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b, built from ljwharbers/modkit@pacbio-conflict-fix (proposed upstream in nanoporetech/modkit#720), until a modkit release includes the fix. The module patch is refreshed to carry the container change alongside the existing fasta/fai input split, and the module snapshot version string follows the binary (0.6.4). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
There was a problem hiding this comment.
🟡 Changes recommended
The module still declares a conda environment pinned to ont-modkit=0.6.1, so -profile conda/mamba would not use the patched container and would undermine the PR’s intended PacBio fix unless explicitly blocked or updated.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the nf-core modkit/pileup module to use a patched Modkit 0.6.4 container image that avoids dropping PacBio HiFi reads where independent 5mC+5hmC probabilities can sum > 1, and aligns the module test snapshot with the reported Modkit version.
Changes:
- Swap
MODKIT_PILEUPto a patchedghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7bcontainer image (with inline rationale + revert note). - Refresh the module patch (
modkit-pileup.diff) to include the container update. - Update nf-test snapshots to expect Modkit
0.6.4in the versions channel.
File summaries
| File | Description |
|---|---|
| modules/nf-core/modkit/pileup/main.nf | Switches process container to patched Modkit image and documents why/when to revert. |
| modules/nf-core/modkit/pileup/modkit-pileup.diff | Regenerated module patch to capture the container change (and snapshot update). |
| modules/nf-core/modkit/pileup/tests/main.nf.test.snap | Updates expected Modkit version string to 0.6.4. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| conda "${moduleDir}/environment.yml" | ||
| container "${ workflow.containerEngine == 'singularity' && !task.ext.singularity_pull_docker_container ? | ||
| 'https://depot.galaxyproject.org/singularity/ont-modkit:0.6.1--hcdda2d0_0': | ||
| 'biocontainers/ont-modkit:0.6.1--hcdda2d0_0' }" | ||
| // Patched modkit 0.6.4 (ljwharbers/modkit@697de7b, nanoporetech/modkit#720): stock 0.4.3-0.6.4 | ||
| // drops 32-65 % of reads from recent PacBio HiFi BAMs whose 5mC+5hmC probabilities sum above 1 | ||
| // (nanoporetech/modkit#612). Return to the nf-core biocontainer once a release includes the fix. | ||
| container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b' |
There was a problem hiding this comment.
🟡 Changes recommended
The module’s conda path still pins ont-modkit=0.6.1 (so the fix is not applied for conda/non-container runs) and the new hard-coded ghcr.io/... container string can break Singularity/Apptainer usage without an explicit docker:// handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
modules/nf-core/modkit/pileup/main.nf:9
- The module still declares a conda environment that pins
ont-modkit=0.6.1(modules/nf-core/modkit/pileup/environment.yml:7). That means runs using conda (or with containers disabled) will still hit the PacBio-read drop/empty--cpgbehavior this PR is addressing.
To make the fix complete and avoid divergent behavior across execution modes, either update the conda environment to a version that includes the fix (if/when available) or consider removing/guarding conda support for this module and documenting that containers are required for the PacBio fix.
conda "${moduleDir}/environment.yml"
// Patched modkit 0.6.4 (ljwharbers/modkit@697de7b, nanoporetech/modkit#720): stock 0.4.3-0.6.4
// drops 32-65 % of reads from recent PacBio HiFi BAMs whose 5mC+5hmC probabilities sum above 1
// (nanoporetech/modkit#612). Return to the nf-core biocontainer once a release includes the fix.
container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b'
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
| // Patched modkit 0.6.4 (ljwharbers/modkit@697de7b, nanoporetech/modkit#720): stock 0.4.3-0.6.4 | ||
| // drops 32-65 % of reads from recent PacBio HiFi BAMs whose 5mC+5hmC probabilities sum above 1 | ||
| // (nanoporetech/modkit#612). Return to the nf-core biocontainer once a release includes the fix. | ||
| container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b' |
|
|
||
| ### `Fixed` | ||
|
|
||
| - [#188](https://github.com/IntGenomicsLab/lrsomatic/pull/188) - `MODKIT_PILEUP` now runs a patched modkit 0.6.4 (`ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b`, built from [ljwharbers/modkit@pacbio-conflict-fix](https://github.com/ljwharbers/modkit/tree/pacbio-conflict-fix)) that keeps reads whose 5mC and 5hmC probabilities sum above 1 and fixes pileup on PacBio-style MM tags. Stock modkit 0.4.3-0.6.4 silently dropped 32-65 % of reads from recent PacBio HiFi BAMs (Jasmine >= 26.1.3) and returned empty `--cpg` pileups ([nanoporetech/modkit#612](https://github.com/nanoporetech/modkit/issues/612); fix proposed upstream in [nanoporetech/modkit#720](https://github.com/nanoporetech/modkit/pull/720)). The module should return to the nf-core biocontainer once a modkit release includes the fix (@ljwharbers). |
…d modkit_phased options MODKIT_PILEUP ran with no arguments, so it emitted every modification code in every sequence context: 30-45 GB bedMethyl per sample on full-size runs. params.modkit_args now feeds ext.args and defaults to 5mC at CpG sites. params.modkit_phased adds --phased and switches the pileup input from the unphased merged BAM to PHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch, the Longphase-haplotagged BAM (HP tag) that nothing consumed until now. The MODKIT_PILEUP call moves below the phasing subworkflow so it can pick either input; meta.id and meta.type are present on both channels. modkit writes <prefix>_hp1/_hp2/_combined.bed.gz into the output directory and the nf-core module already moves those out, so no module change is needed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The modkit execution path is inconsistent across profiles (conda still pins 0.6.1) and the updated user-facing docs/schema contain statements that conflict with the patched-modkit behavior described in this PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
modules/nf-core/modkit/pileup/main.nf:9
MODKIT_PILEUPnow hard-codes a patched modkit 0.6.4 container, but the process still declares a conda env viaenvironment.yml, which currently pinsont-modkit=0.6.1. That means runs using conda (or where container use is disabled) will silently fall back to the unfixed version and still drop affected PacBio reads; it also makes the module's reported version dependent on execution profile.
conda "${moduleDir}/environment.yml"
// Patched modkit 0.6.4 (ljwharbers/modkit@697de7b, nanoporetech/modkit#720): stock 0.4.3-0.6.4
// drops 32-65 % of reads from recent PacBio HiFi BAMs whose 5mC+5hmC probabilities sum above 1
// (nanoporetech/modkit#612). Return to the nf-core biocontainer once a release includes the fix.
container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b'
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
| // Modkit options | ||
| modkit_args = '--cpg --modified-bases 5mC' | ||
| modkit_phased = false |
| | `--skip_normalfiber` | A boolean to skip fibertools processing for the normal sample. Default = `false` | | ||
| | `--skip_modcall` | A boolean to skip modkit methylation calling. Default = `false` | | ||
| | `--skip_modkit` | A boolean to skip the modkit pileup step. Default = `false` | | ||
| | `--modkit_args` | Additional arguments passed to `modkit pileup`. The default restricts output to 5mC calls at CpG sites; note that `--modified-bases` only filters the output, so PacBio positions where the 5mC and 5hmC probabilities sum above 1 are still dropped. Default = `--cpg --modified-bases 5mC` | |
| "properties": { | ||
| "modkit_args": { | ||
| "type": "string", | ||
| "description": "Additional command line arguments passed to `modkit pileup`. The default restricts the bedMethyl output to 5mC calls at CpG sites. Note that `--modified-bases` only filters the output: on PacBio data, positions where the 5mC and 5hmC probabilities sum above 1 are still dropped, and non-conflicting 5hmC calls are counted in the `N_other` column.", |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The MODKIT_PILEUP container change may break Singularity/Apptainer usage because it no longer follows the repo’s established container-selection pattern for non-OCI Singularity runs.
Review details
Suppressed comments (1)
modules/nf-core/modkit/pileup/main.nf:7
MODKIT_PILEUPnow hard-codes a GHCR container tag without adocker://prefix or any Singularity/Apptainer handling. In this codebase most modules avoid relying on OCI auto-pull in thesingularityprofile (e.g. by usinghttps://depot.galaxyproject.org/singularity/...), so this change risks breaking-profile singularityruns if the environment expects SIF/URL images rather than pulling from a Docker registry.
conda "${moduleDir}/environment.yml"
// Patched modkit 0.6.4 (nanoporetech/modkit#720) that keeps PacBio reads with 5mC+5hmC > 1; revert to the biocontainer once released
container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b'
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
) Full-size reruns showed that modkit's general pileup workers, used whenever a BAM carries opposite-strand modification calls such as PacBio 6mA, ignore the HP partition (empty _hp1/_hp2 files) and still emit h rows under --modified-bases 5mC. ONT output is as expected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The modkit container reference is likely to break Singularity/Apptainer runs without explicit OCI pull handling, and the usage docs section header is now misleading.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
docs/usage.md:138
- This section is titled "Skipping options", but it now includes
--modkit_argsand--modkit_phased, which are not skip flags. This makes the docs misleading when scanning for skip-related parameters.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| // Patched modkit 0.6.4 (nanoporetech/modkit#720) that keeps PacBio reads with 5mC+5hmC > 1; revert to the biocontainer once released | ||
| container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b' |
There was a problem hiding this comment.
🔵 Needs a closer look
The Modkit module change can cause profile-dependent behavior (conda env still pins 0.6.1) and needs small config/module adjustments to avoid inconsistent phased argument handling and container resolution issues.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
conf/modules.config:334
modkit_argsis described as “additional arguments”, but--phasedis also appended automatically whenmodkit_phasedis true and the workflow switches inputs based on that boolean. If a user includes--phasedinsidemodkit_args, it can (a) get duplicated and/or (b) produce confusing behavior ifmodkit_phasedis false (phased args on unphased BAM). Stripping--phasedfrommodkit_argshere prevents that inconsistency and reserves phasing control formodkit_phased.
modules/nf-core/modkit/pileup/main.nf:7
MODKIT_PILEUPnow hardcodes a patched 0.6.4 container, but the module still defines a conda env (environment.yml) that pinsont-modkit=0.6.1. Running with-profile conda/mambawould therefore execute an older modkit and miss the PacBio fixes (and the version in the module test snapshots). Consider updating the conda env to match the intended version/behavior or explicitly documenting/disabling conda for this module so execution profiles don’t silently diverge.
conda "${moduleDir}/environment.yml"
// Patched modkit 0.6.4 (nanoporetech/modkit#720) that keeps PacBio reads with 5mC+5hmC > 1; revert to the biocontainer once released
container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b'
modules/nf-core/modkit/pileup/main.nf:7
- The module’s
containerdirective no longer follows the repo’s standard engine-aware pattern, and for Singularity/Apptainer it may be safer to use an explicitdocker://reference (many environments rely on OCI pulls). Using an engine-conditional container string keeps behavior consistent with other modules and reduces profile-specific container resolution issues.
// Patched modkit 0.6.4 (nanoporetech/modkit#720) that keeps PacBio reads with 5mC+5hmC > 1; revert to the biocontainer once released
container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b'
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
… on PacBio) The new image includes the fix for modkit's general pileup workers, which are used for PacBio BAMs with 6mA calls: `--phased` now fills `_hp1`/`_hp2` and `--modified-bases 5mC` no longer emits an `h` row next to every `m` row. The "Known limitation" note for `--modkit_phased` is therefore removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The MODKIT_PILEUP container change introduces portability/reproducibility concerns (Singularity/OCI handling) and the container tag referenced in code conflicts with the PR description, which should be reconciled before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
docs/usage.md:155
- The
--modkit_argshelp text here omits theN_otherbehavior that’s described in the schema (and can surprise users on PacBio data):--modified-basesfilters output rows, but non-conflicting 5hmC calls can still contribute toN_other. Consider adding that note here too so CLI docs and schema stay consistent.
modules/nf-core/modkit/pileup/main.nf:7
- The MODKIT_PILEUP container selection no longer follows the repo-wide pattern that switches between prebuilt Singularity/Apptainer images and OCI images via
workflow.containerEngine+task.ext.singularity_pull_docker_container(e.g.modules/nf-core/mosdepth/main.nf:6-8,modules/nf-core/samtools/merge/main.nf:6-8). This makes the process always depend on pulling from GHCR, which can break sites that run Singularity without Docker/OCI pulls enabled and expect the prebuilt SIF/Blob URLs used elsewhere in the pipeline.
// Patched modkit 0.6.4 (nanoporetech/modkit#720) that keeps PacBio reads with 5mC+5hmC > 1 and honours --phased/--modified-bases in the general workers; revert to the biocontainer once released
container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2'
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| 'https://depot.galaxyproject.org/singularity/ont-modkit:0.6.1--hcdda2d0_0': | ||
| 'biocontainers/ont-modkit:0.6.1--hcdda2d0_0' }" | ||
| // Patched modkit 0.6.4 (nanoporetech/modkit#720) that keeps PacBio reads with 5mC+5hmC > 1 and honours --phased/--modified-bases in the general workers; revert to the biocontainer once released | ||
| container 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2' |
…ainer The modkit fork workflow now also publishes an Apptainer SIF at oras://ghcr.io/ljwharbers/modkit-sif:<tag>; use it for the singularity and apptainer engines instead of converting the OCI image on every node. Module patch refreshed accordingly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The MODKIT_PILEUP container selection introduces a portability regression by ignoring the repo’s established singularity_pull_docker_container handling for Singularity/Apptainer.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
workflows/lrsomatic.nf:658
- Assigning to
ch_modkit_inputwithout a declaration inside the workflow block creates/updates a script binding variable in Groovy, which can accidentally leak state or clash with other workflows. Declare it withdefto keep it local to this workflow scope.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| container "${workflow.containerEngine == 'singularity' || workflow.containerEngine == 'apptainer' | ||
| ? 'oras://ghcr.io/ljwharbers/modkit-sif:0.6.4-pacbiofix-6e0afa2' | ||
| : 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2'}" |
Description
Recent PacBio HiFi BAMs (Jasmine >= 26.1.3) carry independent 5mC (
C+m?) and 5hmC (C+h?) probabilities. At roughly 2 CpGs per read they sum above 1, and stock modkit 0.4.3-0.6.4 rejects the whole read (conflict-explicit-prob-greater-than-one), which cost 32-65 % of reads inmodkit pileup(nanoporetech/modkit#612). Two further 0.6.1+ pileup bugs made--cpgpileups on those files return 0 rows. The fix lives on ljwharbers/modkit@pacbio-conflict-fix (commits 697de7b and 6e0afa2) and is proposed upstream in nanoporetech/modkit#720.Until a modkit release includes it,
MODKIT_PILEUPrunsghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2, built from that branch by this workflow. The same string serves Docker and Singularity/Apptainer (Nextflow addsdocker://).The second fork commit (6e0afa2, 2026-09-04) fixes modkit's general pileup workers, which modkit selects for PacBio BAMs with 6mA calls (opposite-strand
T-a.sub-tags): they ignored the--phasedpartition (empty_hp1/_hp2) and did not apply--modified-basesas a row filter (anhrow next to everymrow). Both now behave like the optimized workers that ONT data uses.New pileup options
A full-size run of the container swap alone produced 30-45 GB bedMethyl files per sample, because
modkit pileupran with no arguments (every modification code, every sequence context). This PR therefore also adds:--modkit_args(string, default--cpg --modified-bases 5mC): extra arguments passed tomodkit pileupviaext.args. The default restricts output to 5mC calls at CpG sites, per strand. Note that--modified-basesonly filters the output after the conflict check: on PacBio data, positions where P(5mC)+P(5hmC) > 1 are still dropped (about 1.5 % of CpG calls on h-carrying reads) and non-conflicting 5hmC calls are counted inN_other. Resolving those positions (adjust-mods --ignore h, or an--ignoreflag on plainpileupin the fork) is a follow-up.--modkit_phased(boolean, defaultfalse): adds--phasedand switches the pileup input from the unphased merged BAM toPHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch, the Longphase-haplotagged BAM (HP tag) that nothing consumed before. modkit then writes<sample>_hp1.bed.gz,<sample>_hp2.bed.gzand<sample>_combined.bed.gz; the nf-core module already handles the directory output, so no module change was needed. TheMODKIT_PILEUPcall moved below the phasing subworkflow so it can pick either input.Changes:
modules/nf-core/modkit/pileup/main.nf: container swapped, with a comment saying when to revert to the biocontainer.modules/nf-core/modkit/pileup/modkit-pileup.diff: patch refreshed so it carries the container change next to the existing fasta/fai input split.modules/nf-core/modkit/pileup/tests/main.nf.test.snap: version string follows the binary (0.6.4).nextflow.config,nextflow_schema.json:modkit_args,modkit_phasedin a new "Modkit options" group.conf/modules.config:ext.argsforMODKIT_PILEUP.workflows/lrsomatic.nf: pileup input selection, call moved afterPHASING_HAPLOTYPING.docs/usage.md,docs/output.md,CHANGELOG.md.Verification
Container (chr20 subset of an affected PacBio BAM, Apptainer build of the same commit):
adjust-mods --ignore hwrites 2,687 records (stock: 1,035),pileup --cpggives 21,957 CpG positions (stock: 0).Full-size pipeline run of the container swap (3 tumor-only samples: old PacBio, new PacBio, ONT; CHM13; 121/121 tasks OK in 5h12m) produced 30-45 GB unfiltered pileups per sample, which motivated the new defaults.
nextflow run -previewwith-profile test,singularitysucceeds withmodkit_phasedon and off; the DAG showsMODKIT_PILEUPfed byLONGPHASE_HAPLOTAG+SAMTOOLS_INDEXin phased mode and by the merged alignment BAM otherwise.pre-commitandnf-core pipelines lint(schema/config keys) pass.Real-data
-resumereruns (same 3 samples and work dir as above; the threeMODKIT_PILEUPtasks re-ran, ~30 min each for the phased run with the6e0afa2image on 2026-09-04):--cpg --modified-bases 5mC_hp1/_hp2/_combinedrows (image697de7b)_hp1/_hp2/_combinedrows (image6e0afa2)mrows onlyh+mrows)mrows only)With the
6e0afa2image every file of every sample contains onlymrows, and the PacBio haplotype files are populated (the haplotagged PacBio BAMs carryHP/PSon 68-78 % of reads). The unphased default-mode rerun with the same image gives single files of 63.5 M (FL11, was 127.1 M withhrows) / 64.0 M (DLBCL16) / 66.2 M (BL1)mrows. Logs showproducing phased output,parsed 1 base modification(s) ... counted as 'N_other', and for PacBiousing general workersplus (FL11)dropped 11166779 position(s) where the base modification probabilities summed to more than 1.0. On a chr20 subset of FL11, hp1 + hp2 coverage never exceeds the combined coverage at any position and_combinedis identical to the unphased run.nf-test (
--profile=+singularity, all five pipeline tests): 4 pass;clair_onlyfails on one snapshot entry, the md5 ofsample4_tumor.bam(the two-replicatesamtools mergeoutput), which is unrelated to this PR since the test profile skips modkit. That md5 looks order-dependent and could be excluded viatests/.nftignorein a follow-up.Note: the pipeline test profile sets
skip_modkit = true, so CI does not exercise this process. Thedownload_pipelinejob resolves the new container string, which requires the ghcr package to be public.PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).CHANGELOG.mdis updated.🤖 Generated with Claude Code