Fix: Add rate limit to OpenAIAnswerGenerator, change token computation #3078
Conversation
| presence_penalty: float = -2.0, | ||
| frequency_penalty: float = -2.0, | ||
| max_calls_per_minute: int = 60, |
There was a problem hiding this comment.
Shouldn't we make the default here 3000? I'd prefer having here the "production" setting for a paid OpenAI account. For people on the free trial we can make the error message that you added further down more descriptive.
| try: | ||
| response = requests.request("POST", url, headers=headers, data=json.dumps(payload)) | ||
| time.sleep(60 / self.max_calls_per_minute) | ||
| except Exception as e: |
There was a problem hiding this comment.
Can we catch the exception type / API response code for rate-limits? If yes, we can make the error message here more actionable like "Seems like the OpenAI API rate-limited your requests. See limits of plans here (link) and consider setting max_calls_per_minute to a lower value (currently: XX) when initializing the OpenAIAnswerGenerator node in haystack`
There was a problem hiding this comment.
Sorry, this try catch is was not meant for catching the rate limit.
But I see the value of catching the rate limit as you suggest.
I think there is the need to catch more general problems, since when I was running eval after adjusting the rate limit, the API also ran into errors from time to time. That's why here is a very general try-catch block.
There was a problem hiding this comment.
I agree with @tholor's comments. Also, we need to fix the import order as requested by pylint and generate new schemas.
| @@ -191,7 +209,7 @@ def _build_prompt(self, query: str, documents: List[Document]) -> Tuple[str, Lis | |||
| ) | |||
|
|
|||
| # Top ranked documents should go at the end | |||
| context = " ".join(reversed(input_docs_content)) | |||
| context = "\n".join(reversed(input_docs_content)) | |||
There was a problem hiding this comment.
We use " " here as this is what's used in the transition guide for transition from answers endpoint to completions endpoint (see here for the transition guide and here for the place where " " is used).
What was your motivation for changing this to "\n"? Anyway, I assume that this change won't affect the the way the answers are produced, as tokenization should treat both " " and "\n" the same way.
There was a problem hiding this comment.
My motivation was to make it more readable. With the newline you can see very quickly the different articles in the context.
|
Sorry, I realize I won't be able to work on this the next week. I can either look into it afterward or you start taking over? |
|
Closing this in favor of #3398 |
Related Issues
temperatureparam and adjust defaults forOpenAIAnswerGenerator#3073Proposed Changes:
How did you test it?
manual verification
Checklist