Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Oct 10, 2025

User description

Current UX is problematic with lots of staging reviews created.


PR Type

Enhancement


Description

  • Disable optimization impact gating logic

  • Always respect explicit PR/staging flags

  • Preserve previous code via comments


Diagram Walkthrough

flowchart LR
  A["process_review"] -- "previously queried impact" --> B["get_optimization_impact"]
  B -- "low -> force staging" --> C["staging_review=True"]
  A -- "now" --> D["skip impact check"]
  D -- "use args only" --> E["raise_pr / staging_review flow"]
Loading

File Walkthrough

Relevant files
Enhancement
function_optimizer.py
Skip optimization impact-based PR/staging decision             

codeflash/optimization/function_optimizer.py

  • Comment out calls to get_optimization_impact.
  • Remove auto-switching to staging on low impact.
  • Keep decision solely on no_pr and staging_review args.
+8/-8     

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Dead Code

Large commented block of previously active logic is left in place; consider removing or guarding with a feature flag to avoid drift and confusion.

# try:
#     # modify argument of staging vs pr based on the impact
#     opt_impact_response = self.aiservice_client.get_optimization_impact(**data)
#     if opt_impact_response == "low":
#         raise_pr = False
#         staging_review = True
# except Exception as e:
#     logger.debug(f"optimization impact response failed, investigate {e}")
Behavior Change

Optimization impact gating is fully disabled; verify downstream flows still meet expectations and that explicit flags alone cover all intended scenarios.

if raise_pr or staging_review:
    data["root_dir"] = git_root_dir()
    # try:
    #     # modify argument of staging vs pr based on the impact
    #     opt_impact_response = self.aiservice_client.get_optimization_impact(**data)
    #     if opt_impact_response == "low":
    #         raise_pr = False
    #         staging_review = True
    # except Exception as e:
    #     logger.debug(f"optimization impact response failed, investigate {e}")
if raise_pr and not staging_review:
    data["git_remote"] = self.args.git_remote
    check_create_pr(**data)
elif staging_review:

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Restore impact-based gating

By fully commenting out the optimization impact check, raise_pr and staging_review
are never adjusted, potentially triggering PRs when only staging was intended.
Restore the logic without swallowing exceptions silently by guarding it behind a
feature flag but keeping a safe fallback. Ensure failures default to the previous
behavior and are logged.

codeflash/optimization/function_optimizer.py [1462-1471]

 if raise_pr or staging_review:
     data["root_dir"] = git_root_dir()
-    # try:
-    #     # modify argument of staging vs pr based on the impact
-    #     opt_impact_response = self.aiservice_client.get_optimization_impact(**data)
-    #     if opt_impact_response == "low":
-    #         raise_pr = False
-    #         staging_review = True
-    # except Exception as e:
-    #     logger.debug(f"optimization impact response failed, investigate {e}")
+    if getattr(self.args, "use_optimization_impact", True):
+        try:
+            # modify argument of staging vs pr based on the impact
+            opt_impact_response = self.aiservice_client.get_optimization_impact(**data)
+            if opt_impact_response == "low":
+                raise_pr = False
+                staging_review = True
+        except Exception as e:
+            logger.debug(f"optimization impact response failed, investigate {e}")
Suggestion importance[1-10]: 7

__

Why: The PR comments out logic that adjusted raise_pr vs staging_review, changing behavior; restoring it behind a flag is reasonable and the improved code accurately reflects the intended change, referencing the correct lines. Impact is moderate since it affects workflow behavior, but it's a feature-choice rather than a critical bug.

Medium

staging_review = self.args.staging_review

if raise_pr or staging_review:
data["root_dir"] = git_root_dir()

Choose a reason for hiding this comment

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

Is this condition now required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's from the original code, it's still required

@aseembits93 aseembits93 merged commit 5fae405 into main Oct 11, 2025
20 of 23 checks passed
@KRRT7 KRRT7 deleted the aseembits93-patch-1 branch October 12, 2025 08:59
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.

4 participants