Skip to content

Optimization Impact #750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Oct 7, 2025
Merged

Optimization Impact #750

merged 18 commits into from
Oct 7, 2025

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Sep 23, 2025 •
edited by github-actions bot
Loading

PR Type

Enhancement

Description

  • Add optimization impact assessment endpoint call

  • Gate PR creation by impact level

  • Include profiler and runtime metadata

  • Minor typing and import improvements

Diagram Walkthrough

flowchart LR
  optimizer["function_optimizer.process_review"] --> impact["AiServiceClient.get_optimization_impact"]
  impact -- "payload with code, tests, metrics" --> api["/optimization_impact endpoint"]
  api -- "impact: low/medium/high" --> optimizer
  optimizer -- "low/medium => staging review" --> staging["Set staging_review True"]
  optimizer -- "high => create PR" --> pr["check_create_pr()"]
Loading

File Walkthrough
Relevant files
Enhancement
aiservice.py
Add API client for optimization impact computation             

codeflash/api/aiservice.py

  • Add get_optimization_impact API method.
  • Build formatted code/test payload with metrics.
  • Use humanize_runtime, typing casts, and Path handling.
  • Telemetry and error handling for impact request.
 +88/-3   
function_optimizer.py
Gate PR creation based on impact result                                   

codeflash/optimization/function_optimizer.py

  • Call impact API before PR creation.
  • Route low/medium impact to staging review.
  • Include line profiler outputs in request.
 +11/-0   
Formatting
env_utils.py
Minor formatting in env utils                                                       

codeflash/code_utils/env_utils.py

  • Minor formatting/import spacing adjustment.
  • No behavioral change.
 +1/-0     

@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

Possible Issue

Code fences opened without closing markers when concatenating code blocks for payload may produce malformed markdown, potentially confusing the /optimization_impact endpoint parsing.

 
logger.info("!lsp|Computing Optimization Impact…")
original_code_str = ""
new_code_str = ""
for p, code in original_code.items():
    original_code_str += f"```python:{Path(p).relative_to(root_dir).as_posix()}"
    original_code_str += "\n"
    original_code_str += code
for p, code in new_code.items():
    new_code_str += f"```python:{Path(p).relative_to(root_dir).as_posix()}"
    new_code_str += "\n"
    new_code_str += code
Robustness

Casting response JSON fields without validating presence or type; unexpected payloads could raise exceptions or mask errors (e.g., missing 'impact' or 'error' keys).

 
if response.status_code == 200:
    return cast("str", response.json()["impact"])
try:
    error = cast("str", response.json()["error"])
except Exception:
    error = response.text
logger.error(f"Error generating impact candidates: {response.status_code} - {error}")
ph("cli-optimize-error-response", {"response_status_code": response.status_code, "error": error})
console.rule()
return ""
Control Flow Risk

Changing PR gating based on impact silently flips from PR to staging; consider logging the decision and the impact value for traceability and debugging.

 
if raise_pr:
    # modify argument of staging vs pr based on the impact
    opt_impact_response = self.aiservice_client.get_optimization_impact(
        **data,
        original_line_profiler_results=original_code_baseline.line_profile_results["str_out"],
        optimized_line_profiler_results=best_optimization.line_profiler_test_results["str_out"],
    )
    if opt_impact_response in ["low", "medium"]:
        raise_pr = False
        self.args.staging_review = True

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Properly close code fences

Ensure the fenced code blocks are correctly terminated to avoid malformed payloads
that can break downstream parsing. Append the closing backticks after each file's
content so the service reliably recognizes file boundaries.

codeflash/api/aiservice.py [556-563]

 for p, code in original_code.items():
-    original_code_str += f"```python:{Path(p).relative_to(root_dir).as_posix()}"
-    original_code_str += "\n"
+    original_code_str += f"```python:{Path(p).relative_to(root_dir).as_posix()}\n"
     original_code_str += code
+    if not code.endswith("\n"):
+        original_code_str += "\n"
+    original_code_str += "```\n"
 for p, code in new_code.items():
