Skip to content

feat: Add timeout params#36

Merged
dexhunter merged 9 commits intodevfrom
feat/add_step_timeout
Jul 9, 2025
Merged

feat: Add timeout params#36
dexhunter merged 9 commits intodevfrom
feat/add_step_timeout

Conversation

@dexhunter
Copy link
Copy Markdown
Member

@dexhunter dexhunter commented Jul 6, 2025

  • add eval-timeout param (default set to None/ no limit)
  • add a constants.py to put values in a central place
  • remove duplicated values with a constant

@dexhunter dexhunter changed the title feat: Add step-timeout param feat: Add timeout param Jul 7, 2025
@dexhunter dexhunter force-pushed the feat/add_step_timeout branch 2 times, most recently from 3e34159 to 13d4519 Compare July 7, 2025 03:40
@dexhunter dexhunter changed the title feat: Add timeout param feat: Add timeout params Jul 7, 2025
@dexhunter dexhunter self-assigned this Jul 7, 2025
@dexhunter dexhunter added the enhancement New feature or request label Jul 7, 2025
@DhruvSrikanth DhruvSrikanth requested a review from Copilot July 7, 2025 12:27

This comment was marked as outdated.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@DhruvSrikanth DhruvSrikanth changed the base branch from main to dev July 7, 2025 12:30
@DhruvSrikanth DhruvSrikanth requested a review from Copilot July 8, 2025 15:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes timeout values and exposes an evaluation timeout flag for CLI runs:

  • Adds an optional timeout parameter to run_evaluation and handles TimeoutExpired.
  • Introduces weco/constants.py with DEFAULT_API_TIMEOUT and replaces hardcoded API timeouts.
  • Adds a --eval-timeout CLI argument and propagates it through execute_optimization.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
weco/utils.py Extended run_evaluation with a timeout parameter and exception handling.
weco/constants.py New file defining DEFAULT_API_TIMEOUT.
weco/optimizer.py Propagates eval_timeout and swaps hardcoded timeouts for DEFAULT_API_TIMEOUT.
weco/cli.py Added --eval-timeout argument and pass-through to optimization function.
weco/api.py Replaced hardcoded default timeouts with DEFAULT_API_TIMEOUT.
README.md Documented the new --eval-timeout flag.
Comments suppressed due to low confidence (2)

weco/utils.py:127

  • The docstring for run_evaluation should be updated to describe the new timeout parameter and its behavior.
def run_evaluation(eval_command: str, timeout: int | None = None) -> str:

weco/utils.py:132

  • New timeout logic in run_evaluation should have corresponding unit tests to verify both normal execution and timeout behavior.
        result = subprocess.run(eval_command, shell=True, capture_output=True, text=True, check=False, timeout=timeout)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Member

@DhruvSrikanth DhruvSrikanth left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Would this require any changes to the onboarding copilot? Probably not since we would use the default value for eval-timeout which allows a unlimited time for the eval script's execution.

If this is the case, then feel free to merge @dexhunter

@dexhunter
Copy link
Copy Markdown
Member Author

yes, timeout=None by default (unlimited)

@dexhunter dexhunter force-pushed the feat/add_step_timeout branch from 38ca262 to b79b379 Compare July 9, 2025 02:14
@dexhunter dexhunter merged commit fe105d6 into dev Jul 9, 2025
2 checks passed
@dexhunter dexhunter deleted the feat/add_step_timeout branch July 9, 2025 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants