Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Apr 21, 2025

PR Type

Bug fix


Description

  • Trigger write in file-only optimization mode

  • Add trailing commas and consistent formatting

  • Reformat multiline group_by_benchmarks calls

  • Improve logging message formatting


Changes walkthrough 📝

Relevant files
Bug fix
function_optimizer.py
Fix file-mode logic and clean up formatting                           

codeflash/optimization/function_optimizer.py

  • Added (self.args.file and not self.args.function) to write logic
  • Inserted trailing commas on parameters and arguments
  • Reformatted group_by_benchmarks calls to multiline
  • Adjusted logger calls and operator spacing
  • +33/-15 

    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

    Missing Attribute Assignment

    The new replay_tests_dir parameter in __init__ is not assigned to self, causing AttributeError when later accessed as self.replay_tests_dir.

        replay_tests_dir: Path | None = None,
    ) -> None:
    None Handling

    Unguarded use of self.replay_tests_dir when calling group_by_benchmarks may pass None, leading to runtime errors; consider validating or defaulting this value.

    test_results_by_benchmark = (
        candidate_result.benchmarking_test_results.group_by_benchmarks(
            self.total_benchmark_timings.keys(), self.replay_tests_dir, self.project_root
        )
    )

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Initialize replay_tests_dir attribute

    Assign the replay_tests_dir parameter to an instance attribute in init so
    self.replay_tests_dir is defined before use.

    codeflash/optimization/function_optimizer.py [97]

     replay_tests_dir: Path | None = None,
    +...
    +self.replay_tests_dir = replay_tests_dir
    Suggestion importance[1-10]: 9

    __

    Why: Without assigning self.replay_tests_dir in init, later references will raise an AttributeError, so this is a critical fix.

    High
    Guard against None replay_tests_dir

    Guard against self.replay_tests_dir being None before passing it into
    group_by_benchmarks, and handle the None case.

    codeflash/optimization/function_optimizer.py [469-471]

    -candidate_result.benchmarking_test_results.group_by_benchmarks(
    -    self.total_benchmark_timings.keys(), self.replay_tests_dir, self.project_root
    -)
    +if self.replay_tests_dir is not None:
    +    test_results_by_benchmark = candidate_result.benchmarking_test_results.group_by_benchmarks(
    +        self.total_benchmark_timings.keys(), self.replay_tests_dir, self.project_root
    +    )
    +else:
    +    test_results_by_benchmark = {}
    Suggestion importance[1-10]: 5

    __

    Why: Adding a None check before calling group_by_benchmarks prevents runtime errors when no replay_tests_dir is provided, improving robustness.

    Low
    Safely access args flags

    Safely access file and function attributes on self.args using getattr to avoid
    AttributeError if they’re unset.

    codeflash/optimization/function_optimizer.py [331]

    -if self.args.all or env_utils.get_pr_number() or (self.args.file and not self.args.function):
    +if self.args.all or env_utils.get_pr_number() or (getattr(self.args, 'file', False) and not getattr(self.args, 'function', False)):
    Suggestion importance[1-10]: 3

    __

    Why: Using getattr for args.file and args.function guards against missing attributes, but args likely always include these flags, so the impact is low.

    Low

    @aseembits93 aseembits93 enabled auto-merge April 21, 2025 03:23
    @aseembits93 aseembits93 disabled auto-merge April 21, 2025 03:23
    @misrasaurabh1 misrasaurabh1 merged commit d4dd33b into main Apr 21, 2025
    17 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