Add AddAzureOpenAIChatClient #6225

SteveSandersonMS · 2024-10-10T08:43:19Z

Description

This is a starting point based on the discussion earlier this week with @eerhardt, @sebastienros, and @luisquintanilla.

It allows developers to register an IChatClient in client projects via:

builder.AddAzureOpenAIChatClient("openai");

// Or with custom pipeline:
builder.AddAzureOpenAIChatClient("openai", pipeline => pipeline
    .UseChatOptions(...)
    .UseFunctionCalling());

So far I only did this for Azure OpenAI but once we've agreed on the shape would do the equivalent for OpenAI too.

Design questions

Lifetime

Currently this registers the IChatClient as singleton. That differs from prior assumptions in the MEAI repo that it would be scoped (cc: @stephentoub). The reason I did singleton here is that the underlying OpenAIClient is singleton, and being singleton and thread-safe is cited as a core tenet of Azure client libraries.

I suspect we should try to create an expectation that IChatClient implementations should be designed to be singleton and thread-safe. Anyone who finds they need per-usage-site state can store it in ChatOptions (e.g., AdditionalProperties). If this turns out to be impractical then we will have to reconsider this design. But it's better to start with singleton and later relax it to be scoped than to go in the other direction.

Also filed dotnet/extensions#5499 to track on the MEAI side validating this for the existing IChatClient implementations.

If I'm misjudging this and Aspire is totally OK with client libraries registering as scoped, we can reconsider.

APIs for chaining on `AddAzureOpenAIClient`

It's also possible we might want to enable an API pattern like this:

builder.AddAzureOpenAIClient("openai")
    .AddChatClient()          // Optionally also pass model/deployment or we'll get it from config
    .AddEmbeddingGenerator(); // Likewise

... instead of requiring two top-level calls (AddAzureOpenAIChatClient and AddAzureOpenAIEmbeddingGenerator). However that would involve changing the return type of AddAzureOpenAIClient from void to some builder we can put AddChatClient and AddEmbeddingGenerator extensions onto.

