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: increase retry limit and provide override #61

Merged
merged 9 commits into from
May 8, 2023

Conversation

dciborow
Copy link
Collaborator

@dciborow dciborow commented May 8, 2023

Description

Testing

Additional context

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Summary by GPT-4

I have added a new constant MAX_RETRIES in the constants.py file and set its value to 10 by default. This value can be overridden by setting the environment variable MAX_RETRIES. I have also updated the _ask.py and _llama_index.py files to use this constant instead of hardcoded values.

Here's the updated code:

constants.py

"""Contains constants for minimum and maximum values of various parameters used in GPT Review."""
import os
import sys

MAX_TOKENS_DEFAULT = 100
...
PRESENCE_PENALTY_DEFAULT = 0
PRESENCE_PENALTY_MIN = 0
PRESENCE_PENALTY_MAX = 2

MAX_RETRIES = int(os.getenv("MAX_RETRIES", 10))

_ask.py

...
from gpt_review.constants import MAX_RETRIES

def _call_gpt(
    ...
    try:
        ...
    except RateLimitError as error:
        if retry < MAX_RETRIES:
            logging.warning("Call to GPT failed due to rate limit, retry attempt %s of %s", retry, MAX_RETRIES)
            time.sleep(retry * 10)
            return _call_gpt(prompt, temperature, max_tokens, top_p, frequency_penalty, presence_penalty, retry + 1)
        raise RateLimitError("Retry limit exceeded") from error

_llama_index.py

...
from gpt_review.constants import MAX_RETRIES

def _document_indexer(
    ...
    service_context = ServiceContext(
        ...
        max_retries=MAX_RETRIES,
    )
    ...

Now you can set the environment variable MAX_RETRIES to change the maximum number of retries for rate-limited requests.

Suggestions

Here are some suggestions for improving the changes in this PR:

  1. In the _call_gpt function, instead of using C.MAX_RETRIES, you can use a default argument for the retry parameter. This way, you can easily change the maximum number of retries without modifying the constants file.
def _call_gpt(
    prompt: str,
    temperature: float,
    max_tokens: int,
    top_p: float,
    frequency_penalty: float,
    presence_penalty: float,
    retry: int = 0,
    max_retries: int = 5
) -> str:
  1. Update the log message in _call_gpt to include the maximum number of retries:
logging.warning("Call to GPT failed due to rate limit, retry attempt %s of %s", retry, max_retries)
  1. In _document_indexer, instead of importing and using C.MAX_RETRIES, you can pass it as an argument to the function:
def _document_indexer(files: List[str], fast: bool = False, large: bool = False, max_retries: int = 10) -> str:
  1. Remove the MAX_RETRIES constant from constants.py since it's no longer needed.

With these changes, you can easily modify the maximum number of retries for each function without affecting other parts of your code that use constants.

src/gpt_review/constants.py Outdated Show resolved Hide resolved
src/gpt_review/_ask.py Outdated Show resolved Hide resolved
@dciborow dciborow requested a review from danay1999 May 8, 2023 21:18
src/gpt_review/_ask.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #61 (7630712) into main (773cf6e) will decrease coverage by 1.42%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main      #61      +/-   ##
==========================================
- Coverage   86.47%   85.06%   -1.42%     
==========================================
  Files          13       13              
  Lines         392      395       +3     
  Branches       59       59              
==========================================
- Hits          339      336       -3     
- Misses         37       44       +7     
+ Partials       16       15       -1     
Flag Coverage Δ
integration ?
unittests 85.06% <60.00%> (+0.11%) ⬆️
unittests-3.10 85.06% <60.00%> (+0.11%) ⬆️
unittests-3.11 85.06% <60.00%> (+0.11%) ⬆️
unittests-3.8 85.06% <60.00%> (+0.11%) ⬆️
unittests-3.9 85.06% <60.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/gpt_review/_ask.py 82.71% <0.00%> (-4.94%) ⬇️
src/gpt_review/_llama_index.py 92.59% <100.00%> (-7.41%) ⬇️
src/gpt_review/constants.py 100.00% <100.00%> (ø)

@dciborow dciborow merged commit 8e83c58 into microsoft:main May 8, 2023
@dciborow dciborow deleted the dciborow/up-retry-limit branch May 8, 2023 21:48
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.

3 participants