Enable AIFunctionFactory to resolve parameters from an IServiceProvider

[stephentoub]

In a discussion last week, @eiriktsarpalis suggested that sooner or later we'll want to support AIFunctionFactory being able to populate parameters from DI. The second commit here is how I think we could do that. (Ignore the first commit, it's just #5954.) What do we think? Is this something we'd want to proceed with, or if not, are we comfortable with this as a future solution, in which case this demonstrates it's possible without breakage?

Effectively, it enables someone to write:

AIFunctionFactory.Create(([FromServiceProvider] WhateverType wt = null) =>
{
    ...
});

ala what you can do with MVC and SK and others. When constructing a FunctionInvokingChatClient, it'll hold onto whatever IServiceProvider was used to construct it, and then feed that into AIFunction.InvokeAsync via an AIFunctionArguments type that AIFunctionFactory type checks for.

cc: @eiriktsarpalis, @SteveSandersonMS, @halter73

Microsoft Reviewers: Open in CodeFlow

[dotnet-comment-bot]

‼️ Found issues ‼️

Project	Coverage Type	Expected	Actual
Microsoft.Extensions.AI	Line	89	88.27 🔻

🎉 Good job! The coverage increased 🎉
Update MinCodeCoverage in the project files.

Project	Expected	Actual
Microsoft.Extensions.AI.Abstractions	82	83
Microsoft.Gen.MetadataExtractor	57	70

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=960014&view=codecoverage-tab

[eiriktsarpalis]

The need to use a magic dictionary type makes me sad, but at the same time I can't think of a better solution that addresses our current dependency constraints.

Making it optional does seem like a pit of failure for library authors whose test coverage doesn't include DI scenaria. Should we consider requiring that input dictionaries are always of this type regardless?

[eiriktsarpalis]

One possibility I considered is using a dependency inversion scheme where the InvokeAsync method accepts a Func<Type, object> either directly or through an "AIFunctionInvocationContext" type that contains it as a property.

[stephentoub]

consider requiring that input dictionaries are always of this type regardless?

I'd rather not, both because it limits what input types are possible but also because it makes the IServiceProvider a prominent thing that's part of the base abstraction, when it's not really.

Making it optional does seem like a pit of failure for library authors whose test coverage doesn't include DI scenaria. Should we consider requiring that input dictionaries are always of this type regardless?

I wouldn't expect this to be an issue for most library authors. What are these libraries doing? The only component that would need to instantiate this type to flow additional data through would be something that's invoking arbitrary AIFunction instances, ala FunctionInvokingChatClient.

I agree it's not ideal. I just don't have a better answer that keeps AIFunction.InvokeAsync simple and clean while still allowing for other ambient context to be passed through. If we want to change InvokeAsync now, before it's stable, options would include changing the type of the argument, adding an additional dictionary argument for ambient context in addition to named arguments, adding some kind of callback argument that the AIFunction could use to retrieve the value for an argument, e.g. Func<string, object?>? argumentFactory (where we establish a search order between the dictionary and the callback), etc. I'm not sure any of those is better, though I'm open to arguments (no pun intended).

[stephentoub]

@SteveSandersonMS, could you review this as well?

[SteveSandersonMS]

I guess it's a challenge with .NET in general, but we do already have a type with this name and of course the same meaning. This could lead to some pretty awkward debugging for people who believe they are using [FromServices] correctly but it's not working (either breaking MVC code or breaking MEAI code).

Possible solutions:

• Use a different name here
• Try to push MVC's FromServicesAttribute up into System.ComponentModel or something
• Use a different pattern here

If we wanted a different pattern here, we could embrace the whole "AIFunctionCallContext" parameter notion. It could deal with services and cancellation tokens in one, and would make it easy for us to add any other per-call context in the future without having to special-case any other parameter types. I know we considered that before, but maybe adding further per-call context starts to tip the balance another way.

What do you think? Is this enough of a problem to warrant a different approach?

[SteveSandersonMS]

I've now read the other comments on this PR and see there's already been talk of this. Looks like one blocker is that we would need AIFF etc to move from M.E.AI.Abstractions to M.E.AI. But is there a plan to do that regardless?

If we could avoid creating a duplicate-named attribute that certainly sounds preferable to me. I'm surprised this didn't seem to bother @halter73 so maybe I'm overblowing the issue. What do you think, @halter73?

[stephentoub]

But is there a plan to do that regardless?

No, that would require M.E.AI.Abstractions depending on DI, which I want to avoid.

[stephentoub]

name

I'd started with a different name and was urged by Stephen and others to use the same one. Happy to go back to a different one if folks can agree.

Try to push MVC's FromServicesAttribute up into System.ComponentModel or something

It'd be really weird to depend on something in the Microsoft.AspNetCore.Mvc name space, even if we could figure out how to move it into a different assembly on an appropriate time line.

Just to confirm, you're factoring into your concerns that it's in an MVC namespace?

we could embrace the whole "AIFunctionCallContext" parameter

Got a lot of push back on that during API review, especially around CT... we won't be putting the CT into such a thing even if we add it back, it's too hard to discover and doesn't match ..NET conventions... we had multiple bugs filled spot not supporting CT even when we did via that mechanism But I'm also not clear how that helps, unless you're suggesting it would just carry an IServiceProvider and the ap code would query directly? We could also do that just by special-casing IServiceProvider if we wanted to.

[SteveSandersonMS]

Just to confirm, you're factoring into your concerns that it's in an MVC namespace?

Yes, I do know that. It means we're relying on the developer understanding that, in a typical ASP.NET Core application, there are two different [FromServices] attributes and that they have to get the right one in each case, with the compiler/IDE not helping identify the correct one (presumably, the IDE will offer to auto-add both namespaces and leaves the developer to select which one). Picking the wrong one means it just won't work at runtime.

But I'm also not clear how that helps, unless you're suggesting it would just carry an IServiceProvider and the ap code would query directly?

Yes, that's what I meant.

We could also do that just by special-casing IServiceProvider if we wanted to.

That's a perfectly fine solution as long as we consider this a relatively niche case.

If we thought that, say, 50%+ of AIFunctions would need to obtain DI services directly (and couldn't acquire them from context, e.g., by being instance methods on some type that already has access to the desired services, which I think is also common), then it would make sense to try to make it more first-class.

One option is, for now, to support IServiceProvider parameters and thus bypass the other issues. It would still leave open the option to add other ways to resolve params-as-services in the future.

Altogether this isn't something that worries me deeply. If you and others are keen to have [FromServices] and don't think the duplicate type name will be an issue, that's OK.

[stephentoub]

The purpose of this PR was to demonstrate that we can add such functionality without taking any breaking changes, and we can (there'd be a tiny possible behavioral breaking change, in that you could have a parameter attributed as [FromKeyedService] and this functionality would change how that's handled, but if you're doing that, it's not behaving today how you almost certainly expect it to). We can either expose it as is done in this PR, or @SteveSandersonMS also suggested it could be simplified to just special-casing IServiceProvider in AIFunctionFactory (similarly, there'd be a small behavioral change, in that we'd start preferring to pull that from DI and excluding it from the schema, but AI can't do anything reasonable to populate an IServiceProvider instance, anyway).

Given that, I'll close this for now until there's a demonstrated need.