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

.Net: Replace SK's IAIServiceProvider with IServiceProvider #3714

Merged
merged 11 commits into from
Nov 28, 2023

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 27, 2023

Closes #3694
Closes #3693
Closes #3659
Closes #3571
Closes #3191
Closes #2463

(This replaces #3676, including that as a first commit. That resiliency functionality is no longer necessary after this change, as developers can simply use libraries like Microsoft.Extensions.Http.Resilience to achieve the desired support.)

SK has had a custom IAIServiceProvider interface that's represented a provider of AI related services. Kernel has then wrapped one of these, but also wrapped other individual non-AI services, like ILoggerFactory and HttpClient. This leads to a variety of problems, such as it being more complicated to construct a Kernel instance, it not integrating as nicely with a broader dependency injection system, and not being as future proof because there isn't a good way to flow additional non-AI-related services through the Kernel to anywhere that might want access. On the DI front, it's also more difficult to integrate because all of the KernelBuilder extension methods are specific to KernelBuilder, and even though they're creating services, the helpers aren't usable with IServiceCollection and thus with the rest of the DI system; a developer needs to go directly to the underlying types and register those directly.

There are also a handful of features that are of little value and in some cases exposed but not actually implemented, such as setAsDefault and alsoAsTextCompletion. And other cases where there's something valuable but missing, such as the ability to create certain kinds of Azure-related service instances with an existing OpenAIClient.

This PR attempts to address all of these issues and set up SK to evolve more easily:

  • Kernel now wraps an IServiceProvider, and all services (AI and non-AI) are retrieved from it. This has a variety of benefits, including future-proofing for wanting to flow more state around, better integration with other systems (e.g. Microsoft.Extensions.DependencyInjection), simpler construction, etc.
  • KernelBuilder still has dedicated WithXx methods for first-class features, but they're all layered on top of a new ConfigureServices method, which lets you add anything to an IServiceCollection. Because everything is based on IServiceCollection/IServiceProvider, dependencies are automatically satisfied from the container. For example, for services that accept an optional HttpClient, if one isn't specified to the WithXx method, the system will pull one out of the DI container if it can, which means all existing support for adding configured HttpClient instances to an IServiceCollection immediately apply to Kernel{Builder} as well.
  • KernelBuilder also exposes a ConfigurePlugins method, largely for parity with ConfigureServices, as fundmantally a Kernel is just the combination of a collection of services and a collection of plugins (plus some additional transient data, event handlers, etc.) This also integrates with IServiceProvider, with one passed into the callback, so that if the plugin itself needs access to something like logging, it can simply get it from the container. These ConfigureServices and ConfigurePlugins methods can be called any number of times.
  • KernelBuilder is now just a simplified, opinionated way to achieve something you can achieve in two other ways: construct the Kernel directly, or construct the Kernel via DI. For the latter, you can simply add Kernel as a DI resource, and it'll automatically be composed out of what's available. For example, if in ASP.NET you AddAzureOpenChatCompletion, AddLogging, AddHttp, etc., and then ask for a Kernel to be injected, that Kernel will be constructed from the IServiceProvider that contains all of those other resources, and thus anyone using the Kernel will get that logger, will get that HttpClient, etc.; the HttpClient will also be used by the Azure OpenAI chat completion.

@matthewbolanos, @markwallace-microsoft, @SergeyMenshykh, please review from an SK perspective to let me know if you agree with the direction, whether there are specific things you'd like to see changed, etc. There are some straggling things we should still do after this (like add a few more extension methods for adding plugins to a plugin collection, adding more tests, revamping XML comments, cleaning up more code, etc.), but this is a sweeping change and I wanted to get it in.

@eerhardt, @halter73, I'd appreciate it if you could review this from a DI perspective, e.g. are there variations in the patterns I should be following, any best-practices I'm violating, whether I'm using singleton when I should be using transient, etc. etc.

Note that I ran into one issue as part of this: the Microsoft.Extensions.DependencyInjection support for keyed services doesn't let you enumerate all services for a particular T, regardless of key. That's something SK depends on. To workaround that, KernelBuilder tracks all of the type/keys and injects a dictionary as a service with that information; then places the Kernel needs it, it can query for that dictionary and use it. This means that a Kernel constructed with KernelBuilder will fully support keys, whereas a Kernel constructed with DI or directly won't work as nicely with named services. My hope is that M.E.DI can fix this asap (offline conversation) in a way that will allow SK to upgrade to a patched version, delete its hack, and have those broader scenarios "just work". In the meantime, though, KernelBuilder behaves as desired.

I also stopped short of adding Add{Azure}OpenAIXx methods that resolve the OpenAIClient from the service provider. That'll be an important thing to do once there's an Aspire component for Azure.AI.OpenAI, but in the meantime, I didn't want there to be an attractive Add{Azure}OpenAIXx method that looked like you didn't need to supply the necessary information even though you do.

Better to just use established conventions. These removes SK's custom IDelegatingHandlerFactory and all implementations of it, including the two "Basic" and Polly-based libraries. It replaces it WithHttpHandlerFactory (which wasn't related to either HttpClientFactory or HttpClientHandler) with a WithHttpClient method that puts a specific HttpClient into the Kernel and then makes that available to any services that want it. The OpenAIClent's built-in retry support is now only suppressed if an HttpClient is supplied.

