microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.38k stars 3.14k forks source link

.Net: New Feature: Improve Dependency Injection #8822

Open rosieks opened 3 days ago

rosieks commented 3 days ago

Please make Dependency Injection easier. According to documentation current way to do that is:

// Create singletons of your plugins
builder.Services.AddSingleton(() => new LightsPlugin());
builder.Services.AddSingleton(() => new SpeakerPlugin());

// Create the plugin collection (using the KernelPluginFactory to create plugins from objects)
builder.Services.AddSingleton<KernelPluginCollection>((serviceProvider) => 
    [
        KernelPluginFactory.CreateFromObject(serviceProvider.GetRequiredService<LightsPlugin>()),
        KernelPluginFactory.CreateFromObject(serviceProvider.GetRequiredService<SpeakerPlugin>())
    ]
);

// Finally, create the Kernel service with the service provider and plugin collection
builder.Services.AddTransient((serviceProvider)=> {
    KernelPluginCollection pluginCollection = serviceProvider.GetRequiredService<KernelPluginCollection>();

    return new Kernel(serviceProvider, pluginCollection);
});

This looks really more like workaround, requires me to understand Kernel internals. What I would prefer is to simplified it to something like that.

builder.Services.AddScoped<LightsPlugin>();
builder.Services.AddScoped<SpeakerPlugin>();

// Registrer Kernel
builder.Services.AddKernel(options => 
{
    options.Plugins.Add<LightsPlugin>();
    options.Plugins.Add<SpeakerPlugin>();
});

This is the same pattern that people are well familiar from ASP.NET Core for registering eg. ActionFilters.

rosieks commented 2 days ago

After just digging into implementation current approach might be expensive if someone switch from singleton plugins to scoped/transient. Then every time you call CreateFromObject you use reflection to enumerate all methods and check if any is KernelFunction. It would be better if you introduce some kind of metadata of plugin that is separated from exact instance that will be used.

dmytrostruk commented 2 days ago

@rosieks We do have simplified method for DI, which will register Plugins and Kernel as Transient: https://github.com/microsoft/semantic-kernel/blob/4d73de4ad4998dbbdc17f5ae51d47f25526e079a/dotnet/src/SemanticKernel.Abstractions/Services/KernelServiceCollectionExtensions.cs#L18-L34

I think if you want to use other lifecycles for your Plugins and Kernel, you can do that by registering your components manually, as shown in your example. Let us know if AddKernel method is something you were looking for. Thanks!

rosieks commented 2 days ago

No, it doesn't simplify registration of plugins with respect to lifecycle. AddKernel in current form do not address how to register scoped plugins.

dmytrostruk commented 2 days ago

No, it doesn't simplify registration of plugins with respect to lifecycle. AddKernel in current form do not address how to register scoped plugins.

You can register scoped and singleton plugins and use AddKernel with following syntax, where KernelPlugin is an abstract representation of Plugins in Kernel:

var serviceCollection = new ServiceCollection();

serviceCollection.AddScoped<KernelPlugin>(sp => KernelPluginFactory.CreateFromType<LightsPlugin>(serviceProvider: sp));
serviceCollection.AddSingleton<KernelPlugin>(sp => KernelPluginFactory.CreateFromType<SpeakerPlugin>(serviceProvider: sp));

serviceCollection.AddKernel();

There is also another way with adding plugins using KernelBuilder:

 var collection = new ServiceCollection();

 var kernelBuilder = collection.AddKernel();

 kernelBuilder.Plugins.AddFromType<LightsPlugin>(); // LightsPlugin will be registered as singleton

Maybe we can simplify this syntax even more. But if this syntax is not helpful and something is still missing, the title and description of this issue should be updated to emphasize the problem with registering plugins with different lifecycles etc, because currently it mentions the problem in complex syntax in general, while at the moment there are ways how to use easier syntax, which I shared above.

We will also need to make sure that simpler syntax is presented in documentation and samples.

rosieks commented 1 day ago
serviceCollection.AddScoped<KernelPlugin>(sp => KernelPluginFactory.CreateFromType<LightsPlugin>(serviceProvider: sp));

The option above is much better than the one presented in documentation, though it still has certain flaws:

I think that having at least such extension method would simplify the whole registration process:

public static IKernelBuilder AddPlugin<T>(this IKernelBuilder builder)
{
    builder.Services.AddTransient(sp => KernelPluginFactory.CreateFromType<T>(serviceProvider: sp));
    return builder;
}

Then developer can just simply call:

services.AddKernel().AddPlugin<LightsPlugin>()

And doesn't care about internal implementation deatils, how Kernel handle plugins.

markwallace-microsoft commented 14 hours ago

Thanks for creating the issue @rosieks we have discussed it and plan to proceed as follows:

  1. We are going to update our documentation, we'll use this issue to track that work item
  2. We cannot introduce any breaking changes to existing methods
  3. We are open to a contribution in this area, so please create a PR with your proposal and the team will review