-
Notifications
You must be signed in to change notification settings - Fork 841
Enable AIFunctionFactory to resolve parameters from an IServiceProvider #5956
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
42 changes: 42 additions & 0 deletions
42
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/AIFunctionArguments.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Collections; | ||
| using System.Collections.Generic; | ||
| using Microsoft.Shared.Diagnostics; | ||
|
|
||
| #pragma warning disable CA1710 // Identifiers should have correct suffix | ||
|
|
||
| namespace Microsoft.Extensions.AI; | ||
|
|
||
| /// <summary>Represents arguments to be used with <see cref="AIFunction.InvokeAsync"/>.</summary> | ||
| /// <remarks> | ||
| /// <see cref="AIFunction.InvokeAsync"/> may be invoked with arbitary <see cref="IEnumerable{T}"/> | ||
| /// implementations. However, some <see cref="AIFunction"/> implementations may dynamically check | ||
| /// the type of the arguments, and if it's an <see cref="AIFunctionArguments"/>, use it to access | ||
| /// an <see cref="IServiceProvider"/> that's passed in separately from the arguments enumeration. | ||
| /// </remarks> | ||
| public class AIFunctionArguments : IEnumerable<KeyValuePair<string, object?>> | ||
| { | ||
| /// <summary>The arguments represented by this instance.</summary> | ||
| private readonly IEnumerable<KeyValuePair<string, object?>> _arguments; | ||
|
|
||
| /// <summary>Initializes a new instance of the <see cref="AIFunctionArguments"/> class.</summary> | ||
| /// <param name="arguments">The arguments represented by this instance.</param> | ||
| /// <param name="serviceProvider">Options services associated with these arguments.</param> | ||
| public AIFunctionArguments(IEnumerable<KeyValuePair<string, object?>>? arguments, IServiceProvider? serviceProvider = null) | ||
| { | ||
| _arguments = Throw.IfNull(arguments); | ||
| ServiceProvider = serviceProvider; | ||
| } | ||
|
|
||
| /// <summary>Gets the services associated with these arguments.</summary> | ||
| public IServiceProvider? ServiceProvider { get; } | ||
|
|
||
| /// <inheritdoc /> | ||
| public IEnumerator<KeyValuePair<string, object?>> GetEnumerator() => _arguments.GetEnumerator(); | ||
|
|
||
| /// <inheritdoc /> | ||
| IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)_arguments).GetEnumerator(); | ||
| } |
16 changes: 16 additions & 0 deletions
16
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FromServicesAttribute.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System; | ||
|
|
||
| namespace Microsoft.Extensions.AI; | ||
|
|
||
| /// <summary>Indicates that a parameter to an <see cref="AIFunction"/> should be sourced from an associated <see cref="IServiceProvider"/>.</summary> | ||
| [AttributeUsage(AttributeTargets.Parameter)] | ||
| public sealed class FromServicesAttribute : Attribute | ||
| { | ||
| /// <summary>Initializes a new instance of the <see cref="FromServicesAttribute"/> class.</summary> | ||
| public FromServicesAttribute() | ||
| { | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
FromServicesAttributeup intoSystem.ComponentModelor somethingIf 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that would require M.E.AI.Abstractions depending on DI, which I want to avoid.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Yes, that's what I meant.
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
IServiceProviderparameters 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.