Aspire.AI.Azure.OpenAI integration #153
Conversation
src/Catalog.API/Program.cs
Outdated
| @@ -5,6 +5,7 @@ | |||
| builder.AddApplicationServices(); | |||
|
|
|||
| builder.Services.AddProblemDetails(); | |||
| builder.AddAzureOpenAI("catalogOpenAi"); | |||
There was a problem hiding this comment.
I don't like the name of this method.
In the AppHost we have AddAzureOpenAI and AddopenAI (for non-azure and azure based open ai services).
In the apps we have AddAzureOpenAI and AddKeyedAzureOpenAI named after the NuGet package.
On the "client" app I'd rather have the method called Add[Keyed]OpenAIClient since is independent of Azure or non-Azure, and represents more what it's doing.
There was a problem hiding this comment.
I think the core of the issue is that the library does both, but is "Azure" branded.
An option is to have both methods. And the AzureOpenAI one ensures both Endpoint and Key are configured. And the OpenAI one just reads Key.
There was a problem hiding this comment.
"Azure" is OK, we could see this coming from the Azure SDK.
What about the AddOpenAI vs AddOpenAIClient though? For Postgres we call it AddNpgsqlDataSource which makes it simpler to grasp, and why I'd prefer to use AddOpenAIClient (or AddAzureOpenAIClient since it's an Azure SDK concept?)
There was a problem hiding this comment.
I'm OK with adding Client to the name.
There was a problem hiding this comment.
FYI: we are having lots of discussions on the Azure SDK about the best way to support both OpenAI (OAI) and Azure OpenAI (AOAI) with just one client. We are thinking that the way it's done today (e.g. if no endpoint passed, OAI is called; and the 3rd parameter meaning either model or deployment) is not ideal and so it might change before the SDK GAs (releases as stable). Because of that, I would suggest that you don't rely too much on how the ctors look today.
There was a problem hiding this comment.
FYI, depending on how that's changed, changing that could negatively impact Semantic Kernel, which has OpenAIClient in its public surface area.
cc: @markwallace-microsoft, @matthewbolanos
|
What's the status of this PR? Are we waiting for Aspire preview3 to ship before completing it? |
|
Correct. I was using locally built packages. |
|
@sebastienros, FYI we're using preview 3 in this repo now (and you have a merge conflict for that). |
Showing how the upcoming Aspire.Azure.AI.OpenAI component can be integrated.