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 max_retries to AzureOpenAIGenerator #7983

Merged

Conversation

nvzard
Copy link
Contributor

@nvzard nvzard commented Jul 5, 2024

Related Issues

Proposed Changes:

AzureOpenAIGenerator can be initiated using max_retries.

How did you test it?

Unit tests

Notes for the reviewer

I thought it'd be a good idea also to return max_retries when to_dict is called since we are doing it for timeout as well. Let me know if this is not required.

Checklist

@nvzard nvzard requested a review from a team as a code owner July 5, 2024 09:00
@nvzard nvzard requested review from vblagoje and removed request for a team July 5, 2024 09:00
@CLAassistant
Copy link

CLAassistant commented Jul 5, 2024

CLA assistant check
All committers have signed the CLA.

@nvzard
Copy link
Contributor Author

nvzard commented Jul 5, 2024

I've signed the CLA.

Copy link
Member

@vblagoje vblagoje 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, let's just make this one simplification suggested

haystack/components/generators/azure.py Outdated Show resolved Hide resolved
@nvzard nvzard force-pushed the feat-azure-open-ai-generator-max-retries branch from f315bfa to 6338aa2 Compare July 5, 2024 10:40
@nvzard nvzard requested a review from vblagoje July 5, 2024 11:13
@vblagoje
Copy link
Member

vblagoje commented Jul 5, 2024

We need to add reno note @nvzard See https://github.com/deepset-ai/haystack/blob/main/CONTRIBUTING.md for more details

@nvzard nvzard force-pushed the feat-azure-open-ai-generator-max-retries branch from 6338aa2 to b77ec92 Compare July 5, 2024 11:58
@nvzard nvzard requested a review from a team as a code owner July 5, 2024 11:58
@nvzard nvzard requested review from dfokina and removed request for a team July 5, 2024 11:58
@nvzard
Copy link
Contributor Author

nvzard commented Jul 5, 2024

My bad @vblagoje, I totally forgot about it.

@vblagoje
Copy link
Member

vblagoje commented Jul 5, 2024

There are still some failures @nvzard

@anakin87
Copy link
Member

anakin87 commented Jul 5, 2024

Please let's also read the timeout from an env var if not set at init, as we do in OpenAIGenerator.

@nvzard nvzard force-pushed the feat-azure-open-ai-generator-max-retries branch 2 times, most recently from fbf9409 to 9d52a77 Compare July 6, 2024 06:23
@nvzard
Copy link
Contributor Author

nvzard commented Jul 6, 2024

Good catch @anakin87. I've added it as well.

All tests are passing locally now 🤞🏻

@coveralls
Copy link
Collaborator

coveralls commented Jul 6, 2024

Pull Request Test Coverage Report for Build 9817242905

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.001%) to 90.004%

Files with Coverage Reduction New Missed Lines %
components/generators/azure.py 3 92.5%
Totals Coverage Status
Change from base Build 9804009410: 0.001%
Covered Lines: 6771
Relevant Lines: 7523

💛 - Coveralls

max_retries: if not set is read from the OPENAI_MAX_RETRIES
env variable or set to 5.

timeout: if not set is read from the OPENAI_TIMEOUT
env variable or set to 30.

Signed-off-by: Nitanshu Vashistha <[email protected]>
@nvzard nvzard force-pushed the feat-azure-open-ai-generator-max-retries branch from 9d52a77 to 1f3dbdc Compare July 7, 2024 14:41
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9828135301

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.001%) to 90.004%

Files with Coverage Reduction New Missed Lines %
components/generators/azure.py 3 92.5%
Totals Coverage Status
Change from base Build 9804009410: 0.001%
Covered Lines: 6771
Relevant Lines: 7523

💛 - Coveralls

Copy link
Member

@vblagoje vblagoje left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution @nvzard

@vblagoje vblagoje merged commit 167e886 into deepset-ai:main Jul 8, 2024
17 checks passed
@nvzard
Copy link
Contributor Author

nvzard commented Jul 8, 2024

Thanks for your patience and reviews 😄

@nvzard nvzard deleted the feat-azure-open-ai-generator-max-retries branch July 8, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add max_retries and timeout params to all AzureOpenAI classes
5 participants