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

Change ChatClientBuilder to register singletons and support lambda-less chaining #5642

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Nov 14, 2024

Fixes #5572 as well as changing the default DI lifetime from scoped to singleton.

This is also needed for the API design we want in dotnet/aspire#6225

Usage with DI (common)

Before:

serviceCollection.AddChatClient(pipeline => pipeline
    .UseDistributedCache()
    .UseFunctionCalling()
    .Use(innerClientOrFactory));

After:

serviceCollection.AddChatClient(innerClientOrFactory)
    .UseDistributedCache()
    .UseFunctionCalling();

Usage without DI (uncommon)

Before:

var client = new ChatClientBuilder(serviceProvider)
    .UseDistributedCache()
    .UseFunctionCalling()
    .Use(innerClientOrFactory);

After:

var client = new ChatClientBuilder(innerClientOrFactory)
    .UseDistributedCache()
    .UseFunctionCalling()
    .Build(serviceProvider);    // Or just .Build() to use an empty service provider for scenarios where you're really not using DI at all

Summary

In the non-DI case it doesn't make much difference. It's a small improvement though, since it avoids the Use method having totally different semantics based on which oveload is called.

But in the DI case which is more common, it's simpler and more approachable as there's no need for the lambda.

It also has value for the Aspire helpers as it make it possible to do this API, which we can't otherwise achieve since you can't specify the inner client up front and can only do so once the rest of the pipeline is built.

Microsoft Reviewers: Open in CodeFlow

@SteveSandersonMS
Copy link
Member Author

Note that I'll do the equivalent thing for EmbeddingGeneratorBuilder before this is merged.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the new approach looks much more intuitive.

@stephentoub stephentoub merged commit 56e720c into main Nov 14, 2024
6 checks passed
@stephentoub stephentoub deleted the stevesa/chatclientbuilder-redux branch November 14, 2024 13:36
stephentoub pushed a commit to stephentoub/extensions that referenced this pull request Nov 19, 2024
…ss chaining (dotnet#5642)

* Change ChatClientBuilder to register singletons and support lambda-less chaining

* Add generic keyed version

* Improve XML doc

* Update README files

* Remove generic DI registration methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider alternate chaining pattern for DI/builder
3 participants