fix(modkit): patched modkit 0.6.4 for PacBio 5mC+5hmC reads, 5mC-CpG pileup defaults and --modkit_phased option - #188
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.
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
…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>
ljwharbers
changed the title
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
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
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>
Resolves the CHANGELOG and docs/usage.md conflicts from the nf-core template 4.1.0 merge (#183): kept both sides' changelog entries, and re-inserted the --modkit_args / --modkit_phased rows into dev's reflowed skip-options table. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The MODKIT_PILEUP container selection should respect task.ext.singularity_pull_docker_container (and the changelog should reflect the actual Singularity/Apptainer image URI) to avoid breaking established container-pull behavior and to keep documentation accurate.
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 MODKIT_PILEUP container selection ignores
task.ext.singularity_pull_docker_container. In this repo other modules (e.g. MOSDEPTH) use that flag to allow Singularity/Apptainer to pull the OCI image instead of requiring a native SIF/ORAS source; as written, users who set the flag will still be forced onto the ORAS SIF image, which can break environments without ORAS support.
// 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 "${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'}"
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The MODKIT_PILEUP container selection deviates from established container-handling conventions (not honoring task.ext.singularity_pull_docker_container), which can cause avoidable execution/configuration issues for Singularity/Apptainer users.
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 MODKIT_PILEUP container selection no longer follows the repository’s standard pattern of honoring
task.ext.singularity_pull_docker_container, which can prevent users running-profile singularity/apptainerfrom opting to pull the OCI image (and keeps this module inconsistent with other modules’ container handling).
// 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 "${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'}"
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
| | `--skip_m6a` | A boolean to skip `fibertools_m6a`, used if you have m6a calls but would still like nucleosome positions for PacBio data (ONT data is required to have m6a calls). Default = `false` | | ||
| | `--skip_nanoplot` | A boolean to skip NanoPlot QC on aligned and unaligned BAM files. Default = `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` | |
There was a problem hiding this comment.
Adjust --skip_modcall so it's clearer to what it points (modified calls being used in phasing) instead of modkit methylation calling. It's confusing with skip-modkit what is what
There was a problem hiding this comment.
Reworded in 613dfdd, in both docs/usage.md and nextflow_schema.json: "Skip Longphase modcall, the 5mC base-modification calling whose VCF is used as extra evidence during phasing. Unrelated to the modkit pileup (see --skip_modkit)."
|
Reviewed assisted by Claude. The container fix itself looks right, and the size problem it solves is real. Three things I'd want addressed before merge, plus a few smaller ones. 1.
|
…#188) - Restore the `!task.ext.singularity_pull_docker_container` escape hatch in the MODKIT_PILEUP container ternary (nf-core default and SIGPROFILER_ASSIGNMENT keep it), so sites where an oras:// pull is blocked can fall back to the Docker image. Module patch refreshed to match; the module comment now names the fork source. - Split the changelog: the container fix stays under Fixed and names both image URIs; the new `--cpg --modified-bases 5mC` default moves to Changed, with a note that reruns produce different bedMethyl content and how to restore the old output. - `modkit_args` docs/schema now say the value replaces the default rather than extending it. `--modkit_args ''` (and a bare `--modkit_args`) reach the config as the flag value `true`, which would have been passed to modkit verbatim; the ext.args closure now treats that as "no arguments" so the documented escape works. - `skip_modcall` description clarified: it skips Longphase modcall (phasing evidence), not the modkit pileup. - `modkit_phased` docs note the dependency on small-variant calling, phasing and haplotagging; `modkit_args`/`modkit_phased` moved out of the "Skipping options" table into a "Modkit options" section; output docs state there is no separate file for untagged reads. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-up on the smaller points, in 613dfdd:
The two small asks under point 2:
Also moved |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved Conda compatibility, arm64 image support, and CI coverage issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
modules/nf-core/modkit/pileup/main.nf:9
- This process still declares
conda "${moduleDir}/environment.yml", whose environment pins stockont-modkit=0.6.1(modules/nf-core/modkit/pileup/environment.yml:7). With the repository'sconda/mambaprofiles, Nextflow therefore uses that binary instead of this patched image, so the PacBio conflict fix and the phased/general-worker fixes are absent (and the updated module snapshot will report 0.6.1). Please provide a patched Conda environment or make this process explicitly image-only and document the supported profile.
workflows/lrsomatic.nf:671
- The pipeline nf-tests all use
conf/test.config, which setsskip_modkit = true, so neither this unphased input selection nor the newmodkit_phasedhaplotagged-channel path is exercised in CI. A regression in this conditional routing could therefore leave the full pipeline suite green; please add a lightweight preview/assertion or fixture covering both parameter values.
ch_modkit_input = params.modkit_phased
? PHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch
: ch_index_minimap
// ch_modkit_input: [meta, bam, bai] -- BAM to pile up; meta.type selects the publish directory
MODKIT_PILEUP(ch_modkit_input, ch_fasta, ch_fai, [[:],[]])
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Container compatibility and workflow/argument test coverage issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
conf/modules.config:338
- This new parameter-to-
ext.argscomposition is not covered by the current tests:conf/test.configskips modkit, while the module tests setparams.module_argsdirectly and bypassparams.modkit_args. Add assertions for the default, empty, and phased combinations so the documented CLI behavior is verified in CI.
ext.args = {
[
// Nextflow turns `--modkit_args ''` (and a bare `--modkit_args`) into the flag value `true`; treat that as "no arguments"
(params.modkit_args == null || params.modkit_args in [true, 'true']) ? '' : params.modkit_args,
params.modkit_phased ? '--phased' : ''
].join(' ').trim()
modules/nf-core/modkit/pileup/main.nf:9
- The referenced container workflow publishes only
linux/amd64, while this repository exposes anarm64profile. That profile will select this tag without a native arm64 image/SIF, so MODKIT_PILEUP cannot run natively on arm64. Add an arm64 build/tag or explicitly guard and document this process as unsupported on arm64.
? 'oras://ghcr.io/ljwharbers/modkit-sif:0.6.4-pacbiofix-6e0afa2'
: 'ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2'}"
workflows/lrsomatic.nf:673
- The pipeline test profile sets
skip_modkit=true, so CI never exercises this new channel selection, especially the haplotagged[meta, bam, bai]input whenmodkit_phasedis enabled. Add a small workflow test/stub or another automated assertion for both phased modes; otherwise a channel-shape regression here can pass the existing pipeline tests.
ch_modkit_input = params.modkit_phased
? PHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch
: ch_index_minimap
// ch_modkit_input: [meta, bam, bai] -- BAM to pile up; meta.type selects the publish directory
MODKIT_PILEUP(ch_modkit_input, ch_fasta, ch_fai, [[:],[]])
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
|
Result of the
Also merged |
|
Thanks — the smaller points all look right, and the The documented
|
| invocation | params.modkit_args |
result |
|---|---|---|
| (default) | String --cpg --modified-bases 5mC |
✅ ext.args = --cpg --modified-bases 5mC |
--modkit_args '' |
Boolean true |
❌ run aborts |
--modkit_args (bare) |
Boolean true |
❌ run aborts |
--modkit_args='' |
String `` |
✅ ext.args = (empty) |
--modkit_args ' ' |
String `` |
✅ ext.args = (empty) |
-params-file with "modkit_args": "" |
String `` |
✅ ext.args = (empty) |
--modkit_args '' --validate_params false |
Boolean true |
✅ guard converts it to empty |
The failure:
ERROR ~ Validation of pipeline parameters failed!
The following invalid input values have been detected:
* --modkit_args (true): Value is [boolean] but should be [string]
So the escape hatch now documented in docs/usage.md, nextflow_schema.json and the changelog fails from the CLI in the form it's written, and the in [true, 'true'] guard is only reachable with --validate_params false.
Two forms do work and are worth documenting instead: --modkit_args='' (the equals form keeps it a string) or --modkit_args ' ' — Nextflow trims the space to an empty string, and .trim() in the closure makes it a no-op either way. A -params-file entry works too. Keeping the boolean guard is still sensible as a backstop.
…rking --modkit_args escape (#188) Review follow-up for PR #188. - MODKIT_PILEUP: drop the conda directive and stop with an error under -profile conda/mamba (deepvariant/sigprofiler pattern). environment.yml would install stock ont-modkit 0.6.1, which silently reintroduces the PacBio read drop this PR fixes. Verified: the module test under -profile conda fails with the new message. - modules/nf-core/modkit/pileup/tests: the test passed three inputs to a four-input patched module (fasta/fai split), so it could not run and the 0.6.1 -> 0.6.4 snapshot edit had no run behind it. Inputs fixed for all ten cases; snapshot regenerated from a real run of the patched image on Mindwell (10/10 pass, versions 0.6.4, bedMethyl md5s follow the binary). Module patch refreshed (a trailing newline was added to the snapshot so the last hunk reverse-applies). - --modkit_args '' does not work from the CLI: Nextflow turns it into `true`, which nf-schema rejects as a boolean on Nextflow 25.10.4 and which arrives as the string 'true' on Nextflow 26 (verified with -preview on both). Docs, schema and changelog now say --modkit_args='' or a params file entry; the modules.config guard stays as a backstop. - docs/usage.md and CHANGELOG.md: the image is linux/amd64 only and conda and the arm64 profile are unsupported for this step (--skip_modkit); N_other note copied from the schema into the usage table. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks, confirmed and fixed in 272d1f8, together with the two points from this morning that were still open.
Conda (point 1): the module now drops the Module test (point 3): you were right, the test could not have run. All ten cases now pass the four-channel inputs ( Fiber-seq default (your optional point): left as is for now. The m6A signal for fiber-seq samples comes from the fibertools steps, and the pileup default is a size/scope choice that |
There was a problem hiding this comment.
🟡 Changes recommended
The Conda test guard blocks the required module-test matrix, and regression coverage plus container documentation reconciliation remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
conf/modules.config:339
- This new parameter wiring is not exercised by the pipeline tests:
conf/test.config:60setsskip_modkit = true, and the module tests replaceext.argswithparams.module_argsinmodules/nf-core/modkit/pileup/tests/nextflow.config, so neither the default/emptymodkit_argshandling nor themodkit_phasedchannel selection is verified. Please add a stub or preview-level regression test covering the default, custom/empty arguments, and phased input/output path.
ext.args = {
[
// Nextflow turns `--modkit_args ''` (and a bare `--modkit_args`) into `true`: a boolean that nf-schema rejects on
// Nextflow 25, the string 'true' on Nextflow 26. The documented form is `--modkit_args=''`; this backstop treats `true` as "no arguments"
(params.modkit_args == null || params.modkit_args in [true, 'true']) ? '' : params.modkit_args,
params.modkit_phased ? '--phased' : ''
].join(' ').trim()
workflows/lrsomatic.nf:673
- Could you add a pipeline-level stub/preview test for both values of
modkit_phased(and the default/custommodkit_args)? The pipeline tests loadconf/test.configwithskip_modkit = true, while the module tests injectparams.module_argsdirectly, so neither this channel selection nor theconf/modules.configargument-building closure is exercised. A regression could therefore send the wrong BAM or omit/duplicate--phasedwhile all current tests still pass.
ch_modkit_input = params.modkit_phased
? PHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch
: ch_index_minimap
// ch_modkit_input: [meta, bam, bai] -- BAM to pile up; meta.type selects the publish directory
MODKIT_PILEUP(ch_modkit_input, ch_fasta, ch_fai, [[:],[]])
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
| // Exit if running this module with -profile conda / -profile mamba | ||
| if (workflow.profile.tokenize(',').intersect(['conda', 'mamba']).size() >= 1) { | ||
| error "MODKIT_PILEUP does not support Conda: the PacBio fixes only exist in the patched container. Use Docker / Singularity / Apptainer, or --skip_modkit." |
| container "${(workflow.containerEngine == 'singularity' || workflow.containerEngine == 'apptainer') && !task.ext.singularity_pull_docker_container | ||
| ? '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_PILEUPruns a patched image built from that branch by this workflow:ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2under Docker, and the native SIForas://ghcr.io/ljwharbers/modkit-sif:0.6.4-pacbiofix-6e0afa2under Singularity/Apptainer (unlesstask.ext.singularity_pull_docker_containeris set, which falls back to the Docker image). The image islinux/amd64only, so thearm64profile is not supported for this step. Conda is not supported either: the module drops its conda directive and stops with an error under-profile conda/mamba(use--skip_modkitthere), becauseenvironment.ymlwould install stock ont-modkit 0.6.1 and silently reintroduce the bug this PR fixes.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): arguments passed tomodkit pileupviaext.args. The value replaces the default rather than extending it. The default restricts output to 5mC calls at CpG sites, per strand. To get the unfiltered pileup use--modkit_args=''(the=form) or an emptymodkit_argsentry in a params file:--modkit_args ''becomestrue, which nf-schema rejects as a boolean on Nextflow 25.10 and which arrives as the string'true'on Nextflow 26, whereconf/modules.configdiscards it. 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; conda directive removed and a conda/mamba guard added in the script block.modules/nf-core/modkit/pileup/tests/main.nf.test: inputs fixed for the four-channel module (the fasta/fai split predates this PR) so the test can run.modules/nf-core/modkit/pileup/tests/main.nf.test.snap: regenerated from a real run of the patched image (10/10 pass): versions0.6.4, bedMethyl md5s follow the binary.modules/nf-core/modkit/pileup/modkit-pileup.diff: patch refreshed so it carries the container change, the conda guard and the test changes next to the existing fasta/fai input split.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.Module test (
nf-test test modules/nf-core/modkit/pileup/tests/main.nf.test, Apptainer on Mindwell, 2026-09-14): 10/10 pass with the patched image and the snapshot was re-recorded from that run; under-profile condaevery case stops withMODKIT_PILEUP does not support Conda.-previewwith--modkit_args ''fails validation on Nextflow 25.10.4 (--modkit_args (true): Value is [boolean] but should be [string]) and passes on 26.04 withmodkit_args: true;--modkit_args=''passes on both with an empty value.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