Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented May 1, 2025

PR Type

Bug fix, Enhancement


Description

  • Increase noise floor multiplier for very small runtimes

  • Skip non-existent files in optimization discovery

  • Return None instead of exception on AST parse failure

  • Ignore None in replay test to prevent crashes


Changes walkthrough 📝

Relevant files
Error handling
functions_to_optimize.py
Add file existence check and parse error handling               

codeflash/discovery/functions_to_optimize.py

  • Skip function entries when file doesn't exist
  • Return None on AST parse exceptions
  • +5/-4     
    Enhancement
    critic.py
    Increase performance noise floor threshold                             

    codeflash/result/critic.py

    • Increase noise floor multiplier to 3x for runtimes<10000µs
    +1/-1     
    Bug fix
    replay_test.py
    Handle missing function properties in tests                           

    codeflash/tracing/replay_test.py

  • Support None values in function_properties list
  • Skip None entries in replay test generation
  • +5/-1     

    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 May 1, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 3cb6cb0)

    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

    Silent Failure

    AST parse exceptions are swallowed and return None without logging, which can hide parsing issues and make debugging difficult.

    ) -> FunctionProperties | None:
        with open(file_name, encoding="utf8") as file:
            try:
                ast_module = ast.parse(file.read())
            except Exception:
                return None
    Magic Number

    The noise floor multiplier is hardcoded as 3 for small runtimes; consider defining a named constant or documenting the rationale and units.

    noise_floor = 3 * MIN_IMPROVEMENT_THRESHOLD if original_code_runtime < 10000 else MIN_IMPROVEMENT_THRESHOLD
    Silent Skip

    Skipped function properties when None with no warning; this may hide missing or unparsed functions and should be logged or surfaced to the user.

    if function_property is None:
        continue

    @github-actions
    Copy link

    github-actions bot commented May 1, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 3cb6cb0
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Log missing file paths

    Add a log warning when a file path doesn't exist to aid debugging and avoid silent
    skips. This will help trace missing modules in the discovery process.

    codeflash/discovery/functions_to_optimize.py [291-292]

     if not file_path.exists():
    +    logger.warning(f"File not found, skipping: {file_path}")
         continue
    Suggestion importance[1-10]: 6

    __

    Why: Adding a warning via logger.warning improves visibility into which files are skipped without changing logic, aiding debugging.

    Low
    Log parsing exceptions

    Log the exception when parsing fails to provide context for errors and make
    debugging easier. Retain the return of None while capturing the error.

    codeflash/discovery/functions_to_optimize.py [398-399]

    -except Exception:
    +except Exception as e:
    +    logger.exception(f"Failed to parse AST for {file_name}: {e}")
         return None
    Suggestion importance[1-10]: 6

    __

    Why: Capturing and logging the exception when AST parsing fails provides context for errors while preserving the intended return behavior.

    Low

    Previous suggestions

    Suggestions up to commit c3b74ae
    CategorySuggestion                                                                                                                                    Impact
    General
    Log parsing errors

    Log the exception before returning to aid debugging of parsing failures. Use
    logger.exception to capture stack trace and file context.

    codeflash/discovery/functions_to_optimize.py [396-397]

    -except Exception:
    +except Exception as e:
    +    logger.exception("Failed to parse AST for %s: %s", file_name, e)
         return None
    Suggestion importance[1-10]: 6

    __

    Why: Logging the swallowed exception with logger.exception improves debugging without affecting functionality.

    Low
    Filter valid functions once

    Consolidate and filter valid function pairs once before looping to eliminate
    redundant None and attribute checks. This simplifies the logic and improves
    readability.

    codeflash/tracing/replay_test.py [63-68]

    -for function, function_property in zip(functions, function_properties):
    -    if function_property is None:
    -        continue
    -    if not function_property.is_top_level:
    -        # can't be imported and run in the replay test
    -        continue
    +valid_pairs = [(f, p) for f, p in zip(functions, function_properties) if p and p.is_top_level]
    +for function, function_property in valid_pairs:
    +    # proceed with import logic for valid top-level functions
    Suggestion importance[1-10]: 6

    __

    Why: Pre-filtering the (function, function_property) pairs enhances readability and reduces repetitive checks in the loop.

    Low
    Replace magic threshold

    Replace the magic number 10000 with a named constant to clarify its meaning and
    avoid hardcoding thresholds. Define SMALL_RUNTIME_THRESHOLD at the module level.

    codeflash/result/critic.py [35-37]

    -noise_floor = 3 * MIN_IMPROVEMENT_THRESHOLD if original_code_runtime < 10000 else MIN_IMPROVEMENT_THRESHOLD
    +SMALL_RUNTIME_THRESHOLD = 10000
    +
    +noise_floor = 3 * MIN_IMPROVEMENT_THRESHOLD if original_code_runtime < SMALL_RUNTIME_THRESHOLD else MIN_IMPROVEMENT_THRESHOLD
     if in_github_actions_mode:
    -    noise_floor = noise_floor * 2  # Increase the noise floor in GitHub Actions mode
    +    noise_floor *= 2  # Increase the noise floor in GitHub Actions mode
    Suggestion importance[1-10]: 5

    __

    Why: Introducing a named SMALL_RUNTIME_THRESHOLD clarifies the purpose of 10000 but is primarily a stylistic improvement.

    Low

    @misrasaurabh1 misrasaurabh1 requested a review from KRRT7 May 1, 2025 05:04
    @misrasaurabh1 misrasaurabh1 marked this pull request as ready for review May 1, 2025 05:04
    @github-actions
    Copy link

    github-actions bot commented May 1, 2025

    Persistent review updated to latest commit 3cb6cb0

    @misrasaurabh1 misrasaurabh1 merged commit 1eb674d into main May 1, 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.

    3 participants