Skip to content
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.

Sign up for GitHub

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

Enable AIFunctionFactory.Create functions to get DI services, Alternate 1 #6144

Closed
wants to merge 1 commit into from
Closed

Enable AIFunctionFactory.Create functions to get DI services, Alternate 1 #6144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Shared.Diagnostics;
Expand Down Expand Up @@ -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)
: base(innerClient)
{
_logger = logger ?? NullLogger.Instance;
_logger = logger ?? (ILogger?)services?.GetService<ILogger<FunctionInvokingChatClient>>() ?? NullLogger.Instance;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more conventional to retrieve an ILoggerFactory and use that to construct an ILogger<FunctionInvokingChatClient>. Developers won't normally register an ILogger<FunctionInvokingChatClient> explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more conventional to retrieve an ILoggerFactory and use that to construct an ILogger

Doesn't DI do that automatically?

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

ServiceCollection c = new();
c.AddLogging(c => c.AddConsole());
IServiceProvider services = c.BuildServiceProvider();

ILogger<MySpecialType> logger = services.GetRequiredService<ILogger<MySpecialType>>();
Console.WriteLine(logger);

class MySpecialType { }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem/benefit of taking an ILogger is that whoever constructs the FunctionInvokingChatClient controls the category name of logs emitted by it. That's kind of cool, but very unusual. I now once again lean towards sticking with convention and taking an ILoggerFactory even though I said it was okay to leave as-is before.

You could take an ILogger<FunctionInvokingChatClient> to essentially force the caller to user the right category name, but then there's no real benefit over just taking an ILoggerFactory. It would prevent us from ever logging in a subcategory or something like that in the future.

And while on the topic of sticking with conventions, I don't think we should fall back to the IServiceProvider to get the logger when a null logger is passed in. We talked about this before here. I said it was okay to leave it as-is then, but it still leaves a bad taste in my mouth.

I can't think of any other place where a constructor takes an IServiceProvider and another service in the same signature and uses the IServiceProvider as a fallback when the other service is missing. UserManager doesn't have this type of fallback.

I'd feel better about the fallback behavior if FunctionInvokingChatClient(IChatClient innerClient, IServiceProvider services) was a separate constructor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of taking an ILogger was that someone could use this if they already have one.

If we're not going to do that, what's the benefit of taking an ILoggerFactory at all? If you can pass in an IServiceProvider, the implementation could fish the ILoggerFactory or ILogger out of there.

Are there scenarios where you have an ILoggerFactory that doesn't come from DI?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, logging to someone else's ILogger isn't supported by any classes we ship. It's always an ILoggerFactory or ILogger<ServiceType>, because it's the type controls the logging category, not whoever constructed an instance of that type. That way you get consistent logging categories and IDs when looking at logs for a random application.

We don't have too many types that straddle the world between being a DI service and being easily constructable on their own, but MemoryCache follows this pattern and takes an ILoggerFactory rather than an ILogger.

I do think that controlling the log category on a per-instance basis might be an interesting feature. I've even thought controlling the log category might be useful for MemoryCache, since it can be used for so many different purposes in the same application, but I don't think it's so important for this API that we should break our standard conventions. We could always add an ILogger overload later if we really need this functionality.

Are there scenarios where you have an ILoggerFactory that doesn't come from DI?

You could use LoggerFactory.Create(). I've noticed the mcpdotnet tests use it.

https://learn.microsoft.com/dotnet/core/extensions/logging

If we're not going to do that, what's the benefit of taking an ILoggerFactory at all? If you can pass in an IServiceProvider, the implementation could fish the ILoggerFactory or ILogger out of there.

We could, but we'd basically be following the "service locator pattern" at that point. It wouldn't be nearly as discoverable that FunctionInvokingChatClient will do its own logging if you provide it a logger.

I like the clean break where the IServiceProvider is for providing arbitrary services to the AIFunction, and any service types we know get used internally by the FunctionInvokingChatClient are provided as separate constructor parameters in the longest overload.

I'm fine with also introducing a shorter overload that takes just the inner IChatClient and an IServiceProvider, and that having that resolve the logger from DI. We could go even further and use the IServiceProvider from the inner IChatClient as I suggest here. But in either case, I still think there should be a longer overload that takes an ILoggerFactory. At least that would make it clear to callers that FunctionInvokingChatClient can emit its own logs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the clean break where the IServiceProvider is for providing arbitrary services to the AIFunction, and any service types we know get used internally by the FunctionInvokingChatClient are provided as separate constructor parameters in the longest overload.

OK, so you're viewing the IServiceProvider not as something thee FICC uses itself but that is just an opaque object that's passed through. That's reasonable, though then having other overloads that do query the service provider muddies the waters, no? There are lots of APIs and delegates that pass around an IServiceProvider for resolution purposes... how is a consumer supposed to know when it'll be used in that capacity or not? My assumption is giving something a service provider is giving it access to everything, and it may access everything. Whether it does so explicitly via GetService or instantiaies objects that take a logger and have that populated using ActivatorUtilities, I don't see the difference. Both do logging. Both resolve a logger from the service provider. One just explicitly calls the method and the other does so by calling a method that calls it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so you're viewing the IServiceProvider not as something thee FICC uses itself but that is just an opaque object that's passed through. That's reasonable, though then having other overloads that do query the service provider muddies the waters, no?

I admit allowing the overload to query the service provider muddies the waters a bit, but that's still where I draw the line. It leaves the door open for us to add new dependencies like IMeterFactory to FunctionInvokingChatClient post 1.0 and use it without being forced to update old code to call the new constructor.

However, in that case, I think we should still add a new constructor that takes IMeterFactory and doesn't fall back to the service provider. If a null meter factory is passed in, there should be no metrics same as with logging. And unlike with logging, I don't think there's a NullMeterFactory.Instance to make it easy to disable metrics otherwise.

I think that if you're calling an overload that takes an optional dependency in the parameter list and you omit it, it should not be used. The fact that you happened to need the IServiceProvider for other reasons doesn't make it a good idea to use the service locator pattern for every other dependency.

I realize that it's a matter of taste, and there's no objectively right answer here though. If I had to rank my preference from most preferred to least, I'd go with:

  1. Only use the IServiceProvider as a fallback for dependencies that couldn't be provided directly with the given overload.
  2. Don't use the IServiceProvider as a fallback for any dependencies.
  3. Use the IServiceProvider anywhere an optional dependency hasn't been provided.

But I think any of these are fine as long as we're consistent.

_activitySource = innerClient.GetService<ActivitySource>();
Services = services;
}

