Skip to content

Conversation

mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Aug 29, 2025

PR Type

Enhancement, Tests


Description

  • Introduce CodeflashError and errors module

  • Refactor Result.failure to return CodeflashError

  • Update CLI, LSP, and optimizer to use .message

  • Adjust tests to assert failure().message


File Walkthrough

Relevant files
Error handling
6 files
cmd_init.py
Print structured error message in CLI                                       
+1/-1     
shell_utils.py
Use error factories for shell rc errors                                   
+3/-9     
errors.py
Define structured error code functions                                     
+78/-0   
beta.py
Return `.message` for LSP error responses                               
+16/-4   
function_optimizer.py
Replace raw failures with error codes                                       
+42/-19 
optimizer.py
Log `failure().message` in optimizer run                                 
+1/-1     
Enhancement
1 files
either.py
Implement `CodeflashError` and structured failures             
+28/-3   
Tests
1 files
test_unused_helper_revert.py
Update assertions to use `failure().message`                         
+9/-9     
Additional files
1 files
__init__.py [link]   

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

Template Formatting

Ensure message templates correctly escape and format placeholders. The use of nested f-strings with double braces may result in literal braces or incorrect substitution.

    f"I tried adding your Codeflash API key to {{shell_rc_path}} - but seems like I don't have permissions to do so.{LF}"
    f"You'll need to open it yourself and add the following line:{LF}{LF}{{api_key_line}}{LF}",
    **locals(),
)
Failure Semantics

Verify that Failure stores a CodeflashError while its .value is the formatted message, and ensure .unwrap() and .failure() behave consistently across error flows.

def failure(self) -> CodeflashError:
    if self.is_successful():
        msg = "Cannot get failure value from a success"
        raise ValueError(msg)
    if isinstance(self, Failure):
        return self.error_code
    raise ValueError("Result is not a failure")
Missing Tests

Add unit tests for CodeflashError and the error factory functions to validate error codes, formatting arguments, and .message output.

from __future__ import annotations

from codeflash.code_utils.compat import LF
from codeflash.either import CodeflashError

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Fix typo in error message

Correct the typo in the error message template by changing "bevhavioral" to
"behavioral" so the message reads clearly.

codeflash/errors/errors.py [74-78]

 def behavioral_test_failure_error() -> CodeflashError:
     return CodeflashError(
         "BEHAVIORAL_TEST_FAILURE_ERROR",
-        "Failed to establish a baseline for the original code - bevhavioral tests failed.",
+        "Failed to establish a baseline for the original code - behavioral tests failed.",
     )
Suggestion importance[1-10]: 5

__

Why: Corrects the misspelled "bevhavioral" in the behavioral_test_failure_error message, improving clarity for the user.

Low

codeflash-ai bot added a commit that referenced this pull request Aug 29, 2025
#695 (`enhancement/codeflash-errors`)

The key optimization is replacing `**locals()` with an explicit keyword argument `failure_msg=failure_msg`. 

The `**locals()` call forces Python to:
1. Create a dictionary containing all local variables in the function scope
2. Unpack that dictionary as keyword arguments to the constructor

Since this function only has one local variable (`failure_msg`), the explicit approach avoids the overhead of dictionary creation and unpacking. The line profiler shows the `**locals()` line took 12.6% of total execution time (926544ns per hit) in the original vs 10.2% (686248ns per hit) for the direct assignment - a 26% reduction in that line's cost.

This optimization is particularly effective for:
- **Frequent function calls** - as shown in the large-scale tests where 1000 calls saw 13.5-16.1% speedups
- **Simple error creation patterns** - all test cases showed 21-37% improvements
- **Memory-constrained scenarios** - eliminates unnecessary dictionary allocation

The 15% overall speedup comes from eliminating Python's introspection overhead while maintaining identical functionality and error object structure.
codeflash-ai bot added a commit that referenced this pull request Aug 29, 2025
… PR #695 (`enhancement/codeflash-errors`)

The optimization replaces `**locals()` with explicit parameter passing (`error=error`) in the `CodeflashError` constructor call. This eliminates the overhead of:

1. **Dictionary creation**: `locals()` creates a new dictionary containing all local variables at runtime
2. **Dictionary unpacking**: The `**` operator unpacks this dictionary into keyword arguments

By passing `error=error` explicitly, we avoid these dictionary operations entirely. The line profiler shows this reduces the function's total time from 2.32ms to 2.21ms across test runs.

The optimization is particularly effective for:
- **High-frequency calls**: The large-scale test with 1000 iterations shows 12.5% speedup
- **Long error messages**: Tests with very long strings (1000+ characters) see up to 38% improvement
- **All test patterns**: Every test case shows 15-38% speedup, indicating the optimization benefits all usage patterns consistently

This is a classic Python performance pattern - avoiding unnecessary dictionary operations when the required parameters are known at compile time.
… in PR #695 (`enhancement/codeflash-errors`)

The optimization replaces dynamic object creation with a pre-instantiated constant. Instead of creating a new `CodeflashError` object on every function call, the optimized version creates the error object once at module load time (`_TEST_CONFIDENCE_ERROR`) and returns that same instance from the function.

**Key changes:**
- Added module-level constant `_TEST_CONFIDENCE_ERROR` containing the pre-created error object
- Function now simply returns the pre-existing object instead of constructing a new one

**Why this is faster:**
- **Eliminates object instantiation overhead**: The original version calls `CodeflashError.__init__()` and allocates memory for a new object on every call (2591.1ns per hit). The optimized version just returns a reference to an existing object (741ns per hit).
- **Reduces string handling**: The constructor arguments (error code and message strings) are processed only once at module load rather than on every function call.
- **Memory efficiency**: Only one error object exists in memory rather than potentially many identical instances.

**Performance gains by test type:**
The 64% speedup is consistent across all test scenarios, with individual test calls showing 90-160% improvements (781ns→371ns, 722ns→331ns, etc.). This optimization is particularly effective for:
- High-frequency error creation scenarios
- Functions that return constant error objects
- Cases where the error object's immutable nature makes instance reuse safe

The optimization maintains identical behavior since `CodeflashError` objects with the same parameters are functionally equivalent, and the tests confirm the returned object has the correct properties.
Copy link
Contributor

codeflash-ai bot commented Aug 29, 2025

⚡️ Codeflash found optimizations for this PR

📄 64% (0.64x) speedup for test_confidence_threshold_not_met_error in codeflash/errors/errors.py

⏱️ Runtime : 27.1 microseconds 16.5 microseconds (best of 227 runs)

I created a new dependent PR with the suggested changes. Please review:

If you approve, it will be merged into this PR (branch enhancement/codeflash-errors).

… (`enhancement/codeflash-errors`)

The optimization replaces object creation on every function call with object reuse through module-level caching. 

**Key Changes:**
- Created a module-level constant `_BEHAVIORAL_TEST_FAILURE_ERROR` that instantiates the `CodeflashError` once at import time
- Modified the function to simply return the pre-created object instead of constructing a new one each time

**Why This Is Faster:**
- **Eliminates repeated object allocation**: The original code created a new `CodeflashError` object on every call, requiring memory allocation and constructor execution. The line profiler shows the constructor call (`CodeflashError(...)`) took 81.4% of the original execution time.
- **Reduces function call overhead**: Pre-creating the object eliminates the need to pass arguments to the constructor on each invocation.
- **Leverages Python's object model**: Since error objects are typically immutable, sharing the same instance is safe and efficient.

**Performance Gains:**
The optimization delivers consistent 100-136% speedup across all test cases, with the function executing in ~8μs vs ~17μs originally. This pattern is particularly effective for frequently called utility functions that return constant values, as evidenced by the uniform performance improvements across different test scenarios.

Note: One test case shows the optimization maintains object equality while potentially changing object identity (the "unique instance" test), which is acceptable since error objects are typically compared by value, not reference.
Copy link
Contributor

codeflash-ai bot commented Aug 29, 2025

⚡️ Codeflash found optimizations for this PR

📄 107% (1.07x) speedup for behavioral_test_failure_error in codeflash/errors/errors.py

⏱️ Runtime : 16.7 microseconds 8.07 microseconds (best of 214 runs)

I created a new dependent PR with the suggested changes. Please review:

If you approve, it will be merged into this PR (branch enhancement/codeflash-errors).

mohammedahmed18 and others added 3 commits August 29, 2025 17:52
…25-08-29T14.42.12

⚡️ Speed up function `behavioral_test_failure_error` by 107% in PR #695 (`enhancement/codeflash-errors`)
Copy link
Contributor

codeflash-ai bot commented Aug 30, 2025

…25-08-29T14.38.33

⚡️ Speed up function `test_confidence_threshold_not_met_error` by 64% in PR #695 (`enhancement/codeflash-errors`)
Copy link
Contributor

codeflash-ai bot commented Aug 30, 2025

Comment on lines +22 to +28
_TEST_RESULT_DIDNT_MATCH_ERROR = CodeflashError(
"TEST_RESULT_DIDNT_MATCH_ERROR", "Test results did not match the test results of the original code."
)


def test_result_didnt_match_error() -> CodeflashError:
return _TEST_RESULT_DIDNT_MATCH_ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the benefit of this abstraction? it's not very pythonic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the top level error variable
Or the abstraction of the errors in a separate file

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.

2 participants