Aligning OllamaSharp with the Aspire OpenAI + MEAI design #465
Conversation
Moving away from our current ways of registering an IChatClient/IEmbeddingGenerator to leverage the builder pattern from MEAI. This allows the consumer to add middleware to the AI pipeline. Existing methods have been marked as Obsolete. Set some default middleware with logging and OTEL. Fixes #461
aaronpowell
temporarily deployed
to
azure-artifacts
GitHub Actions
Inactive
Minimum allowed line rate is |
| /// <param name="hostBuilder">The <see cref="IHostApplicationBuilder"/> with which services are being registered.</param> | ||
| /// <param name="serviceKey">The service key used to register the <see cref="OllamaApiClient"/> service, if any.</param> | ||
| /// <param name="disableTracing">A flag to indicate whether tracing should be disabled.</param> | ||
| public class AspireOllamaApiClientBuilder(IHostApplicationBuilder hostBuilder, string serviceKey, bool disableTracing) |
There was a problem hiding this comment.
Maybe remove this constructor and just use required properties? Feels like we're going to add more properties over time.
There was a problem hiding this comment.
| /// <param name="configureSettings">An optional delegate that can be used for customizing options. It's invoked after the settings are read from the configuration.</param> | ||
| /// <exception cref="UriFormatException">Thrown when no Ollama endpoint is provided.</exception> | ||
| public static void AddOllamaApiClient(this IHostApplicationBuilder builder, string connectionName, Action<OllamaSharpSettings>? configureSettings = null) | ||
| public static AspireOllamaApiClientBuilder AddOllamaApiClient(this IHostApplicationBuilder builder, string connectionName, Action<OllamaSharpSettings>? configureSettings = null) |
There was a problem hiding this comment.
This is a breaking change. Are you OK with that?
There was a problem hiding this comment.
Yes, it is a breaking change, but in terms of breaking changes it's a pretty unobtrusive one. Having it as a void method previously means that you weren't capturing anything to now having a returned value, but since C# doesn't require return assignment it shouldn't result in a compilation error.
It would be a problem if anyone is using reflection to find the method and invoking it based off an expected signature, and maybe a problem in F# as the return needs to be handled (or piped to ignore), but both are pretty obscure edge cases.
|
APIs look much nicer! It's a breaking change though. What's the policy on that? |
"Aaron does what he wants" 😂. As I mentioned on the specific comment, I consider this a minor breaking change and acceptable at that. We could introduce a new method and deprecate all the current ones, but the naming of that would deviate from the generally used Aspire method naming structure of Behaviourally, the code is the same though, with the exception that we now add the middleware for logging and OTEL to |
Moving away from our current ways of registering an IChatClient/IEmbeddingGenerator to leverage the builder pattern from MEAI. This allows the consumer to add middleware to the AI pipeline.
Existing methods have been marked as Obsolete.
Set some default middleware with logging and OTEL.
Fixes #461