Enable AIFunctionFactory.Create functions to get DI services #6141
Conversation
| @@ -248,6 +253,30 @@ public static ReflectionAIFunctionDescriptor GetOrCreate(MethodInfo method, AIFu | |||
|
|
|||
| private ReflectionAIFunctionDescriptor(DescriptorKey key, JsonSerializerOptions serializerOptions) | |||
| { | |||
| AIJsonSchemaCreateOptions schemaOptions = new() | |||
| { | |||
| // This needs to be kept in sync with the shape of AIJsonSchemaCreateOptions. | |||
There was a problem hiding this comment.
Should AIJsonSchemaCreateOptions expose a Clone method? Or be a record if that won't introduce other issues?
There was a problem hiding this comment.
Given that AIJsonSchemaCreateOptions has custom implementations for Equals and GetHashCode that seem to mirror what records would give us, I think that makes sense. Especially now if we're cloning in some places.
If we don't go the record route, it would probably be best to extract this clone into an internal testable method, so you could write a reflection-based test like this to verify no one accidentally adds a property and forgets to update the clone method.
| /// as the arguments to a call to <see cref="AIFunction.InvokeAsync"/>. | ||
| /// </remarks> | ||
| [AttributeUsage(AttributeTargets.Parameter)] | ||
| public sealed class FromServicesAttribute : Attribute |
There was a problem hiding this comment.
I know there's some hesitancy about duplicating the same name MVC uses. I don't have a better idea, though.
There was a problem hiding this comment.
Do we have further reasons to think it wouldn't suffice to support passing in an IServiceProvider, and having the function resolve services from there?
There was a problem hiding this comment.
Would be worth starting an effort lowering all those attributes to the M.E.DI package? We could then take a dependency on the next preview package.
There was a problem hiding this comment.
Would be worth starting an effort lowering all those attributes to the M.E.DI package? We could then take a dependency on the next preview package.
Not for a GA release. We wouldn't be able to add that until November. And FromServices is in an ASP.NET MVC namespace.
There was a problem hiding this comment.
I left a comment on #6146 explaining why I like that proposal the best. Is there any major pushback to making the IServiceProvider a top-level parameter like CancellationToken to AIFunction.InvokeAsync?
| @@ -63,11 +64,13 @@ public partial class FunctionInvokingChatClient : DelegatingChatClient | |||
| /// </summary> | |||
| /// <param name="innerClient">The underlying <see cref="IChatClient"/>, or the next instance in a chain of clients.</param> | |||
| /// <param name="logger">An <see cref="ILogger"/> to use for logging information about function invocation.</param> | |||
| public FunctionInvokingChatClient(IChatClient innerClient, ILogger? logger = null) | |||
| /// <param name="services">An optional <see cref="IServiceProvider"/> to use for resolving services required by the <see cref="AIFunction"/> instances being invoked.</param> | |||
| public FunctionInvokingChatClient(IChatClient innerClient, ILogger? logger = null, IServiceProvider? services = null) | |||
There was a problem hiding this comment.
Will it be confusing to pass an IServiceProvider into the constructor and not have it get used at all by FunctionInvokingChatClient.GetService(Type serviceType, object? serviceKey = null)?
I think it might be more consistent to rely on the IServiceProvider implementation from the innerClient and ensuring that the inner clients are initialized with the host's service provider whenever that's important. Another upside is that this would allow AIFunctions to inject services provided by the innerClient like ChatClientMetadata.
This might be made easier if we changed the IChatClient.GetService method to an IChatClient.Services property. We could get rid of a bunch of redundant ChatClientExtensions methods while we're at it.
I've already said my piece about how I don't like falling back to DI for the logger when null is passed in the same argument list, so I won't repeat that argument here.
There was a problem hiding this comment.
Will it be confusing to pass an IServiceProvider into the constructor and not have it get used at all by FunctionInvokingChatClient.GetService(Type serviceType, object? serviceKey = null)?
GetService has the same shape as with IServiceProvider, but it's not consulting any IServiceProvider in any of the implementations we have or have seen thus far. It's just a mechanism to ask the components in the pipeline for a particular object, e.g. if you have a pipeline of IChatClients that ends in an OnnxRuntimeGenAIClient, and for some reason you want to ask it for its Tokenizer, you can do so with GetService, and that call will be passed down through the pipeline until OnnxRuntimeGenAIClient says "yeah, I've got a Tokenizer, here". That also means it doesn't really work as a property.
There was a problem hiding this comment.
I think it might be more consistent to rely on the IServiceProvider implementation from the innerClient
Most inner clients aren't going to have one. e.g. the OpenAIChatClient doesn't have an IServiceProvider, the AzureAIInferenceChatClient doesn't have one, etc. In fact I don't think any implementations I've seen do. Some other middleware components might, but there's no guarantee FunctionInvocationChatClient will be wrapping one of those. I can't think of a better way than to store the IServiceProvider that's pass in from the builder, typically in UseFunctionInvocation().
Microsoft Reviewers: Open in CodeFlow