microsoft/semantic-kernel

.Net Filters only through ServiceCollection

Opened this issue · 4 comments

Currently Filters are already available on both Kernel and in the ServiceCollection.

The proposition is to drop the Kernel specific collections in favor of Service centric filters.

This will reduce the maintenance and code changes needed whenever a new filter type is added as Kernel Components grow over time.

@markwallace-microsoft @matthewbolanos Regarding this issue:
Kernel specific collections (e.g. kernel.FunctionInvocationFilters etc) allow to modify filter ordering in runtime. Users can insert/remove/sort filter at any position (beginning, middle, end) in collection, at any time of application execution. and all filters will be executed in correct order.

If we remove these collections and leave ServiceCollection as the only option how to register filters - I'm not sure the capabilities above can be achieved.

This will reduce the maintenance and code changes needed whenever a new filter type is added as Kernel Components grow over time.

It's always possible to add new type of filter and allow to register it through ServiceCollection only and expose collection on Kernel level later when requested from users. Maybe this won't be consistent, but it will be a trade-off.

We can also come up with new data structure that will hold dictionary of filter collections. In this case, we can simplify filter registration on Kernel level and preserve the capabilities described above (insert/remove/sort) with something like this:

public interface IFunctionInvocationFilter { }
public interface IPromptRenderFilter { }

public class FunctionInvocationFilter1 : IFunctionInvocationFilter { }
public class FunctionInvocationFilter2 : IFunctionInvocationFilter { }
public class PromptRenderFilter1 : IPromptRenderFilter { }
public class PromptRenderFilter2 : IPromptRenderFilter { }

public class Filters : Dictionary<Type, List<object>>
{
    public void Add(object filter)
    {
        this.AddIf<IFunctionInvocationFilter>(filter);
        this.AddIf<IPromptRenderFilter>(filter);
    }

    private void AddIf<TFilter>(object filter)
    {
        if (filter is TFilter)
        {
            var filterType = typeof(TFilter);
            if (this.ContainsKey(filterType))
            {
                this[filterType].Add(filter);
            }
            else
            {
                this[filterType] = [filter];
            }
        }
    }
}

public class Kernel
{
    public Filters Filters = [];
}

public static void Main(string[] args)
{
    var kernel = new Kernel();

    kernel.Filters.Add(new FunctionInvocationFilter1());
    kernel.Filters.Add(new FunctionInvocationFilter2());

    kernel.Filters.Add(new PromptRenderFilter1());
    kernel.Filters.Add(new PromptRenderFilter2());
}

This will reduce the maintenance and code changes needed whenever a new filter type is added as Kernel Components grow over time.

This wouldn't seem to impact it particularly meaningfully. The Kernel ctor would still need to fetch all the relevant filters. The invocation logic that uses the filters would still need to be there. The only thing it saves is a public property exposing the collection the Kernel constructor got. That's a few lines of code. Am I misunderstanding?

The only thing it saves is a public property exposing the collection the Kernel constructor got. That's a few lines of code. Am I misunderstanding?

That's correct, public property that exposes collection is the only place where we will need to add it. The only question is how many filters we will have in the future and whether a lot of filter collection properties will make Kernel type more complex or not.

The only thing it saves is a public property exposing the collection the Kernel constructor got. That's a few lines of code. Am I misunderstanding?

That's correct, public property that exposes collection is the only place where we will need to add it. The only question is how many filters we will have in the future and whether a lot of filter collection properties will make Kernel type more complex or not.

I'd argue the complexity is there regardless of whether there's a public property or not. At least having it there means someone is aware of what state the Kernel instance is carrying, and bonus, they get to change it. DI is intended for things that can be centrally configured, but there are various scenarios where the filters desired to be used are specific to the use site.