Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Aug 6, 2025

PR Type

Bug fix


Description

  • Introduce raise_pr flag for PR logic

  • Simplify PR and staging review condition branches

  • Add new revert_code_and_helpers method

  • Revert code on staging and certain scenarios


Diagram Walkthrough

flowchart LR
  A["process_review"] --> B{"raise_pr?"}
  B -- "True & !staging_review" --> C["check_create_pr"]
  C --> D{"should_revert?"}
  B -- "True & staging_review" --> E["create_staging"]
  E --> D
  D -- "True" --> F["revert_code_and_helpers"]
  B -- "False & !staging_review" --> G["mark_optimization_success"]
Loading

File Walkthrough

Relevant files
Bug fix
function_optimizer.py
Refactor PR logic and extract revert method                           

codeflash/optimization/function_optimizer.py

  • Define raise_pr flag to simplify PR checks
  • Update PR and staging review condition logic
  • Extract revert_code_and_helpers into method
  • Ensure code revert in staging and test cases
+20/-6   

@github-actions
Copy link

github-actions bot commented Aug 6, 2025

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

Revert logic confusion

The new conditional for calling revert_code_and_helpers may trigger immediately after PR creation when flags like --all, --replay_test, or an existing PR number are present. This could undo optimizations before the PR is merged or produce unexpected side effects. Verify that the revert happens only in intended scenarios.

if raise_pr and (
    self.args.all
    or env_utils.get_pr_number()
    or self.args.replay_test
    or (self.args.file and not self.args.function)
):
    self.revert_code_and_helpers(original_helper_code)
    return
Redundant staging branch

self.args.staging_review is checked twice: first to call create_staging and later to revert code. Consider consolidating these checks or clarifying the flow to avoid duplicate branches and ensure the correct order of operations.

if raise_pr and not self.args.staging_review:
    data["git_remote"] = self.args.git_remote
    check_create_pr(**data)
elif self.args.staging_review:
    create_staging(**data)
else:
    # Mark optimization success since no PR will be created
    mark_optimization_success(
        trace_id=self.function_trace_id, is_optimization_found=best_optimization is not None
    )

if raise_pr and (
    self.args.all
    or env_utils.get_pr_number()
    or self.args.replay_test
    or (self.args.file and not self.args.function)
):
    self.revert_code_and_helpers(original_helper_code)
    return

if self.args.staging_review:
    # always revert code and helpers when staging review
    self.revert_code_and_helpers(original_helper_code)
    return

@github-actions
Copy link

github-actions bot commented Aug 6, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@mohammedahmed18 mohammedahmed18 added the bug Something isn't working label Aug 7, 2025

if ((not self.args.no_pr) or not self.args.staging_review) and (
self.args.all or env_utils.get_pr_number() or (self.args.file and not self.args.function)
if raise_pr and (
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We could have just merged the condition (self.args.staging_review:) in same conditional statement here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I kept them separated for readability

@mohammedahmed18 mohammedahmed18 merged commit a60de79 into main Aug 13, 2025
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants