Upgrade to .NET Aspire 9.1 #759
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "sdk": { | ||
| "version": "9.0.200", | ||
| "rollForward": "latestFeature", | ||
| "allowPrerelease": true | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| using eShop.Catalog.API.Services; | ||
| using Microsoft.Extensions.AI; | ||
| using OllamaSharp; | ||
| using OpenAI; | ||
|
|
||
| public static class Extensions | ||
|
|
@@ -40,7 +41,7 @@ public static void AddApplicationServices(this IHostApplicationBuilder builder) | |
| if (builder.Configuration["OllamaEnabled"] is string ollamaEnabled && bool.Parse(ollamaEnabled)) | ||
| { | ||
| builder.AddOllamaSharpEmbeddingGenerator("embedding"); | ||
| builder.Services.AddEmbeddingGenerator(sp => (IEmbeddingGenerator<string, Embedding<float>>)sp.GetRequiredKeyedService<IOllamaApiClient>("embedding")) | ||
|
Member
Author
There was a problem hiding this comment. FYI @stephentoub @SteveSandersonMS on the change here and in WebApp/Extensions/Extensions.cs. These 2 things were pretty tricky.
Member
There was a problem hiding this comment. I didn't quite follow that. But is there something you'd recommend we change to make something easier?
Member
Author
There was a problem hiding this comment.
For the 2nd problem (the call to Build()), there is some awkwardness IMO. Take a look at the existing code: eShop/src/Catalog.API/Extensions/Extensions.cs Lines 50 to 53 in 1ba1cc7 Code like this will never work. The call to public IEmbeddingGenerator<TInput, TEmbedding> Build(IServiceProvider? services = null)
{
services ??= EmptyServiceProvider.Instance;Maybe For the infinite recursion problem, I'm not sure I have a suggestion. I think this was just confusion on my part trying to understand the old vs the new API usage. When writing new code, I don't think anyone would would implement the "innerGeneratorFactory" by directly returning the same service from the IServiceProvider . This is the function that is supposed to create the generator.
Member
There was a problem hiding this comment. To make sure I understand, you're saying that you not being forced to think about passing a serviceProvider to Build (even if you decide not to pass one) is the problem?
Member
Author
There was a problem hiding this comment. I'm saying I shouldn't ever call The way the existing code currently looks, I would have expected the IServiceProvider from
Member
There was a problem hiding this comment. The call to builder.Services.AddChatClient(sp => new Implementation())
.UseOpenTelemetry()
.UseLogging();Why not just have the method return void and encourage the equivalent pattern: builder.Services.AddChatClient(sp => new Implementation()
.AsBuilder()
.UseOpenTelemetry()
.UseLogging()
.Build());It might be more lines of code but the intent is clearer.
Member
Author
There was a problem hiding this comment. Once Return AspireOpenAIClientBuilder from AddOpenAIClientFromConfiguration (dotnet/aspire#7763) is merged, this code will get even more simple. (At least the OpenAI part will. We will need to update the Ollama Aspire integration to follow the same API pattern.) builder.AddOpenAIClientFromConfiguration("openai");
builder.Services.AddEmbeddingGenerator(sp => sp.GetRequiredService<OpenAIClient>().AsEmbeddingGenerator(builder.Configuration["AI:OpenAI:EmbeddingModel"]!))
.UseOpenTelemetry()
.UseLogging();becomes builder.AddOpenAIClientFromConfiguration("openai")
.AddEmbeddingGenerator(builder.Configuration["AI:OpenAI:EmbeddingModel"]!)
.UseLogging(); // I'm not positive this is required, but I don't see Aspire calling it todayAnd with further changes like Configure OpenAI models in the app host (dotnet/aspire#6577) we will be able to drop the call to builder.AddOpenAIClientFromConfiguration("openai-embedding")
.AddEmbeddingGenerator()
.UseLogging(); // I'm not positive this is required, but I don't see Aspire calling it todaySince the model/deployment name will be passed via the connection string named "openai-embedding".
Member
There was a problem hiding this comment.
Assuming UseOpenTelemetry is being used, UseLogging is probably not necessary.
Member
Author
There was a problem hiding this comment.
Yes, it is by default. The Aspire integration enables OpenTelemetry unless someone disables tracing: return builder.DisableTracing
? result
: new OpenTelemetryEmbeddingGenerator<string, Embedding<float>>(result);but diffing this code with what's in loggerFactory ??= services.GetService<ILoggerFactory>();
var generator = new OpenTelemetryEmbeddingGenerator<TInput, TEmbedding>(
innerGenerator,
loggerFactory?.CreateLogger(typeof(OpenTelemetryEmbeddingGenerator<TInput, TEmbedding>)),
sourceName);
configure?.Invoke(generator);
return generator;I've opened Update OpenAI OpenTelemetry integration to pass an ILogger (dotnet/aspire#7771) for this.
Member
Author
There was a problem hiding this comment.
This was done in CommunityToolkit/Aspire#465. We can update here once it ships in a stable version of the Aspire Ollama integration. cc @aaronpowell |
||
| .UseOpenTelemetry() | ||
| .UseLogging(); | ||
| } | ||
|
|
@@ -49,8 +50,7 @@ public static void AddApplicationServices(this IHostApplicationBuilder builder) | |
| builder.AddOpenAIClientFromConfiguration("openai"); | ||
| builder.Services.AddEmbeddingGenerator(sp => sp.GetRequiredService<OpenAIClient>().AsEmbeddingGenerator(builder.Configuration["AI:OpenAI:EmbeddingModel"]!)) | ||
| .UseOpenTelemetry() | ||
| .UseLogging(); | ||
| } | ||
|
|
||
| builder.Services.AddScoped<ICatalogAI, CatalogAI>(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working around Array.Contains no longer works in latest .NET 9 SDK (9.0.200) (npgsql/efcore.pg#3461)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @roji, @jaredpar