Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse order of Use pipeline with ChatClientBuilder? #5661

Closed
stephentoub opened this issue Nov 18, 2024 · 1 comment
Closed

Reverse order of Use pipeline with ChatClientBuilder? #5661

stephentoub opened this issue Nov 18, 2024 · 1 comment
Labels

Comments

@stephentoub
Copy link
Member

We used to have this:

var client = new ChatClientBuilder()
    .UseOuter()
    .UseMiddle()
    .Use(inner);

and the order of how messages flow could easily be explained as being top to bottom, with an arrow going through outer through middle to inner.

But now, we have this:

var client = new ChatClientbuilder(inner)
   .UseOuter()
   .UseMiddle()
   .Build();

That ordering explanation no longer works, because we now visually have inner to outer to middle.

Should we reverse the order of how registration happens? So you'd instead write:

var client = new ChatClientBuilder(inner)
    .UseMiddle()
    .UseOuter()
    .Build();

That way, the ordering is bottom to top, with an arrow going through outer through middle to inner.

Bonus, the main raised concern with exposing "With" extension on IChatClient is that the ordering would be opposite of that from the builder. But if we swap the ordering in the builder, then "With" methods would produce the same ordering, e.g.

var client = innerClient
    .WithMiddle()
    .WithOuter();

cc: @SteveSandersonMS

@SteveSandersonMS
Copy link
Member

For me, reversing the order feels pretty bad, as it would no longer feel like a middleware pattern at all. We would need to stop using the term "middleware" as it would produce very messed-up intuition.

For a long time I was keen on having the inner client specified last in the code to produce the pure top-to-bottom flow. But eventually I realised that's not how I intuitively think about it anyway. My natural intuition is that the "inner" client isn't the same kind of thing as the other pipeline steps at all. Instead, the "inner" client is true underlying nature of what's being built (i.e., this is an OpenAI client, or this is an Ollama client, etc.), and the pipeline steps are a secondary, distinct thing that configures how it gets executed.

So it was liberating to accept that we can and should specify the inner client first (matching how I think people will actually think of it, as the underlying client is the primary object), and then the whole notion of a middleware pipeline is a separate and optional thing you can also specify. As such I'm not at all concerned that it's not a pure top-to-bottom definition. It seems right that the pipeline phases are specified in order top-to-bottom, and that the inner client is specified in an entirely different place. This is actually similar to ASP.NET Core in that there's a distinct notion of endpoints configured as the final destinations for HTTP handlers, while the middleware pipeline is defined in top-to-bottom order in a different place.

I do appreciate there's a different sense of elegance in just having a series of wrappers, and not thinking of it as middleware. If you're really keen on doing that I'm open to considering it but we should use different API terminology (e.g., WrapWithFunctionInvocation, WrapInDistributedCache, or something like that) to avoid confusion. Right now my vote would still be with preserving the top-to-bottom flow and a conventional kind of middleware pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants