Change type of AIFunctionFactoryOptions.CreateInstance delegate

[stephentoub]

To flow the [DynamicallyAccessedMembers] annotation, we need a custom delegate.

Closes #6415

Microsoft Reviewers: Open in CodeFlow

[stephentoub]

Any reason we might want this to be async, returning ValueTask<object> and accepting a CancellationToken?

[SteveSandersonMS]

TBH it seems weird to me that we have the AIFunctionFactory.Create overload that takes a targetType. Conceptually it seems like it should be possible for the AIFunctionFactory.Create caller to construct their own target instance and call the overload that accepts a target, or to call the overload that works with static delegates and inside that to construct a target instance on a per-call basis before forwarding the call.

Tracing back the history, it looks like this feature was added in #6193 to help with an MCP scenario but it's not clear to me it's a general requirement beyond MCP (and hence maybe could be implemented inside the MCP library for its own usage). I appreciate I didn't raise this question on #6193 at the time, but it looks like it all went through in under a day. Would be interested to know the context I'm missing.

I'm not saying we need to create disruption by changing this if there are reasons to have the implementation be inside MEAI.Abstractions instead of the modelcontextprotocol package. However, it is difficult to answer this question:

Any reason we might want this to be async

... without really knowing the use cases.

[eiriktsarpalis]

If the target type is fixed for every AIFunction, why couldn't this be expressed as a closed Func<object>? It might incur an additional closure allocation per function instance, but I doubt that would be a dominant factor in AIFunctionFactory.

[stephentoub]

It's not just about MCP. It's about the same kinds of scenarios for which ActivatorUtilities.CreateInstance exists in general. With libs like SK and MCP, developers want to write types and tag not just static methods but also instance methods for use as AIFunctions... something imports members of those types, and for instance, it needs to create the instance. This happens in other domains, too. There are of course ways to express the same thing via statics that take the ctor parameters as well, but that's true for the language in general, and even with that we kept getting requests for this until we added it.

Are you suggesting we remove this? That would basically mean not using AIFunctionFactory in MCP or other similar situations.

[SteveSandersonMS]

Just trying to understand the use cases. Thanks for clarifying about the "write types and tag not just static methods but also instance methods" point - that explains why it's not good to require people to make their own static wrappers that do the instantiation inline, e.g.:

var fn = AIFunctionFactory.Create(async static (string arg1, int arg2) =>
{
    var instance = ActivatorUtilities.CreateInstance<MyThing>(services);
    try
    {
        return await instance.SomeMethodAsync(arg1, arg2);
    }
    finally
    {
        await instance.DisposeAsync();
    }
});

Not good because a framework might only have access to the attributed MethodInfo so it can't write any of this strongly-typed code. It would have to use reflection to get the parameters and make the call, which would all be painful.

It's not a big deal, but one remaining question: isn't it mostly redundant to specify both targetType and options.CreateInstance? Could it suffice just to specify options.CreateInstance? It seems like the options.CreateInstance flow is nearly always what a framework would have to use (in order to hook up instantiation with DI), so conceptually we could say that is what you do if you want to use an instance MethodInfo.

Now getting back to your question:

Any reason we might want this to be async

I don't think application code would likely have reason to do async instantiation. Application code knows the types and methods it's working with, so any async process can itself be deferred until it's inside the function callback.

I guess some framework code could theoretically have a reason to go through an async process inside options.CreateInstance and can't defer that until it's inside the callback for all the same reasons as above. However I can't cite a practical concrete example right now. If such a need ever arose, though it seems unlikely, we could add a further options.CreateInstanceAsync that people could optionally use.

[stephentoub]

isn't it mostly redundant to specify both targetType and options.CreateInstance?

Mostly, yes, but not entirely. Two issues:

1. NativeAOT / linking. It can't see through the MethodInfo to the Type to ensure that the public ctors on the type are preserved. The dedicated targetType attribute has a [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] attribute on it that can't be applied to the first parameter.
2. Inheritance. You could have a MethodInfo from a base type and want a derived type instantiated. This is basically MemberInfo.ReflectedType vs DeclaringType.

(2) shouldn't be a big deal, as someone could just specify the MethodInfo for the right type and we'd use ReflectedType. But we don't have a good alternative for (1).

If such a need ever arose, though it seems unlikely, we could add a further options.CreateInstanceAsync that people could optionally use.

👍

[stephentoub]

If the target type is fixed for every AIFunction, why couldn't this be expressed as a closed Func<object>? It might incur an additional closure allocation per function instance, but I doubt that would be a dominant factor in AIFunctionFactory.

I believe that would work functionally. As you say, it would incur an extra few allocations when using this functionality. But, if you're using this Create overload, you're unlikely to be doing so on a hot path, so, it's probably fine.

Is that what you'd prefer?

[stephentoub]

Actually, that raises another possibility: don't have this delegate in AIFunctionFactoryOptions, and instead change the signature of Create from:

public static AIFunction Create(
    MethodInfo method,
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type targetType,
    AIFunctionFactoryOptions? options = null);

to:

public static AIFunction Create(
    MethodInfo method,
    Func<AIFunctionArguments, object> createInstanceFunc,
    AIFunctionFactoryOptions? options = null);

I suggest that in particular because it's weird having a creation func that ignores the target type; at that point it's even more strange that we require this target type to be passed. It's also a bit cleaner as it doesn't have a CreateInstance delegate in the options that only applies to this overload. And it removes some of the duplication concerns @SteveSandersonMS highlighted, because it's no longer being duplicated: now in addition to the MethodInfo, you're specifying how the instance should be created rather than the type that should be created, so it's obviously conveying additional information.

[SteveSandersonMS]

Func<AIFunctionArguments, object> createInstanceFunc

That does look more natural and avoids the seeming duplication (assuming this means we also drop options.CreateInstance).

I guess it means we're handing off responsibility for NativeAOT compatibility to the caller, i.e., it's up to them to either call a constructor directly from their createInstanceFunc or they have to do whatever upstream annotations are needed if they are a framework and their createInstanceFunc works by calling Activator.CreateInstance(someType).

This design also places us better if in the future we did want an async overload. It would be entirely natural to have another overload that takes Func<AIFunctionArguments, Task<object>> without the same wartiness of having both CreateInstance and CreateInstanceAsync where you could confusingly set both.

[stephentoub]

assuming this means we also drop options.CreateInstance

Yup

I guess it means we're handing off responsibility for NativeAOT compatibility to the caller

Yes, which was already the case: the targetType parameter was annotated with [DAM(PublicConstructors)], so whatever call site was invoking Create already had to have the appropriate information available. If that call site is changed to just use _ => Activator.CreateInstance(targetType) or args => ActivatorUtilities.CreateInstance(args.Services, targetType), it'll continue to be as compatible.

[stephentoub]

Replaced by #6424