Skip to content

fix(modkit): patched modkit 0.6.4 for PacBio 5mC+5hmC reads, 5mC-CpG pileup defaults and --modkit_phased option - #188

Draft
ljwharbers wants to merge 9 commits into
devfrom
fix/modkit-pacbio-container
Draft

fix(modkit): patched modkit 0.6.4 for PacBio 5mC+5hmC reads, 5mC-CpG pileup defaults and --modkit_phased option#188
ljwharbers wants to merge 9 commits into
devfrom
fix/modkit-pacbio-container

Conversation

@ljwharbers

@ljwharbers ljwharbers commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

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 in modkit pileup (nanoporetech/modkit#612). Two further 0.6.1+ pileup bugs made --cpg pileups 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_PILEUP runs ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-6e0afa2, built from that branch by this workflow. The same string serves Docker and Singularity/Apptainer (Nextflow adds docker://).

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 --phased partition (empty _hp1/_hp2) and did not apply --modified-bases as a row filter (an h row next to every m row). 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 pileup ran 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 to modkit pileup via ext.args. The default restricts output to 5mC calls at CpG sites, per strand. Note that --modified-bases only 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 in N_other. Resolving those positions (adjust-mods --ignore h, or an --ignore flag on plain pileup in the fork) is a follow-up.
  • --modkit_phased (boolean, default false): 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 before. modkit then writes <sample>_hp1.bed.gz, <sample>_hp2.bed.gz and <sample>_combined.bed.gz; the nf-core module already handles the directory output, so no module change was needed. The MODKIT_PILEUP call 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_phased in a new "Modkit options" group.
  • conf/modules.config: ext.args for MODKIT_PILEUP.
  • workflows/lrsomatic.nf: pileup input selection, call moved after PHASING_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 h writes 2,687 records (stock: 1,035), pileup --cpg gives 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 -preview with -profile test,singularity succeeds with modkit_phased on and off; the DAG shows MODKIT_PILEUP fed by LONGPHASE_HAPLOTAG + SAMTOOLS_INDEX in phased mode and by the merged alignment BAM otherwise. pre-commit and nf-core pipelines lint (schema/config keys) pass.

Real-data -resume reruns (same 3 samples and work dir as above; the three MODKIT_PILEUP tasks re-ran, ~30 min each for the phased run with the 6e0afa2 image on 2026-09-04):

Sample Unfiltered (before) --cpg --modified-bases 5mC phased _hp1 / _hp2 / _combined rows (image 697de7b) phased _hp1 / _hp2 / _combined rows (image 6e0afa2)
BL1_ont 45 GB 1.0 GB, m rows only 53.7 M / 54.9 M / 66.2 M 53.7 M / 54.9 M / 66.2 M (unchanged)
DLBCL16_oldpb 39 GB 852 MB 0 / 0 / 64.0 M 49.3 M / 51.1 M / 64.0 M
FL11_newpb 30 GB 1.6 GB 0 / 0 / 127.1 M (h + m rows) 51.0 M / 52.2 M / 63.5 M (m rows only)

With the 6e0afa2 image every file of every sample contains only m rows, and the PacBio haplotype files are populated (the haplotagged PacBio BAMs carry HP/PS on 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 with h rows) / 64.0 M (DLBCL16) / 66.2 M (BL1) m rows. Logs show producing phased output, parsed 1 base modification(s) ... counted as 'N_other', and for PacBio using general workers plus (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 _combined is identical to the unphased run.

nf-test (--profile=+singularity, all five pipeline tests): 4 pass; clair_only fails on one snapshot entry, the md5 of sample4_tumor.bam (the two-replicate samtools merge output), which is unrelated to this PR since the test profile skips modkit. That md5 looks order-dependent and could be excluded via tests/.nftignore in a follow-up.

Note: the pipeline test profile sets skip_modkit = true, so CI does not exercise this process. The download_pipeline job resolves the new container string, which requires the ghcr package to be public.

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! (no methylation-tagged test data in the test profile)
  • 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>).
  • CHANGELOG.md is updated.

🤖 Generated with Claude Code

… 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>
Copilot AI lite review requested due to automatic review settings September 2, 2026 14:18
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit a43f844

+| ✅ 204 tests passed       |+
#| ❔  22 tests were ignored |#
!| ❗  38 tests had warnings |!
Details

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 1.1.0
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in lint_log.txt: Named file extensions MUST be emitted for ALL output channels
  • pipeline_todos - TODO string in lint_log.txt: List additional required output channels/values here
  • pipeline_todos - TODO string in README.md: Include a figure that guides the user through the major workflow steps. Many nf-core
  • pipeline_todos - TODO string in nextflow.config: Specify your pipeline's command line flags
  • pipeline_todos - TODO string in nextflow.config: Update the field with the details of the contributors to your pipeline. New with Nextflow version 24.10.0
  • pipeline_todos - TODO string in nextflow.config: Specify any additional parameters here
  • pipeline_todos - TODO string in CONTRIBUTING.md: Add any pipeline specific contribution guidelines here, such as coding styles, procedures, checklists etc.
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in base.config: Customise requirements for specific processes.
  • pipeline_todos - TODO string in meta.yml: #Add a description of the module and list keywords
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • schema_description - Ungrouped param in schema: skip_modkit
  • schema_description - No description provided in schema for parameter: generate_gvcf
  • schema_description - No description provided in schema for parameter: autocorrelation
  • schema_description - No description provided in schema for parameter: vep_custom
  • schema_description - No description provided in schema for parameter: vep_custom_tbi
  • schema_description - No description provided in schema for parameter: severus_minsupport
  • schema_description - No description provided in schema for parameter: wakhan_chroms
  • local_component_structure - deepsomatic.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - prepare_reference_files.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - small_variant_consensus.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - prepare_annotation.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure
  • local_component_structure - phasing_haplotyping.nf in subworkflows/local should be moved to a SUBWORKFLOW_NAME/main.nf structure

❔ Tests ignored:

  • files_exist - File is ignored: CODE_OF_CONDUCT.md
  • files_exist - File is ignored: assets/nf-core-lrsomatic_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-lrsomatic_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-lrsomatic_logo_dark.png
  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • files_exist - File is ignored: .github/CONTRIBUTING.md
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • files_unchanged - File ignored due to lint config: CODE_OF_CONDUCT.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: .github/workflows/branch.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting_comment.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/email_template.txt
  • files_unchanged - File ignored due to lint config: assets/nf-core-lrsomatic_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-lrsomatic_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-lrsomatic_logo_dark.png
  • files_unchanged - File ignored due to lint config: docs/README.md
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/lrsomatic/lrsomatic/.github/workflows/awstest.yml

✅ Tests passed:

Run details

  • nf-core/tools version 4.0.2
  • Run at 2026-09-04 12:20:12

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.

🟡 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_PILEUP to a patched ghcr.io/ljwharbers/modkit:0.6.4-pacbiofix-697de7b container 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.4 in 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.

Comment thread modules/nf-core/modkit/pileup/main.nf Outdated
Comment on lines +5 to +9
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'
Copilot AI review requested due to automatic review settings September 2, 2026 14:23

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.

🟡 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 --cpg behavior 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

Comment thread modules/nf-core/modkit/pileup/main.nf Outdated
Comment on lines +6 to +9
// 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'
Comment thread CHANGELOG.md Outdated

### `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).
ljwharbers and others added 2 commits September 3, 2026 13:22
…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>
Copilot AI review requested due to automatic review settings September 3, 2026 11:22
@ljwharbers ljwharbers changed the title fix(modkit): run a patched modkit 0.6.4 that keeps PacBio reads with 5mC+5hmC > 1 fix(modkit): patched modkit 0.6.4 for PacBio 5mC+5hmC reads, 5mC-CpG pileup defaults and --modkit_phased option Sep 3, 2026

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.

🟡 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_PILEUP now hard-codes a patched modkit 0.6.4 container, but the process still declares a conda env via environment.yml, which currently pins ont-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

Comment thread nextflow.config
Comment on lines +28 to +30
// Modkit options
modkit_args = '--cpg --modified-bases 5mC'
modkit_phased = false
Comment thread docs/usage.md
| `--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` |
Comment thread nextflow_schema.json
"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>
Copilot AI review requested due to automatic review settings September 3, 2026 12:51

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.

🔵 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_PILEUP now hard-codes a GHCR container tag without a docker:// prefix or any Singularity/Apptainer handling. In this codebase most modules avoid relying on OCI auto-pull in the singularity profile (e.g. by using https://depot.galaxyproject.org/singularity/...), so this change risks breaking -profile singularity runs 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>
Copilot AI review requested due to automatic review settings September 4, 2026 09:20
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

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.

🟡 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_args and --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

Comment thread modules/nf-core/modkit/pileup/main.nf Outdated
Comment on lines +6 to +7
// 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'
Copilot AI review requested due to automatic review settings September 4, 2026 09:24

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.

🔵 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_args is described as “additional arguments”, but --phased is also appended automatically when modkit_phased is true and the workflow switches inputs based on that boolean. If a user includes --phased inside modkit_args, it can (a) get duplicated and/or (b) produce confusing behavior if modkit_phased is false (phased args on unphased BAM). Stripping --phased from modkit_args here prevents that inconsistency and reserves phasing control for modkit_phased.

modules/nf-core/modkit/pileup/main.nf:7

  • MODKIT_PILEUP now hardcodes a patched 0.6.4 container, but the module still defines a conda env (environment.yml) that pins ont-modkit=0.6.1. Running with -profile conda/mamba would 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 container directive no longer follows the repo’s standard engine-aware pattern, and for Singularity/Apptainer it may be safer to use an explicit docker:// 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>
Copilot AI review requested due to automatic review settings September 4, 2026 10:07

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.

🟡 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_args help text here omits the N_other behavior that’s described in the schema (and can surprise users on PacBio data): --modified-bases filters output rows, but non-conflicting 5hmC calls can still contribute to N_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

Comment thread modules/nf-core/modkit/pileup/main.nf Outdated
'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>
Copilot AI review requested due to automatic review settings September 4, 2026 12:17

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.

🟡 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_input without 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 with def to keep it local to this workflow scope.
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment on lines +7 to +9
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'}"
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.

2 participants