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

Configure OpenAI models in the app host #6577

Open
wants to merge 10 commits into
base: stevesa/meai-integration
Choose a base branch
from

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Oct 31, 2024

Building over @SteveSandersonMS PR to demonstrate the end-to-end experience when ai models/deployments are configured at the host level only.

Microsoft Reviewers: Open in CodeFlow

@sebastienros sebastienros changed the title Sebros/hostmodels Configure OpenAI models in the app host Oct 31, 2024
@@ -14,4 +14,3 @@ Aspire.Hosting.ApplicationModel.AzureOpenAIResource.Deployments.get -> System.Co
Aspire.Hosting.AzureOpenAIExtensions
static Aspire.Hosting.AzureOpenAIExtensions.AddAzureOpenAI(this Aspire.Hosting.IDistributedApplicationBuilder! builder, string! name) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.AzureOpenAIResource!>!
static Aspire.Hosting.AzureOpenAIExtensions.AddAzureOpenAI(this Aspire.Hosting.IDistributedApplicationBuilder! builder, string! name, System.Action<Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.AzureOpenAIResource!>!, Aspire.Hosting.ResourceModuleConstruct!, Azure.Provisioning.CognitiveServices.CognitiveServicesAccount!, System.Collections.Generic.IEnumerable<Azure.Provisioning.CognitiveServices.CognitiveServicesAccountDeployment!>!>? configureResource) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.AzureOpenAIResource!>!
static Aspire.Hosting.AzureOpenAIExtensions.AddDeployment(this Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.AzureOpenAIResource!>! builder, Aspire.Hosting.ApplicationModel.AzureOpenAIDeployment! deployment) -> Aspire.Hosting.ApplicationModel.IResourceBuilder<Aspire.Hosting.ApplicationModel.AzureOpenAIResource!>!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to do this without making breaking changes? For example, can we obsolete something and add new APIs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted as I didn't remember why the change was done. Do we want to keep AddDeployment or replaced it with WithDeployment?

@SteveSandersonMS
Copy link
Member

Overall looks great. @eerhardt's point about the breaking change is probably the main thing to consider. Besides that it looks pretty close to being ready.

…ebros/hostmodels

# Conflicts:
#	playground/OpenAIEndToEnd/OpenAIEndToEnd.WebStory/Program.cs
#	src/Components/Aspire.Azure.AI.OpenAI/AspireAzureOpenAIClientBuilderChatClientExtensions.cs
#	src/Components/Aspire.Azure.AI.OpenAI/PublicAPI.Unshipped.txt
@sebastienros
Copy link
Member Author

I updated the PR. @SteveSandersonMS is this something we could merge in your PR as it's impacting how configuration is passed to the clients. Other option is I wait for your PR to be done and then react to the changes.

@sebastienros sebastienros marked this pull request as ready for review November 14, 2024 22:31
@SteveSandersonMS
Copy link
Member

@sebastienros I'm fine with it being merged into my PR, but my PR itself can't be completed until we get the next build of Microsoft.Extensions.AI, because the agreed API shape depends on API changes we've just merged into dotnet/extensions today.

@sebastienros
Copy link
Member Author

@eerhardt does it make sense to split this PR in two: the deployment API changes creating the ENVs, and the MEAI part that is using it and would be part of Steve's.

Or just move everything to Steve's branch and ship everything at once.

@eerhardt
Copy link
Member

@eerhardt does it make sense to split this PR in two: the deployment API changes creating the ENVs, and the MEAI part that is using it and would be part of Steve's.

Or just move everything to Steve's branch and ship everything at once.

Completely up to you and Steve. I don't see a reason why they would need to be separate, but if you think they should be, I wouldn't argue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants