Skip to content

[FIX] skip draft pr optimization (CF-667)#322

Merged
mohammedahmed18 merged 12 commits into
mainfrom
fix/skip-optimization-for-draft-prs
Jun 23, 2025
Merged

[FIX] skip draft pr optimization (CF-667)#322
mohammedahmed18 merged 12 commits into
mainfrom
fix/skip-optimization-for-draft-prs

Conversation

@mohammedahmed18
Copy link
Copy Markdown
Contributor

@mohammedahmed18 mohammedahmed18 commented Jun 12, 2025 •
edited
Loading

User description

2025-06-12-153759_hyprshot

PR Type

Bug fix, Enhancement

Description

  • Add get_pr_info CFAPI client method

  • Retrieve PR info before optimization

  • Skip optimization for draft pull requests

  • Warn when PR missing or in draft

Changes walkthrough 📝

Relevant files
Enhancement
cfapi.py
Add get_pr_info CFAPI method                                                         

codeflash/api/cfapi.py

  • Add get_pr_info request function
  • Return JSON or None on failure
 +10/-0   
Bug fix
optimizer.py
Skip draft PR optimization                                                             

codeflash/optimization/optimizer.py

  • Import and use get_pr_info service call
  • Retrieve repo owner and PR number
  • Skip optimization for draft PRs
  • Log warnings for missing or draft PR
    		• +14/-0   
    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @github-actions
    Copy link
    Copy Markdown
    Contributor

    github-actions Bot commented Jun 12, 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

    URL Encoding

    The GET endpoint is constructed with f-strings and interpolated values, which may require URL encoding for special characters in owner, repo, or pr_number.

     
    def get_pr_info(owner: str, repo: str, pr_number: int) -> Any:  # noqa
    """Get information about a pull request."""
    response = make_cfapi_request(
        endpoint=f"/get-pr-info?owner={owner}&repo={repo}&pr_number={pr_number}", method="GET"
    )
    Input Validation

    Calling int(get_pr_number()) without checking if get_pr_number() returns a valid integer string could raise a ValueError when the PR number is missing or malformed.

     
    pr_info = cfapi.get_pr_info(owner, repo, int(get_pr_number()))
if pr_info is None:
    Exception Handling

    The API call in get_pr_info only checks response.ok and returns None, but does not handle network errors or exceptions from make_cfapi_request.

     
    def get_pr_info(owner: str, repo: str, pr_number: int) -> Any:  # noqa
    """Get information about a pull request."""
    response = make_cfapi_request(
        endpoint=f"/get-pr-info?owner={owner}&repo={repo}&pr_number={pr_number}", method="GET"
    )
    if response.ok:
        return response.json()
    return None

    @github-actions
    Copy link
    Copy Markdown
    Contributor

    github-actions Bot commented Jun 12, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Safely parse PR number

    Capture and validate the PR number string before converting to int to avoid
    ValueError when the environment variable is missing or malformed. Log a warning and
    return early if conversion fails.

    codeflash/optimization/optimizer.py [94]

    -pr_info = cfapi.get_pr_info(owner, repo, int(get_pr_number()))
+pr_number_str = get_pr_number()
+try:
+    pr_number = int(pr_number_str)
+except (TypeError, ValueError):
+    logger.warning(f"Invalid PR number: {pr_number_str!r}, skipping optimization")
+    return
+pr_info = cfapi.get_pr_info(owner, repo, pr_number)
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion prevents a potential ValueError from converting a malformed pr_number and adds a clear warning, improving robustness.

    		Medium
    Safely access draft flag

    Use .get("draft", False) to guard against missing keys in the API response and avoid
    a KeyError, defaulting to non-draft.

    codeflash/optimization/optimizer.py [98]

    -is_draft = pr_info["draft"]
+is_draft = pr_info.get("draft", False)
    Suggestion importance[1-10]: 5

    __

    Why: Using get("draft", False) avoids a possible KeyError on missing draft, which is a minor but helpful safeguard.

    		Low
    General
    Log API failure details

    Log the failure status and response text when the GET request fails to aid
    debugging, instead of silently returning None.

    codeflash/api/cfapi.py [245-247]

     if response.ok:
     return response.json()
