Skip to content

give convert_BAM2Pileup 5G - #354

Merged
sorelfitzgibbon merged 4 commits into
mainfrom
sfitz-update-convert_BAM2Pileup-memory
Aug 18, 2025
Merged

give convert_BAM2Pileup 5G#354
sorelfitzgibbon merged 4 commits into
mainfrom
sfitz-update-convert_BAM2Pileup-memory

Conversation

@sorelfitzgibbon

Copy link
Copy Markdown
Contributor

Description

22 out of 149 (15%) of recent call-sSNV runs had out of memory errors for somaticsniper:convert_BAM2Pileup_SAMtool. They all worked on automatic retry. The initial (failed) runs took an average of 1 hr 47 min. Looking at the trace.txt files, all but one used less than 5 GB in the successful retry and that one was barely over and might have worked with 5 GB.

I haven't tested it because it doesn't seem worth running it on an F72.

Checklist

  • I have read the code review guidelines and the code review best practice on GitHub check-list.

  • I have reviewed the Nextflow pipeline standards.

  • The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].

  • I have set up or verified the branch protection rule following the github standards before opening this pull request.

  • I have added my name to the contributors listings in the manifest block in the nextflow.config as part of this pull request, am listed already, or do not wish to be listed. (This acknowledgement is optional.)

  • I have added the changes included in this pull request to the CHANGELOG.md under the next release version or unreleased, and updated the date.

  • I have updated the version number in the metadata.yaml and manifest block of the nextflow.config file following semver, or the version number has already been updated. (Leave it unchecked if you are unsure about new version number and discuss it with the infrastructure team in this PR.)

  • I have tested the pipeline on at least one A-mini sample.

Copilot AI review requested due to automatic review settings August 13, 2025 23:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR increases the memory allocation for the convert_BAM2Pileup process from 2 GB to 5 GB to address out-of-memory errors that were occurring in 15% of recent runs.

  • Increases minimum and maximum memory allocation from 2 GB to 5 GB for convert_BAM2Pileup
  • Updates memory fraction from 0.01 to 0.07 to reflect the increased allocation
  • Documents the change in the CHANGELOG.md

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
config/resources.json Updates memory configuration for convert_BAM2Pileup process
CHANGELOG.md Documents the memory allocation increase

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Comment thread config/resources.json Outdated
"fraction": 0.01,
"max": "2 GB"
"min": "5 GB",
"fraction": 0.07,

Copilot AI Aug 13, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] The memory fraction of 0.07 appears arbitrary and may not accurately represent the relationship between the 5 GB allocation and total available memory. Consider documenting the calculation basis or using a fraction that reflects actual memory requirements relative to system capacity.

Copilot uses AI. Check for mistakes.
Comment thread config/resources.json
"min": "2 GB",
"fraction": 0.01,
"max": "2 GB"
"min": "5 GB",

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.

suggestion: It may be worth increasing the value in the default profile as well

@yashpatel6 yashpatel6 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.

Looks good!

@sorelfitzgibbon
sorelfitzgibbon merged commit 7adf149 into main Aug 18, 2025
7 checks passed
@sorelfitzgibbon
sorelfitzgibbon deleted the sfitz-update-convert_BAM2Pileup-memory branch August 18, 2025 22:05
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.

3 participants