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

feat: new implementation of timeout functionality for all toolkits #1595

Merged
merged 13 commits into from
Feb 18, 2025

Conversation

MuggleJinx
Copy link
Collaborator

Description

New implementation of #1519 and #1507. Adding timeout function to all toolkits.

Checklist

  • I have read the CONTRIBUTION guide (required)
  • I have linked this PR to an issue using the Development section on the right sidebar or by adding Fixes #issue-number in the PR description (required)
  • I have checked if any dependencies need to be added or updated in pyproject.toml and poetry.lock
  • I have updated the tests accordingly (required for a bug fix or a new feature)
  • I have updated the documentation if needed:
  • I have added examples if this is a new feature

If you are unsure about any of these, don't hesitate to ask. We are here to help!

@MuggleJinx MuggleJinx self-assigned this Feb 13, 2025
@MuggleJinx MuggleJinx mentioned this pull request Feb 13, 2025
13 tasks
def with_timeout(func):
@functools.wraps(func)
def wrapper(self, *args, **kwargs):
timeout = getattr(self, "timeout", None)
Copy link
Member

Choose a reason for hiding this comment

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

Can we put timeout as the input argument? Also can we move this into utils so we can use this for other modules?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now the specification is that timeout is an argument in toolkit initialization. Do you mean to have it in every member function of the toolkits class?

Copy link
Member

Choose a reason for hiding this comment

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

update here: 0fc43f7
and will merge this PR, thanks @MuggleJinx for the contribution! Let me know if you have any further questions~

Copy link
Member

@lightaime lightaime left a comment

Choose a reason for hiding this comment

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

Thanks @MuggleJinx. Left some comments

@Wendong-Fan Wendong-Fan merged commit 02bf930 into master Feb 18, 2025
4 of 6 checks passed
@Wendong-Fan Wendong-Fan deleted the toolkit_timeout_new branch February 18, 2025 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
3 participants