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

eiriktsarpalis · 2024-10-11T19:32:43Z

Renames FunctionCallHelpers to AIJsonUtilities and exposes the class as public.

Fix #5496. Fix #5482.

Microsoft Reviewers: Open in CodeFlow

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallUtilities.cs

…nctionCallUtilities.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/JsonFunctionCallUtilities.cs

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/JsonSchemaInferenceOptions.cs

src/Libraries/Microsoft.Extensions.AI.Ollama/OllamaChatClient.cs

src/Libraries/Microsoft.Extensions.AI/ChatCompletion/ChatClientStructuredOutputExtensions.cs

…onSchemaInferenceOptions.cs Co-authored-by: Stephen Toub <stoub@microsoft.com>

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/JsonFunctionCallUtilities.cs

eiriktsarpalis · 2024-10-15T16:29:30Z

src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonSchemaCreateOptions.cs

+/// <summary>
+/// An options class for configuring the behavior of <see cref="AIJsonUtilities"/> JSON schema creation functionality.
+/// </summary>
+public sealed class AIJsonSchemaCreateOptions


Should we try to hide these classes behind a nested namespace? Microsoft.Extensions.AI.Utilities?

In general I think we should revisit using nested namespace for grouping. Everything is in a flat list right now.
cc: @SteveSandersonMS

…I.Abstractions.

eiriktsarpalis · 2024-10-16T17:00:52Z

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs

+    /// <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>(


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.

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

stephentoub · 2024-10-17T16:09:11Z

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs

+        string callId,
+        string name,
+        Func<TEncoding, IDictionary<string, object?>?> argumentParser,
+        Func<Exception, bool>? exceptionFilter = null)


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

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

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.

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

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

src/Libraries/Microsoft.Extensions.AI.AzureAIInference/AzureAIInferenceChatClient.cs

stephentoub · 2024-10-17T16:15:35Z

src/Libraries/Microsoft.Extensions.AI.AzureAIInference/AzureAIInferenceChatClient.cs

@@ -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;


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?

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

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;

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

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'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.

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.

src/Libraries/Microsoft.Extensions.AI.Ollama/OllamaChatClient.cs

src/Libraries/Microsoft.Extensions.AI/Utilities/AIJsonUtilities.Schema.cs

src/Libraries/Microsoft.Extensions.AI.Abstractions/Microsoft.Extensions.AI.Abstractions.csproj

stephentoub · 2024-10-17T16:25:33Z

eng/Versions.props

@@ -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>


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?

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

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.

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) ?

Which package are you asking about?

stephentoub

Thanks!

stephentoub · 2024-10-17T16:29:22Z

Was this set to auto-merge and that's why it was merged without comments being addressed?

We should either stop using auto-merge or I'll need to adapt and stop approving with comments.

eiriktsarpalis · 2024-10-17T16:39:42Z

Was this set to auto-merge and that's why it was merged without comments being addressed?

Exactly what happened.

eiriktsarpalis · 2024-10-17T16:42:00Z

We should either stop using auto-merge or I'll need to adapt and stop approving with comments.

Some repos don't trigger auto merges unless all review items have been resolved. Seems like the best of both worlds if we turned that on.

eiriktsarpalis requested review from stephentoub and SteveSandersonMS October 11, 2024 19:32

dotnet-policy-service bot assigned eiriktsarpalis Oct 11, 2024

stephentoub reviewed Oct 11, 2024

View reviewed changes

src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallUtilities.cs Outdated Show resolved Hide resolved

eiriktsarpalis commented Oct 11, 2024

View reviewed changes