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 response UnboundError when request fails and custom tenacity.Retrying is used #713

Merged
merged 2 commits into from
May 31, 2024

Conversation

lazyhope
Copy link
Contributor

@lazyhope lazyhope commented May 29, 2024

This PR closes #693
It also removes InstructorRetryException inside with attempts clause as it seems redundant.


🚀 This description was created by Ellipsis for commit dc467dd

Summary:

This PR fixes an UnboundError by initializing response to None in retry functions and simplifies error handling by removing redundant exception raising, with added tests for verification.

Key points:

  • Initialize response to None in retry_sync and retry_async functions in instructor/retry.py to prevent UnboundError.
  • Remove redundant InstructorRetryException raising within retry loops, simplifying error handling.
  • Update test_openai/test_retries.py with a new test case test_custom_retry_response_error to verify correct error handling with custom retry strategy.

Generated with ❤️ by ellipsis.dev

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 dc467dd in 35 seconds

More details
  • Looked at 89 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. instructor/retry.py:144
  • Draft comment:
    Initializing response to None before the retry loop is a good practice to ensure it has a default value, preventing UnboundLocalError if it's accessed before being assigned in any retry attempts.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR description mentions that the InstructorRetryException inside each with attempt clause is redundant and has been removed. However, the PR does not address the issue of the UnboundLocalError that was originally reported in the issue. The PR adds a response = None initialization before the retry loops in both retry_sync and retry_async functions, which should prevent the UnboundLocalError from occurring when the response variable is accessed in the exception handling outside the loop. This seems to be a valid fix for the issue reported.

Workflow ID: wflow_VqX7X11xaQjOYP1G


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

@lazyhope
Copy link
Contributor Author

@jxnl Thanks for the review, I don't have write access so please feel free to merge it

@jxnl jxnl merged commit 3d8d449 into jxnl:main May 31, 2024
8 of 12 checks passed
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.

Error handling broken in retry.py under certain circumstances
2 participants