-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.)
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
bot
changed the title
Replace SK's IAIServiceProvider with IServiceProvider
.Net: Replace SK's IAIServiceProvider with IServiceProvider
Nov 27, 2023
eerhardt
reviewed
Nov 28, 2023
stephentoub
force-pushed
the
useiserviceprovider
branch
from
November 28, 2023 01:24
8ec650b
to
92cb31d
Compare
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.
stephentoub
force-pushed
the
useiserviceprovider
branch
from
November 28, 2023 01:32
92cb31d
to
fac3cef
Compare
4 tasks
markwallace-microsoft
approved these changes
Nov 28, 2023
SergeyMenshykh
approved these changes
Nov 28, 2023
RogerBarreto
approved these changes
Nov 28, 2023
…rchestration.Flow project have been removed.
eerhardt
reviewed
Nov 28, 2023
stephentoub
force-pushed
the
useiserviceprovider
branch
from
November 28, 2023 17:48
d6b892e
to
8850e57
Compare
SergeyMenshykh
approved these changes
Nov 28, 2023
markwallace-microsoft
approved these changes
Nov 28, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Nov 28, 2023
SergeyMenshykh
approved these changes
Nov 28, 2023
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
@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.