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

Expose an AIJsonUtilities class in M.E.AI and lower M.E.AI.Abstractions to STJv8 #5513

Merged
merged 11 commits into from
Oct 17, 2024
Merged

Expose an AIJsonUtilities class in M.E.AI and lower M.E.AI.Abstractions to STJv8 #5513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
68a9d45
Expose FunctionCallUtilities class.
eiriktsarpalis Oct 11, 2024
f6e6b98
Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/Fu…
eiriktsarpalis Oct 11, 2024
514f846
Remove function call formatting helpers.
eiriktsarpalis Oct 14, 2024
aa76abf
Extract JSON schema inference settings into a separate options class.
eiriktsarpalis Oct 14, 2024
1b1752b
Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/Js…
eiriktsarpalis Oct 14, 2024
fd85775
Address feedback
eiriktsarpalis Oct 14, 2024
5ea90ed
Return FunctionCallContent in parser helpers.
eiriktsarpalis Oct 14, 2024
b02f38b
Address feedback
eiriktsarpalis Oct 15, 2024
40f9ec8
Update src/Libraries/Microsoft.Extensions.AI/Functions/AIFunctionFact…
eiriktsarpalis Oct 15, 2024
6e51c3d
Refactor to AIJsonUtilities class.
eiriktsarpalis Oct 15, 2024
b9dbf77
Move all utilities to M.E.AI and downgrade STJ version to 8 for M.E.A…
eiriktsarpalis Oct 16, 2024
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
1 change: 1 addition & 0 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
<SystemSecurityCryptographyXmlVersion>9.0.0-rc.2.24473.5</SystemSecurityCryptographyXmlVersion>
<SystemTextEncodingsWebVersion>9.0.0-rc.2.24473.5</SystemTextEncodingsWebVersion>
<SystemTextJsonVersion>9.0.0-rc.2.24473.5</SystemTextJsonVersion>
<SystemTextJson8Version>8.0.5</SystemTextJson8Version>
Copy link
Member

Choose a reason for hiding this comment

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

If STJ revs, e.g. with a security patch, we'll need to remember to update this, or will it be automatically updated by darc? Do we need a corresponding entry in Version.Details.xml?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not clear to me that it would be updated automatically. Maybe @ViktorHofer knows.

Copy link
Member

@ViktorHofer ViktorHofer Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Version.Details.xml is the input source for Maestro dependency flow which handles repo -> repo internal flow. If you want to get a version updated based on a new package being available on nuget.org, you would need to use Dependabot.

If you just want to make sure that you don't use a vulnerable version of STJ, then you could enable NuGet Audit in this repository which would flag the dependency at restore and build time.

Copy link

Choose a reason for hiding this comment

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

Forgive my ignorance but if this extension doesn't have a dependency on .Net 9, why the package is a 9.x-preview and not a 8.x-preview (targeting NET 8 dependencies only) ?

Copy link
Member

Choose a reason for hiding this comment

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

Which package are you asking about?

<SystemNumericsTensorsVersion>9.0.0-rc.2.24473.5</SystemNumericsTensorsVersion>
<!-- Dependencies from https://github.com/aspnet/AspNetCore -->
<MicrosoftAspNetCoreAppRefVersion>9.0.0-rc.2.24474.3</MicrosoftAspNetCoreAppRefVersion>
Expand Down
40 changes: 40 additions & 0 deletions src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,46 @@ public FunctionCallContent(string callId, string name, IDictionary<string, objec
[JsonIgnore]
public Exception? Exception { get; set; }

/// <summary>
/// Creates a new instance of <see cref="FunctionCallContent"/> parsing arguments using a specified encoding and parser.
/// </summary>
/// <typeparam name="TEncoding">The encoding format from which to parse function call arguments.</typeparam>
/// <param name="encodedArguments">The input arguments encoded in <typeparamref name="TEncoding"/>.</param>
/// <param name="callId">The function call ID.</param>
/// <param name="name">The function name.</param>
/// <param name="argumentParser">The parsing implementation converting the encoding to a dictionary of arguments.</param>
/// <param name="exceptionFilter">Filters potential parsing exceptions that should be caught and included in the result.</param>
/// <returns>A new instance of <see cref="FunctionCallContent"/> containing the parse result.</returns>
public static FunctionCallContent CreateFromParsedArguments<TEncoding>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This change removes all "helper" code from the Abstractions repo with the exception of this method required by client impls. I used a callback approach to invert dependencies if that's acceptable.

Copy link
Member

Choose a reason for hiding this comment

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

Seems ok, though at this point I'm not sure how much benefit this method really provides.

TEncoding encodedArguments,
string callId,
string name,
Func<TEncoding, IDictionary<string, object?>?> argumentParser,
Func<Exception, bool>? exceptionFilter = null)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the filter? Could we just catch and wrap any exception from the parser and store it as the inner?

