[C# Port] Refactor the action handling system

[singhk97]

No description provided.

[singhk97]

The current action delegate is quite rigid and has a few flaws. Look at the default actions and how the handler functions are implemented here:

teams-ai/dotnet/packages/Microsoft.Bot.Builder.M365/Microsoft.Bot.Builder.M365/AI/Action/DefaultActions.cs

Lines 10 to 12 in bfe4412

	// TODO: Resolve this issue before private preview. Need more research and thinking. How are developers going to use this?
	// 1. Unused parameters, 2. Making the data parameter type more explicit and not just "object".
	public class DefaultActions<TState> where TState : TurnState

The flaws are:

1. Unused parameters - the delegate signature has a fixed set of parameters which might not be used in the handler implementation. We ideally don't want users to write functions with parameters that they won't be using. Possible solutions:

   • Supporting multiple delegate types - this is what SK does. They have at least 15 different delegate types that they support. Those delegate signatures are a combination of a 2 parameters and a few return types. It might be harder for us to do this as we support 5 parameter
   • Having a single parameter object - This is problematic as it's unclear which properties are required for an arbitrary function.
   • Dynamic delegates - It might be possible to have a dynamic delegates type. Take a look at: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/minimal-apis/overview?view=aspnetcore-7.0.

The delegate type for the MapGet method is dynamic and dependent on the previous argument. They might be using the reflection api to figure out the parameters and populate the delegate accordingly.

2. Making the data parameter type more explicit and not just "object" - We have it at that right now because the parameter must be able to take all kinds of types. However, we want to take advantage of the strongly-typedness of C#. Generic types will probably fix this.

[swatDong]

@singhk97 @Stevenic , I have another idea to use parameter attribute to support dynamic argument. This idea comes from Azure Functions Binding that Azure Functions support dynamic argument by different parameter attributes, such as:

For example, we provide parameter attributes like TurnContextAttribute, TurnStateAttribute, ActionDataAttribute, ActionNameAttribute. Then it's up to action method to use none or one or more of those attributes.

    public class MyInstance
    {
        [Action("FullAction")]
        public void MyFullActionHandler(
            [TurnContext] TurnContext context,
            [TurnState(typeof(MyTurnState))] MyTurnState state,
            [ActionData(typeof(MyData))] MyData data,
            [ActionName] string actionName)
        {
            return;
        }

        [Action("PartialAction")]
        public void MyPartialActionHandler([ActionName] string actionName, [ActionData(typeof(MyData))] MyData data)
        {
            return;
        }

        [Action("DoNothing")]
        public void DoNothingActionHandler()
        {
            return;
        }
    }

By the way, Generic Attributes is just proposed, so for now we may need to pass type to the attribute.

[singhk97]

Adding @vule-msft to the discussion.

[stephentoub]

Supporting multiple delegate types - this is what SK does. They have at least 15 different delegate types that they support. Those delegate signatures are a combination of a 2 parameters and a few return types. It might be harder for us to do this as we support 5 parameter

It's what SK used to do. It's no longer what it does. It now supports any number of arguments of any relevant type.
microsoft/semantic-kernel#1195

[swatDong]

Supporting multiple delegate types - this is what SK does. They have at least 15 different delegate types that they support. Those delegate signatures are a combination of a 2 parameters and a few return types. It might be harder for us to do this as we support 5 parameter

It's what SK used to do. It's no longer what it does. It now supports any number of arguments of any relevant type. microsoft/semantic-kernel#1195

Thanks for this reminder. It's great to know there's such use pattern in SK.
BTW, I went through the code and noticed that it's using MethodInfo.Invoke(...) to call user-defined native method.
Is there any usage or performance consideration on choosing MethodInfo.Invoke or Expression.Compile?

[stephentoub]

Is there any usage or performance consideration on choosing MethodInfo.Invoke or Expression.Compile?

Expression.Compile is using reflection emit under the covers to generate custom IL. It's thus much more expensive on first use, in exchange for having invocation performance be much faster. If you're going to be executing the call a lot and the overhead of those calls matter, it can be a worthwhile tradeoff. The difference is measured in nanoseconds, though, so you really do need to be making many, many calls to the resulting delegate for the startup costs to be worth it.

Also, recent versions of .NET optimize MethodInfo.Invoke further. In .NET 7, the implementation can actually use reflection emit under the covers to achieve a similar optimization. See https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/#reflection for more details.

[swatDong]

Expression.Compile is using reflection emit under the covers to generate custom IL. It's thus much more expensive on first use, in exchange for having invocation performance be much faster. If you're going to be executing the call a lot and the overhead of those calls matter, it can be a worthwhile tradeoff. The difference is measured in nanoseconds, though, so you really do need to be making many, many calls to the resulting delegate for the startup costs to be worth it.

Also, recent versions of .NET optimize MethodInfo.Invoke further. In .NET 7, the implementation can actually use reflection emit under the covers to achieve a similar optimization. See https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/#reflection for more details.

This is really helpful. Thanks a lot!

[swatDong]

@stephentoub , the sample and doc you provided are really helpful, and I've implemented new attribute-based handler.

Discussed with @singhk97 and one of the good suggestions is to provide an analyzer to check the parameter type of the attribute.

I went through SK repo but did not find any similar part. In addition, the only similar analyzer I found is from several years ago.

Is there any suggested getting-started or potential risk for providing analyzer to check attribute-binding parameter type?

Thanks again!

[stephentoub]

Is there any suggested getting-started or potential risk for providing analyzer to check attribute-binding parameter type?

There are tons of analyzers implemented in https://github.com/dotnet/roslyn-analyzers... you should be able to find some good fodder there. The risk is simply that you're writing code that will run as part of compilation, e.g. if you have a bug and it's really slow, that will significantly slow down compile times.