-
Notifications
You must be signed in to change notification settings - Fork 840
Description
(Written by @SteveSandersonMS; I'm copying this into the repo from an offline location...)
Now
Currently the API looks like:
services.AddChatClient(pipeline => pipeline
.UseDistributedCache()
.UseFunctionCalling()
.Use(new SomeConcreteClient()); // Or pass a factory (Func<IServiceProvider, IChatClient>)... and the proposed Aspire+MEAI integration looks like:
services.AddOpenAIChatClient("service-name", pipeline => pipeline
.UseDistributedCache()
.UseFunctionCalling());Possibility
However, we could change AddChatClient so that it actually returns the ChatClientBuilder instead of the IServiceCollection. Then it would look like:
services.AddChatClient(new SomeConcreteClient()) // Or pass a factory (Func<IServiceProvider, IChatClient>)
.UseDistributedCache()
.UseFunctionCalling();... and the proposed Aspire+MEAI integration would become:
services.AddOpenAIChatClient("service-name")
.UseDistributedCache()
.UseFunctionCalling();In my opinion this looks a lot better, as it removes the lambda, nested brackets, and the meaningless-looking pipeline => pipeline that hangs at the end of the first line.
This would be a legitimate pattern for DI with plenty of prior art of AddX calls that return a builder you can chain further setup steps on.
Challenges
- We'd have to change
ChatClientBuilderso that, instead of acceptingIServiceProvideras a constructor arg and exposing it as a property, it would have to work without access to anIServiceProvideruntil the DI service is resolved.- Mostly this is fine because the
Use(Func<ChatClientBuilder, ChatClientBuilder>)method already has an overload that takesFunc<IServiceProvider, ChatClientBuilder, ChatClientBuilder>and so that would become the only overload - The one thing that would have to change is its
Use(IChatClient)that terminates the pipeline. This would have to change toUse(Func<IServiceProvider, IChatClient>). And we should probably rename it toUseInnerClientor back toBuildotherwise it gets really hard to differentiate.- Alternatively we remove that method entirely and change the constructor to have two overloads, one accepting
IChatClientand the other acceptingFunc<IServiceProvider, IChatClient>. Then the usage pattern is that you pass the inner client (or factory for one) to the constructor, and anyUsecalls are assumed to come before that in the pipeline.
- Alternatively we remove that method entirely and change the constructor to have two overloads, one accepting
- Mostly this is fine because the
- It becomes harder for us to detect whether you've configured a pipeline or not. Today we can check if the
pipelinefunc is null, and if so, have a default pipeline.- We could do something slightly trickier where
ChatClientBuilderkeeps track of whether anyUsecalls have been made, and then at service resolution time, we apply a default pipeline if not - Or we move away from the concept of a default pipeline, which is dubious anyway because it's weird that adding a pipeline phase erases the default pipeline you had before
- We could do something slightly trickier where
Altogether I think the improved aesthetics of doing this without pipeline => pipeline make it worthwhile to pursue.