Copy link
Member Author

Choose a reason for hiding this comment

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

For JSON stuff we only wrap JsonException and let NotSupportedException bubble up, but maybe it isn't necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem worth promoting an extra delegate into the API for that. I'd suggest either we just hardcode ones we want to allow through and wrap everything else, or just wrap everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't hardcode exception types given that it's a generic helper, so I'll just remove the filter.

Copy link
Member

Choose a reason for hiding this comment

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

We can't

We could; we do elsewhere around calling out to what's possibly user code. We can certainly choose not to.

so I'll just remove the filter

Ok

{
_ = Throw.IfNull(callId);
_ = Throw.IfNull(name);
_ = Throw.IfNull(encodedArguments);
_ = Throw.IfNull(argumentParser);

IDictionary<string, object?>? arguments = null;
Exception? parsingException = null;

try
{
arguments = argumentParser(encodedArguments);
}
catch (Exception ex) when (exceptionFilter is null || exceptionFilter(ex))
{
parsingException = new InvalidOperationException("Error parsing function call arguments.", ex);
}

return new FunctionCallContent(callId, name, arguments)
{
Exception = parsingException
};
}

/// <summary>Gets a string representing this instance to display in the debugger.</summary>
private string DebuggerDisplay
{
Expand Down
395 changes: 0 additions & 395 deletions src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallHelpers.cs
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions ...ibraries/Microsoft.Extensions.AI.Abstractions/Microsoft.Extensions.AI.Abstractions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
<InjectStringSyntaxAttributeOnLegacy>true</InjectStringSyntaxAttributeOnLegacy>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="System.Text.Json" />
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<PackageReference Include="System.Text.Json" VersionOverride="$(SystemTextJson8Version)" />
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' == 'net462'">
Expand Down
44 changes: 26 additions & 18 deletions src/Libraries/Microsoft.Extensions.AI.AzureAIInference/AzureAIInferenceChatClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@

#pragma warning disable S1135 // Track uses of "TODO" tags
#pragma warning disable S3011 // Reflection should not be used to increase accessibility of classes, methods, or fields
#pragma warning disable SA1204 // Static elements should appear before instance elements

namespace Microsoft.Extensions.AI;

/// <summary>An <see cref="IChatClient"/> for an Azure AI Inference <see cref="ChatCompletionsClient"/>.</summary>
public sealed partial class AzureAIInferenceChatClient : IChatClient
{
private static readonly JsonElement _defaultParameterSchema = JsonDocument.Parse("{}").RootElement;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>The underlying <see cref="ChatCompletionsClient" />.</summary>
private readonly ChatCompletionsClient _chatCompletionsClient;

Expand Down Expand Up @@ -93,14 +96,11 @@ public async Task<ChatCompletion> CompleteAsync(
{
if (toolCall is ChatCompletionsFunctionToolCall ftc && !string.IsNullOrWhiteSpace(ftc.Name))
{
Dictionary<string, object?>? arguments = FunctionCallHelpers.ParseFunctionCallArguments(ftc.Arguments, out Exception? parsingException);
FunctionCallContent callContent = ParseCallContentFromJsonString(ftc.Arguments, toolCall.Id, ftc.Name);
callContent.ModelId = response.Model;
callContent.RawRepresentation = toolCall;

returnMessage.Contents.Add(new FunctionCallContent(toolCall.Id, ftc.Name, arguments)
{
ModelId = response.Model,
Exception = parsingException,
RawRepresentation = toolCall
});
returnMessage.Contents.Add(callContent);
}
}
}
Expand Down Expand Up @@ -226,15 +226,14 @@ public async IAsyncEnumerable<StreamingChatCompletionUpdate> CompleteStreamingAs
FunctionCallInfo fci = entry.Value;
if (!string.IsNullOrWhiteSpace(fci.Name))
{
var arguments = FunctionCallHelpers.ParseFunctionCallArguments(
FunctionCallContent callContent = ParseCallContentFromJsonString(
fci.Arguments?.ToString() ?? string.Empty,
out Exception? parsingException);
fci.CallId!,
fci.Name!);

completionUpdate.Contents.Add(new FunctionCallContent(fci.CallId!, fci.Name!, arguments)
{
ModelId = modelId,
Exception = parsingException
});
callContent.ModelId = modelId;

completionUpdate.Contents.Add(callContent);
}
}

Expand Down Expand Up @@ -358,7 +357,7 @@ private ChatCompletionsOptions ToAzureAIOptions(IList<ChatMessage> chatContents,
}

/// <summary>Converts an Extensions function to an AzureAI chat tool.</summary>
private ChatCompletionsFunctionToolDefinition ToAzureAIChatTool(AIFunction aiFunction)
private static ChatCompletionsFunctionToolDefinition ToAzureAIChatTool(AIFunction aiFunction)
{
BinaryData resultParameters = AzureAIChatToolJson.ZeroFunctionParametersSchema;

Expand All @@ -371,7 +370,7 @@ private ChatCompletionsFunctionToolDefinition ToAzureAIChatTool(AIFunction aiFun
{
tool.Properties.Add(
parameter.Name,
FunctionCallHelpers.InferParameterJsonSchema(parameter, aiFunction.Metadata, ToolCallJsonSerializerOptions));
parameter.Schema is JsonElement schema ? schema : _defaultParameterSchema);

if (parameter.IsRequired)
{
Expand Down Expand Up @@ -428,9 +427,10 @@ private IEnumerable<ChatRequestMessage> ToAzureAIInferenceChatMessages(IEnumerab
string? result = resultContent.Result as string;
if (result is null && resultContent.Result is not null)
{
JsonSerializerOptions options = ToolCallJsonSerializerOptions ?? JsonContext.Default.Options;
Copy link
Member

Choose a reason for hiding this comment

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

Could be a separate change if we decide to do it, but should ToolCallJsonSerializerOptions be made non-nullable like it is on the middleware clients, just defaulting it to the default options?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible, but it would require moving the defaults to Abstractions so that clients can reference it.

Copy link
Member

@stephentoub stephentoub Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not understanding why this would require any movement. I'm simply suggesting changing:

public JsonSerializerOptions? ToolCallJsonSerializerOptions { get; set; }

to

private JsonSerializerOptions _toolCallOptions = JsonContext.Default.Options;
...
public JsonSerializerOptions ToolCallJsonSerializerOptions
{
    get => _toolCallOptions;
    set => _toolCallOptions = Throw.IfNull(value);
}

and then here instead of:

JsonSerializerOptions options = ToolCallJsonSerializerOptions ?? JsonContext.Default.Options;

it'd just be:

JsonSerializerOptions options = ToolCallJsonSerializerOptions;

Copy link
Member

Choose a reason for hiding this comment

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

Unless we're trying to avoid exposing this options instance?

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'm just pointing out that unless we expose a default options instance on Abstractions users will have to define their own contexts to fill in the default _toolCallOptions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just pointing out that unless we expose a default options instance on Abstractions users will have to define their own contexts to fill in the default _toolCallOptions.

I don't understand why. The current code doesn't require that, just substituting JsonContext.Default.Options when the user hasn't supplied their own. I'm simply suggesting moving around where we default back to that.

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 see what you mean. It is appropriate to default to JsonContext.Default.Options in the particular locations where it is being done because they're serializing specific types (JsonElement for FCR and IDictionary<string, object> for FCC) which are known to be defined in the local JsonContext. I don't think it would be safe to assume the same thing in other locations where ToolCallJsonSerializerOptions is being used (e.g. when serializing AdditionalProperties), so if we wanted to default to something in that case we should be using the global options instead.

try
{
result = FunctionCallHelpers.FormatFunctionResultAsJson(resultContent.Result, ToolCallJsonSerializerOptions);
result = JsonSerializer.Serialize(resultContent.Result, options.GetTypeInfo(typeof(object)));
}
catch (NotSupportedException)
{
Expand Down Expand Up @@ -461,7 +461,8 @@ private IEnumerable<ChatRequestMessage> ToAzureAIInferenceChatMessages(IEnumerab
{
if (content is FunctionCallContent callRequest && callRequest.CallId is not null && toolCalls?.ContainsKey(callRequest.CallId) is not true)
{
string jsonArguments = FunctionCallHelpers.FormatFunctionParametersAsJson(callRequest.Arguments, ToolCallJsonSerializerOptions);
JsonSerializerOptions serializerOptions = ToolCallJsonSerializerOptions ?? JsonContext.Default.Options;
string jsonArguments = JsonSerializer.Serialize(callRequest.Arguments, serializerOptions.GetTypeInfo(typeof(IDictionary<string, object>)));
(toolCalls ??= []).Add(
callRequest.CallId,
new ChatCompletionsFunctionToolCall(
Expand Down Expand Up @@ -489,7 +490,14 @@ private IEnumerable<ChatRequestMessage> ToAzureAIInferenceChatMessages(IEnumerab
}
}

private static FunctionCallContent ParseCallContentFromJsonString(string json, string callId, string name) =>
FunctionCallContent.CreateFromParsedArguments(json, callId, name,
argumentParser: static json => JsonSerializer.Deserialize(json, JsonContext.Default.IDictionaryStringObject),
exceptionFilter: static ex => ex is JsonException);

/// <summary>Source-generated JSON type information.</summary>
[JsonSerializable(typeof(AzureAIChatToolJson))]
[JsonSerializable(typeof(IDictionary<string, object?>))]
[JsonSerializable(typeof(JsonElement))]
private sealed partial class JsonContext : JsonSerializerContext;
}
6 changes: 1 addition & 5 deletions .../Microsoft.Extensions.AI.AzureAIInference/Microsoft.Extensions.AI.AzureAIInference.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
</PropertyGroup>

<ItemGroup>
<Compile Include="../Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallHelpers.cs" />
</ItemGroup>


<ItemGroup>
<PackageReference Include="Azure.AI.Inference" />
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" />
Expand Down
4 changes: 4 additions & 0 deletions src/Libraries/Microsoft.Extensions.AI.Ollama/JsonContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace Microsoft.Extensions.AI;
Expand All @@ -21,4 +23,6 @@ namespace Microsoft.Extensions.AI;
[JsonSerializable(typeof(OllamaToolCall))]
[JsonSerializable(typeof(OllamaEmbeddingRequest))]
[JsonSerializable(typeof(OllamaEmbeddingResponse))]
[JsonSerializable(typeof(IDictionary<string, object?>))]
[JsonSerializable(typeof(JsonElement))]
internal sealed partial class JsonContext : JsonSerializerContext;
4 changes: 0 additions & 4 deletions src/Libraries/Microsoft.Extensions.AI.Ollama/Microsoft.Extensions.AI.Ollama.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
</PropertyGroup>

<ItemGroup>
<Compile Include="../Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallHelpers.cs" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="System.Net.Http.Json" />
<PackageReference Include="System.Text.Json" />
Expand Down
17 changes: 13 additions & 4 deletions src/Libraries/Microsoft.Extensions.AI.Ollama/OllamaChatClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
using Microsoft.Shared.Diagnostics;

#pragma warning disable EA0011 // Consider removing unnecessary conditional access operator (?)
#pragma warning disable SA1204 // Static elements should appear before instance elements

namespace Microsoft.Extensions.AI;

/// <summary>An <see cref="IChatClient"/> for Ollama.</summary>
public sealed class OllamaChatClient : IChatClient
{
private static readonly JsonElement _defaultParameterSchema = JsonDocument.Parse("{}").RootElement;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>The api/chat endpoint URI.</summary>
private readonly Uri _apiChatEndpoint;

Expand Down Expand Up @@ -356,20 +359,25 @@ private IEnumerable<OllamaChatRequestMessage> ToOllamaChatRequestMessages(ChatMe
break;

case FunctionCallContent fcc:
{
JsonSerializerOptions serializerOptions = ToolCallJsonSerializerOptions ?? JsonContext.Default.Options;
yield return new OllamaChatRequestMessage
{
Role = "assistant",
Content = JsonSerializer.Serialize(new OllamaFunctionCallContent
{
CallId = fcc.CallId,
Name = fcc.Name,
Arguments = FunctionCallHelpers.FormatFunctionParametersAsJsonElement(fcc.Arguments, ToolCallJsonSerializerOptions),
Arguments = JsonSerializer.SerializeToElement(fcc.Arguments, serializerOptions.GetTypeInfo(typeof(IDictionary<string, object?>))),
}, JsonContext.Default.OllamaFunctionCallContent)
};
break;
}

case FunctionResultContent frc:
JsonElement jsonResult = FunctionCallHelpers.FormatFunctionResultAsJsonElement(frc.Result, ToolCallJsonSerializerOptions);
{
JsonSerializerOptions serializerOptions = ToolCallJsonSerializerOptions ?? JsonContext.Default.Options;
JsonElement jsonResult = JsonSerializer.SerializeToElement(frc.Result, serializerOptions.GetTypeInfo(typeof(object)));
yield return new OllamaChatRequestMessage
{
Role = "tool",
Expand All @@ -380,6 +388,7 @@ private IEnumerable<OllamaChatRequestMessage> ToOllamaChatRequestMessages(ChatMe
}, JsonContext.Default.OllamaFunctionResultContent)
};
break;
}
}
}

Expand All @@ -389,7 +398,7 @@ private IEnumerable<OllamaChatRequestMessage> ToOllamaChatRequestMessages(ChatMe
}
}

private OllamaTool ToOllamaTool(AIFunction function) => new()
private static OllamaTool ToOllamaTool(AIFunction function) => new()
{
Type = "function",
Function = new OllamaFunctionTool
Expand All @@ -400,7 +409,7 @@ private IEnumerable<OllamaChatRequestMessage> ToOllamaChatRequestMessages(ChatMe
{
Properties = function.Metadata.Parameters.ToDictionary(
p => p.Name,
p => FunctionCallHelpers.InferParameterJsonSchema(p, function.Metadata, ToolCallJsonSerializerOptions)),
p => p.Schema is JsonElement e ? e : _defaultParameterSchema),
Required = function.Metadata.Parameters.Where(p => p.IsRequired).Select(p => p.Name).ToList(),
},
}
Expand Down
4 changes: 0 additions & 4 deletions src/Libraries/Microsoft.Extensions.AI.OpenAI/Microsoft.Extensions.AI.OpenAI.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@
<InjectStringHashOnLegacy>true</InjectStringHashOnLegacy>
</PropertyGroup>

<ItemGroup>
<Compile Include="../Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallHelpers.cs" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="OpenAI" />
<PackageReference Include="System.Text.Json" />
Expand Down
Loading
Loading