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 prepdocs compatibility with openai key and add test #573

Closed
wants to merge 0 commits into from
Closed

Fix prepdocs compatibility with openai key and add test #573

wants to merge 0 commits into from

Conversation

pamelafox
Copy link
Collaborator

@pamelafox pamelafox commented Aug 31, 2023

Purpose

This PR fixes an issue mentioned in #431 (comment), adds tests for compute_embedding, and fixes an issue I found while writing tests.

I also made a few changes to increase the testability of prepdocs.py, like reducing the number of globals.

Does this introduce a breaking change?

[ ] Yes
[X] No

Pull Request Type

What kind of change does this Pull Request introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ ] Other... Please describe:

How to Test

  • python3 -m pytest
  • ./scripts/prepdocs.py

yield section

def before_retry_sleep(retry_state):
if args.verbose: print("Rate limited on the OpenAI embeddings API, sleeping before retrying...")

@retry(wait=wait_random_exponential(min=1, max=60), stop=stop_after_attempt(15), before_sleep=before_retry_sleep)
def compute_embedding(text):
@retry(retry=retry_if_exception_type(openai.error.RateLimitError), wait=wait_random_exponential(min=1, max=60), stop=stop_after_attempt(15), before_sleep=before_retry_sleep)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was also retrying for AuthenticationError before, so I've made it more specific now. No sense retrying 15 times for an auth error.

wait_random_exponential,
)

args = argparse.Namespace(verbose=False)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would prefer to not have args be a global at all, but at least this makes it possible to test.

@pamelafox
Copy link
Collaborator Author

Re-created in #605

ratkinsoncinz pushed a commit to cinzlab/govgpt-azure-search-openai-demo that referenced this pull request Oct 6, 2024
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.

1 participant