Skip to content

Conversation

@albertvillanova
Copy link
Member

Implement robust timeout mechanism:

  • cross-platform
  • thread-safe

This implementation fixes the issues from the reverted:

The issues of the previous implementation were:

Key Improvements Over the Reverted Implementation

  • Cross-Platform Support:

    • Uses ThreadPoolExecutor instead of signal.SIGALRM
    • Works on Windows (which doesn't have SIGALRM)
  • Thread-Safe:

    • Can be called from any thread, not just the main thread
    • Creates a new executor per call to avoid threading issues

Copy link
Collaborator

@aymeric-roucher aymeric-roucher left a comment

Choose a reason for hiding this comment

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

Super cool, thanks!

DEFAULT_MAX_LEN_OUTPUT = 50000
MAX_OPERATIONS = 10000000
MAX_WHILE_ITERATIONS = 1000000
MAX_EXECUTION_TIME_SECONDS = 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could increase this timeout just a bit? Some tools like playwright based web search might take few seconds to run.

Or even better : rather than hardcoding this value, why make it possible to pass this as an arg to the function, since users might have different needs? In particular that would allow PythonInterpreterTool and LocalPythonExecutor to be configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot for your review. 🤗

I totally agree and indeed I was thinking about a follow-up PR to support this parameter being user-configurable.

But better, I will address this in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants