Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Apr 11, 2025

PR Type

  • Enhancement

Description

  • Add system memory-based limit settings.

  • Use platform checks for Linux and Darwin.

  • Set resource limit to 80% of total memory.


Changes walkthrough 📝

Relevant files
Enhancement
pytest_plugin.py
Add dynamic memory limit in pytest plugin                               

codeflash/verification/pytest_plugin.py

  • Import platform, resource, and os modules.
  • Compute total system memory dynamically.
  • Set both soft and hard memory limits.
  • +13/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    github-actions bot commented Apr 11, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit eeac1c0)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Memory Limit Safety

    Ensure that the memory limit calculation and resource limit application are robust and consider possible failures of system calls. Adding error handling could prevent unexpected crashes in edge cases.

    import platform
    if platform.system() == 'Linux' or platform.system() == 'Darwin':
        import resource
        import os
    
        # Get total system memory
        total_memory = os.sysconf('SC_PAGE_SIZE') * os.sysconf('SC_PHYS_PAGES')    # Set memory limit to 80% of total system memory
        memory_limit = int(total_memory * 0.8)
    
        # Set both soft and hard limits
        resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    

    @github-actions
    Copy link

    github-actions bot commented Apr 11, 2025

    PR Code Suggestions ✨

    Latest suggestions up to eeac1c0
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Cache platform system call

    Cache the result of platform.system() in a variable to avoid redundant calls.

    codeflash/verification/pytest_plugin.py [40-49]

    -if platform.system() == 'Linux' or platform.system() == 'Darwin':
    +sys_platform = platform.system()
    +if sys_platform == 'Linux' or sys_platform == 'Darwin':
         import resource
         import os
         # Get total system memory
         total_memory = os.sysconf('SC_PAGE_SIZE') * os.sysconf('SC_PHYS_PAGES')    # Set memory limit to 80% of total system memory
         memory_limit = int(total_memory * 0.8)
         # Set both soft and hard limits
         resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    Suggestion importance[1-10]: 5

    __

    Why: Caching the result of platform.system() avoids redundant calls and minor overhead, improving efficiency and readability, though the change has only moderate impact.

    Low
    Possible issue
    Add safe exception handling

    Wrap the resource.setrlimit call in a try/except block to safely handle potential
    runtime errors.

    codeflash/verification/pytest_plugin.py [49]

    -resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    +try:
    +    resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    +except Exception:
    +    pass
    Suggestion importance[1-10]: 4

    __

    Why: While adding a try/except block around the resource.setrlimit call may help prevent runtime crashes, the broad exception handling silently swallows errors, which could mask significant issues.

    Low

    Previous suggestions

    Suggestions up to commit b6bd030
    CategorySuggestion                                                                                                                                    Impact
    General
    Optimize platform check

    Cache the result of platform.system() in a variable and check membership in a tuple
    to avoid duplicate function calls.

    codeflash/verification/pytest_plugin.py [39-50]

     import platform
    -if platform.system() == 'Linux' or platform.system() == 'Darwin':
    +sys_platform = platform.system()
    +if sys_platform in ('Linux', 'Darwin'):
         import resource
         import psutil
         # Get total system memory
         total_memory = psutil.virtual_memory().total
         # Set memory limit to 80% of total system memory
         memory_limit = int(total_memory * 0.8)
         # Set both soft and hard limits
         resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    Suggestion importance[1-10]: 7

    __

    Why: Caching the result of platform.system() avoids duplicate calls and improves performance with a cleaner check, and the improved code accurately reflects the intended changes.

    Medium
    Possible issue
    Add robust error handling

    Wrap the memory limit setting in a try-except block to gracefully handle any errors
    during resource limit configuration.

    codeflash/verification/pytest_plugin.py [49-50]

     # Set both soft and hard limits
    -resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    +try:
    +    resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    +except Exception:
    +    pass  # Optionally log the error or handle it appropriately.
    Suggestion importance[1-10]: 5

    __

    Why: Wrapping resource.setrlimit in a try-except block can prevent crashes from unexpected errors; however, silently passing the exception might hide underlying issues, making this a moderate improvement.

    Low

    @misrasaurabh1 misrasaurabh1 marked this pull request as ready for review April 12, 2025 00:01
    @github-actions
    Copy link

    Persistent review updated to latest commit eeac1c0

    @aseembits93 aseembits93 enabled auto-merge April 12, 2025 00:04
    @aseembits93 aseembits93 disabled auto-merge April 12, 2025 00:08
    @aseembits93
    Copy link
    Contributor

    good to go, enabling automerge

    @aseembits93 aseembits93 enabled auto-merge April 12, 2025 00:23
    Copy link
    Contributor

    @aseembits93 aseembits93 left a comment

    Choose a reason for hiding this comment

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

    good to go

    @misrasaurabh1 misrasaurabh1 disabled auto-merge April 12, 2025 01:14
    @misrasaurabh1 misrasaurabh1 merged commit c1302e1 into main Apr 12, 2025
    7 checks passed
    @aseembits93 aseembits93 deleted the prevent-memory-crashes branch April 23, 2025 19:33
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants