Simplifying and improving Aspire integration in scenario 8 #28
Conversation
|
👋 Thanks for contributing @aaronpowell! We will review the pull request and get back to you soon. |
There was a problem hiding this comment.
Pull Request Overview
This PR streamlines the Aspire (Azure OpenAI) integration in Scenario 8 by adopting Microsoft.Extensions.AI abstractions, consolidating configuration via environment variables, and modernizing endpoint and initializer code with async/vector generation.
- Refactored host
Program.csto register Azure OpenAI resources once, expose deployment names via environment, and unify connection‐string logic. - Updated the Products application to use
AddAzureOpenAIClient,AddEmbeddingGeneratorandAddChatClientinstead of manualOpenAIClientwiring. - Modernized
DbInitializerand API endpoints: made initialization async withIEmbeddingGenerator, switched toTypedResults, and added structured logging.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scenarios/08-Sql2025/src/eShopAppHost/Program.cs | Refactored Azure OpenAI resource registration and environment-variable wiring for deployments. |
| scenarios/08-Sql2025/src/Products/Program.cs | Replaced manual ChatClient/EmbeddingClient setup with AddAzureOpenAIClient extension methods. |
| scenarios/08-Sql2025/src/Products/Models/DbInitializer.cs | Updated initializer to async, switched to IEmbeddingGenerator and GenerateVectorAsync API. |
| scenarios/08-Sql2025/src/Products/Endpoints/ProductApiActions.cs | Switched to TypedResults, injected ILogger, made AI search async with vector generation. |
Comments suppressed due to low confidence (3)
scenarios/08-Sql2025/src/eShopAppHost/Program.cs:64
- [nitpick] The environment key uses a lowercase 'embeddings' while
AI_ChatDeploymentNameuses uppercase 'Chat'. Consider a consistent naming convention (e.g.AI_EmbeddingsDeploymentName) for clarity.
.WithEnvironment("AI_embeddingsDeploymentName", embeddingsDeploymentName);
scenarios/08-Sql2025/src/Products/Models/DbInitializer.cs:8
- [nitpick] The parameter
embeddingClientis actually anIEmbeddingGenerator. Consider renaming it toembeddingGeneratorto better reflect its purpose and type.
public static async Task Initialize(Context context, IEmbeddingGenerator<string, Embedding<float>> embeddingClient, int dimensions = 1536)
scenarios/08-Sql2025/src/Products/Endpoints/ProductApiActions.cs:69
- This AI-based search endpoint introduces async vector generation and database ordering logic. Consider adding unit or integration tests to cover these new code paths.
public static async Task<Ok<SearchResponse>> AISearch(
| @@ -22,22 +22,25 @@ | |||
| .WaitFor(products) | |||
| .WithExternalHttpEndpoints(); | |||
|
|
|||
| IResourceBuilder<IResourceWithConnectionString>? openai; | |||
There was a problem hiding this comment.
The openai variable is declared nullable but is always assigned in both branches of the if/else. Consider making it non-nullable to simplify the code and express that it’s guaranteed to be initialized.
| IResourceBuilder<IResourceWithConnectionString>? openai; | |
| IResourceBuilder<IResourceWithConnectionString> openai; |
Copilot uses AI. Check for mistakes.
| builder.AddAzureOpenAIClient(azureOpenAiClientName, configureSettings: settings => | ||
| { | ||
| var logger = serviceProvider.GetService<ILogger<Program>>()!; | ||
| logger.LogInformation($"Chat client configuration, modelId: {chatDeploymentName}"); | ||
| ChatClient chatClient = null; | ||
| try | ||
| { | ||
| OpenAIClient client = serviceProvider.GetRequiredService<OpenAIClient>(); | ||
| chatClient = client.GetChatClient(chatDeploymentName); | ||
| } | ||
| catch (Exception exc) | ||
| { | ||
| logger.LogError(exc, "Error creating chat client"); | ||
| } | ||
| return chatClient; | ||
| }); | ||
|
|
||
| // get azure openai client and create embedding client from aspire hosting configuration | ||
| builder.Services.AddSingleton<EmbeddingClient>(serviceProvider => | ||
| { | ||
| var logger = serviceProvider.GetService<ILogger<Program>>()!; | ||
| logger.LogInformation($"Embeddings client configuration, modelId: {embeddingsDeploymentName}"); | ||
| EmbeddingClient embeddingsClient = null; | ||
| try | ||
| { | ||
| OpenAIClient client = serviceProvider.GetRequiredService<OpenAIClient>(); | ||
| embeddingsClient = client.GetEmbeddingClient(embeddingsDeploymentName); | ||
| } | ||
| catch (Exception exc) | ||
| { | ||
| logger.LogError(exc, "Error creating embeddings client"); | ||
| } | ||
| return embeddingsClient; | ||
| }); | ||
| settings.Credential = new AzureCliCredential(); | ||
| }).AddEmbeddingGenerator(embeddingsDeploymentName); | ||
|
|
||
| builder.Services.AddSingleton<IConfiguration>(sp => | ||
| var chatDeploymentName = builder.Configuration["AI_ChatDeploymentName"] ?? "gpt-4.1-mini"; | ||
| builder.AddAzureOpenAIClient(azureOpenAiClientName, configureSettings: settings => | ||
| { | ||
| return builder.Configuration; | ||
| }); | ||
| settings.Credential = new AzureCliCredential(); | ||
| }).AddChatClient(chatDeploymentName); |
There was a problem hiding this comment.
You call AddAzureOpenAIClient twice with the same client name, which could cause duplicate registrations. Consider calling it once and chaining both AddEmbeddingGenerator and AddChatClient on the same builder.
Copilot uses AI. Check for mistakes.
| using Products.Models; | ||
| using SearchEntities; | ||
|
|
||
| namespace Products.Endpoints; | ||
|
|
||
| public static class ProductApiActions | ||
| public class ProductApiActions |
There was a problem hiding this comment.
The class is declared non-static but only contains static methods. Consider making the class itself static to indicate it has no instance state.
| public class ProductApiActions | |
| public static class ProductApiActions |
Copilot uses AI. Check for mistakes.
No description provided.