.Net: Does IKernel / Kernel need to store an IPromptTemplateEngine? #3191
Assignees
Labels
.NET
Issue or Pull requests regarding .NET code
sk team issue
A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community)
Milestone
Comments
|
That's exactly what I thought about all the time. Here's the same idea from our latest design review meeting: |
evchaki
added
the
sk team issue
markwallace-microsoft
moved this from Backlog - New features
to Sprint: Planned
in Semantic Kernel
markwallace-microsoft
moved this from Sprint: Planned
to Sprint: In Review
in Semantic Kernel
markwallace-microsoft
moved this from Sprint: In Review
to Sprint: In Progress
in Semantic Kernel
markwallace-microsoft
moved this from Sprint: In Progress
to Community PRs
in Semantic Kernel
github-merge-queue bot
pushed a commit
that referenced
this issue
Nov 28, 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. --------- Co-authored-by: SergeyMenshykh <sergemenshikh@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
There are 0 calls to KernelBuilder.WithPromptTemplateEngine anywhere in the code base... including tests.
The only direct use of Kernel's ctor that accepts a prompt template engine is in a test that then completely ignores the constructed kernel.
Kernel itself does not use the PromptTemplateEngine (though its constructor apparently thinks it's so important it stores it into the same field twice).
PromptTemplate has a ctor
public PromptTemplate(string template, PromptTemplateConfig promptTemplateConfig, IKernel kernel)that's used 0 times in the whole codebase, and all it does anyway is just delegate to the ctor that takes an IPromptTemplateEngine instead of an IKernel.In the entire repo there are only 5 call sites that use
.PromptTemplateEngine, 1 of which is the above ctor and 2 of which are tests of RegisterSemanticFunction. The remaining 2 uses are in the CreateSemanticFunction and ImportSemanticFunctionsFromDirectory extension methods, which take an IKernel but could easily accept an IPromptTemplateEngine as well.What's the benefit of having this top-level in IKernel? Why can't it be just another entry in the ServiceCollection?
I'm looking at IKernel from the perspective of trying to understand what it really is. And once you look past the cruft, it seems like it's really just:
and I'm wondering if maybe IKernel/Kernel as types should go away: code can just pass around a ServiceCollection and a FunctionCollection as needed, and have a static RunAsync method somewhere that's purely about the invocation.
The text was updated successfully, but these errors were encountered: