Skip to content

Pre commit update #620

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 5 commits into from
Aug 6, 2025
Merged

Pre commit update #620

merged 5 commits into from
Aug 6, 2025

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Aug 6, 2025 •
edited by github-actions bot
Loading

PR Type

Enhancement

Description

  • Refresh lint config: bump ruff to v0.12.7, add UP045 ignore

  • Style fixes: added return None, starred tuple unpack, removed unused import

  • Refine critic signature: make parameter keyword-only default False

  • Add noqa to TestResults class, bump version placeholder

File Walkthrough
Relevant files

@KRRT7 KRRT7 requested a review from aseembits93 August 6, 2025 04:04
@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: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Signature Change

The updated speedup_critic function now requires disable_gh_action_noise as a keyword-only boolean defaulting to False, which could break existing positional calls.

 
    *,
    disable_gh_action_noise: bool = False,
) -> bool:
Mutable Defaults

Using mutable default values for test_results and test_result_idx may lead to shared state across instances; consider using default_factory.

 
class TestResults(BaseModel):  # noqa: PLW1641
    # don't modify these directly, use the add method
    # also we don't support deletion of test results elements - caution is advised
    test_results: list[FunctionTestInvocation] = []

misrasaurabh1
misrasaurabh1 reviewed Aug 6, 2025
View reviewed changes
@github-actions
Copy link

github-actions bot commented Aug 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Allow positional disable flag

Making disable_gh_action_noise keyword-only can break callers that rely on
positional arguments. Remove the * to restore positional support and retain backward
compatibility.

codeflash/result/critic.py [28-34]

 def speedup_critic(
     candidate_result: OptimizedCandidateResult,
     original_code_runtime: int,
     best_runtime_until_now: int | None,
-    *,
     disable_gh_action_noise: bool = False,
 ) -> bool:
Suggestion importance[1-10]: 3

__

Why: This is a minor backwards-compatibility concern; marking the disable_gh_action_noise flag as keyword-only in speedup_critic is likely intentional for clarity and the function appears internal, so breaking positional calls is not a major issue.

Low

aseembits93
aseembits93 reviewed Aug 6, 2025
View reviewed changes
@aseembits93
Copy link
Contributor

@KRRT7 tests failing

codeflash-ai bot added a commit that referenced this pull request Aug 6, 2025
@codeflash-ai
⚡️ Speed up function speedup_critic by 23% in PR #620 (`pre-commit-…
65d131c 
…update`)

The optimization introduces a **cached GitHub Actions detection mechanism** that eliminates repeated expensive environment lookups in the hot path.

**Key optimization**: 
- Added `_in_github_actions_mode()` function with `@lru_cache(maxsize=1)` that caches the result of `env_utils.get_pr_number()` 
- Replaced the inline `bool(env_utils.get_pr_number())` call inside `speedup_critic` with a call to the cached function

**Why this is faster**:
The original code called `env_utils.get_pr_number()` on every invocation of `speedup_critic`. While `get_pr_number()` itself is cached, it still involves function call overhead and the boolean conversion. The line profiler shows this operation took **5.69ms** (28.8% of total time) in the original vs only **1.09ms** (7.7%) in the optimized version.

**Performance characteristics**:
- **Best for high-frequency scenarios**: The optimization shines when `speedup_critic` is called many times (as shown in the large-scale tests with 500+ candidates), where the 22% speedup compounds significantly
- **Minimal impact on single calls**: For individual calls, the improvement is small but consistent
- **GitHub Actions environments benefit most**: Since the cached result prevents repeated environment variable lookups that are expensive in CI environments

The optimization maintains identical behavior while reducing redundant work through strategic caching of the GitHub Actions detection logic.
@codeflash-ai codeflash-ai bot mentioned this pull request Aug 6, 2025
Closed
@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Aug 6, 2025

⚡️ Codeflash found optimizations for this PR

📄 23% (0.23x) speedup for speedup_critic in codeflash/result/critic.py

⏱️ Runtime : 1.27 milliseconds → 1.03 milliseconds (best of 153 runs)

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

If you approve, it will be merged into this PR (branch pre-commit-update).

aseembits93
aseembits93 approved these changes Aug 6, 2025
View reviewed changes
Copy link
Contributor

@aseembits93 aseembits93 left a comment

Choose a reason for hiding this comment

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

lgtm

@KRRT7 KRRT7 merged commit baee96e into main Aug 6, 2025
19 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

@aseembits93 aseembits93 aseembits93 approved these changes

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KRRT7 @aseembits93 @misrasaurabh1