/// <summary>
Expand All @@ -82,6 +85,9 @@ public static FunctionInvocationContext? CurrentContext
protected set => _currentContext.Value = value;
}

/// <summary>Gets the <see cref="IServiceProvider"/> used for resolving services required by the <see cref="AIFunction"/> instances being invoked.</summary>
public IServiceProvider? Services { get; }

/// <summary>
/// Gets or sets a value indicating whether to handle exceptions that occur during function calls.
/// </summary>
Expand Down Expand Up @@ -721,8 +727,14 @@ FunctionResultContent CreateFunctionResultContent(FunctionInvocationResult resul
object? result = null;
try
{
IEnumerable<KeyValuePair<string, object?>>? arguments = context.CallContent.Arguments;
if (Services is not null)
{
arguments = new AIFunctionArguments(arguments) { Services = Services };
}

CurrentContext = context;
result = await context.Function.InvokeAsync(context.CallContent.Arguments, cancellationToken).ConfigureAwait(false);
result = await context.Function.InvokeAsync(arguments, cancellationToken).ConfigureAwait(false);
}
catch (Exception e)
{
Expand Down
2 changes: 1 addition & 1 deletion ...ies/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClientBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public static ChatClientBuilder UseFunctionInvocation(
{
loggerFactory ??= services.GetService<ILoggerFactory>();

var chatClient = new FunctionInvokingChatClient(innerClient, loggerFactory?.CreateLogger(typeof(FunctionInvokingChatClient)));
var chatClient = new FunctionInvokingChatClient(innerClient, loggerFactory?.CreateLogger(typeof(FunctionInvokingChatClient)), services);
configure?.Invoke(chatClient);
return chatClient;
});
Expand Down
82 changes: 82 additions & 0 deletions src/Libraries/Microsoft.Extensions.AI/Functions/AIFunctionArguments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// 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 System.Linq;
using Microsoft.Shared.Collections;

#pragma warning disable SA1111 // Closing parenthesis should be on line of last parameter
#pragma warning disable SA1112 // Closing parenthesis should be on line of opening parenthesis
#pragma warning disable SA1114 // Parameter list should follow declaration
#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 use the concrete type to perform more specific operations. By
/// checking for <see cref="AIFunctionArguments"/>, and implementation may optionally access
/// additional context provided, such as any <see cref="IServiceProvider"/> that may be associated
/// with the operation.
/// </remarks>
public class AIFunctionArguments : IReadOnlyDictionary<string, object?>
{
private readonly IReadOnlyDictionary<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>
public AIFunctionArguments(IEnumerable<KeyValuePair<string, object?>>? arguments)
{
if (arguments is IReadOnlyDictionary<string, object?> irod)
{
_arguments = irod;
}
else if (arguments is null
#if NET
|| (Enumerable.TryGetNonEnumeratedCount(arguments, out int count) && count == 0)
#endif
)
{
_arguments = EmptyReadOnlyDictionary<string, object?>.Instance;
}
else
{
_arguments = arguments.ToDictionary(
#if !NET
x => x.Key, x => x.Value
#endif
);
}
}

/// <summary>Gets any services associated with these arguments.</summary>
public IServiceProvider? Services { get; init; }

/// <inheritdoc />
public object? this[string key] => _arguments[key];

/// <inheritdoc />
public IEnumerable<string> Keys => _arguments.Keys;

/// <inheritdoc />
public IEnumerable<object?> Values => _arguments.Values;

/// <inheritdoc />
public int Count => _arguments.Count;

/// <inheritdoc />
public bool ContainsKey(string key) => _arguments.ContainsKey(key);

/// <inheritdoc />
public IEnumerator<KeyValuePair<string, object?>> GetEnumerator() => _arguments.GetEnumerator();

/// <inheritdoc />
public bool TryGetValue(string key, out object? value) => _arguments.TryGetValue(key, out value);

/// <inheritdoc />
IEnumerator IEnumerable.GetEnumerator() => ((IEnumerable)_arguments).GetEnumerator();
}
56 changes: 49 additions & 7 deletions src/Libraries/Microsoft.Extensions.AI/Functions/AIFunctionFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
using Microsoft.Shared.Collections;
using Microsoft.Shared.Diagnostics;

#pragma warning disable CA1031 // Do not catch general exception types
#pragma warning disable S3011 // Reflection should not be used to increase accessibility of classes, methods, or fields

namespace Microsoft.Extensions.AI;

/// <summary>Provides factory methods for creating commonly used implementations of <see cref="AIFunction"/>.</summary>
Expand Down Expand Up @@ -196,8 +199,8 @@ private ReflectionAIFunction(ReflectionAIFunctionDescriptor functionDescriptor,
object?[] args = paramMarshallers.Length != 0 ? new object?[paramMarshallers.Length] : [];

IReadOnlyDictionary<string, object?> argDict =
arguments is null || args.Length == 0 ? EmptyReadOnlyDictionary<string, object?>.Instance :
arguments as IReadOnlyDictionary<string, object?> ??
arguments is null ? EmptyReadOnlyDictionary<string, object?>.Instance :
arguments as IReadOnlyDictionary<string, object?> ?? // if arguments is an AIFunctionArguments, which is an IROD, use it as-is
arguments.
#if NET8_0_OR_GREATER
ToDictionary();
Expand Down Expand Up @@ -248,6 +251,28 @@ 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.
TransformSchemaNode = key.SchemaOptions.TransformSchemaNode,
IncludeParameter = parameterInfo =>
{
// Explicitly exclude IServiceProvider. It'll be satisifed via AIFunctionArguments.
if (parameterInfo.ParameterType == typeof(IServiceProvider))
{
return false;
}

// For all other parameters, delegate to whatever behavior is specified in the options.
// If none is specified, include the parameter.
return key.SchemaOptions.IncludeParameter?.Invoke(parameterInfo) ?? true;
},
IncludeTypeInEnumSchemas = key.SchemaOptions.IncludeTypeInEnumSchemas,
DisallowAdditionalProperties = key.SchemaOptions.DisallowAdditionalProperties,
IncludeSchemaKeyword = key.SchemaOptions.IncludeSchemaKeyword,
RequireAllProperties = key.SchemaOptions.RequireAllProperties,
};

// Get marshaling delegates for parameters.
ParameterInfo[] parameters = key.Method.GetParameters();
ParameterMarshallers = new Func<IReadOnlyDictionary<string, object?>, CancellationToken, object?>[parameters.Length];
Expand All @@ -268,7 +293,7 @@ private ReflectionAIFunctionDescriptor(DescriptorKey key, JsonSerializerOptions
Name,
Description,
serializerOptions,
key.SchemaOptions);
schemaOptions);
}