(My aim is that after this PR, we're able to replace the custom service provider in Kernel with an actual IServiceProvider and then remove the custom support for things like ILoggingFactory and HttpClient with just having those resolved from that IServiceProvider. But for now, WithHttpClient is a step forward from what we had.)
@stephentoub stephentoub requested a review from a team as a code owner November 27, 2023 23:16
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core memory connector labels Nov 27, 2023
@github-actions github-actions bot changed the title Replace SK's IAIServiceProvider with IServiceProvider .Net: Replace SK's IAIServiceProvider with IServiceProvider Nov 27, 2023
SK has had a custom IAIServiceProvider interface that's represented a provider of AI related services. Kernel has then wrapped one of these, but also wrapped other individual non-AI services, like ILoggerFactory and HttpClient.  This leads to a variety of problems, such as it being more complicated to construct a Kernel instance, it not integrating as nicely with a broader dependency injection system, and not being as future proof because there isn't a good way to flow additional non-AI-related services through the Kernel to anywhere that might want access. On the DI front, it's also more difficult to integrate because all of the KernelBuilder extension methods are specific to KernelBuilder, and even though they're creating services, the helpers aren't usable with IServiceCollection and thus with the rest of the DI system; a developer needs to go directly to the underlying types and register those directly.

There are also a handful of features that are of little value and in some cases exposed but not actually implemented, such as setAsDefault and alsoAsTextCompletion.  And other cases where there's something valuable but missing, such as the ability to create certain kinds of Azure-related service instances with an existing OpenAIClient.

This PR attempts to address all of these issues and set up SK to evolve more easily:
- Kernel now wraps an IServiceProvider, and all services (AI and non-AI) are retrieved from it. This has a variety of benefits, including future-proofing for wanting to flow more state around, better integration with other systems (e.g. Microsoft.Extensions.DependencyInjection), simpler construction, etc.
- KernelBuilder still has dedicated WithXx methods for first-class features, but they're all layered on top of a new ConfigureServices method, which lets you add anything to an IServiceCollection. Because everything is based on IServiceCollection/IServiceProvider, dependencies are automatically satisfied from the container. For example, for services that accept an optional HttpClient, if one isn't specified to the WithXx method, the system will pull one out of the DI container if it can, which means all existing support for adding configured HttpClient instances to an IServiceCollection immediately apply to Kernel{Builder} as well.
- KernelBuilder also exposes a ConfigurePlugins method, largely for parity with ConfigureServices, as fundmantally a Kernel is just the combination of a collection of services and a collection of plugins (plus some additional transient data, event handlers, etc.) This also integrates with IServiceProvider, with one passed into the callback, so that if the plugin itself needs access to something like logging, it can simply get it from the container.  These ConfigureServices and ConfigurePlugins methods can be called any number of times.
- KernelBuilder is now just a simplified, opinionated way to achieve something you can achieve in two other ways: construct the Kernel directly, or construct the Kernel via DI.  For the latter, you can simply add Kernel as a DI resource, and it'll automatically be composed out of what's available. For example, if in ASP.NET you AddAzureOpenChatCompletion, AddLogging, AddHttp, etc., and then ask for a Kernel to be injected, that Kernel will be constructed from the IServiceProvider that contains all of those other resources, and thus anyone using the Kernel will get that logger, will get that HttpClient, etc.; the HttpClient will also be used by the Azure OpenAI chat completion.
…rchestration.Flow project have been removed.
@stephentoub stephentoub added this pull request to the merge queue Nov 28, 2023
@stephentoub stephentoub removed this pull request from the merge queue due to a manual request Nov 28, 2023
@stephentoub stephentoub added this pull request to the merge queue Nov 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 28, 2023
@stephentoub stephentoub added this pull request to the merge queue Nov 28, 2023
Merged via the queue into microsoft:main with commit 73afd47 Nov 28, 2023
18 checks passed
@stephentoub stephentoub deleted the useiserviceprovider branch November 28, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel memory connector .NET Issue or Pull requests regarding .NET code
Projects
None yet
7 participants