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?

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

When targeting < net9.0, yes.

If so then the Aspire integration package will still have to depend on STJv9 (even on net8.0)... Is this going to be a problem?

Not sure. Who do we think will be concerned?

@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.

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
    .AddFunctionCalling()
    .AddSomeCustomMiddleware());

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 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

Add AddAzureOpenAIChatClient

c565c53

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

SteveSandersonMS added 5 commits October 25, 2024 15:14

Set exact dependency versions in MEAI

0c85b8f

Fix typo

7ba67f3

Handle connection string missing values

7db6771

Handle connection string including duplicate values

6920496

Change to chaining API

9b7150e

Merge branch 'main' into stevesa/meai-integration

4587b4b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AddAzureOpenAIChatClient #6225

Add AddAzureOpenAIChatClient #6225

SteveSandersonMS commented Oct 10, 2024 •

edited

Loading

SteveSandersonMS commented Oct 10, 2024

stephentoub commented Oct 10, 2024

stephentoub commented Oct 10, 2024 •

edited

Loading

sebastienros commented Oct 10, 2024

stephentoub Oct 10, 2024

SteveSandersonMS Oct 10, 2024

stephentoub commented Oct 10, 2024 •

edited

Loading

stephentoub Oct 10, 2024

stephentoub Oct 10, 2024

sebastienros Oct 10, 2024

sebastienros commented Oct 10, 2024

stephentoub left a comment

sebastienros Oct 10, 2024

SteveSandersonMS Oct 10, 2024

eerhardt Oct 11, 2024

stephentoub Oct 11, 2024

SteveSandersonMS Oct 25, 2024

stephentoub Oct 25, 2024

SteveSandersonMS Oct 25, 2024

eerhardt Oct 29, 2024

SteveSandersonMS commented Oct 10, 2024

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

sebastienros commented Oct 16, 2024

davidfowl commented Oct 16, 2024

eerhardt commented Oct 16, 2024

SteveSandersonMS commented Oct 25, 2024

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

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

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