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: Add retry with exponential back-off to PromptNode's OpenAI models #3886

Merged
merged 2 commits into from
Jan 19, 2023
Merged

feat: Add retry with exponential back-off to PromptNode's OpenAI models #3886

merged 2 commits into from
Jan 19, 2023

Conversation

vblagoje
Copy link
Member

Related Issues

Proposed Changes:

Added a well-known (to us at least) and previously used decorator @retry_with_exponential_backoff(backoff_in_seconds=10, max_retries=5) that handles the OpenAIRateLimitError with retry and exponential backoff

How did you test it?

Manual tests by forcing OpenAIRateLimitError

Notes for the reviewer

We used this decorator in other OpenAI APIs, I checked it manually and didn't add new tests. We'll see how this works in our CI ;-)

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added tests that demonstrate the correct behavior of the change
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue

@vblagoje vblagoje requested a review from a team as a code owner January 18, 2023 13:45
@vblagoje vblagoje requested review from ZanSara and removed request for a team January 18, 2023 13:45
@vblagoje vblagoje changed the title Add retry with exponential back-off to PromptNode's OpenAI models feat: Add retry with exponential back-off to PromptNode's OpenAI models Jan 18, 2023
@ZanSara
Copy link
Contributor

ZanSara commented Jan 18, 2023

I like the concept a lot but I think the backoff parameters should be controllable by the users. Right now 10 and 5 are really hardcoded and quite invisible... They would be hard to change even with reflection! Can we find a way (any, really) to let the user control such values? I'd be happy with a hacky-ish solution too (env vars, module constants, ...)

@vblagoje
Copy link
Member Author

vblagoje commented Jan 18, 2023

In an ideal world, yes, but IMHO the cost-benefit of providing such a fine detail of control might be an overkill. IIRC, Bogdan and I worked on this feature; users have used it in other components (EmbeddingEncoder) for some time now, and we have yet to receive such a request. Let's hear the opinion of others and proceed forward. cc @bogdankostic @masci

@bogdankostic
Copy link
Contributor

For context: we had a similar discussion in #3398.

@vblagoje
Copy link
Member Author

I remember now, thanks! Maybe env variables can be used? I'll check tomorrow

@vblagoje
Copy link
Member Author

vblagoje commented Jan 19, 2023

Ok, I stand corrected. We can use env vars with defaults - and we should. See 962281a

@vblagoje
Copy link
Member Author

What would be the best names for these two variables? Let's use them not only for OPENAI remote models but also for all remote model invocations. If we go down naming env vars for backoff and max retries for every model, we'll have many of these env vars.

@vblagoje
Copy link
Member Author

@ZanSara @bogdankostic @sjrl @masci I came up with these names. Please feel free to suggest others. I liked this approach because all env vars are imported in prompt_node.py with from haystack.environment, which is nice. This PR will help the CI as well. Let's introduce the same env vars in other places but in a separate refactor PR.

Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

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

Great! Looks like a good solution to me 😊

Just as a follow up, do we set these env vars to 0 in the CI? I think that might help speed the tests up a bit

@vblagoje
Copy link
Member Author

Cool, I would leave them either as is or lower HAYSTACK_REMOTE_API_BACKOFF_SEC to 1 or 2 seconds and leave HAYSTACK_REMOTE_API_MAX_RETRIES at 5. If HAYSTACK_REMOTE_API_BACKOFF_SEC is 0, we will attempt another remote call almost immediately. Even with exponential back-off, such a small value would cause rapid-fire re-attempts. We can even leave it as is, and then if we notice issues, we can fix them.

@vblagoje
Copy link
Member Author

Yeah, in fact let's set HAYSTACK_REMOTE_API_BACKOFF_SEC to 1 in CI

@vblagoje vblagoje merged commit 04deb3b into deepset-ai:main Jan 19, 2023
@vblagoje vblagoje deleted the prompt_node_retry branch March 31, 2023 20: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.

PromptNode should handle OpenAIRateLimitError with back-offs and retry inference attempt before giving up
3 participants