Add DI extensions that accept implementations as type parameters.

[eiriktsarpalis]

These are useful in the context of defining transient Copilot chat clients.

Microsoft Reviewers: Open in CodeFlow

[stephentoub]

@SteveSandersonMS had experimented with adding these previously but ultimately decided not to:
#5642 (comment)
#5642 (review)

[dotnet-comment-bot]

‼️ Found issues ‼️

Project	Coverage Type	Expected	Actual
Microsoft.Extensions.AI	Line	89	88.52 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project	Expected	Actual
Microsoft.Gen.MetadataExtractor	57	70
Microsoft.Extensions.AI.Abstractions	82	83

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=957828&view=codecoverage-tab

[eiriktsarpalis]

@SteveSandersonMS had experimented with adding these previously but ultimately decided not to: #5642 (comment) #5642 (review)

Interesting. I followed a different approach in this implementation. Rather than calling provider.GetRequiredService<T>() I'm using ActivatorUtilities.CreateInstance<TChatClient>(provider) meaning that registering IChatClient itself in this context will result in an exception because the implementation is no a concrete type.

[halter73]

So aside from AddChatClient<IChatClient> potentially leading to infinite recursion which is addressed by ActivatorUtilities, the issue is that people might assume that AddChatClient<MyChatClient> would create a registration with MyChatClient rather than IChatClient as the service type?

If that's the only remaining objection, one option might be to register the TChatClient both as TChatClient and IChatClient. You'd want to use a factory to link the registrations and make sure you get the same service instance whether you resolve it as an IChatClient or a TChatClient, but that's doable and feels similar to what AddHttpClient<TClient> does.

Also, given that the instance parameter name is innerClient, should the type parameter name be TInnerClient instead?

[SteveSandersonMS]

Am I right to think this approach relies on TChatClient not requiring any parameters other than DI services? I had expected that to be a relatively rare case, e.g., you normally need to specify a model and an endpoint or key or both. Our mainstream IChatClient implementations don't obtain this info from config; they require them to be passed in at construction time.

Could you give an example of what the code looks like to obtain transient copilot chat clients without this? It would be good to get a sense of how much payoff there is, if there's also some drawback that people will try to use it wrongly (e.g., to register an OpenAI-based chat client) and get confused.

[eiriktsarpalis]

If that's the only remaining objection, one option might be to register the TChatClient both as TChatClient and IChatClient.

What's interesting about the AddChatClient methods (generally speaking, not just the newly introduced overload) is that it won't necessarily end up using TChatClient as the final IChatClient implementation. What the methods do is create a ChatClientBuilder instance using TChatClient as the seed value, so the final IChatClient would be a combination of all the layers that have been appended to the builder. As such, I'm not sure if we should be registering TChatClient at all.

Could you give an example of what the code looks like to obtain transient copilot chat clients without this? It would be good to get a sense of how much payoff there is, if there's also some drawback that people will try to use it wrongly (e.g., to register an OpenAI-based chat client) and get confused.

The motivating use case is Copilot extensions:

https://github.com/eiriktsarpalis/blackbeard-extension-cs/blob/fc02c695a48b35d9c54b6f195ccefeabe43ff191/src/blackbeard-extension-cs/Program.cs#L4-L5

Clients hitting the Github API need to be passed the Github token from the request headers. Not having the generic method necessitates writing this instead:

builder.Services.AddChatClient(provider => new GithubChatClient(provider.GetRequiredService<IHttpContextAccessor>()), ServiceLifetime.Scoped).UseFunctionInvocation();

[SteveSandersonMS]

In general the expected DI pattern is that any concrete inner client that wants to streamline the DI registration experience should define its own extension methods that do that. This allows them to take in whatever parameters are required, as well as encapsulate acquiring whatever other DI services are needed. For example we already have builder.Services.AddOpenAIClient("serviceName").AddChatClient("deploymentName") in Aspire.OpenAI. Similarly you could define a AddGithubChatClient DI helper as an extension method.

If we had AddChatClient<T> it would only be useful in the niche case where T doesn't need any parameters other than DI services, and risks looking like the easy and appealing way to go when it won't work at runtime (e.g., AddChatClient<OpenAIChatClient> will lead to an error from Activator.CreateInstance<T>, right?).

[stephentoub]

Out of all the IChatClient implementations I've seen, that's the only one that might benefit. But even there, I'd expect the library exposing it to provide an AddGithubChatClient helper, so no consumers would ever need to actually write that line. It's similar to wanting to add a memory distributed cache to DI. The hard way is to write:

.UseDistributedCache(new MemoryDistributedCache(Options.Create(new MemoryDistributedCacheOptions())))

and the easy way:

.AddDistributedMemoryCache()

I'm concerned at the idea of adding overloads that'll help almost no one but could easily be a pit of failure for the majority, eg someone getting stuck on:

.AddChatClient<OllamaChatClient>()

which can't work.