Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Oct 15, 2025

User description

WIP


PR Type

Enhancement, Tests


Description

  • Add optimization impact plumbing to API

  • Introduce call analysis AST visitor

  • Add ripgrep-based code search helper

  • Extend models with impact metrics


Diagram Walkthrough

flowchart LR
  Optimizer["function_optimizer: process_review"] -- "compute + pass" --> AISvc["aiservice.get_optimization_impact"]
  Optimizer -- "send to CFAPI" --> CFAPI["cfapi.suggest_changes(payload)"]
  CFAPI -- "payload field" --> Backend["/suggest-pr-changes"]
  Models["models.ImpactMetrics"] -- "intended usage" --> Optimizer
  UtilsRG["code_extractor.search_with_ripgrep"] -- "search repo" --> Optimizer
  Visitor["function_call_visitor: FunctionCallVisitor"] -- "analyze calls" --> Metrics["Impact metrics (future)"]
  CreatePR["result.create_pr.check_create_pr"] -- "forward optimizationImpact" --> CFAPI
Loading

File Walkthrough

Relevant files
Documentation
2 files
aiservice.py
Add TODO metrics notes in optimization impact flow             
+7/-0     
example_usage.py
Add example for ripgrep search usage                                         
+28/-0   
Enhancement
7 files
cfapi.py
Plumb optimizationImpact through suggest_changes payload 
+2/-0     
code_extractor.py
Add ripgrep search helper and impact metrics stub               
+86/-3   
compat.py
Expose SAFE_GREP_EXECUTABLE via Compat                                     
+3/-0     
models.py
Define ImpactMetrics dataclass for impact data                     
+8/-0     
function_optimizer.py
Compute optimization impact and attach metrics stub           
+10/-8   
create_pr.py
Forward optimization impact to suggestion API                       
+2/-0     
function_call_visitor.py
Implement AST visitor for call and loop detection               
+317/-0 
Tests
1 files
test_function_call_visitor.py
Add tests and demos for call visitor                                         
+263/-0 

@github-actions github-actions bot changed the title More metrics for determining Optimization Impact More metrics for determining Optimization Impact Oct 15, 2025
@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 Security concerns

Command execution:
search_with_ripgrep shells out to 'rg' with a pattern that appears to be passed directly. Although subprocess.run uses a list (shell=False), consider guarding against unbounded patterns and very large outputs. Also, printing command lines and errors may leak paths; use logger with appropriate levels instead of print.

⚡ Recommended focus areas for review

Hardcoded Path

The ripgrep search excludes a user-specific absolute path which will not exist in other environments; this should be configurable or relative to the project root.

cmd = [
    "rg",
    "-n",
    "--json",
    pattern,
    path,
    "-g",
    "!/Users/aseemsaxena/Downloads/codeflash_dev/codeflash/code_to_optimize/tests/**",
]
print(" ".join(cmd))
Incomplete Return

get_opt_impact_metrics returns 0 but is annotated to return ImpactMetrics, which will cause type/usage errors when consumed.

def get_opt_impact_metrics(file_path: Path, qualified_name: str, project_root: Path, tests_root: Path) -> ImpactMetrics:
    # grep for function / use rg (respects gitignore)
    # SAFE_GREP_EXECUTABLE command
    # ast visitor for occurances and loop occurances
    # radon lib for complexity metrics
    print(file_path, qualified_name, project_root, tests_root)

    # grep windows alternative
    return 0
Argument Mismatch

get_opt_impact_metrics is called with two arguments but its signature expects four; this will raise at runtime.

data["impact_metrics"] = get_opt_impact_metrics(
    self.project_root, self.test_cfg.tests_root
)  # need module root, tests root only

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix mismatched function call signature

The call to get_opt_impact_metrics does not match its signature, which expects
(file_path, qualified_name, project_root, tests_root). Pass the required arguments
to avoid runtime TypeError. If some values are not available yet, pass reasonable
placeholders or defer the call.

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

+# Example placeholders; replace with real target file and qualified name when available
+target_file: Path = self.project_root
+target_qualified_name: str = ""
 data["impact_metrics"] = get_opt_impact_metrics(
-    self.project_root, self.test_cfg.tests_root
-)  # need module root, tests root only
+    target_file, target_qualified_name, self.project_root, self.test_cfg.tests_root
+)
Suggestion importance[1-10]: 8

__

Why: The call site clearly passes two args while the function signature requires four; this would raise a runtime TypeError. Providing proper arguments prevents a crash and is a high-impact correctness fix.

Medium
Return correct ImpactMetrics type

The function advertises returning ImpactMetrics but returns an int, which will crash
callers expecting the model. Return a properly constructed ImpactMetrics (even with
placeholder values) and remove debug prints to avoid stdout noise.

codeflash/code_utils/code_extractor.py [825-834]

 def get_opt_impact_metrics(file_path: Path, qualified_name: str, project_root: Path, tests_root: Path) -> ImpactMetrics:
-    # grep for function / use rg (respects gitignore)
-    # SAFE_GREP_EXECUTABLE command
-    # ast visitor for occurances and loop occurances
-    # radon lib for complexity metrics
-    print(file_path, qualified_name, project_root, tests_root)
+    # TODO: Implement actual metrics collection
+    # Placeholder safe defaults to maintain contract
+    return ImpactMetrics(
+        complexity_score=0,
+        occurances=0,
+        loop_occurances=0,
+        presence_of_decorators=False,
+    )
 
-    # grep windows alternative
-    return 0
-
Suggestion importance[1-10]: 8

__

Why: The function is annotated to return ImpactMetrics but returns an int, breaking the contract; returning a valid ImpactMetrics prevents downstream errors and aligns with the new dataclass added in the PR.

Medium
Remove hardcoded paths and debug prints

Avoid hardcoding an absolute exclude path and remove debug prints to prevent leaking
environment details. Also honor the provided path parameter instead of overriding it
with cwd, and use the project logger for errors. This prevents noisy stdout and
makes the function deterministic across environments.

codeflash/code_utils/code_extractor.py [754-823]

 def search_with_ripgrep(pattern: str, path: str = ".") -> dict[str, list[tuple[int, str]]]:
-    ...
-    path = str(Path.cwd())
+    """Use ripgrep to search for a pattern in the repository."""
+    search_root = str(Path(path).resolve())
     cmd = [
         "rg",
         "-n",
         "--json",
         pattern,
-        path,
+        search_root,
         "-g",
-        "!/Users/aseemsaxena/Downloads/codeflash_dev/codeflash/code_to_optimize/tests/**",
+        "!**/tests/**",
     ]
-    print(" ".join(cmd))
-    ...
+    matches_dict: dict[str, list[tuple[int, str]]] = {}
     try:
         result = subprocess.run(
             cmd,
             capture_output=True,
             text=True,
-            check=False,  # Don't raise exception on non-zero return
+            check=False,
         )
-    ...
+
+        if result.returncode not in (0, 1):
+            logger.debug(f"ripgrep returned {result.returncode}: {result.stderr}")
+            return {}
+
+        for line in result.stdout.splitlines():
+            if not line:
+                continue
+            try:
+                json_obj = json.loads(line)
+                if json_obj.get("type") != "match":
+                    continue
+                data = json_obj.get("data", {})
+                file_path = data.get("path", {}).get("text", "")
+                line_number = data.get("line_number")
+                line_content = data.get("lines", {}).get("text", "").rstrip("\n")
+                if file_path and isinstance(line_number, int):
+                    matches_dict.setdefault(file_path, []).append((line_number, line_content))
+            except json.JSONDecodeError:
+                continue
     except FileNotFoundError:
-        print("Error: ripgrep (rg) is not installed or not in PATH")
+        logger.warning("ripgrep (rg) is not installed or not in PATH")
         return {}
     except Exception as e:
-        print(f"Unexpected error: {e}")
+        logger.debug(f"ripgrep invocation failed: {e}")
         return {}
     return matches_dict
Suggestion importance[1-10]: 7

__

Why: Correctly identifies multiple issues: overriding the path arg, hardcoded absolute exclude, and noisy prints; proposed code aligns with context and improves portability and logging without changing functionality.

Medium

@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Oct 17, 2025

⚡️ Codeflash found optimizations for this PR

📄 51% (0.51x) speedup for FunctionCallFinder._extract_source_code in codeflash/code_utils/code_extractor.py

⏱️ Runtime : 1.12 milliseconds 742 microseconds (best of 59 runs)

A dependent PR with the suggested changes has been created. Please review:

If you approve, it will be merged into this PR (branch opt-impact-aseem-improvement).

data["optimization_impact"] = opt_impact_response[0]
new_explanation_with_opt_explanation = Explanation(
raw_explanation_message=f"Impact: {opt_impact_response[0]}\n Impact_explanation: {opt_impact_response[1]} END OF IMPACT EXPLANATION\nCALLING CONTEXT \n{calling_fn_details}\nEND OF CALLING CONTEXT\n"
+ new_explanation.raw_explanation_message,
Copy link
Contributor

Choose a reason for hiding this comment

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

return the right parts, not the full llm output and add a label in the PR with the quality

@aseembits93 aseembits93 enabled auto-merge October 20, 2025 21:02
@aseembits93 aseembits93 merged commit c1250e8 into main Oct 20, 2025
22 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