Draft: Add M.E.AI.IChatClient adapter to OpenAI #1
Conversation
| if (openAICompletion.SystemFingerprint is string systemFingerprint) | ||
| { | ||
| (response.AdditionalProperties ??= [])[nameof(openAICompletion.SystemFingerprint)] = systemFingerprint; | ||
| } |
There was a problem hiding this comment.
ChatCompletion.SerializedAdditionalRawData stores all of the properties, even ones that aren't surfaced in a strong-typed manner. We should try to transfer those over to AdditionalProperties, so that all of the data is available. Same goes for ChatResponseUpdate.
| if (additionalProperties.TryGetValue(nameof(result.TopLogProbabilityCount), out int topLogProbabilityCountInt)) | ||
| { | ||
| result.TopLogProbabilityCount = topLogProbabilityCountInt; | ||
| } |
There was a problem hiding this comment.
Outside of the OpenAI assembly, we didn't have a way to transfer arbitrary AdditionalProperties to the options object. It's possible we do now. That would enable being able to use non-standard properties with OpenAI-compatible endpoints.
| functionCall.FunctionName, | ||
| static json => JsonSerializer.Deserialize(json.Span, ResponseClientJsonContext.Default.IDictionaryStringObject)!)); | ||
| break; | ||
| } |
There was a problem hiding this comment.
I opened dotnet/extensions#6311 for it, but we're going to want to support ReasoningResponseItem here, translating it to TextReasoningContent, and the same for streaming, and the same in the inverse direction when translating TextReasoningContent in history back into the OpenAI object model.
| callContent.Arguments, | ||
| AIJsonUtilities.DefaultOptions.GetTypeInfo(typeof(IDictionary<string, object?>))))); | ||
| break; | ||
| } |
There was a problem hiding this comment.
It occurs to me that although we're creating ErrorContent when processing responses output, we're not here translating that ErrorContent back into something in the ResponseItems. I don't know if there's a way to do so, but if there is, we probably should.
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net6.0'"> | ||
| <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="9.0.4" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
Presumably we'll be adding tests to the OpenAI repo as part of this as well.
| <PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" /> | ||
| <PackageReference Include="System.ClientModel" Version="1.2.1" /> | ||
| <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="6.0.1" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net6.0'"> | ||
| <PackageReference Include="Microsoft.Bcl.AsyncInterfaces" Version="9.0.4" /> |
There was a problem hiding this comment.
I'm surprised this is necessary. Worst case this should be an 8.x version, right?
| /// <summary>Converts an Extensions function to an OpenAI chat tool.</summary> | ||
| private static ChatTool ToOpenAIChatTool(AIFunction aiFunction) | ||
| { | ||
| // Default strict to true, but allow to be overridden by an additional Strict property. |
There was a problem hiding this comment.
All uses of strict need to be re-evaluated.
dotnet/extensions#6285
There was a problem hiding this comment.
Agreed. This is a subjective area since it seems like there are cases where we do know we could turn on strict mode automatically (the cases where we know MEAI generated the schema), but it's unclear how manageable that will be in the long run. We don't really want MEAI internals to have to do a lot of OpenAI-specific stuff when generating schema.
So at this point I'd be happy with going for a simpler rule of just defaulting strict to false in all cases. That aligns with OpenAI's decision for strict not to be the default.
| // Handle loosely-typed properties from AdditionalProperties. | ||
| if (options.AdditionalProperties is { Count: > 0 } additionalProperties) | ||
| { | ||
| if (additionalProperties.TryGetValue(nameof(result.ParallelToolCallsEnabled), out bool allowParallelToolCalls)) |
| /// <summary>Provides extension methods for working with <see cref="OpenAIClient"/>s.</summary> | ||
| public static class OpenAIClientExtensions | ||
| { | ||
|
|
| } | ||
|
|
||
| /// <summary>An <see cref="IEmbeddingGenerator{String, Embedding}"/> for an OpenAI <see cref="EmbeddingClient"/>.</summary> | ||
| internal sealed class OpenAIEmbeddingGenerator : IEmbeddingGenerator<string, Embedding<float>> |
There was a problem hiding this comment.
What's the reason for putting all these classes in one file? Is this to match some style preference from openai-dotnet?
Looking at https://github.com/jozkee/openai-dotnet/tree/main/src/Custom, it seems like they have a lot of subdirectories representing different functional areas, so would seem to follow to have a Microsoft.Extensions.AI subdirectory and then class-per-file as usual.
There was a problem hiding this comment.
It was our initial understanding that it was their preference on how this should be contributed. I will revert it to +/- the original split we had.
|
|
||
| if (audio.Id is string id) | ||
| { | ||
| dc.AdditionalProperties[nameof(audio.Id)] = id; |
There was a problem hiding this comment.
I thought the plan was to avoid setting AdditionalProperties for data that could also be reached in a strongly-typed way through RawRepresentation (except if there's some reason to believe it needs to show up in telemetry, which I don't think is the case for the data here, and especially not for Transcript which might be huge).
| if (openAICompletion.SystemFingerprint is string systemFingerprint) | ||
| { | ||
| (response.AdditionalProperties ??= [])[nameof(openAICompletion.SystemFingerprint)] = systemFingerprint; | ||
| } |
There was a problem hiding this comment.
As above, I think all these AdditionalProperties can be removed.
| /// <summary>Converts an extensions options instance to an OpenAI options instance.</summary> | ||
| private static ChatCompletionOptions ToOpenAIOptions(ChatOptions? options) | ||
| { | ||
| ChatCompletionOptions result = new(); |
There was a problem hiding this comment.
Not sure if you're planning to handle it within this PR or later, but from our previous discussion, I think the intent was to try obtaining the initial ChatCompletionOptions from options.AdditionalProperties, e.g. something like:
ChatCompletionOptions result = options.AdditionalProperties?.TryGetValue(typeof(ChatCompletionOptions).FullName, out var initialOptions)
? initialOptions
: new();... and then we can eliminate all the code from L496-558 that reads individual values from AdditionalProperties. The benefit for app developers is they would have a 100% strongly-typed way to set OpenAI options and it would automatically work with all future OpenAI options.
We might also wish to define an extension method on ChatOptions inside this assembly so that you can do:
var options = new ChatOptions();
options.SetOpenAIOptions(new ChatCompletionOptions { ... }); // Extension method that writes to AdditionalProperties[typeof(ChatCompletionOptions).FullName]... but that's optional and would just be for convenience.
There was a problem hiding this comment.
We should make all of these changes in dotnet/extensions for remaining releases of the M.E.AI.OpenAI lib. Ideally the behavior of what's PR'd to openai/openai-dotnet is then identical.
| bool strict = | ||
| !aiFunction.AdditionalProperties.TryGetValue("Strict", out object? strictObj) || | ||
| strictObj is not bool strictValue || | ||
| strictValue; |
There was a problem hiding this comment.
Can you coordinate with @eiriktsarpalis to make sure his changes in dotnet/extensions#6285 don't get lost here?
There was a problem hiding this comment.
The changes have now been merged, so now would be a good time update the implementation.
| if (contentPart.Refusal is string refusal) | ||
| { | ||
| (aiContent.AdditionalProperties ??= [])[nameof(contentPart.Refusal)] = refusal; | ||
| } |
There was a problem hiding this comment.
I think the two uses of AdditionalProperties in this method can be removed since they can be read from RawRepresentation instead.
|
|
||
| namespace OpenAI.Custom.Microsoft.Extensions.AI; | ||
|
|
||
|
|
| /// <summary>Provides extension methods for working with <see cref="OpenAIClient"/>s.</summary> | ||
| public static class OpenAIClientExtensions | ||
| { | ||
|
|
| using OpenAI.Embeddings; | ||
| using OpenAI.Responses; | ||
|
|
||
| namespace OpenAI.Custom.Microsoft.Extensions.AI; |
There was a problem hiding this comment.
This namespace looks wrong. Should be Microsoft.Extensions.AI?
| OpenAI.Embeddings.EmbeddingGenerationOptions openAIOptions = new() | ||
| { | ||
| Dimensions = options?.Dimensions ?? _dimensions, | ||
| }; |
There was a problem hiding this comment.
Ideally we'd follow the same pattern of getting an initial EmbeddingGenerationOptions from AdditionalProperties here as we will do for OpenAIChatClient, and eliminate L99-105 below.
| { | ||
| ErrorCode = errorUpdate.Code, | ||
| Details = errorUpdate.Param, | ||
| } |
There was a problem hiding this comment.
| { | |
| ErrorCode = errorUpdate.Code, | |
| Details = errorUpdate.Param, | |
| } | |
| { | |
| ErrorCode = errorUpdate.Code, | |
| Details = errorUpdate.Param, | |
| } |
| /// <summary>Converts a <see cref="ChatOptions"/> to a <see cref="ResponseCreationOptions"/>.</summary> | ||
| private static ResponseCreationOptions ToOpenAIResponseCreationOptions(ChatOptions? options) | ||
| { | ||
| ResponseCreationOptions result = new(); |
There was a problem hiding this comment.
Same thing about ResponseCreationOptions as in the OpenAIChatClient equivalent.
| response.AdditionalProperties = new AdditionalPropertiesDictionary | ||
| { | ||
| [nameof(audioTranscription.Language)] = audioTranscription.Language, | ||
| [nameof(audioTranscription.Duration)] = audioTranscription.Duration | ||
| }; |
| /// <summary>Converts an extensions options instance to an OpenAI options instance.</summary> | ||
| private static AudioTranscriptionOptions ToOpenAITranscriptionOptions(SpeechToTextOptions? options) | ||
| { | ||
| AudioTranscriptionOptions result = new(); |
There was a problem hiding this comment.
Same suggestion as with ChatCompletionOptions
| response.AdditionalProperties = new AdditionalPropertiesDictionary | ||
| { | ||
| [nameof(audioTranslation.Language)] = audioTranslation.Language, | ||
| [nameof(audioTranslation.Duration)] = audioTranslation.Duration | ||
| }; |
| /// <summary>Converts an extensions options instance to an OpenAI options instance.</summary> | ||
| private static AudioTranslationOptions ToOpenAITranslationOptions(SpeechToTextOptions? options) | ||
| { | ||
| AudioTranslationOptions result = new(); |
There was a problem hiding this comment.
Same suggestion as with ChatCompletionOptions
|
Thanks for the updates, @jozkee! It's much nicer now it's split across multiple files. |
@jeffhandley @stephentoub