Skip to content

fix: correct intervals channel for parabricks #2029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Parent: Release 3.6.1
merged 34 commits into from
Oct 23, 2025
Merged

fix: correct intervals channel for parabricks #2029

merged 34 commits into from
Oct 23, 2025

Conversation

@famosab
Copy link
Contributor

@famosab famosab commented Oct 15, 2025

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
  • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repository.
  • 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).

@nf-core-bot
Copy link
Member

Warning

Newer version of the nf-core template is available.

Your pipeline is using an old version of the nf-core template: 3.3.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
Copy link

github-actions bot commented Oct 15, 2025 •
edited
Loading

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

Posted for pipeline commit dd5b4db

+| ✅ 225 tests passed       |+
#| ❔  13 tests were ignored |#
!| ❗   8 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: 3.6.1
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • schema_lint - Input mimetype is missing or empty
  • schema_description - No description provided in schema for parameter: markduplicates_pixel_distance
  • schema_description - No description provided in schema for parameter: gatk_pcr_indel_model

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.3.2
  • Run at 2025-10-23 08:05:34

famosab
famosab commented Oct 16, 2025
View reviewed changes
workflows/sarek/main.nf
fasta,
index_alignment,
intervals_and_num_intervals,
intervals_bed_combined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FriederikeHanssen @maxulysse Do you think this is the correct intervals file? I am a little lost but that seems like the one that makes the most sense? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I think so

Did you double check that it is doing the scatter/gather when reaching VC? I think it should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I keep getting stuck on the BQSR step within parabricks ... currently testing align only but in my run the TABIX_BGZIPTABIX_INTERVAL_SPLIT process still gets triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense because possibly some downstream variantcalling tools need it? What tools did you specify? (I don't think we are doing very fine grained control there at the moment in general tbh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically everything that does somatic variant calling.

famosab
famosab commented Oct 16, 2025
View reviewed changes
@famosab famosab linked an issue Oct 16, 2025 that may be closed by this pull request
parabricks only works reliable with --no_intervals #2028
Open
@famosab
Copy link
Contributor Author

famosab commented Oct 16, 2025

So I tried testing these changes and at least for me that resolves the issue but I ran into another problem where I got stuck in the BQSR step within fq2bam and I could not figure out why... I think it would be good if we run this one with full_size data once at least somewhere else and hope that this does only happen to me. I also contacted @gburnett-nvidia but am not sure if he could also reproduce that issue. For more context see https://nfcore.slack.com/archives/C04SDJYV12L/p1760614591495309?thread_ts=1753441237.186579&cid=C04SDJYV12L

famosab
famosab commented Oct 20, 2025
View reviewed changes
@FriederikeHanssen
Copy link
Contributor

https://github.com/nf-core/sarek/blob/master/conf/base.config#L76

withName: 'FQ2BAM '{
    accelerator = 4 // maybe, double check with nvidia ref implementation or benchmarking block
}

@famosab famosab mentioned this pull request Oct 21, 2025
Closed
@famosab
Copy link
Contributor Author

famosab commented Oct 21, 2025

I just need to update the snaps and then we are good to go I think :)

FriederikeHanssen
FriederikeHanssen reviewed Oct 21, 2025
View reviewed changes
FriederikeHanssen
FriederikeHanssen reviewed Oct 21, 2025
View reviewed changes
conf/test_full.config
time = { 1.h * task.attempt }
resourceLimits = [cpus: 96, memory: 370.GB]
maxRetries = 3
errorStrategy = { task.exitStatus in [1,2,143,137,104,134,139,255] ? 'retry' : 'finish' }
Copy link
Contributor

Choose a reason for hiding this comment

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

we ar eloosing the snapshot retry here. Do we need all these error codes @edmundmiller ?

FriederikeHanssen
FriederikeHanssen reviewed Oct 21, 2025
View reviewed changes
famosab
famosab commented Oct 23, 2025
View reviewed changes
famosab
famosab commented Oct 23, 2025
View reviewed changes
conf/base.config Outdated
memory = { 30.GB * task.attempt }
}
withName: 'PARABRICKS_FQ2BAM' {
accelerator = { task.executor in ['awsbatch'] ? 4 : null }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have other executors where we need the accelerator directive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These executors list the accelerator directive here https://www.nextflow.io/docs/latest/executor.html

google-batch, hq, k8s

should we just add them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them now

@famosab famosab merged commit 65fd802 into dev Oct 23, 2025
41 checks passed
@famosab famosab deleted the fix-parabricks-intervals branch October 23, 2025 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FriederikeHanssen FriederikeHanssen FriederikeHanssen approved these changes

@maxulysse maxulysse maxulysse approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

parabricks only works reliable with --no_intervals

4 participants

@famosab @nf-core-bot @FriederikeHanssen @maxulysse