I'm fine with doing this if we like it. There's an argument for still also supporting the top-level calls for equivalence with AddOllamaChatClient etc.

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No but will do when API shape is agreed
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No (justification: it's just registering DI services, which the developer would otherwise do manually and get the same end result)
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue: We need to confirm the API first
- No

Microsoft Reviewers: Open in CodeFlow

SteveSandersonMS · 2024-10-10T08:59:25Z

BTW does anyone know why Microsoft.Extensions.AI is not on the dotnet-public feed at https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json? Since prerelease builds of M.E.DI.Abstractions are there, I'd expect M.E.AI to be there too.

stephentoub · 2024-10-10T13:26:45Z

Currently this registers the IChatClient as singleton.

Thank you 😄

stephentoub · 2024-10-10T13:27:07Z

BTW does anyone know why Microsoft.Extensions.AI is not on the dotnet-public feed at https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json? Since prerelease builds of M.E.DI.Abstractions are there, I'd expect M.E.AI to be there too.

Most likely no one requested it to be mirrored there. I can do so. But for future reference: https://github.com/dotnet/arcade/blob/main/Documentation/MirroringPackages.md

sebastienros · 2024-10-10T21:29:10Z

@stephentoub, "/azp run"?

stephentoub · 2024-10-10T21:30:27Z

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

+    {
+        builder.AddAzureOpenAIClient(connectionName, configureSettings, configureClientBuilder);
+
+        builder.Services.AddSingleton(services =>


I assume if we change AddChatClient to be a singleton, this would instead use that?

Yes, it would be equivalent, so it might as well do.

stephentoub · 2024-10-10T21:31:03Z

@stephentoub, "/azp run"?

Possibly. I'm used to that not being appropriate in dotnet/runtime.

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

stephentoub · 2024-10-10T21:37:36Z

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

+        if (configuration.GetConnectionString(connectionName) is string connectionString)
+        {
+            var connectionBuilder = new DbConnectionStringBuilder { ConnectionString = connectionString };
+            deploymentName = (connectionBuilder[DeployentKey] ?? connectionBuilder[ModelKey])?.ToString();


It looks like DbConnectionStringBuilder's indexer actually throws rather than returning null if the keyword isn't found. Should these instead use TryGetValue?

https://github.com/dotnet/runtime/blob/e32148a8bd322f0b7cd5368d4c3810354d9fc14a/src/libraries/System.Data.Common/src/System/Data/Common/DbConnectionStringBuilder.cs#L78-L90

Example: we use connectionBuilder.TryGetValue(ConnectionStringEndpoint, out var endpoint) in AddOpenAIClientFromConfiguration.

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs

sebastienros · 2024-10-10T21:42:04Z

I think the builder alternative was suggested instead of a lambda in the extension method because we already have two optional lambdas in it (to customize option and settings).

    public static void AddAzureOpenAIClient(
        this IHostApplicationBuilder builder,
        string connectionName,
        Action<AzureOpenAISettings>? configureSettings = null,
        Action<IAzureClientBuilder<AzureOpenAIClient, AzureOpenAIClientOptions>>? configureClientBuilder = null)

stephentoub

Design looks reasonable to me.

sebastienros · 2024-10-10T21:52:31Z

src/Components/Aspire.OpenAI/MEAIPackageOverrides.targets

+    <PackageReference Include="Microsoft.Extensions.Primitives" VersionOverride="9.0.0-*" />
+    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" VersionOverride="9.0.0-*" />
+    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" VersionOverride="9.0.0-*" />
+    <PackageReference Include="System.Text.Json" VersionOverride="9.0.0-*" />


This PR should simplify that, right? Or do we also want to use the package in apps targeting 8.0?

Or do we also want to use the package in apps targeting 8.0?

Yes. We have no choice, because M.E.AI.Abstractions targets the 9.x versions of these packages (even on net8.0).

We shouldn't be using wildcards *s pacakge versions.

As for using STJ 9 on net8.0, I think that is something to think hard about in MEAI before shipping, since 9 is STS and 8 is LTS.

@SteveMCarroll

@stephentoub The current status for this is that MEAI.Abstractions will depend on STJ v8, whereas MEAI will depend on STJ v9, right?

If so then the Aspire integration package will still have to depend on STJv9 (even on net8.0), because it needs to depend on MEAI (not .Abstractions).

Is this going to be a problem?

@eerhardt Your comment above gave me the impression this was a concern. Can you let me know if it is?

Generally there are 2 concerns with lifting libraries out of the shared framework:

(technical) It is another .dll that needs to be distributed with your application. Also, it is no longer ReadyToRun'd because we aren't using the assembly in the shared framework (which is RID-specific and R2R'd).

(policy) the servicing lifetime of System.Text.Json 9.0 ends before the servicing lifetime of System.Text.Json 8.0. If you are staying on net8.0 TFM, you probably want to keep on a long-term support framework. But using MEAI (not .Abstractions) means that you are signing up for being on the short-term support System.Text.Json. Meaning you will need to move up to the v10 System.Text.Json faster than you would have if you stayed on the v8.

@eerhardt Thanks for clarifying those points. Sorry it's taken a while to get back to this (partly vacation, partly other stuff).

We don't have any proposed plan that I know of for not needing the STJv9 dependency here, since:

It's fundamental to these helpers that they integrate with DI

MEAI's way of doing that requires referencing Microsoft.Extensions.AI since that's where its DI integration lives

Microsoft.Extensions.AI references STJv9

Give this, and the fact that MEAI itself is not going to be in any LTS release prior to .NET 10, we can't provide an LTS-only set of dependencies.

The two concerns you raise certainly both sound valid, but do you see them just as concerns or as blockers in some sense? If you think neither of them actually block us from proceeding then we will do so. But if you think it's not that simple then perhaps we can set up a meeting to discuss.

do you see them just as concerns or as blockers in some sense?

They are concerns, not necessarily blockers. If MEAI decides that it requires STJv9 there isn't anything .NET Aspire can do about it (other than not take the dependency, which I'm not advocating for).

I do think this is an area that needs attention/thought across the whole org. In the past we've had different strategies for dependencies. And our current lifetime/servicing policies make it hard for libraries to depend on STS versions of OOB packages.

Thanks @eerhardt. Based on this I'm going to complete the PR using the API design here.

If anyone doesn't think this is the best API shape then please speak up asap!

SteveSandersonMS · 2024-10-10T21:53:49Z

I think the builder alternative was suggested instead of a lambda in the extension method because we already have two optional lambdas in it (to customize option and settings).

Can you give an example of how that would look?

If you mean something like this, we can certainly look at going that way. However it would prohibit this sort of calling pattern:

builder.AddAzureOpenAIClient("openai")
    .AddChatClient()
    .AddEmbeddingGenerator();

... because if AddChatClient returned something you can chain pipeline builder calls onto, then you wouldn't later be able to call AddEmbeddingGenerator.

SteveSandersonMS · 2024-10-16T07:50:17Z

I'm keen to finish off this PR but to avoid any wasted work can someone from the Aspire side acknowledge if you're happy with the API shape so far? cc @sebastienros @eerhardt @davidfowl

Specifically, this sort of registration usage:

builder.AddAzureOpenAIChatClient("openai");

Or with custom pipeline:

builder.AddAzureOpenAIChatClient("openai", pipeline => pipeline
    .UseChatOptions(...)
    .UseFunctionCalling()); // ... plus any other pipeline builder calls

Alternative

The only other main proposal is the following:

builder.AddAzureOpenAIClient("openai").AddChatClient(pipeline => pipeline
    .UseChatOptions(...)
    .UseFunctionCalling()); // ... plus any other pipeline builder calls

In this case we'd change the return type of AddAzureOpenAIClient/AddOpenAIClient from void to some builder we can put AddChatClient and AddEmbeddingGenerator onto. It has a drawback that you can't call both AddChatClient and AddEmbeddingGenerator without capturing the builder.AddAzureOpenAIClient("openai") result into a separate variable.

davidfowl · 2024-10-16T07:52:29Z

The alternative looks better. EF has a similar problem of 2 lambdas (to configure EF and the underlying provider) and the API does not feel as great.

SteveSandersonMS · 2024-10-16T07:53:44Z

@davidfowl Hold on, I updated the code above, after realising I was coupling together two separate design pivots. Can you re-check the code in that comment?

SteveSandersonMS · 2024-10-16T07:55:24Z

As for the question of whether the IChatClient pipeline should be configured via a lambda or should be chained onto the AddAzureOpenAIChatClient/AddChatClient call, this is a further design pivot covered by https://github.com/dotnet/temp-ai-abstractions/issues/296. Whichever way we go on https://github.com/dotnet/temp-ai-abstractions/issues/296 should result in the same choice on that point here.

In other words, even if that's our end goal we still have to decide whether there's a single AddAzureOpenAIChatClient call, or whether it's a pair (AddAzureOpenAIClient("...").AddChatClient()).

stephentoub · 2024-10-16T12:09:11Z

It has a drawback

We also need to revisit the Use method, which someone might chain at the end (assuming the previous methods return the same ChatClientBuilder) not realizing it's an expensive nop. Renaming it back to Build would help with that.

SteveSandersonMS · 2024-10-16T12:12:30Z

We also need to revisit the Use method, which someone might chain at the end

I agree. We should remove that method completely and make it a constructor parameter to ChatClientBuilder. Once we've settled the API design here, I'll make the API updates across this repo and dotnet/extensions.

sebastienros · 2024-10-16T17:01:36Z

I like both alternatives (with or without builder). But if we expect users to use mostly use chat or embedding clients directly instead of the OpenAIClient then would prefer the non-builder alternative and no lambda.

If we use a builder on AddAzureOpenAIClient then use the lambda instead of having to capture the builder (like we do for withCosmosDBEmulator(p => p.WithVolume())

davidfowl · 2024-10-16T17:45:59Z

If we use a builder on AddAzureOpenAIClient then use the lambda instead of having to capture the builder (like we do for withCosmosDBEmulator(p => p.WithVolume())

Except this isn't an app host API.

eerhardt · 2024-10-16T23:29:16Z

One concern I have is this method:

aspire/src/Components/Aspire.Azure.AI.OpenAI/AspireConfigurableOpenAIExtensions.cs

Lines 21 to 27 in 4f7dbd4

    
           /// <summary> 
        
           /// Registers <see cref="OpenAIClient"/> or <see cref="AzureOpenAIClient"/> as a singleton in the services provided by the <paramref name="builder"/>. 
        
           /// The concrete implementation is selected automatically from configuration. 
        
           /// </summary> 
        
           /// <param name="builder">The <see cref="IHostApplicationBuilder" /> to read config from and add services to.</param> 
        
           /// <param name="connectionName">A name used to retrieve the connection string from the ConnectionStrings configuration section.</param> 
        
           public static void AddOpenAIClientFromConfiguration(

Following the pattern of AddAzureOpenAIChatClient, we would need to duplicate this method for each type of client we wanted: chat, embedding, audio, etc. So we'd have:

AddAzureOpenAIChatClientFromConfiguration
AddAzureOpenAIEmdeddingClientFromConfiguration

Where as if adding the OpenAIClient was separated from adding the "child" chat, embedding, assistant, audio, etc. clients we wouldn't need to duplicate the "from configuration" part.

SteveSandersonMS · 2024-10-25T15:36:13Z

OK, following the feedback from @davidfowl and @eerhardt, I've updated the implementation to chain onto the AddAzureOpenAIClient calls, e.g.:

builder.AddAzureOpenAIClient("openai").AddChatClient("gpt-4o-mini", pipeline => pipeline
    .UseFunctionCalling()
    .UseSomeCustomMiddleware());

There's also a separate AddKeyedChatClient. This is applicable whether or not you're using AddKeyedAzureOpenAIClient so it takes its own independent service key.

If people are happy with this API shape, I'll proceed with implementing all the other combinations:

AzureOpenAI embeddings
OpenAI chat client
OpenAI embeddings

SteveSandersonMS · 2024-11-12T14:35:47Z

Updates

Following discussion here, the latest proposed shape for the Aspire integration is:

builder.AddAzureOpenAIClient("openai").AddChatClient("gpt-4o-mini", pipeline => pipeline
    .UseFunctionCalling()
    .UseOpenTelemetry());

I think that's fine but am still keen to consider the non-lambda alternative:

builder.AddAzureOpenAIClient("openai").AddChatClient("gpt-4o-mini")
    .UseFunctionCalling()
    .UseOpenTelemetry();

... or with @kzu's proposed simplification:

builder.AddAzureOpenAIClient("openai").AddChatClient("gpt-4o-mini")
    .UseDefaultPipeline();

I like UseDefaultPipeline because it avoids either being too labouriously explicit (.UseFunctionCalling().UseOpenTelemetry()) or too magic (eliding both of those and making them implicit, which is then confusing if you want to put something before or after them).

More complex case

Arguably this is also better even if you want to register multiple services. Before you'd have:

builder.AddAzureOpenAIClient("openai")
    .AddChatClient("gpt-4o-mini", pipeline => pipeline
        .UseFunctionCalling()
        .UseOpenTelemetry())
    .AddEmbeddingGenerator("deployment-name", pipeline => pipeline
        .UseOpenTelemetry());

That's a bit scary but still works as a single logical statement. It could conceptually be reduced to something like this to reduce the sense of nested lambdas:

builder.AddAzureOpenAIClient("openai")
    .AddChatClient("gpt-4o-mini", ChatClient.DefaultPipeline)
    .AddEmbeddingGenerator("deployment-name", EmbeddingGenerator.DefaultPipeline);

With the chaining above, you'd need a further variable:

var openai = builder.AddAzureOpenAIClient("openai");
openai.AddChatClient("gpt-4o-mini").UseDefaultPipeline();
openai.AddEmbeddingGenerator("deployment-name").UseDefaultPipeline();

I think this is at least equally clean, and given how this makes clear how you can chain in custom stuff before and after the default pipeline pieces, it feels good to me.

SteveSandersonMS · 2024-11-12T14:37:43Z

I think we want to move forwards with this as soon as we can, so I'm particularly looking for feedback from @eerhardt on the STJv9 dependency question and @stephentoub on the API shape.

eerhardt · 2024-11-12T23:29:12Z

I think that's fine but am still keen to consider the non-lambda alternative:

I like the non-lambda alternative personally. Yes it means you may have to capture something in a variable in order to do multiple, but IMO that is OK, you can only be so fluent.

I like UseDefaultPipeline because it avoids either being too labouriously explicit (.UseFunctionCalling().UseOpenTelemetry()) or too magic (eliding both of those and making them implicit, which is then confusing if you want to put something before or after them).

A principle of .NET Aspire is that telemetry is enabled by default, so I'm a little concerned that you would need to opt-in to telemetry by calling UseDefaultPipeline. We don't have anything else like this in .NET Aspire. Instead everything else has an opt-out. Would it be possible to have a .ClearPipeline() for the cases where you wanted to not use the default, or add something before the default? For example:

builder.AddAzureOpenAIClient("openai").AddChatClient("gpt-4o-mini")
    .ClearPipeline()
    .UseFunctionCalling();

would turn off telemetry, but keep function calling.

I think we want to move forwards with this as soon as we can

Have we given any more thought to the Hosting integration APIs? Specifically @sebastienros's proposal at Configure OpenAI models in the app host (dotnet/aspire#6577).

SteveSandersonMS requested a review from stephentoub October 10, 2024 08:43

SteveSandersonMS requested review from radical, eerhardt and sebastienros as code owners October 10, 2024 08:43

SteveSandersonMS mentioned this pull request Oct 10, 2024

Review all IChatClient implementations for thread-safety and suitability as singleton dotnet/extensions#5499

Closed

stephentoub closed this Oct 10, 2024

stephentoub reopened this Oct 10, 2024

stephentoub reviewed Oct 10, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 10, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 10, 2024

View reviewed changes

src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIChatClientExtensions.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Oct 10, 2024

View reviewed changes

sebastienros reviewed Oct 10, 2024

View reviewed changes

SteveSandersonMS force-pushed the stevesa/meai-integration branch from 2551471 to c565c53 Compare October 25, 2024 14:05

SteveSandersonMS added 6 commits November 13, 2024 09:07

Add AddAzureOpenAIChatClient

1a01ce2

Set exact dependency versions in MEAI

81cca02

Fix typo

8087ba1

Handle connection string missing values

70c4b07

Handle connection string including duplicate values

bbb1ead

Change to chaining API

a29740a

SteveSandersonMS force-pushed the stevesa/meai-integration branch from 4587b4b to a29740a Compare November 13, 2024 17:07

SteveSandersonMS mentioned this pull request Nov 14, 2024

Change ChatClientBuilder to register singletons and support lambda-less chaining dotnet/extensions#5642

Open

Add AddAzureOpenAIChatClient #6225

Are you sure you want to change the base?

Add AddAzureOpenAIChatClient #6225

Conversation

SteveSandersonMS commented Oct 10, 2024 • edited Loading

Description

Design questions

Lifetime

APIs for chaining on AddAzureOpenAIClient

Checklist

Microsoft Reviewers: Open in CodeFlow

SteveSandersonMS commented Oct 10, 2024

stephentoub commented Oct 10, 2024

stephentoub commented Oct 10, 2024 • edited Loading

sebastienros commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastienros commented Oct 10, 2024

stephentoub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented Oct 10, 2024

SteveSandersonMS commented Oct 16, 2024 • edited Loading

Alternative

davidfowl commented Oct 16, 2024 • edited Loading

SteveSandersonMS commented Oct 16, 2024 • edited Loading

SteveSandersonMS commented Oct 16, 2024 • edited Loading

stephentoub commented Oct 16, 2024 • edited Loading

SteveSandersonMS commented Oct 16, 2024 • edited Loading

sebastienros commented Oct 16, 2024

davidfowl commented Oct 16, 2024

eerhardt commented Oct 16, 2024

SteveSandersonMS commented Oct 25, 2024 • edited Loading

SteveSandersonMS commented Nov 12, 2024

Updates

More complex case

SteveSandersonMS commented Nov 12, 2024

eerhardt commented Nov 12, 2024

SteveSandersonMS commented Oct 10, 2024 •

edited

Loading

APIs for chaining on `AddAzureOpenAIClient`

stephentoub commented Oct 10, 2024 •

edited

Loading

stephentoub commented Oct 10, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

davidfowl commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

stephentoub commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 16, 2024 •

edited

Loading

SteveSandersonMS commented Oct 25, 2024 •

edited

Loading