Skip to content

Consider alternate chaining pattern for DI/builder #5572

Closed
@stephentoub

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 ChatClientBuilder so that, instead of accepting IServiceProvider as a constructor arg and exposing it as a property, it would have to work without access to an IServiceProvider until the DI service is resolved.
    • Mostly this is fine because the Use(Func<ChatClientBuilder, ChatClientBuilder>) method already has an overload that takes Func<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 to Use(Func<IServiceProvider, IChatClient>). And we should probably rename it to UseInnerClient or back to Build otherwise it gets really hard to differentiate.
      • Alternatively we remove that method entirely and change the constructor to have two overloads, one accepting IChatClient and the other accepting Func<IServiceProvider, IChatClient>. Then the usage pattern is that you pass the inner client (or factory for one) to the constructor, and any Use calls are assumed to come before that in the pipeline.
  • It becomes harder for us to detect whether you've configured a pipeline or not. Today we can check if the pipeline func is null, and if so, have a default pipeline.
    • We could do something slightly trickier where ChatClientBuilder keeps track of whether any Use calls 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

Altogether I think the improved aesthetics of doing this without pipeline => pipeline make it worthwhile to pursue.

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions