OpenAIClient is not respecting HttpClient from DI

[stephentoub]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

The OpenAIClient that Aspire registers in DI doesn't respect other services registered in DI, in particular HttpClient. That means any configuration for HttpClient, such as resiliency pipelines from Microsoft.Extensions.Http.Resilience, an ILogger plugged into that HttpClient's logging, and so on, end up not being respected.

Expected Behavior

If no options were provided in DI explicitly, the Aspire component should respect any HttpClient that was registered in DI. Here's what Semantic Kernel does, for example:
https://github.com/microsoft/semantic-kernel/blob/cd40e2e079f1a657b25334e004f3c2e36f24a59c/dotnet/src/Agents/OpenAI/OpenAIClientProvider.cs#L130-L140
turning off the built-in resiliency mechanisms of OpenAIClient and instead deferring to the HttpClient and the policies configured for it.

    private static void ConfigureClientOptions(HttpClient? httpClient, ClientPipelineOptions options)
    {
        options.AddPolicy(CreateRequestHeaderPolicy(HttpHeaderConstant.Names.SemanticKernelVersion, HttpHeaderConstant.Values.GetAssemblyVersion(typeof(OpenAIAssistantAgent))), PipelinePosition.PerCall);

        if (httpClient is not null)
        {
            options.Transport = new HttpClientPipelineTransport(httpClient);
            options.RetryPolicy = new ClientRetryPolicy(maxRetries: 0); // Disable retry policy if and only if a custom HttpClient is provided.
            options.NetworkTimeout = Timeout.InfiniteTimeSpan; // Disable default timeout
        }
    }

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version info

No response

Anything else?

No response

[eerhardt]

@annelo-msft @tg-msft @jsquire @KrzysztofCwalina @davidfowl @DamianEdwards - thoughts on how to think about the difference between the Azure.Core / System.ClientModel Http pipeline and the Http options configured via Microsoft.Extensions.Http (for example Microsoft.Extensions.Http.Resilience)?

[annelo-msft]

We are currently working on AddClient extensions for SCM-based clients to support this and other DI scenarios. These will support scenarios like this out of the box, and we hope the Aspire team will call these once available from Aspire component extensions.

In the meantime, I believe there's a relatively straightforward short term fix -- in AspireOpenAIExtensions.cs, I believe you should be able to check whether an HttpClient has been added to the service collection, and if it has, add a custom transport to the client options that uses the injected HttpClient. The latter would look similar to the sample @stephentoub shared that SK is doing:

  options.Transport = new HttpClientPipelineTransport(httpClient);

Let me know if this approach won't work for some reason. Thanks!

[annelo-msft]

Per offline conversation, this approach risks overwriting important Azure/SCM client default HttpClient settings, as well as silently ignoring service-specific resiliency settings provided by client authors. We will need to evaluate and make a plan for dealing with redundant functionality provided by Azure.Core/SCM defaults and Azure/SCM client authors vs an injected HttpClient before considering this a viable approach.

[stephentoub]

Per offline conversation, this approach risks overwriting important Azure/SCM client default HttpClient settings, as well as silently ignoring service-specific resiliency settings provided by client authors. We will need to evaluate and make a plan for dealing with redundant functionality provided by Azure.Core/SCM defaults and Azure/SCM client authors vs an injected HttpClient before considering this a viable approach.

I think there are two conversations here: generally with regards to any possible SCM use, and specifically with regards to OpenAI.

Are there OpenAI-specific policies that the approach in the opening post will overwrite?

My take is that this is an Aspire component, and Aspire has opinions about how all of this stuff should behave, which includes registering resilience policies in DI and using HttpClient from DI. Unless there's something else we're concerned would get overwritten, I'd posit the right answer for now is for the Aspire.OpenAI component to grab the HttpClient from DI and use it to configure the OpenAIClientOptions, including disabling the OpenAIClient's built-in resilience mechanisms.

[annelo-msft]

SCM does add custom settings to its default HttpClient in the same way that Azure.Core does. We would need to provide a mechanism to maintain these settings.

I am also concerned that -- while there aren't policies today I'm aware of that have conflicting functionality with HttpClient -- this space is rapidly evolving and it seems likely that we could discover a need to customize e.g. retriable status codes. Aspire may have a specific approach to this, but the OpenAI .NET client needs to function correctly outside of an Aspire context as well. Depending on custom HttpClient injection prior to addressing the "merge functional conflicts" problem could cause changes/bug fixes made to the OpenAI .NET client to fail in Aspire components.

[stephentoub]

SCM does add custom settings to its default HttpClient in the same way that Azure.Core does. We would need to provide a mechanism to maintain these settings.

Specific to OpenAIClient, can you elaborate on exactly what these are?

This issue is in the context of Aspire, and from my perspective one of the main purposes of Aspire integrations is to configure components in a consistent manner throughout Aspire apps. That includes defaulting the HTTP pipeline to include standard resiliency mechanisms, and respecting any and all configuration others do to the HTTP pipeline in the DI container. If someone wants to inject their own OpenAIClient with their own policy, they can, rather than using the Aspire helper to do so. If someone wants to configure the injected OpenAIClienOptions so that the Aspire helper uses that when constructing a client, they can. I simply want it to be case that doing the default thing with the Aspire.OpenAI component respects what's in DI. I find it very surprising when I resolve one of these OpenAIClients the Aspire component configured that it doesn't pay attention to any of the other handiers I've explicitly configured in DI via the rest of the Microsoft.Extensions.* libraries.

If the Aspire component doesn't do this, I think we'll find that customers do. Customers already are, as highlighted by the SK example.

[annelo-msft]

one of the main purposes of Aspire integrations is to configure components in a consistent manner throughout Aspire apps

I love consistency! But it wouldn't be consistent with Azure components, correct? If we haven't solved the general problem for Azure.Core?

[stephentoub]

one of the main purposes of Aspire integrations is to configure components in a consistent manner throughout Aspire apps

I love consistency! But it wouldn't be consistent with Azure components, correct? If we haven't solved the general problem for Azure.Core?

I think the Azure.Core ones should default to using HttpClient from DI as well.

But OpenAIClient is also not Azure. Yes, Azure has a derivative, but we shouldn't be making decisions about OpenAI based on what Azure's needs are.

[annelo-msft]

we shouldn't be making decisions about OpenAI based on what Azure's needs are

Not in the abstract sense, no -- I completely agree that conceptually Azure and third-party services should be independent. But in this concrete instance, both the OpenAI .NET client and the Azure.AI.OpenAI .NET client are SCM-based clients, and I believe SCM-based clients should all behave the same way, regardless of what platform users are working with. Azure clients being Azure.Core-based make them SCM-based as well, so I think it will be broadly beneficial to solve the SCM/HttpClient integration problem before moving forward with this.

One example of an undesirable behavior that could happen today by injecting HttpClient as currently proposed for OpenAI is if an end-user wanted to customize the OpenAI client retry policy. Aspire setting a custom retry policy on the OpenAI client to disable the SCM pipeline's retry policy would overwrite the user-specified policy. I think at a minimum we would want to detect this case and throw an exception to allow the application developer to intervene, vs. failing silently.