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

AddOpenAIRestApiClient to wrap both AzureOpenAI and OpenAI integrations #5755

Open
sebastienros opened this issue Sep 17, 2024 · 6 comments
Open
Assignees
Labels
area-integrations Issues pertaining to Aspire Integrations packages

Comments

@sebastienros
Copy link
Member

sebastienros commented Sep 17, 2024

Background and Motivation

Between the existing Aspire.Azure.AI.OpenAI and the new Aspire.OpenAI integration implemented in #5621 we can now register an AzureOpenAIClient or OpenAIClient with AddAzureOpenAIClient or AddOpenAIClient. Also AzureOpenAIClient inherits from OpenAIClient.

We want to create a new method in the Aspire.Azure.AI.OpenAI component, named AddOpenAIRestApiClient that will be able to return the correct instance, either AzureOpenAIClient or OpenAIClient based on the connection string that is provided.

The logic to detect which concrete implementation to return will be based on the presence of an Endpoint in the connection string:

  • If it's empty it will use the OpenAI service.
  • Otherwise:
    • If IsAzure is set in the connection string, if it's true return AzureOpenAIClient, otherwise OpenAIClient.
    • If Endpoint contains azure.com or azure.cnreturn the AzureOpenAIClient instance
    • Otherwise return an OpenAIClient instance.

Proposed API

+ namespace Microsoft.Extensions.Hosting;

+ public static class AspireOpenAIRestAPIExtensions
+ {
+     /// <summary>
+     /// Registers a concrete implementation of <see cref="OpenAIClient"/> as a singleton in the services provided by the <paramref name="builder"/> based on the provided connection string.
+     /// </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 AddOpenAIRestApiClient(this IHostApplicationBuilder builder, string connectionName)
+ }

Usage Examples

var builder = WebApplication.CreateBuilder(args);
builder.AddServiceDefaults();

builder.AddOpenAIRestApiClient("openai");

These AppHost configurations would register an AzureOpenAIClient instance:

{
  "ConnectionStrings": {
    "openai": "https://{account_name}.openai.azure.com/"
  }
}
{
  "ConnectionStrings": {
    "openai": "Endpoint=https://{account_name}someotherazuredomain.com/;IsAzure=true"
  }
}

These AppHost configurations would register an OpenAIClient instance:

{
  "ConnectionStrings": {
    "openai": "Key={openai_account_key}"
  }
}
{
  "ConnectionStrings": {
    "openai": "Endpoint=http://localhost/myollama"
  }
}

Each Aspire service would then provide the configuration matching the endpoint they provided, using either Aspire:Azure:AI:OpenAI or Aspire:OpenAI keys.

Alternative Designs

N/A

Risks

This is not a breaking change, and it builds on top of the existing OpenAI integrations such that there is no code change in these either.

@sebastienros
Copy link
Member Author

@eerhardt
Copy link
Member

public static void AddOpenAIRestApiClient(this IHostApplicationBuilder builder, string connectionName)

This method doesn't take a configureSettings nor a configureClientBuilder callback. I assume it is because we don't know which type will be configured, so we don't know what the callback should take.

We may not need to solve this immediately. But we should be aware of this limitation in the API.

@sebastienros
Copy link
Member Author

@eerhardt at that level we shouldn't know, so that's why I went with no callback. If we knew then we could just call the final method.

The only viable option I see is to have new types for these settings that can be mapped to each concrete settings/options. Even if they don't make sense, like still provide DisableMetrics/... even if one of them doesn't implement it. And this logic and types would be kept in the AzureAIOpenAI component.

@KrzysztofCwalina
Copy link
Member

The name AddOpenAIRestApiClient is a bit strange IMO. It implies that the other clients are not REST.

@davidfowl davidfowl added area-integrations Issues pertaining to Aspire Integrations packages and removed area-components labels Sep 18, 2024
@sebastienros
Copy link
Member Author

@KrzysztofCwalina So far this was the best we got. We need to find a name that is not:

  • AddAzureOpenAIClient
  • AddOpenAIClient

and yet shows that it returns something to connect to "any" OpenAI service.

Other "not good" suggestions to show what we want to express would be AddBestOpenAIClientForMyConnectionString

@eerhardt
Copy link
Member

Other (not necessarily better) names:

  • AddConfigurableOpenAIClient
  • AddGenericOpenAIClient
  • AddGeneralOpenAIClient
  • AddPotentialAzureOpenAIClient
  • AddAzureOrGeneralOpenAIClient

Maybe "Configurable" might be the best name so far...

Or we could change the naming pattern to something like:

  • TryAddOpenAIClientAsAzure
    • this would "try" it as Azure, and fall back to general OpenAI

Another option is to bake this configuration sniffing directly into AddAzureOpenAIClient and have it NOT inject an AzureOpenAIClient but a plain OpenAIClient, but that feels wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

No branches or pull requests

4 participants