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

fix: max_retries typing #1135

Conversation

jordyjwilliams
Copy link
Contributor

@jordyjwilliams jordyjwilliams commented Oct 30, 2024

When working on a project and reviewing the docs I noticed that there were # type: ignore references when using a tenacity Retrying | AsyncRetrying object.

This PR aims to fix the underlying type issues as it seems max_retries can always be one of: int | Retrying | AsyncRetrying


Important

Fix max_retries typing to accept int, Retrying, or AsyncRetrying in client.py and patch.py.

  • Typing Fix:
    • Update max_retries parameter type to int | Retrying | AsyncRetrying in create, create_partial, create_iterable, and create_with_completion functions in client.py.
    • Update max_retries parameter type to int | Retrying | AsyncRetrying in new_create_async and new_create_sync functions in patch.py.
  • Imports:
    • Add Retrying and AsyncRetrying imports from tenacity in client.py and patch.py.

This description was created by Ellipsis for fce914c. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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! Reviewed everything up to fce914c in 20 seconds

More details
  • Looked at 230 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. instructor/patch.py:43
  • Draft comment:
    Ensure that max_retries is handled correctly when it is an instance of Retrying or AsyncRetrying. The current logic seems to assume it is always an integer.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The type hint for max_retries is updated to include Retrying and AsyncRetrying, but the logic for handling these types is not clear. The code should ensure that max_retries is used correctly when it is an instance of Retrying or AsyncRetrying.
2. instructor/client.py:118
  • Draft comment:
    The max_retries parameter has been updated to accept int | Retrying | AsyncRetrying in multiple functions. Ensure the documentation is updated to reflect this change.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. instructor/patch.py:43
  • Draft comment:
    The max_retries parameter has been updated to accept int | Retrying | AsyncRetrying in multiple functions. Ensure the tests are updated to reflect this change.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_aijkg4LNjdfPE93L


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@jordyjwilliams jordyjwilliams changed the title feat: fix max_retries typing fix: max_retries typing Oct 30, 2024
@jordyjwilliams
Copy link
Contributor Author

@jxnl I am aware that this may be far too simplistic of an approach. from Contributions and existing issues I am not sure if this was a known issue or I could be missing something here.

I am just trying to fix the need for a type: ignore within a project where we use instructor. Happy to address this in a different way or completely re-evaluate if I have missed something here!

Cheers!

@ivanleomk
Copy link
Collaborator

Modifying the typehints since AsyncRetrying is really only for async clients so we should be selective about the typing here

Link : https://python.useinstructor.com/concepts/retrying/#other-features-of-tenacity

@jordyjwilliams
Copy link
Contributor Author

@ivanleomk ahh I missed this, thanks a lot. Appreciate the commits and review

Copy link
Collaborator

@ivanleomk ivanleomk left a comment

Choose a reason for hiding this comment

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

CI is good to go, going to approve this. Thanks for helping to fix the typing.

@ivanleomk ivanleomk merged commit 4327e14 into instructor-ai:main Nov 8, 2024
15 of 16 checks passed
@jordyjwilliams
Copy link
Contributor Author

No worries at all. Thanks for the approval!

@jordyjwilliams jordyjwilliams deleted the chore-000_fix_type_issue_max_retries branch November 8, 2024 06:41
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