Skip to content
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.

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

Allow None for timeout in AskSpec to support indefinite waits #925

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AidanShipperley
Copy link

Overview

Updated the AskSpec data class to support None as a valid value for the timeout field, allowing developers to force a wait without a timeout constraint. Previously, passing timeout = None would trigger a Pydantic validation error, making the only solution to pass an extremely large integer to achieve a pseudo-infinite wait.

Changes

  • Adjusted the timeout field type from int to Optional[int] within the AskSpec data class.
    • This should allow developers to add indefinite waits when using AskUserMessage, AskFileMessage, and AskUserAction by passing timeout = None.
  • The types.py file has several adjustments to it's spacing and import structure as a result of the pre-commit hook set up in your repository, the only thing I actually changed was one line.

Rationale

When asking the user for a file, for example, before a model can answer question, it's possible to not want the operation to timeout ever. The user may leave their computer and move onto something else for hours, but the file input should still work. Setting the variable timeout = None is very intuitive and clearly shows the intention of the developer to not allow a timeout to occur.

Testing

  • I followed all steps in the contribution guidelines and all tests passed locally.
  • I additionally tested that AskUserMessage, AskFileMessage, and AskUserAction worked with passing timeout = None with this change, and they all did as far as I could tell.

This commit modifies the `AskSpec` data class to accept `None` as a valid value for the `timeout` field, enabling operations to proceed without a timeout limit. Previously, setting `timeout` to `None` triggered a Pydantic validation error. This change allows more flexible user interactions by supporting indefinite waiting periods.
@tpatel
Copy link
Collaborator

tpatel commented Apr 23, 2024

Thanks for this PR, this is a valid feature. You will need to change more code to enable the no-timeout behavior.

One example: when you pass timeout = None to AskUserMessage, the timeout value is set to 60 seconds, instead of None.

Could you make the change in the Ask classes to allow for a None timeout and to enable the disable the timeout?

Copy link
Collaborator

@tpatel tpatel left a comment

Choose a reason for hiding this comment

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

just updating the state of the PR to reflect my previous comment

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.

None yet

2 participants