Add support for multiple native function arguments of many types

[stephentoub]

Motivation and Context

Move native functions closer to a normal C# experience.

Description

• Native skills can now have any number of parameters. The parameters are populated from context variables of the same name. If no context variable exists for that name, it'll be populated with a default value if one was supplied via either an attribute or a default parameter value, or if there is none, the function will fail to be invoked. The first parameter may also be populated from "input" if it fails to get input by its name or default value.
• Descriptions are now specified with the .NET DescriptionAttribute, and DefaultValue with the DefaultValueAttribute. The C# compiler is aware of the DefaultValueAttribute and ensures the type of the value provided matches that of the type of the parameter. Default values can now also be specified using optional parameter values.
• SKFunction is now purely a marker attribute, other than for sensitivity. It's sole purpose is to subset which public members are imported as native functions when a skill is imported. It was already the case that the attribute wasn't needed when importing a function directly from a delegate; that requirement has also been lifted when importing from a MethodInfo.
• SKFunctionContextParameterAttribute has been obsoleted and will be removed subsequently. DescriptionAttribute, DefaultValueAttribute, and SKName attribute are used instead. In rare situations where the method needs access to a variable that's not defined in its signature, it can use the SKParameter attribute on the method, which does have Description and DefaultValue optional properties.
• SKFunctionInputAttribute has been obsoleted and will be removed subsequently. DescriptionAttribute, DefaultValueAttribute, and SKName attribute are used instead (the latter with "Input" as the name). However, the need to use SKName should be exceedingly rare.
• InvokeAsync will now catch exceptions and store the exception into the context. This means native skills should handle all failures by throwing exceptions rather than by directly interacting with the context.
• Updated name selection heuristic to strip off an "Async" suffix for async methods. There are now very few reasons to use [SKName] on a method.
• Added support for ValueTasks as return types, just for completeness so that developers don't need to think about it. It just works.
• Added ability to accept an ILogger or CancellationToken into a method; they're populated from the SKContext. With that, there are very few reasons left to pass an SKContext into a native function.
• Added support for non-string arguments. All C# primitive types and many core .NET types are supported, with their corresponding TypeConverters used to parse the string context variable into the appropriate type. Custom types attributed with TypeConverterAttribute may also be used, and the associated TypeConverter will be used as is appropriate. It's the same mechanism used by UI frameworks like WinForms as well as ASP.NET MVC.
• Similarly, added support for non-string return types.

Example
Before:

[SKFunction("Adds value to a value")]
[SKFunctionName("Add")]
[SKFunctionInput(Description = "The value to add")]
[SKFunctionContextParameter(Name = "Amount", Description = "Amount to add")]
public Task<string> AddAsync(string initialValueText, SKContext context)
{
    if (!int.TryParse(initialValueText, NumberStyles.Any, CultureInfo.InvariantCulture, out var initialValue))
    {
        return Task.FromException<string>(new ArgumentOutOfRangeException(
            nameof(initialValueText), initialValueText, "Initial value provided is not in numeric format"));
    }

    string contextAmount = context["Amount"];
    if (!int.TryParse(contextAmount, NumberStyles.Any, CultureInfo.InvariantCulture, out var amount))
    {
        return Task.FromException<string>(new ArgumentOutOfRangeException(
            nameof(context), contextAmount, "Context amount provided is not in numeric format"));
    }

    var result = initialValue + amount;
    return Task.FromResult(result.ToString(CultureInfo.InvariantCulture));
}

After:

[SKFunction, Description("Adds an amount to a value")]
public int Add(
    [Description("The value to add")] int value,
    [Description("Amount to add")] int amount) =>
    value + amount;

Example
Before:

[SKFunction("Wait a given amount of seconds")]
[SKFunctionName("Seconds")]
[SKFunctionInput(DefaultValue = "0", Description = "The number of seconds to wait")]
public async Task SecondsAsync(string secondsText)
{
    if (!decimal.TryParse(secondsText, NumberStyles.Any, CultureInfo.InvariantCulture, out var seconds))
    {
        throw new ArgumentException("Seconds provided is not in numeric format", nameof(secondsText));
    }

    var milliseconds = seconds * 1000;
    milliseconds = (milliseconds > 0) ? milliseconds : 0;

    await this._waitProvider.DelayAsync((int)milliseconds).ConfigureAwait(false);
}