public string Name { get; }
Expand Down Expand Up @@ -341,6 +366,27 @@ static bool IsAsyncMethod(MethodInfo method)
cancellationToken;
}

// For IServiceProvider parameters, we always bind to the services passed directly to InvokeAsync
// via AIFunctionArguments.
if (parameterType == typeof(IServiceProvider))
{
return (arguments, _) =>
{
if ((arguments as AIFunctionArguments)?.Services is IServiceProvider services)
{
return services;
}

if (!parameter.HasDefaultValue)
{
Throw.ArgumentException(nameof(arguments), $"An {nameof(IServiceProvider)} was not provided for the {parameter.Name} parameter.");
}

// The IServiceProvider parameter was optional. Return the default value.
return null;
};
}

// For all other parameters, create a marshaller that tries to extract the value from the arguments dictionary.
return (arguments, _) =>
{
Expand All @@ -359,7 +405,6 @@ static bool IsAsyncMethod(MethodInfo method)

object? MarshallViaJsonRoundtrip(object value)
{
#pragma warning disable CA1031 // Do not catch general exception types
try
{
string json = JsonSerializer.Serialize(value, serializerOptions.GetTypeInfo(value.GetType()));
Expand All @@ -370,7 +415,6 @@ static bool IsAsyncMethod(MethodInfo method)
// Eat any exceptions and fall back to the original value to force a cast exception later on.
return value;
}
#pragma warning restore CA1031
}
}

