.Net: Don't limit [KernelFunction] to public methods

[stephentoub]

A developer already need to opt-in a method on a plugin to being part of the plugin by specifying the [KernelFunction] attribute; requiring that method to also be public is superfluous, and means that a type's plugin surface area must be a subset of its public surface area. That prohibits patterns where a type wants to syntactically be a plugin but not expose those APIs via its .NET public surface area.

(Curious to see if folks think this is controversial.)

[dmytrostruk]

(Curious to see if folks think this is controversial.)

@stephentoub I was actually thinking about the idea of allowing to import plugins without [KernelFunction] attribute at some point. Today, if there is already existing library, that I want to re-use in my application and make it part of LLM-related operations, I have to create adapter class with one-liner methods that will simply call that library, but the methods will be marked with [KernelFunction]. It looks like secure way of adding plugins, because developer explicitly say which methods to include. But if I want to include all of the methods from public API of that library, I still need to create adapter class and list these methods. We can try to remove the requirement to have [KernelFunction] attribute, which immediately makes all existing .NET-libraries as SK compatible plugins. And if developer wants to limit the number of functions to include or specify more information about function and/or parameters with [Description] - it's still possible to create adapter class :)

But with changes in this PR, I'm not sure if the idea above will work, because it will also import something that is not part of public API, which would be kind of incorrect.

Would be interesting to hear your opinion about this!

[stephentoub]

But if I want to include all of the methods from public API of that library, I still need to create adapter class and list these methods.

Just including all public methods of an existing type is unlikely to yield a good plugin. The APIs are very likely to lack the metadata (e.g. Description) that would allow the methods to be appropriately described to the model. There's also very commonly functionality on these types that would not want included, whether they be operators, or methods from a base type (e.g. everything defined on System.Object), or interface implementations for common things like IEquatable, etc.

But with changes in this PR, I'm not sure if the idea above will work, because it will also import something that is not part of public API, which would be kind of incorrect.

Nothing prevents having another overload of methods like CreateFromObject/Type that looks for something other than [KernelFunction], e.g. an extra bool argument. But, as noted above, I'm not convinced that's actually desirable.

[dmytrostruk]

There's also very commonly functionality on these types that would not want included, whether they be operators, or methods from a base type (e.g. everything defined on System.Object), or interface implementations for common things like IEquatable, etc.

We can define on our side what is common and filter that, for example methods like GetType or Equals.

But anyway, just a proposal I was thinking about for some period of time :)

[RogerBarreto]

I would add a dedicated Attribute [KernelPrivateMethod] or a specific overload to the Attribute [KernelMethod(allowPrivate: true)] to allow private methods, with some strong remarks, as I'm not totally convinced of encouraging developers to break (Open/Closed Principle) on their APIs exposing private methods to the AI if those weren't intended for "Humans".

[RogerBarreto]

Recently we were discussing the possibility of having generic methods being able to me marked as [KernelMethod], appreciate your thoughts also @stephentoub.

[stephentoub]

I would add a dedicated Attribute [KernelPrivateMethod] or a specific overload to the Attribute [KernelMethod(allowPrivate: true)] to allow private methods, with some strong remarks, as I'm not totally convinced of encouraging developers to break (Open/Closed Principle) on their APIs exposing private methods to the AI if those weren't intended for "Humans".

Why is language visibility synonymous with Opem/Closed? KernelFunctionAttribute is just another domain of deciding what's open/closed. It's its own language for it. That's one of the reasons I don't like the public requirement... it's conflating two different schemes unnecessarily.

[stephentoub]

Recently we were discussing the possibility of having generic methods being able to me marked as [KernelMethod], appreciate your thoughts also @stephentoub.

Seems fine.

[RogerBarreto]

That prohibits patterns where a type wants to syntactically be a plugin but not expose those APIs via its .NET public surface area.

I can't deny that we may want to expose methods to AI but not to the developer, but this can create a potential vector of attack for developers using plugins functions directly to be able to access those private methods as public, in theory being a private [KernelMethod] enabled function don't avoid the .NET developer to access it.

Why is language visibility synonymous with Open/Closed

Don't need to be

[stephentoub]

That prohibits patterns where a type wants to syntactically be a plugin but not expose those APIs via its .NET public surface area.

I can't deny that we may want to expose methods to AI but not to the developer, but this can create a potential vector of attack for developers using plugins functions directly to be able to access those private methods, in theory being a private enabled function don't avoid the .NET developer to access it.

I don't understand. The developer of the plugin is explicitly marking the method with the attribute... from a safety perspective, how is that any different than explicitly marking it as public? Why is:

public static object GetPlugin() => new A();
public class A
{
    [KernelFunction]
    public void M() {}
}

any safer than:

public static object GetPlugin() => new A();
private class A
{
    [KernelFunction]
    public void M() {}
}

? Those both work today, yet the latter method is not visible via the language to a consumer.

[RogerBarreto]

One example. Assuming I don't want to expose the GetToken.

IMHO when I implement something as private, it in theory should officially not be inaccessible to the public API's at any moment, overall there is not a problem adding [KernelMethod] to a private method, if we are all aware and make it very clear via docs that as soon you mark a method (regardless of accessibility) you are making it public to the AI and indirectly public to the developer too.

public class A
{
    [KernelMethod]
    private string GetToken()
    
    [KernelMethod]
    public string GetEmails()
}

// Not public
var a = new A();
a.GetToken() -> Fails
    
// Works using this way (public)
var functions = kernel.ImportPluginFromType<A>();
var protectedToken = await functions["GetToken"].Invoke(kernel);

[stephentoub]

Assuming I don't want to expose the GetToken.

Then don't mark it as [KernelFunction], the entire point of which is to expose it as part of the plugin 😄

[stephentoub]

you are making it public to the AI and indirectly public to the developer

It's similar to an explicit interface implementation. In:

public class Foo : IBar
{
    void IBar.M();
}

M() is not part of Foo's public surface area, but the developer chose to expose it via the interface implementation, and it's still available via casting to IBar and using IBar.M.

(public/private is also not a security boundary.)

[stephentoub]

We don't need to merge this if folks are really against it. It just seems like an unnecessary restriction that forces a dev that wants to do this to work around it in awkward ways, and unnecessary friction for someone who explicitly tags a non-public method as [KernelFunction] but is then met with an exception.

[markwallace-microsoft]

? Those both work today, yet the latter method is not visible via the language to a consumer.

Allowing a private class with a public method but not a public class with a private method does seem odd. But I wasn't aware the example you provided above would work. We're going to discuss during todays stand up parking lot, I've sent you an invite.

[stephentoub]

Offline there was a question about where else in .NET attributes on private methods are respected. A couple of examples that come to mind include [Event] with EventSource (the method will be included in the manifest / source regardless of its visibility) and [OnSerializing/Deserializing/...] with data contract serializers (the method will be used regardless of its visibility).

[stephentoub]

@matthewbolanos, before this is merged, any thoughts / concerns?

[markwallace-microsoft]

@matthewbolanos, before this is merged, any thoughts / concerns?

I chatted with Matthew about this and he approves of making this change.