Skip to content

Handle extra header for reads#246

Draft
sainsachiko wants to merge 20 commits into
devfrom
extra_header
Draft

Handle extra header for reads#246
sainsachiko wants to merge 20 commits into
devfrom
extra_header

Conversation

@sainsachiko

@sainsachiko sainsachiko commented May 29, 2026

Copy link
Copy Markdown
Contributor

Fixed this #229:

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).

Copilot AI review requested due to automatic review settings May 29, 2026 01:15
@sanger-tolsoft

Copy link
Copy Markdown
Contributor

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.5.2.
Please update your pipeline to the latest version.

For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation.

@github-actions

github-actions Bot commented May 29, 2026

Copy link
Copy Markdown

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

Posted for pipeline commit 6eb286b

+| ✅ 213 tests passed       |+
#| ❔  34 tests were ignored |#
!| ❗   9 tests had warnings |!
Details

❗ Test warnings:

❔ Tests ignored:

  • files_exist - File is ignored: CODE_OF_CONDUCT.md
  • files_exist - File is ignored: assets/multiqc_config.yml
  • files_exist - File is ignored: assets/nf-core-readmapping_logo_light.png
  • files_exist - File is ignored: assets/methods_description_template.yml
  • files_exist - File is ignored: conf/igenomes.config
  • files_exist - File is ignored: docs/images/nf-core-readmapping_logo_dark.png
  • files_exist - File is ignored: docs/images/nf-core-readmapping_logo_light.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: tower.yml
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • nextflow_config - Config default ignored: params.pacbio_adapter_fasta
  • nextflow_config - Config default ignored: params.pacbio_adapter_yaml
  • files_unchanged - File ignored due to lint config: .gitattributes
  • files_unchanged - File does not exist: CODE_OF_CONDUCT.md
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File does not exist: .github/ISSUE_TEMPLATE/config.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.html
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: assets/nf-core-readmapping_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-readmapping_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-readmapping_logo_dark.png
  • files_unchanged - File ignored due to lint config: docs/README.md
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore
  • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/readmapping/readmapping/.github/workflows/awstest.yml
  • multiqc_config - multiqc_config

✅ Tests passed:

Run details

  • nf-core/tools version 3.5.2
  • Run at 2026-06-04 19:31:31

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings May 29, 2026 01:45

This comment was marked as resolved.

Copilot AI review requested due to automatic review settings May 29, 2026 03:13

This comment was marked as resolved.

@muffato muffato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you pay attention to the final headers, you'll notice that the insertion of the extra @PG headers creates a parallel program chain. From that point on, each command (samtools reheader, samtools view, samtools reheader) is inserted twice: one connected to the pre-existing chain, one connected to the inserted one.
The workaround is to insert a PP tag on the first @PG line that is in the BAM file, and link it to the last ID value from the header being inserted.

Secondly, there are now two reheader jobs happening from CRAM output. Each one creates a new file which makes a lot of I/O. I would suggest the following:

  1. Make SAMTOOLS_CRAM (SAMTOOLS_VIEW) accept an optional header to be able embed SAMTOOLS_REHEADER_CRAM. Here, the principle is that the complete output header is the input header without its @SQ lines and adding the @SQ lines from the extra header. I'd like that because this reheader is essentially about fixing the @SQ lines samtools view is created wrong.
  2. Make the sanger-tol FASTX_MAP_LONG_READS accept an optional header file of @PG lines and slot a samtools reheader command between minimap2 and samtools sort. subworkflows/local/align_long.nf could prepare that file (add the .extra_header suffix etc) beforehand in a new module. Connecting this to my first comment, you could use the fact minimap2 inserts a new root @PG and connect the PG chains there.
    It might make sense to expand the PREPARE_READ_GROUPS sub-workflow into a PREPARE_HEADERS for this ?
  3. Similar fix for the sanger-tol short read sub-workflows

Think about it, see if it makes sense

Comment thread .github/workflows/download_pipeline.yml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test a samplesheet without the extra_header column

Comment thread subworkflows/local/input_check.nf Outdated
Comment thread .github/workflows/download_pipeline.yml Outdated
sainsachiko and others added 5 commits June 3, 2026 23:10
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Matthieu Muffato <cortexspam-github@yahoo.fr>
Co-authored-by: Hanh Hoang <134130358+sainsachiko@users.noreply.github.com>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oh I didn't think it'd make the module that complex ! Some comments:

  • the whole point is to avoid having multiple copies of the BAM/CRAM files written on disk where just the header differs. I was hoping samtools view could pipe into samtools reheader. If they have to be separate commands, they might as well be different modules, there's no difference I/O wise (there's a difference LSF wise but not worth losing the nf-core modularity).
  • I can see you're running samtools index separately. Can it not be kept in samtools view ?
  • Do we do reheader + unselected at the same time ? Seeing all those if, I wonder if it'd be cleaner to keep the original nf-core/samtools/view module and create a new one local/samtools/viewreheader

@muffato muffato marked this pull request as draft June 9, 2026 13:27
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.

4 participants