Expand Down Expand Up @@ -482,9 +526,7 @@ private static MethodInfo GetMethodFromGenericMethodDefinition(Type specializedT
#if NET
return (MethodInfo)specializedType.GetMemberWithSameMetadataDefinitionAs(genericMethodDefinition);
#else
#pragma warning disable S3011 // Reflection should not be used to increase accessibility of classes, methods, or fields
const BindingFlags All = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.Instance;
#pragma warning restore S3011 // Reflection should not be used to increase accessibility of classes, methods, or fields
return specializedType.GetMethods(All).First(m => m.MetadataToken == genericMethodDefinition.MetadataToken);
#endif
}
Expand Down
26 changes: 26 additions & 0 deletions ...Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,32 @@ public async Task PropagatesResponseChatThreadIdToOptions()
Assert.Equal("done!", (await service.GetStreamingResponseAsync("hey", options).ToChatResponseAsync()).ToString());
}

[Fact]
public async Task FunctionInvocations_PassesServices()
{
List<ChatMessage> plan =
[
new ChatMessage(ChatRole.User, "hello"),
new ChatMessage(ChatRole.Assistant, [new FunctionCallContent("callId1", "Func1", new Dictionary<string, object?> { ["arg1"] = "value1" })]),
new ChatMessage(ChatRole.Tool, [new FunctionResultContent("callId1", result: "Result 1")]),
new ChatMessage(ChatRole.Assistant, "world"),
];

ServiceCollection c = new();
IServiceProvider expected = c.BuildServiceProvider();

var options = new ChatOptions
{
Tools = [AIFunctionFactory.Create((IServiceProvider actual) =>
{
Assert.Same(expected, actual);
return "Result 1";
}, "Func1")]
};

await InvokeAndAssertAsync(options, plan, services: expected);
}

private static async Task<List<ChatMessage>> InvokeAndAssertAsync(
ChatOptions options,
List<ChatMessage> plan,
Expand Down
81 changes: 81 additions & 0 deletions test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionArgumentsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// 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.Generic;
using System.Linq;
using Microsoft.Extensions.DependencyInjection;
using Xunit;

namespace Microsoft.Extensions.AI;

public class AIFunctionArgumentsTests
{
[Fact]
public void NullArg_RoundtripsAsEmpty()
{
var args = new AIFunctionArguments(null);
Assert.Null(args.Services);
}

[Fact]
public void EmptyArg_RoundtripsAsEmpty()
{
var args = new AIFunctionArguments([]);
Assert.Null(args.Services);
}

[Fact]
public void Services_Roundtrips()
{
ServiceCollection sc = new();
IServiceProvider sp = sc.BuildServiceProvider();

var args = new AIFunctionArguments([])
{
Services = sp
};

Assert.Same(sp, args.Services);
}

[Fact]
public void IReadOnlyDictionary_ImplementsInterface()
{
ServiceCollection sc = new();
IServiceProvider sp = sc.BuildServiceProvider();

var args = new AIFunctionArguments(
[
new KeyValuePair<string, object?>("key1", "value1"),
new KeyValuePair<string, object?>("key2", "value2"),
])
{
Services = sp
};

Assert.Same(sp, args.Services);

Assert.Equal(2, args.Count);

Assert.True(args.ContainsKey("key1"));
Assert.True(args.ContainsKey("key2"));
Assert.False(args.ContainsKey("KEY1"));

Assert.Equal("value1", args["key1"]);
Assert.Equal("value2", args["key2"]);

Assert.Equal(new[] { "key1", "key2" }, args.Keys);
Assert.Equal(new[] { "value1", "value2" }, args.Values);

Assert.True(args.TryGetValue("key1", out var value));
Assert.Equal("value1", value);
Assert.False(args.TryGetValue("key3", out value));
Assert.Null(value);

Assert.Equal([
new KeyValuePair<string, object?>("key1", "value1"),
new KeyValuePair<string, object?>("key2", "value2"),
], args.ToArray());
}
}
Loading
Loading