Skip to content
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

fixed msgfdb index memory bug #420

Merged
merged 5 commits into from
Sep 19, 2024
Merged

fixed msgfdb index memory bug #420

merged 5 commits into from
Sep 19, 2024

Conversation

daichengxin
Copy link
Collaborator

@daichengxin daichengxin commented Sep 19, 2024 •
edited by codiumai-pr-agent-pro bot
Loading

User description

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/quantms branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core 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).

PR Type

Bug fix

Description

  • Fixed a memory allocation bug in the MSGFDBINDEXING process by adding -Xmx and -Xms parameters to the msgf_plus command.
  • The memory settings are now dynamically set based on the task.memory value, improving resource management.

Changes walkthrough 📝

Relevant files
Bug fix
main.nf
Fix memory allocation in MSGFDBINDEXING process                   

modules/local/openms/thirdparty/msgfdb_indexing/main.nf

  • Added memory allocation parameters -Xmx and -Xms to the msgf_plus
    command.
  • Ensured memory settings are dynamically set based on task.memory.
    		•  +2/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Memory Allocation
    The PR adds memory allocation parameters to the msgf_plus command, but it's unclear if this change has been tested with various memory configurations to ensure it works as expected.

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    codiumai-pr-agent-pro bot commented Sep 19, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a safeguard to prevent memory allocation from exceeding system limits

    Add error handling to ensure that the memory allocation doesn't exceed the available
    system memory. This can prevent potential out-of-memory errors.

    modules/local/openms/thirdparty/msgfdb_indexing/main.nf [25-26]

    --Xmx ${task.memory.toMega()} \
--Xms ${task.memory.toMega()} \
+-Xmx ${Math.min(task.memory.toMega(), Runtime.getRuntime().maxMemory() / (1024 * 1024))} \
+-Xms ${Math.min(task.memory.toMega(), Runtime.getRuntime().maxMemory() / (1024 * 1024)) / 4} \
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential issue by adding error handling to prevent memory allocation from exceeding system limits, which is crucial for avoiding out-of-memory errors.

    		9
    Performance
    Adjust the initial heap size to a fraction of the maximum heap size for better memory management

    Consider using a fraction of the total memory for the initial heap size (-Xms) to
    allow for better memory management. A common practice is to set it to 25% of the
    maximum heap size.

    modules/local/openms/thirdparty/msgfdb_indexing/main.nf [25-26]

     -Xmx ${task.memory.toMega()} \
--Xms ${task.memory.toMega()} \
+-Xms ${task.memory.toMega() / 4} \
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion improves memory management by setting the initial heap size to 25% of the maximum, which is a common practice and can lead to better performance and resource utilization.

    		8
    Maintainability
    Use a more universal method for memory unit conversion to ensure cross-system compatibility

    Consider adding memory unit conversion to ensure consistency across different
    systems. The toMega() method might not be available on all Nextflow versions or
    configurations.

    modules/local/openms/thirdparty/msgfdb_indexing/main.nf [25-26]

    --Xmx ${task.memory.toMega()} \
--Xms ${task.memory.toMega()} \
+-Xmx ${task.memory.toBytes() / (1024 * 1024)}m \
+-Xms ${task.memory.toBytes() / (1024 * 1024) / 4}m \
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances maintainability by using a more universal method for memory unit conversion, ensuring compatibility across different systems and Nextflow versions.

    		7

    💡 Need additional feedback ? start a PR chat

    @github-actions GitHub Actions
    Copy link

    github-actions bot commented Sep 19, 2024 •
    edited
    Loading

    nf-core lint overall result: Passed ✅

    Posted for pipeline commit 5e685df

    +| ✅ 278 tests passed       |+
#| ❔  10 tests were ignored |#

    ❔ Tests ignored:

    • files_exist - File is ignored: conf/igenomes.config
    • files_exist - File is ignored: conf/test_full.config
    • files_exist - File is ignored: conf/test.config
    • files_exist - File is ignored: .github/workflows/awstest.yml
    • files_exist - File is ignored: .github/workflows/awsfulltest.yml
    • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
    • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
    • files_unchanged - File ignored due to lint config: docs/README.md
    • actions_awstest - 'awstest.yml' workflow not found: /home/runner/work/quantms/quantms/.github/workflows/awstest.yml
    • multiqc_config - multiqc_config

    ✅ Tests passed:

    Run details

    • nf-core/tools version 2.14.1
    • Run at 2024-09-19 11:46:21

    @ypriverol ypriverol merged commit 7734f96 into bigbio:dev Sep 19, 2024
    17 checks passed
    @ypriverol ypriverol mentioned this pull request Sep 20, 2024
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @ypriverol ypriverol ypriverol approved these changes

    Assignees
    No one assigned
    Labels
    Bug fix Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @daichengxin @ypriverol