After:

[SKFunction, Description("Wait a given amount of seconds")]
public async Task SecondsAsync([Description("The number of seconds to wait")] decimal seconds)
{
    var milliseconds = seconds * 1000;
    milliseconds = (milliseconds > 0) ? milliseconds : 0;

    await this._waitProvider.DelayAsync((int)milliseconds).ConfigureAwait(false);
}

Example
Before:

[SKFunction("Add an event to my calendar.")]
[SKFunctionInput(Description = "Event subject")]
[SKFunctionContextParameter(Name = Parameters.Start, Description = "Event start date/time as DateTimeOffset")]
[SKFunctionContextParameter(Name = Parameters.End, Description = "Event end date/time as DateTimeOffset")]
[SKFunctionContextParameter(Name = Parameters.Location, Description = "Event location (optional)")]
[SKFunctionContextParameter(Name = Parameters.Content, Description = "Event content/body (optional)")]
[SKFunctionContextParameter(Name = Parameters.Attendees, Description = "Event attendees, separated by ',' or ';'.")]
public async Task AddEventAsync(string subject, SKContext context)
{
    ContextVariables variables = context.Variables;

    if (string.IsNullOrWhiteSpace(subject))
    {
        context.Fail("Missing variables input to use as event subject.");
        return;
    }

    if (!variables.TryGetValue(Parameters.Start, out string? start))
    {
        context.Fail($"Missing variable {Parameters.Start}.");
        return;
    }

    if (!variables.TryGetValue(Parameters.End, out string? end))
    {
        context.Fail($"Missing variable {Parameters.End}.");
        return;
    }

    CalendarEvent calendarEvent = new()
    {
        Subject = variables.Input,
        Start = DateTimeOffset.Parse(start, CultureInfo.InvariantCulture.DateTimeFormat),
        End = DateTimeOffset.Parse(end, CultureInfo.InvariantCulture.DateTimeFormat)
    };

    if (variables.TryGetValue(Parameters.Location, out string? location))
    {
        calendarEvent.Location = location;
    }

    if (variables.TryGetValue(Parameters.Content, out string? content))
    {
        calendarEvent.Content = content;
    }

    if (variables.TryGetValue(Parameters.Attendees, out string? attendees))
    {
        calendarEvent.Attendees = attendees.Split(new[] { ',', ';' }, StringSplitOptions.RemoveEmptyEntries);
    }

    this._logger.LogInformation("Adding calendar event '{0}'", calendarEvent.Subject);
    await this._connector.AddEventAsync(calendarEvent).ConfigureAwait(false);
}

After:

[SKFunction, Description("Add an event to my calendar.")]
public async Task AddEventAsync(
    [Description("Event subject"), SKName("input")] string subject,
    [Description("Event start date/time as DateTimeOffset")] DateTimeOffset start,
    [Description("Event end date/time as DateTimeOffset")] DateTimeOffset end,
    [Description("Event location (optional)")] string? location = null,
    [Description("Event content/body (optional)")] string? content = null,
    [Description("Event attendees, separated by ',' or ';'.")] string? attendees = null)
{
    if (string.IsNullOrWhiteSpace(subject))
    {
        throw new ArgumentException($"{nameof(subject)} variable was null or whitespace", nameof(subject));
    }

    CalendarEvent calendarEvent = new()
    {
        Subject = subject,
        Start = start,
        End = end,
        Location = location,
        Content = content,
        Attendees = attendees is not null ? attendees.Split(new[] { ',', ';' }, StringSplitOptions.RemoveEmptyEntries) : Enumerable.Empty<string>(),
    };

    this._logger.LogInformation("Adding calendar event '{0}'", calendarEvent.Subject);
    await this._connector.AddEventAsync(calendarEvent).ConfigureAwait(false);
}

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
• The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
• All unit tests pass, and I have added new tests where possible
• I didn't break anyone 😄

cc: @dluc, @shawncal

[RogerBarreto]

@shawncal @stephentoub
Since we are introducing this such great change my suggestion would to be way more radical and just obsolete the SKFunctionContextParameter, SKFunctionInput, SKFunctionContextParameter attributes altogether.

Suggestion:
Go straight to something similar to what we have in ASP.NET model biding on Controller.Action methods so:

1. A parameter variable name by default will be the name used in the context
2. The Xml parameter description will be the one used by the LLM to understand what the parameter is about.
3. Having the Attribute will be only if we want to map the name to a different name i.e. [ContextVariableName("diffName")]
4. Drop [SKFunctionInput(Description)] and use function parameter XML description instead
5. Drop [SKFunctionInput] confusing, in a multiple input scenario everything is the input.
6. Drop [SKFunction] and use the function XML description for LLM awareness
7. Drop [SKFunctionContextParameter] use the parameter name + XML as its description for LLM awareness
8. Reduce usage of [SKFunctionName], if the method is async, ignore the ending Async (We are mostly enforcing the usage of this attribute to make the non Async mapping)
9. Reduce usage of `

https://learn.microsoft.com/en-us/archive/msdn-magazine/2019/october/csharp-accessing-xml-documentation-via-reflection

[stephentoub]

use the function XML description

We don't have a great way to do this. It would require a) the compilation to always emit the .xml file (which needs to be explicitly enabled), b) for that .xml file to be deployed as part of the application, which generally doesn't happen (e.g. it's typically not copied into a bin / published folder along with the .dll / .pdb), and c) for the code to then be able to discover and load that .xml file and find the relevant nodes at execution time. You can see this, for example, if you just create a console app and reference the SK nuget(s). You'll be able to see IntelliSense for any SK APIs you use, because the nuget contains the XML file in the right place, but when you build, there won't be any .xml files in the output bin directory. While we might be able to hack this in various ways, I'm not aware of a bulletproof way to rely on it, and it's obviously a critical part of the model.

[RogerBarreto]

@stephentoub, good point on the Xml part.

I think we can move forward using the SKFunctionContextParameter for description with a smaller name like Shawn was suggesting then but that got really verbose,

To simplify this verbosity we could have an object (Model) same way ASP.NET does. Define this as the SKFunctionDTO object and pass it as one parameter.

[stephentoub]

I think we can move forward using the SKFunctionContextParameter for description with a smaller name like Shawn was suggesting then but that got really verbose,

Happy to use shorter names. I'd just kept what was already there, but yeah, they're wordy.

To simplify this verbosity we could have an object (Model) same way ASP.NET does. Define this as the SKFunctionDTO object and pass it as one parameter.

Can you give an example of what you're imaging that would look like for an example function, such as one of the ones in the PR description?

[stephentoub]

We also could choose not to introduce an SK attribute for these at all. There's already a DescriptionAttribute and a DefaultValueAttribute in .NET; we could just use those, e.g.

[SKFunction("Add an event to my calendar.", Name = "AddEvent")]
public async Task AddEventAsync(
    [Description("Event subject"), SKInput] string subject,
    [Description("Event start date/time as DateTimeOffset")] string start,
    [Description("Event end date/time as DateTimeOffset")] string end,
    [Description("Event location (optional)")] string? location = null,
    [Description("Event content/body (optional)")] string? content = null,
    [Description("Event attendees, separated by ',' or ';'.")] string? attendees = null)
{
    ...
}

A parameter variable name by default will be the name used in the context
Having the Attribute will be only if we want to map the name to a different name i.e. [ContextVariableName("diffName")]

Yup, that's what this PR does (or, rather, the parameter attribute also is there to supply the description and default value, but it's not required).

Drop [SKFunctionInput] confusing, in a multiple input scenario everything is the input.

I didn't understand this part. Can you elaborate?

Reduce usage of [SKFunctionName], if the method is async, ignore the ending Async (We are mostly enforcing the usage of this attribute to make the non Async mapping)

We can trim the ending "Async" if that's desirable, or do so only if the return type is a task. I wonder if that might be too much magic, but I'm ok with whatever you both feel is appropriate here.

[RogerBarreto]

@stephentoub I would first like to revisit my suggestions also with @shawncal and @dluc, I'm just thinking what would be my take, but would definitely await for @dluc and @shawncal to agree on my suggestions before doing any of suggested changes.

I didn't understand this part. Can you elaborate?

Today all the function "inputs" are mapped to the property INPUT of the ContextVariable, so in ultimate instance all of the parameters are indeed ContextVariables, dropping the usage of this attribute might reduce the complexity where we can by convention understand that the first parameter/argument is the Internal input for retrocompatibility (regardless of its name) + adding the first attribute as also a name to the ContextVariable to the function contextvariables in execution

Can you give an example of what you're imaging that would look like for an example function, such as one of the ones in the PR description?

Sure lets take this example:

[SKFunction("Add an event to my calendar.", Name = "AddEvent")]
public async Task AddEventAsync(
    [Description("Event subject"), SKInput] string subject,
    [Description("Event start date/time as DateTimeOffset")] string start,
    [Description("Event end date/time as DateTimeOffset")] string end,
    [Description("Event location (optional)")] string? location = null,
    [Description("Event content/body (optional)")] string? content = null,
    [Description("Event attendees, separated by ',' or ';'.")] string? attendees = null)
{
    ...
}

Using the suggestion:

public record EventModel(
    [Description("Event subject")] string Subject,
    [Description("Event start date/time as DateTimeOffset")] string Start,
    [Description("Event end date/time as DateTimeOffset")] string End,
    [Description("Event location (optional)")] string? Location = null,
    [Description("Event content/body (optional)")] string? Content = null,
    [Description("Event attendees, separated by ',' or ';'")] string? Attendees = null);
    
// (Not necessarily needs an argument, but this could be a good potential for the SKFunctionInput or better name

public async Task AddEventAsync([SKFunctionInput] EventModel event}
   ...
}

[stephentoub]

Hmm, I'm not understanding why that's advantageous... it seems like it's more verbose, and it's not clear to me what benefit it provides over just specifying the arguments to the method itself?

[stephentoub]

dropping the usage of this attribute might reduce the complexity where we can by convention understand that the first parameter/argument is the Internal input for retrocompatibility (regardless of its name) + adding the first attribute as also a name to the ContextVariable to the function contextvariables in execution

In this PR, if there's a single string argument and it's not using [SKFunctionInput], it first tries to look up a context variable with that parameter name, and if that fails, then it tries with "input". We could easily tweak the heuristic to allow that to work for any string argument if desired... the problem with that, though, is (at least as far as I can tell) there's always an "Input" value, so we'd never end up using a specified default or failing.

I think it'd be reasonable to drop [SKFunctionInput] entirely and just say that if you want it to be mapped to "input", name it "input" 😄, either in the C# parameter name or in the override name in the attribute.

[stephentoub]

Thanks for all the feedback. I made a bunch of changes in response.

The examples from earlier in the post are now:

[SKFunction, Description("Makes a PUT request to a uri")]
public Task<string> PutAsync(
    [Description("The URI of the request"), SKName("input")] string uri,
    [Description("The body of the request")] string body,
    CancellationToken cancellationToken = default)
{
    return this.SendRequestAsync(uri, HttpMethod.Put, new StringContent(body), cancellationToken);
}

and

[SKFunction, Description("Add an event to my calendar.")]
public async Task AddEventAsync(
    [Description("Event subject"), SKName("input")] string subject,
    [Description("Event start date/time as DateTimeOffset")] string start,
    [Description("Event end date/time as DateTimeOffset")] string end,
    [Description("Event location (optional)")] string? location = null,
    [Description("Event content/body (optional)")] string? content = null,
    [Description("Event attendees, separated by ',' or ';'.")] string? attendees = null)
{
    ...
}

[stephentoub]

I added an additional commit that adds support for non-string args. 😄

For example, this now works:

[SKFunction]
static string Test(int a, long b, decimal c, Guid d, DateTimeOffset e, DayOfWeek? f) =>
    $"{a} {b} {c} {d} {e:R} {f}";

It relies on TypeDescriptors/TypeConverters to do all the conversion, so it’s using the same mechanism as client frameworks like WinForms and server frameworks like ASP.NET MVC, and it’s extensible, in that a type can attribute itself as [TypeConverter(…)] and specify what type should be used for the conversion (converters can also be globally registered). We can of course change or augment the mechanism if desired; there’s a clear place in the code where we’d just plug in whatever different parser routine we want to use for a given type.

[outofgamut]

For teams who are using the CopilotChatApp as a starter project for experimenting with SK, does this PR allow for simplified syntax around skills definitions? i cloned the latest version of the semantic kernel repo, but i'm getting errors when using [SKFunction]

[stephentoub]

For teams who are using the CopilotChatApp as a starter project for experimenting with SK, does this PR allow for simplified syntax around skills definitions? i cloned the latest version of the semantic kernel repo, but i'm getting errors when using [SKFunction]

Yes:
#1811