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 Filters only through ServiceCollection #6085

Open
RogerBarreto opened this issue May 1, 2024 · 4 comments
Open

.Net Filters only through ServiceCollection #6085

RogerBarreto opened this issue May 1, 2024 · 4 comments
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)

Comments

@RogerBarreto
Copy link
Member

RogerBarreto commented May 1, 2024

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 markwallace-microsoft added .NET Issue or Pull requests regarding .NET code triage labels May 1, 2024
@RogerBarreto RogerBarreto added sk team issue A tag to denote issues that where created by the Semantic Kernel team (i.e., not the community) and removed triage labels May 1, 2024
@dmytrostruk
Copy link
Member

dmytrostruk commented May 2, 2024

@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());
}

@stephentoub
Copy link
Member

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?

@dmytrostruk
Copy link
Member

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.

@stephentoub
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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)
Projects
Status: No status
Development

No branches or pull requests

4 participants