-    new_code_str += f"```python:{Path(p).relative_to(root_dir).as_posix()}"
-    new_code_str += "\n"
+    new_code_str += f"```python:{Path(p).relative_to(root_dir).as_posix()}\n"
     new_code_str += code
+    if not code.endswith("\n"):
+        new_code_str += "\n"
+    new_code_str += "```\n"
Suggestion importance[1-10]: 7

__

Why: The existing payload builds fenced code blocks without closing backticks, which can break downstream parsing; adding closing fences is accurate and improves robustness, though not critical.

Medium
Validate and default speedup value

Guard against non-numeric or None explanation.speedup values to prevent runtime
ValueError/TypeError. Provide a safe default and ensure the formatted string is
always valid.

codeflash/api/aiservice.py [574]

-"speedup": f"{1 + float(explanation.speedup):.2f}x",
+try:
+    speedup_val = 1 + float(explanation.speedup)  # type: ignore[arg-type]
+    if speedup_val < 0:
+        speedup_val = 1.0
+except Exception:
+    speedup_val = 1.0
+...
+"speedup": f"{speedup_val:.2f}x",
Suggestion importance[1-10]: 6

__

Why: Defensive handling of explanation.speedup prevents runtime exceptions during formatting; it's a reasonable safeguard but not a confirmed bug and of moderate impact.

Low
General
Safeguard unexpected impact response

Handle empty or unexpected responses from the impact endpoint to avoid silently
altering control flow. Treat unknown values as non-blocking or log and default,
preventing unintended suppression of PR creation.

codeflash/optimization/function_optimizer.py [1408-1418]

 if raise_pr:
-    # modify argument of staging vs pr based on the impact
     opt_impact_response = self.aiservice_client.get_optimization_impact(
         **data,
         original_line_profiler_results=original_code_baseline.line_profile_results["str_out"],
         optimized_line_profiler_results=best_optimization.line_profiler_test_results["str_out"],
     )
     if opt_impact_response in ["low", "medium"]:
         raise_pr = False
         self.args.staging_review = True
+    elif opt_impact_response not in ["high", "medium", "low"]:
+        logger.warning(f"Unexpected optimization impact response: {opt_impact_response!r}; proceeding with PR.")
Suggestion importance[1-10]: 6

__

Why: Adding a warning for unexpected or empty impact responses avoids silently suppressing PR creation and aids observability; it's contextually correct and moderately improves control flow safety.

Low

misrasaurabh1
misrasaurabh1 reviewed Sep 25, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Sep 25, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Sep 25, 2025
View reviewed changes
misrasaurabh1
misrasaurabh1 reviewed Sep 25, 2025
View reviewed changes
aseembits93
aseembits93 commented Oct 6, 2025
View reviewed changes
Saga4
Saga4 reviewed Oct 7, 2025
View reviewed changes
codeflash/optimization/function_optimizer.py
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":
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add a statement for logging on response here

Saga4
Saga4 reviewed Oct 7, 2025
View reviewed changes
codeflash/api/aiservice.py
payload = {
"code_diff": code_diff,
"explanation": explanation.raw_explanation_message,
"existing_tests": existing_tests_source,
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: How much existing tests and generated tests are effective in context. Did we try the results ignoring them as well?

@aseembits93 aseembits93 enabled auto-merge October 7, 2025 23:22
@aseembits93 aseembits93 merged commit fae5b46 into main Oct 7, 2025
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@misrasaurabh1 misrasaurabh1 misrasaurabh1 left review comments

@Saga4 Saga4 Saga4 approved these changes

@HeshamHM28 HeshamHM28 Awaiting requested review from HeshamHM28

@KRRT7 KRRT7 Awaiting requested review from KRRT7

@mohammedahmed18 mohammedahmed18 Awaiting requested review from mohammedahmed18

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@aseembits93 @misrasaurabh1 @Saga4 @HeshamHM28 @KRRT7