Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.24k stars 4.58k forks source link

Azure.AI.OpenAI ClientOptions doesn't work well with ASP.NET Core and Microsoft.Extensions #45066

Open eerhardt opened 1 month ago

eerhardt commented 1 month ago

Many important properties on AzureOpenAIClientOptions are inherited from OpenAIClientOptions. These properties use get; init; which causes problems when trying to configure these options in ASP.NET Core apps.

public partial class OpenAIClientOptions : ClientPipelineOptions
{
    /// <summary>
    /// A non-default base endpoint that clients should use when connecting.
    /// </summary>
    public Uri Endpoint { get; init; }

    /// <summary>
    /// An optional application ID to use as part of the request User-Agent header.
    /// </summary>
    public string ApplicationId { get; init; }

    /// <summary>
    /// An optional ID added to OpenAI-Organization header
    /// </summary>
    public string OrganizationId { get; init; }

    /// <summary>
    /// An optional ID added to OpenAI-Project header
    /// </summary>
    public string ProjectId { get; init; }
}

For example, when using Microsoft.Extensions.Azure to add an AzureOpenAIClient, it is common to bind these client options to an IConfiguration. See

https://github.com/Azure/azure-sdk-for-net/blob/090ea968785f42b4602082ee40d6a5b85ae3f6e3/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientBuilderExtensions.cs#L84-L87

or you can provide a configureOptions callback to set these values:

https://github.com/Azure/azure-sdk-for-net/blob/090ea968785f42b4602082ee40d6a5b85ae3f6e3/sdk/extensions/Microsoft.Extensions.Azure/src/AzureClientBuilderExtensions.cs#L61-L69

However, with OpenAIClientOptions and init only properties, you can't set these properties after the ClientOptions object has been created. Meaning you can't configure these values in an ASP.NET app using Microsoft.Extensions.Azure.

We should consider making these properties normal get;set; properties, so they can be configured like other "ClientOptions" objects in the Azure SDK.

cc @KrzysztofCwalina @tg-msft @annelo-msft @damianedwards @davidfowl

github-actions[bot] commented 1 month ago

Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @jpalvarezl @ralph-msft @trrwilson.

annelo-msft commented 1 month ago

Hi @eerhardt -- adding @joseharriaga as FYI as well, as he has been evaluating using get; init; for various types in the OpenAI client that Azure.AI.OpenAI inherits from.

From a System.ClientModel perspective, nothing about ClientPipelineOptions that OpenAIClientOptions inherits from requires that these properties use init, so we could feasibly change that. However, ClientPipelineOptions does have a feature where, if properties on it are changed after a ClientPipeline is constructed from the options, it will throw. So, the properties can be get; set; and changed up until the point that the pipeline is created, and not after. Pipeline creation typically happens in a client constructor.

In Azure.Core we have effectively the same functionality -- while options properties can technically be changed after pipeline construction, if the client is implemented correctly, any changes to the options will not have an effect on the pipeline -- if they do then the client is likely violating a thread safety guarantee. So in Azure clients, it's effectively a silent failure whereas in SCM-based clients, the user is notified.

I'll reach out to you offline to learn more about the configuration sequencing in ASP.NET to see if we can find a workable solution here. Thanks!

trrwilson commented 1 month ago

@eerhardt, per the breakdown from @annelo-msft, do you believe it'd be sufficient to have these properties settable up until the first pipeline (client) is created using said properties? That is, if we roughly had:

AzureOpenAIClientOptions options = new()
{
    ApplicationId = "init here",
}
options.ApplicationId = "set here, works before pipeline creation";
AzureOpenAIClient azureClient = new(aoaiResourceEndpoint, credential, options);
options.ApplicationId = "!!! this and all subsequent sets will throw an exception !!!";

...would that then resolve the configuration needs as you see them?

eerhardt commented 1 month ago

...would that then resolve the configuration needs as you see them?

Yes, I think that would address this issue.

We have a few classes that have this kind of behavior, the one that jumps out to me is JsonSerializerOptions. You can create and modify the options up until the point where you use it to serialize/deserialize an object. After that it becomes read-only and any attempts at modification will throw an exception.

Some thoughts to consider that make the JsonSerializerOptions object a bit easier to work with: