Enable AIFunctions to be passed an IServiceProvider, Alternate 3 #6146
Conversation
| /// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for cancellation requests. The default is <see cref="CancellationToken.None"/>.</param> | ||
| /// <returns>The result of the function's execution.</returns> | ||
| public Task<object?> InvokeAsync( | ||
| IEnumerable<KeyValuePair<string, object?>>? arguments = null, | ||
| IServiceProvider? services = null, |
There was a problem hiding this comment.
Of all the alternative proposals, I think I like this one the best.
This makes the IServiceProvider way more discoverable compared to putting it in the IEnumerable<KeyValuePair<string, object?>>? param or the FunctionInvokingChatClient.CurrentContext async local.
I'd be more okay with putting the IServiceProvider in the arguments parameter if it was typed as a AIFunctionArguments rather than IEnumerable<KeyValuePair<string, object?>>?, since then the IServiceProvider would be a little more discoverable, but still I think the IServiceProvider should be a first-class concept similar to the CancellationToken. It's very different than all the other non-CancellationToken arguments that are deserialized from the tool invocation.
I also like that it doesn't introduce a FromServicesAttribute in the Microsoft.Extensions.AI namespace. I get the other proposals don't require that, but I think it's a good decision. I think wanting to use attributes to identify service parameters is a common enough scenario to move into Microst.Extensions.DependencyInjection.Abstractions alongside the FromKeyedServicesAttribute.
However, it might be interesting to have an overload of AIFunction.Create that took an IServiceProvider and used IServiceProviderIsService to determine which parameters should come from the service provider. Than I think it would be reasonable to respect the existing FromKeyedServicesAttribute without adding a new FromServicesAttribute which would no longer be necessary.
There was a problem hiding this comment.
However, it might be interesting to have an overload of AIFunction.Create that took an IServiceProvider and used IServiceProviderIsService to determine which parameters should come from the service provider.
It still seems really strange to me that we'd base decisions for the returned AIFunction on an IServiceProvider instance that is possibly not the same IServiceProvider instance that's used later for invocation.
I think wanting to use attributes to identify service parameters is a common enough scenario to move into Microst.Extensions.DependencyInjection.Abstractions alongside the FromKeyedServicesAttribute.
We'd need to introduce a new attribute, presumably, in order to fix the namespace. And ASP.NET would then need to support both (the interface it uses wouldn't be implementable on the M.E.DI.Abstractions one).
Seems like for now the best answer is to just special-case IServerProvider in the signature of AIFunctionFactory.Create methods, and we could later support the convenience attribute mechanism.
That is separate from how the IServiceProvider finds its way into the AIFunction invocation. Choices are basically:
- Introduce an AIFunctionArguments type, leave InvokeAsync signature as it is, AIFunction implementations can type test for AIFunctionArguments, callers of InvokeAsync that have an IServiceProvider instantiate an AIFunctionArguments.
- Introduce an AIFunctionArguments type, change InvokeAsync's arguments parameter to be strongly typed as AIFunctionArguments.
- Change InvokeAsync's signature to take the arguments, the service provider, and the cancellation token, all as peers.
(1) is the only one we could do if we were doing this in a month. As we're doing it now, we can choose to take the break for (2) or (3).
(1) is hacky and relies on callers knowing they should instantiate the special type and implementers knowing they should check for it.
(2) makes it clear exactly what type should be used, and doesn't promote IServiceProvider to the same importance conceptually as the nominal arguments or cancellation token, while still allowing for future expansion should there be other state we want to allow to flow in. But it also forces all inputs to be wrapped in this special collection, which has some overhead and inconvenience (though most code won't directly invoke AIFunctions).
(3) doesn't require such wrapping and makes it obvious that an IServiceProvider can be passed in, but effectively makes DI primary in the API and doesn't afford us the ability to pass in more state in the future (we'd end up falling back to (1) if we need to be able to do that).
I'm leaning towards (2). @halter73 is leaning towards (3). @eiriktsarpalis? @SteveSandersonMS?
There was a problem hiding this comment.
It still seems really strange to me that we'd base decisions for the returned AIFunction on an IServiceProvider instance that is possibly not the same IServiceProvider instance that's used later for invocation.It still seems really strange to me that we'd base decisions for the returned AIFunction on an IServiceProvider instance that is possibly not the same IServiceProvider instance that's used later for invocation.
I don't think it's that strange. It's something we do a lot in ASP.NET Core. Here's an example from minimal APIs. We do the same thing for MVC and SignalR to avoid regenerating the method invocation code every request.
In theory, someone could replace HttpContext.RequestServices to not match IHost.Services, but that hasn't been a problem. In practice, RequestServices usually represents a scope created from IHost.Services.
It is really convenient to get services automatically injected without needing to attribute them or rely on the service locator pattern, so it'd be sad to give that up to avoid the small risk of someone getting confused because they used multiple service providers with different sets of services. However, I suppose we could always add support for this later, since this would involve new overloads to AIFunction.Create.
There was a problem hiding this comment.
The difference in my mind is how integrated the usage is into the DI-rooted programming model in the ASP..NET cases. The IServiceProvider ends up largely being ambient implicit context. That is not the case for the largely independent AIFunctionFactory, where you'd be very explicitly providing it with a specific IServiceProvider instance rather than it being picked up automatically from the environment.
we could always add support for this later, since this would involve new overloads to AIFunction.Create
Yes, though I don't think it would need new overloads. We'd just change the behavior if an IServiceProvider was set into a new property on the creation options.
There was a problem hiding this comment.
I'm leaning towards (2). @halter73 is leaning towards (3). @eiriktsarpalis? @SteveSandersonMS?
Barring other's input, I'm going to go with (2). I think we'll regret not having the mechanism to pass in additional state.
|
Replaced by #6158 |
Alternate to #6141. Change AIFunction.InvokeAsync to directly take IServiceProvider as an argument, and special-case IServiceProvider for schema generation. Basically IServiceProvider is a first-class citizen in the AIFunction world.
Microsoft Reviewers: Open in CodeFlow