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

.Net: Disable Azure SDK network timeout when a custom HttpClient is supplied #5553

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

stephentoub
Copy link
Member

Fixes #5310

@stephentoub stephentoub requested a review from a team as a code owner March 19, 2024 17:00
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Mar 19, 2024
@github-actions github-actions bot changed the title Disable Azure SDK network timeout when a custom HttpClient is supplied .Net: Disable Azure SDK network timeout when a custom HttpClient is supplied Mar 19, 2024
@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Mar 20, 2024
Merged via the queue into microsoft:main with commit 02866be Mar 20, 2024
20 checks passed
@@ -749,6 +749,7 @@ internal static OpenAIClientOptions GetOpenAIClientOptions(HttpClient? httpClien
{
options.Transport = new HttpClientTransport(httpClient);
options.RetryPolicy = new RetryPolicy(maxRetries: 0); // Disable Azure SDK retry policy if and only if a custom HttpClient is provided.
options.Retry.NetworkTimeout = Timeout.InfiniteTimeSpan; // Disable Azure SDK default timeout
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit late with the review but I would like to point out that there's a similar code over here - AzureOpenAITextToImageService GetClientOptions that needs to be updated as well.

Choose a reason for hiding this comment

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

Created this #5569

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we do #5570 instead? It'd be good to avoid duplicated logic like this whenever possible. For example, the text-to-image one was also missing the header policy the other was adding... was that omission on purpose?

@stephentoub stephentoub deleted the fixtimeout branch March 20, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HttpClient 1m40s (100s) timeout
4 participants