+logger.warning(f"get_pr_info failed: {response.status_code} {response.text!r}")
 return None
    Suggestion importance[1-10]: 6

    __

    Why: Logging the failed status and response.text aids in debugging API issues without altering functionality.

    		Low
    Use explicit submodule import

    Import the cfapi submodule explicitly to ensure the module loads correctly, avoiding
    reliance on package init.py.

    codeflash/optimization/optimizer.py [11]

    -from codeflash.api import cfapi
+import codeflash.api.cfapi as cfapi
    Suggestion importance[1-10]: 3

    __

    Why: Changing to an explicit submodule import is mostly stylistic and offers minimal functional benefit.

    		Low

    codeflash-ai Bot added a commit that referenced this pull request Jun 16, 2025
    @codeflash-ai
    ⚡️ Speed up function is_pr_draft by 35% in PR #322 (`fix/skip-optim…
    db8241f 
    …ization-for-draft-prs`)

Here’s a faster, more memory-efficient rewrite of your program. Optimizations include.

- Removed duplicate `get_pr_number` import and moved/corrected its singleton responsibility.
- Used direct, local variable access rather than repeatedly referencing imported modules.
- Used `os.environ[]` instead of `os.environ.get` for critical env lookups in a try-block (avoids lookup cost when you know failure will land in except anyway).
- Used direct file open for reading (avoiding Path overhead).
- Avoided reading/parsing JSON and dictionary keys if the PR number/env is missing.
- Reduced exception handling scope to only JSON/file-related operations.

All existing comments are preserved except where the code was made more efficient.



**Key points:**  
- File reading is done only if both env vars are present, before JSON parsing.
- The exception only wraps file I/O + JSON parsing, not the env checks, so it's tighter/faster in normal runs.
- No `Path`. Used built-in open for speed.
- Early returns for failures, so the function does the minimum work necessary.  
- Single access to environment variables (no redundancy).

**Functional output is preserved.**
    @codeflash-ai codeflash-ai Bot mentioned this pull request Jun 16, 2025
    Closed
    @codeflash-ai
    Copy link
    Copy Markdown
    Contributor

    codeflash-ai Bot commented Jun 16, 2025

    ⚡️ Codeflash found optimizations for this PR

    📄 35% (0.35x) speedup for is_pr_draft in codeflash/optimization/optimizer.py

    ⏱️ Runtime : 6.43 milliseconds → 4.78 milliseconds (best of 92 runs)

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

    If you approve, it will be merged into this PR (branch fix/skip-optimization-for-draft-prs).

    Saga4
    Saga4 reviewed Jun 16, 2025
    View reviewed changes
    Comment thread codeflash/optimization/optimizer.py
    return

    if is_pr_draft():
    logger.warning("PR is in draft mode, skipping optimization")
    Copy link
    Copy Markdown
    Contributor

    @Saga4 Saga4 Jun 16, 2025

    Choose a reason for hiding this comment

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

    Q: Will the logger.warning logs visible in non debug mode in GHA?

    Copy link
    Copy Markdown
    Contributor Author

    @mohammedahmed18 mohammedahmed18 Jun 18, 2025 •
    edited
    Loading

    Choose a reason for hiding this comment

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

    yes, it should, the default level is "INFO"

    Saga4
    Saga4 previously approved these changes Jun 16, 2025
    View reviewed changes
    @mohammedahmed18 mohammedahmed18 merged commit 859d50b into main Jun 23, 2025
    16 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @Saga4 Saga4 Saga4 approved these changes

    @misrasaurabh1 misrasaurabh1 Awaiting requested review from misrasaurabh1 misrasaurabh1 is a code owner

    @KRRT7 KRRT7 Awaiting requested review from KRRT7

    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.

    2 participants

    @mohammedahmed18 @Saga4