Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented May 30, 2025

PR Type

Bug fix, Tests


Description

  • Replace instance.method calls with class.method invocation

  • Include all args when generating replay test code

  • Remove unnecessary instance extraction from traces

  • Update tests to use direct class method calls


Changes walkthrough 📝

Relevant files
Bug fix
replay_test.py
Fix method invocation in replay test generation                   

codeflash/benchmarking/replay_test.py

  • Removed instance extraction for method calls
  • Changed invocation to ClassAlias.method(*args, **kwargs)
  • +1/-2     
    Tests
    test_trace_benchmarks.py
    Fix test code method invocation                                                   

    tests/test_trace_benchmarks.py

  • Removed instance = args[0] extraction
  • Updated tests to call class methods directly
  • +2/-4     

    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 30, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit df97c30)

    Here are some key observations to aid the review process:

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

    Attribute Access

    Ensure method_name includes the leading dot when concatenating to class_name_alias, otherwise the generated call ClassNameMethodName(...) will be invalid.

    ret = {class_name_alias}{method_name}(*args, **kwargs)
    Dead Code

    The else block for the __init__ test is unreachable since function_name equals "__init__" and always takes the if branch; consider removing it to avoid confusion.

    if function_name == "__init__":
        ret = code_to_optimize_bubble_sort_codeflash_trace_Sorter(*args[1:], **kwargs)
    else:
        ret = code_to_optimize_bubble_sort_codeflash_trace_Sorter(*args, **kwargs)

    @github-actions
    Copy link

    github-actions bot commented May 30, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use instance method call

    **Restore instance-based invocation for non-init methods to preserve bound method
    semantics and correct argument slicing. Re-introduce extracting instance = args[0]
    and call instance{method_name}(*args[1:], kwargs) instead of invoking the unbound
    method on the class alias.

    codeflash/benchmarking/replay_test.py [118]

    -ret = {class_name_alias}{method_name}(*args, **kwargs)
    +instance = args[0]  # self
    +ret = instance{method_name}(*args[1:], **kwargs)
    Suggestion importance[1-10]: 9

    __

    Why: The change to invoke the unbound class method breaks bound-method semantics by passing self incorrectly and drops slicing of args, so restoring the instance = args[0] call is critical for correct functionality.

    High
    Restore instance method call in test

    **Revert to instance-based invocation for the sorter method to correctly bind self and
    pass only the actual method arguments. Extract instance = args[0] and call
    instance.sorter(*args[1:], kwargs) instead of the unbound class method call.

    tests/test_trace_benchmarks.py [114]

    -ret = code_to_optimize_bubble_sort_codeflash_trace_Sorter.sorter(*args, **kwargs)
    +instance = args[0]  # self
    +ret = instance.sorter(*args[1:], **kwargs)
    Suggestion importance[1-10]: 8

    __

    Why: The test now invokes an unbound class method without self and incorrect argument slicing, so switching to instance.sorter(*args[1:], **kwargs) ensures test accuracy.

    Medium

    @aseembits93 aseembits93 marked this pull request as ready for review May 30, 2025 22:08
    @github-actions
    Copy link

    Persistent review updated to latest commit df97c30

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use instance method call

    **Restore instance-based invocation for non-init methods to preserve bound method
    semantics and correct argument slicing. Re-introduce extracting instance = args[0]
    and call instance{method_name}(*args[1:], kwargs) instead of invoking the unbound
    method on the class alias.

    codeflash/benchmarking/replay_test.py [118]

    -ret = {class_name_alias}{method_name}(*args, **kwargs)
    +instance = args[0]  # self
    +ret = instance{method_name}(*args[1:], **kwargs)
    Suggestion importance[1-10]: 9

    __

    Why: The change to invoke the unbound class method breaks bound-method semantics by passing self incorrectly and drops slicing of args, so restoring the instance = args[0] call is critical for correct functionality.

    High
    Restore instance method call in test

    **Revert to instance-based invocation for the sorter method to correctly bind self and
    pass only the actual method arguments. Extract instance = args[0] and call
    instance.sorter(*args[1:], kwargs) instead of the unbound class method call.

    tests/test_trace_benchmarks.py [114]

    -ret = code_to_optimize_bubble_sort_codeflash_trace_Sorter.sorter(*args, **kwargs)
    +instance = args[0]  # self
    +ret = instance.sorter(*args[1:], **kwargs)
    Suggestion importance[1-10]: 8

    __

    Why: The test now invokes an unbound class method without self and incorrect argument slicing, so switching to instance.sorter(*args[1:], **kwargs) ensures test accuracy.

    Medium

    @openhands-ai
    Copy link

    openhands-ai bot commented May 30, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • end-to-end-test
      • end-to-end-test

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #255

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    @aseembits93 aseembits93 merged commit 485bc19 into main May 31, 2025
    16 of 19 checks passed
    @aseembits93 aseembits93 deleted the replay-test-bug branch June 6, 2025 23:55
    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