Skip to content

Conversation

@Saga4
Copy link
Contributor

@Saga4 Saga4 commented Jun 27, 2025

User description

  • Add no-pr argument to CF
  • Fix Optimizable Functions issue and add error logs

PR Type

Enhancement, Bug fix


Description

  • Add logs around optimizable functions calls

  • Save and restore optimizer.args state

  • Handle missing current function errors

  • Disable PRs in LSP server initialization


Changes diagram

flowchart LR
  Request["get_optimizable_functions request"]
  SetArgs["Set temporary optimizer.args"]
  CallGet["Call get_optimizable_functions"]
  BuildMap["Build path-to-names map"]
  Restore["Restore original optimizer.args"]
  Request --> SetArgs
  SetArgs --> CallGet
  CallGet --> BuildMap
  BuildMap --> Restore
  Restore --> Response["Return functions map"]
Loading

Changes walkthrough 📝

Relevant files
Error handling
beta.py
Enhance logging and state safety in beta                                 

codeflash/lsp/beta.py

  • Added info logs for optimization flow steps
  • Saved/restored optimizer.args to prevent corruption
  • Added error handling for missing functions
  • Cleared function filter post-optimization
  • +59/-8   
    Configuration changes
    server.py
    Disable PR creation in LSP server                                               

    codeflash/lsp/server.py

    • Set args.no_pr = True in optimizer init
    +1/-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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    State Restoration

    The restoration logic in get_optimizable_functions does not explicitly reset optimizer.args.file and previous_checkpoint_functions when their original values are None, potentially leaving stale state.

    finally:
        # Restore original args to prevent state corruption
        if original_file is not None:
            server.optimizer.args.file = original_file
        if original_function is not None:
            server.optimizer.args.function = original_function
        else:
            server.optimizer.args.function = None
        if original_checkpoint is not None:
            server.optimizer.args.previous_checkpoint_functions = original_checkpoint
    State Leakage

    The early exit in perform_function_optimization when no current function is set does not clear optimizer.args.function, causing it to persist across requests.

    if not current_function:
        server.show_message_log(f"No current function being optimized for {params.functionName}", "Error")
        return {"functionName": params.functionName, "status": "error", "message": "No function currently being optimized"}
    Verbose Logging

    A large number of show_message_log calls may flood logs and impact performance; consider reducing verbosity or batching logs.

    server.show_message_log(f"Getting optimizable functions for: {file_path}", "Info")
    
    # Save original args to restore later
    original_file = getattr(server.optimizer.args, 'file', None)
    original_function = getattr(server.optimizer.args, 'function', None)
    original_checkpoint = getattr(server.optimizer.args, 'previous_checkpoint_functions', None)
    
    server.show_message_log(f"Original args - file: {original_file}, function: {original_function}", "Info")
    
    try:
        # Set temporary args for this request only
        server.optimizer.args.file = file_path
        server.optimizer.args.function = None  # Always get ALL functions, not just one
        server.optimizer.args.previous_checkpoint_functions = False
    
        server.show_message_log("Calling get_optimizable_functions...", "Info")
        optimizable_funcs, _ = server.optimizer.get_optimizable_functions()
    
        path_to_qualified_names = {}
        for path, functions in optimizable_funcs.items():
            path_to_qualified_names[path.as_posix()] = [func.qualified_name for func in functions]
    
        server.show_message_log(f"Found {len(path_to_qualified_names)} files with functions: {path_to_qualified_names}", "Info")
        return path_to_qualified_names
    finally:
        # Restore original args to prevent state corruption
        if original_file is not None:
            server.optimizer.args.file = original_file
        if original_function is not None:
            server.optimizer.args.function = original_function
        else:
            server.optimizer.args.function = None
        if original_checkpoint is not None:
            server.optimizer.args.previous_checkpoint_functions = original_checkpoint
    
        server.show_message_log(f"Restored args - file: {server.optimizer.args.file}, function: {server.optimizer.args.function}", "Info")

    @Saga4 Saga4 requested review from KRRT7 and mohammedahmed18 and removed request for mohammedahmed18 June 27, 2025 23:31
    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Restore optimizer args unconditionally

    Always restore all three optimizer arguments unconditionally so that even None
    values are reapplied and no state leaks occur. Remove the conditional checks and
    assign back the saved originals directly.

    codeflash/lsp/beta.py [63-72]

     finally:
         # Restore original args to prevent state corruption
    -    if original_file is not None:
    -        server.optimizer.args.file = original_file
    -    if original_function is not None:
    -        server.optimizer.args.function = original_function
    -    else:
    -        server.optimizer.args.function = None
    -    if original_checkpoint is not None:
    -        server.optimizer.args.previous_checkpoint_functions = original_checkpoint
    +    server.optimizer.args.file = original_file
    +    server.optimizer.args.function = original_function
    +    server.optimizer.args.previous_checkpoint_functions = original_checkpoint
    Suggestion importance[1-10]: 7

    __

    Why: The unconditional restoration ensures that args.previous_checkpoint_functions doesn't remain mutated when the original was None, preventing state leaks and aligning with how the other args are handled.

    Medium
    Fully reset optimizer filters

    Also clear the file and previous_checkpoint_functions filters after optimization to
    fully reset optimizer state. This prevents leftover filters from affecting
    subsequent calls.

    codeflash/lsp/beta.py [272-274]

    -# CRITICAL: Clear the function filter after optimization to prevent state corruption
    +# CRITICAL: Clear optimizer filters after optimization to prevent state corruption
     server.optimizer.args.function = None
    -server.show_message_log("Cleared function filter to prevent state corruption", "Info")
    +server.optimizer.args.file = None
    +server.optimizer.args.previous_checkpoint_functions = False
    +server.show_message_log("Cleared optimizer filters to prevent state corruption", "Info")
    Suggestion importance[1-10]: 6

    __

    Why: Clearing file and previous_checkpoint_functions after optimization prevents residual state from affecting future calls, improving robustness in subsequent operations.

    Low

    @KRRT7
    Copy link
    Contributor

    KRRT7 commented Jun 27, 2025

    what cause this fix to be needed?

    @Saga4
    Copy link
    Contributor Author

    Saga4 commented Jun 30, 2025

    what cause this fix to be needed?

    The LSP server was modifying shared state of the object -> server.optimizer.args, without actually restoring it. When one request set args.function to optimize a specific function, that filter remained active for all subsequent requests, causing them to return incorrect results and thats why we just starting each request with clean state right now.
    The long term solution would be to fix the mutable state changes across all allocating locations in code or make each request creating own request args than sharing it across.

    @Saga4 Saga4 merged commit b759192 into main Jun 30, 2025
    16 checks passed
    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.

    2 participants