From 0e2db1d04f1880f8f516729d1f25e05613b4ec00 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Thu, 20 Jun 2024 18:47:43 +0100 Subject: [PATCH 01/16] Ground Zero --- dotnet/Directory.Packages.props | 2 + dotnet/SK-dotnet.sln | 36 ++++++++++ dotnet/samples/ConceptsV2/ConceptsV2.csproj | 72 +++++++++++++++++++ .../Connectors.OpenAIV2.UnitTests.csproj | 51 +++++++++++++ .../Connectors.OpenAIV2.csproj | 34 +++++++++ .../IntegrationTestsV2.csproj | 67 +++++++++++++++++ 6 files changed, 262 insertions(+) create mode 100644 dotnet/samples/ConceptsV2/ConceptsV2.csproj create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj create mode 100644 dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj diff --git a/dotnet/Directory.Packages.props b/dotnet/Directory.Packages.props index d514e22cb5f4..146311afca6f 100644 --- a/dotnet/Directory.Packages.props +++ b/dotnet/Directory.Packages.props @@ -5,6 +5,8 @@ true + + diff --git a/dotnet/SK-dotnet.sln b/dotnet/SK-dotnet.sln index 2d11481810cb..9f09181e3846 100644 --- a/dotnet/SK-dotnet.sln +++ b/dotnet/SK-dotnet.sln @@ -314,6 +314,14 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "TimePlugin", "samples\Demos EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Connectors.Memory.AzureCosmosDBNoSQL", "src\Connectors\Connectors.Memory.AzureCosmosDBNoSQL\Connectors.Memory.AzureCosmosDBNoSQL.csproj", "{B0B3901E-AF56-432B-8FAA-858468E5D0DF}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Connectors.OpenAIV2", "src\Connectors\Connectors.OpenAIV2\Connectors.OpenAIV2.csproj", "{8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}" +EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Connectors.OpenAIV2.UnitTests", "src\Connectors\Connectors.OpenAIV2.UnitTests\Connectors.OpenAIV2.UnitTests.csproj", "{A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}" +EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ConceptsV2", "samples\ConceptsV2\ConceptsV2.csproj", "{932B6B93-C297-47BE-A061-081ACC6105FB}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "IntegrationTestsV2", "src\IntegrationTestsV2\IntegrationTestsV2.csproj", "{FDEB4884-89B9-4656-80A0-57C7464490F7}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -771,6 +779,30 @@ Global {B0B3901E-AF56-432B-8FAA-858468E5D0DF}.Publish|Any CPU.Build.0 = Publish|Any CPU {B0B3901E-AF56-432B-8FAA-858468E5D0DF}.Release|Any CPU.ActiveCfg = Release|Any CPU {B0B3901E-AF56-432B-8FAA-858468E5D0DF}.Release|Any CPU.Build.0 = Release|Any CPU + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}.Debug|Any CPU.Build.0 = Debug|Any CPU + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}.Publish|Any CPU.ActiveCfg = Debug|Any CPU + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}.Publish|Any CPU.Build.0 = Debug|Any CPU + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}.Release|Any CPU.ActiveCfg = Release|Any CPU + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8}.Release|Any CPU.Build.0 = Release|Any CPU + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}.Debug|Any CPU.Build.0 = Debug|Any CPU + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}.Publish|Any CPU.ActiveCfg = Debug|Any CPU + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}.Publish|Any CPU.Build.0 = Debug|Any CPU + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}.Release|Any CPU.ActiveCfg = Release|Any CPU + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF}.Release|Any CPU.Build.0 = Release|Any CPU + {932B6B93-C297-47BE-A061-081ACC6105FB}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {932B6B93-C297-47BE-A061-081ACC6105FB}.Debug|Any CPU.Build.0 = Debug|Any CPU + {932B6B93-C297-47BE-A061-081ACC6105FB}.Publish|Any CPU.ActiveCfg = Debug|Any CPU + {932B6B93-C297-47BE-A061-081ACC6105FB}.Publish|Any CPU.Build.0 = Debug|Any CPU + {932B6B93-C297-47BE-A061-081ACC6105FB}.Release|Any CPU.ActiveCfg = Release|Any CPU + {932B6B93-C297-47BE-A061-081ACC6105FB}.Release|Any CPU.Build.0 = Release|Any CPU + {FDEB4884-89B9-4656-80A0-57C7464490F7}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {FDEB4884-89B9-4656-80A0-57C7464490F7}.Debug|Any CPU.Build.0 = Debug|Any CPU + {FDEB4884-89B9-4656-80A0-57C7464490F7}.Publish|Any CPU.ActiveCfg = Debug|Any CPU + {FDEB4884-89B9-4656-80A0-57C7464490F7}.Publish|Any CPU.Build.0 = Debug|Any CPU + {FDEB4884-89B9-4656-80A0-57C7464490F7}.Release|Any CPU.ActiveCfg = Release|Any CPU + {FDEB4884-89B9-4656-80A0-57C7464490F7}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -877,6 +909,10 @@ Global {1D3EEB5B-0E06-4700-80D5-164956E43D0A} = {5D4C0700-BBB5-418F-A7B2-F392B9A18263} {F312FCE1-12D7-4DEF-BC29-2FF6618509F3} = {5D4C0700-BBB5-418F-A7B2-F392B9A18263} {B0B3901E-AF56-432B-8FAA-858468E5D0DF} = {24503383-A8C4-4255-9998-28D70FE8E99A} + {8AC4D976-BBBA-44C7-9CFD-567F0B4751D8} = {1B4CBDE0-10C2-4E7D-9CD0-FE7586C96ED1} + {A2E659A5-0CE5-4CBF-B9F6-F8604B2AF0BF} = {1B4CBDE0-10C2-4E7D-9CD0-FE7586C96ED1} + {932B6B93-C297-47BE-A061-081ACC6105FB} = {FA3720F1-C99A-49B2-9577-A940257098BF} + {FDEB4884-89B9-4656-80A0-57C7464490F7} = {831DDCA2-7D2C-4C31-80DB-6BDB3E1F7AE0} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {FBDC56A3-86AD-4323-AA0F-201E59123B83} diff --git a/dotnet/samples/ConceptsV2/ConceptsV2.csproj b/dotnet/samples/ConceptsV2/ConceptsV2.csproj new file mode 100644 index 000000000000..a9fe41232166 --- /dev/null +++ b/dotnet/samples/ConceptsV2/ConceptsV2.csproj @@ -0,0 +1,72 @@ + + + + Concepts + + net8.0 + enable + false + true + + $(NoWarn);CS8618,IDE0009,CA1051,CA1050,CA1707,CA1054,CA2007,VSTHRD111,CS1591,RCS1110,RCS1243,CA5394,SKEXP0001,SKEXP0010,SKEXP0020,SKEXP0040,SKEXP0050,SKEXP0060,SKEXP0070,SKEXP0101,SKEXP0110 + Library + 5ee045b0-aea3-4f08-8d31-32d1a6f8fed0 + + + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + PreserveNewest + + + + + Always + + + diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj new file mode 100644 index 000000000000..6c32aad74b0e --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj @@ -0,0 +1,51 @@ + + + + SemanticKernel.Connectors.OpenAI.UnitTests + $(AssemblyName) + net8.0 + true + enable + false + $(NoWarn);SKEXP0001;SKEXP0070;CS1591;IDE1006;RCS1261;CA1031;CA1308;CA1861;CA2007;CA2234;VSTHRD111 + + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj new file mode 100644 index 000000000000..3e51e9674e21 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj @@ -0,0 +1,34 @@ + + + + + Microsoft.SemanticKernel.Connectors.OpenAI + $(AssemblyName) + net8.0;netstandard2.0 + true + $(NoWarn);NU5104;SKEXP0001,SKEXP0010 + true + + + + + + + + + Semantic Kernel - OpenAI and Azure OpenAI connectors + Semantic Kernel connectors for OpenAI and Azure OpenAI. Contains clients for text generation, chat completion, embedding and DALL-E text to image. + + + + + + + + + + + + + + diff --git a/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj b/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj new file mode 100644 index 000000000000..cbfbfe9e4df3 --- /dev/null +++ b/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj @@ -0,0 +1,67 @@ + + + IntegrationTests + SemanticKernel.IntegrationTests + net8.0 + true + false + $(NoWarn);CA2007,CA1861,VSTHRD111,SKEXP0001,SKEXP0010,SKEXP0020,SKEXP0040,SKEXP0050,SKEXP0060,SKEXP0070,SKEXP0110 + b7762d10-e29b-4bb1-8b74-b6d69a667dd4 + + + + + + + + + + + + + + + + + + + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + + + + + + Always + + + Always + + + Always + + + + + + Always + + + \ No newline at end of file From 77ce33e92ae2db66195e941197862b7d080a3151 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Thu, 20 Jun 2024 19:04:36 +0100 Subject: [PATCH 02/16] Removing global usings to avoid error in AssertExtensions --- .../Connectors.OpenAIV2.UnitTests.csproj | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj index 6c32aad74b0e..046b5999bee6 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj @@ -30,18 +30,6 @@ - - - - - - - - - - - - From e63b7d6803b8d381eb2718728493f1b40a127590 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Thu, 20 Jun 2024 19:34:57 +0100 Subject: [PATCH 03/16] Adding Embeddings and ClientCore --- .../Connectors.OpenAIV2.csproj | 1 + .../Connectors.OpenAIV2/Core/ClientCore.cs | 213 ++++++++++++++++++ .../Core/ClientResultExceptionExtensions.cs | 38 ++++ .../Core/Models/AddHeaderRequestPolicy.cs | 19 ++ .../Core/Models/CustomHostPipelinePolicy.cs | 22 ++ .../Core/Models/HttpPipelinePolicy.cs | 33 +++ .../Models/HttpPipelineSynchronousPolicy.cs | 71 ++++++ .../OpenAITextEmbbedingGenerationService.cs | 85 +++++++ 8 files changed, 482 insertions(+) create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj index 3e51e9674e21..b64bda323adc 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Connectors.OpenAIV2.csproj @@ -30,5 +30,6 @@ + diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs new file mode 100644 index 000000000000..da8b0e5fd4fe --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs @@ -0,0 +1,213 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel; +using System.ClientModel.Primitives; +using System.Collections.Generic; +using System.Net.Http; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.SemanticKernel.Http; +using OpenAI; +using OpenAI.Embeddings; + +#pragma warning disable CA2208 // Instantiate argument exceptions correctly + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +/// +/// Base class for AI clients that provides common functionality for interacting with OpenAI services. +/// +internal class ClientCore +{ + /// + /// Model Id or Deployment Name + /// + internal string ModelName { get; set; } = string.Empty; + + internal Uri? Endpoint { get; set; } = null; + + /// + /// Logger instance + /// + internal ILogger Logger { get; set; } + + private const string DefaultPublicEndpoint = "https://api.openai.com/v1"; + + /// + /// Gets the attribute name used to store the organization in the dictionary. + /// + public static string OrganizationKey => "Organization"; + + /// + /// OpenAI / Azure OpenAI Client + /// + internal OpenAIClient Client { get; } + + /// + /// Initializes a new instance of the class. + /// + /// Model name. + /// OpenAI API Key. + /// OpenAI compatible API endpoint. + /// OpenAI Organization Id (usually optional). + /// Custom for HTTP requests. + /// The to use for logging. If null, no logging will be performed. + internal ClientCore( + string modelId, + string? apiKey = null, + Uri? endpoint = null, + string? organization = null, + HttpClient? httpClient = null, + ILogger? logger = null) + { + Verify.NotNullOrWhiteSpace(modelId); + + this.Logger = logger ?? NullLogger.Instance; + this.ModelName = modelId; + + var options = GetOpenAIClientOptions(httpClient); + + if (!string.IsNullOrWhiteSpace(organization)) + { + options.AddPolicy(new AddHeaderRequestPolicy("OpenAI-Organization", organization!), PipelinePosition.PerCall); + } + + // Accepts the endpoint if provided, otherwise uses the default OpenAI endpoint. + var providedEndpoint = endpoint ?? httpClient?.BaseAddress; + if (providedEndpoint is null) + { + Verify.NotNullOrWhiteSpace(apiKey); // For Public OpenAI Endpoint a key must be provided. + this.Endpoint = new Uri(DefaultPublicEndpoint); + } + else + { + options.AddPolicy(new CustomHostPipelinePolicy(providedEndpoint), PipelinePosition.PerTry); + this.Endpoint = providedEndpoint; + } + + this.Client = new OpenAIClient(apiKey ?? string.Empty, options); + } + + /// + /// Initializes a new instance of the class using the specified OpenAIClient. + /// Note: instances created this way might not have the default diagnostics settings, + /// it's up to the caller to configure the client. + /// + /// Azure OpenAI model ID or deployment name, see https://learn.microsoft.com/azure/cognitive-services/openai/how-to/create-resource + /// Custom . + /// The to use for logging. If null, no logging will be performed. + internal ClientCore( + string modelId, + OpenAIClient openAIClient, + ILogger? logger = null) + { + Verify.NotNullOrWhiteSpace(modelId); + Verify.NotNull(openAIClient); + + this.Logger = logger ?? NullLogger.Instance; + this.ModelName = modelId; + this.Client = openAIClient; + } + + /// + /// Logs OpenAI action details. + /// + /// Caller member name. Populated automatically by runtime. + internal void LogActionDetails([CallerMemberName] string? callerMemberName = default) + { + if (this.Logger.IsEnabled(LogLevel.Information)) + { + this.Logger.LogInformation("Action: {Action}. OpenAI Model ID: {ModelId}.", callerMemberName, this.ModelName); + } + } + + /// + /// Storage for AI service attributes. + /// + internal Dictionary Attributes { get; } = []; + + /// + /// Generates an embedding from the given . + /// + /// List of strings to generate embeddings for + /// The containing services, plugins, and other state for use throughout the operation. + /// The number of dimensions the resulting output embeddings should have. Only supported in "text-embedding-3" and later models. + /// The to monitor for cancellation requests. The default is . + /// List of embeddings + internal async Task>> GetEmbeddingsAsync( + IList data, + Kernel? kernel, + int? dimensions, + CancellationToken cancellationToken) + { + var result = new List>(data.Count); + + if (data.Count > 0) + { + var embeddingsOptions = new EmbeddingGenerationOptions() + { + Dimensions = dimensions + }; + + ClientResult response = await RunRequestAsync(() => this.Client.GetEmbeddingClient(this.ModelName).GenerateEmbeddingsAsync(data, embeddingsOptions, cancellationToken)).ConfigureAwait(false); + var embeddings = response.Value; + + if (embeddings.Count != data.Count) + { + throw new KernelException($"Expected {data.Count} text embedding(s), but received {embeddings.Count}"); + } + + for (var i = 0; i < embeddings.Count; i++) + { + result.Add(embeddings[i].Vector); + } + } + + return result; + } + internal void AddAttribute(string key, string? value) + { + if (!string.IsNullOrEmpty(value)) + { + this.Attributes.Add(key, value); + } + } + + /// Gets options to use for an OpenAIClient + /// Custom for HTTP requests. + /// An instance of . + private static OpenAIClientOptions GetOpenAIClientOptions(HttpClient? httpClient) + { + OpenAIClientOptions options = new() + { + ApplicationId = HttpHeaderConstant.Values.UserAgent, + }; + + options.AddPolicy(new AddHeaderRequestPolicy(HttpHeaderConstant.Names.SemanticKernelVersion, HttpHeaderConstant.Values.GetAssemblyVersion(typeof(ClientCore))), PipelinePosition.PerCall); + + if (httpClient is not null) + { + options.Transport = new HttpClientPipelineTransport(httpClient); + options.RetryPolicy = new ClientRetryPolicy(maxRetries: 0); // Disable SDK retry policy if and only if a custom HttpClient is provided. + options.NetworkTimeout = Timeout.InfiniteTimeSpan; // Disable SDK default timeout + } + + return options; + } + + private static async Task RunRequestAsync(Func> request) + { + try + { + return await request.Invoke().ConfigureAwait(false); + } + catch (ClientResultException e) + { + throw e.ToHttpOperationException(); + } + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs new file mode 100644 index 000000000000..d53ec355d12b --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs @@ -0,0 +1,38 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.ClientModel; +using System.Net; + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +/// +/// Provides extension methods for the class. +/// +internal static class ClientResultExceptionExtensions +{ + /// + /// Converts a to an . + /// + /// The original . + /// An instance. + public static HttpOperationException ToHttpOperationException(this ClientResultException exception) + { + const int NoResponseReceived = 0; + + string? responseContent = null; + + try + { + responseContent = exception.GetRawResponse()?.Content?.ToString(); + } +#pragma warning disable CA1031 // Do not catch general exception types + catch { } // We want to suppress any exceptions that occur while reading the content, ensuring that an HttpOperationException is thrown instead. +#pragma warning restore CA1031 + + return new HttpOperationException( + exception.Status == NoResponseReceived ? null : (HttpStatusCode?)exception.Status, + responseContent, + exception.Message, + exception); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs new file mode 100644 index 000000000000..bbdfd07219a8 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs @@ -0,0 +1,19 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.ClientModel.Primitives; + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +/// +/// Helper class to inject headers into Azure SDK HTTP pipeline +/// +internal sealed class AddHeaderRequestPolicy(string headerName, string headerValue) : HttpPipelineSynchronousPolicy +{ + private readonly string _headerName = headerName; + private readonly string _headerValue = headerValue; + + public override void OnSendingRequest(PipelineMessage message) + { + message.Request.Headers.Add(this._headerName, this._headerValue); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs new file mode 100644 index 000000000000..37a51d5d1927 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel.Primitives; + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +internal sealed class CustomHostPipelinePolicy : HttpPipelineSynchronousPolicy +{ + private readonly Uri _endpoint; + + internal CustomHostPipelinePolicy(Uri endpoint) + { + this._endpoint = endpoint; + } + + public override void OnSendingRequest(PipelineMessage message) + { + // Update current host to provided endpoint + message.Request.Uri = this._endpoint; + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs new file mode 100644 index 000000000000..14d256e3849f --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel.Primitives; +using System.Threading.Tasks; + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +internal abstract class HttpPipelinePolicy : PipelinePolicy +{ + /// + /// Invokes the next in the . + /// + /// The next policy would be applied to. + /// The set of to execute after next one. + /// Current index in the pipeline + /// The representing the asynchronous operation. + protected static ValueTask ProcessNextAsync(PipelineMessage message, ReadOnlyMemory pipeline, int currentIndex) + { + return pipeline.Span[0].ProcessAsync(message, pipeline.Slice(1).Span.ToArray(), currentIndex); + } + + /// + /// Invokes the next in the . + /// + /// The next policy would be applied to. + /// The set of to execute after next one. + /// Current index in the pipeline + protected static void ProcessNext(PipelineMessage message, ReadOnlyMemory pipeline, int currentIndex) + { + pipeline.Span[0].Process(message, pipeline.Slice(1).Span.ToArray(), currentIndex); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs new file mode 100644 index 000000000000..b94d6403a592 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel.Primitives; +using System.Collections.Generic; +using System.Reflection; +using System.Threading.Tasks; + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +/// +/// Represents a that doesn't do any asynchronous or synchronously blocking operations. +/// +internal class HttpPipelineSynchronousPolicy : HttpPipelinePolicy +{ + private static readonly Type[] s_onReceivedResponseParameters = new[] { typeof(PipelineMessage) }; + + private readonly bool _hasOnReceivedResponse = true; + + /// + /// Initializes a new instance of + /// + protected HttpPipelineSynchronousPolicy() + { + var onReceivedResponseMethod = this.GetType().GetMethod(nameof(OnReceivedResponse), BindingFlags.Instance | BindingFlags.Public, null, s_onReceivedResponseParameters, null); + if (onReceivedResponseMethod != null) + { + this._hasOnReceivedResponse = onReceivedResponseMethod.GetBaseDefinition().DeclaringType != onReceivedResponseMethod.DeclaringType; + } + } + + /// + public override void Process(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) + { + this.OnSendingRequest(message); + ProcessNext(message, pipeline, currentIndex); + this.OnReceivedResponse(message); + } + + /// + public override ValueTask ProcessAsync(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) + { + if (!this._hasOnReceivedResponse) + { + // If OnReceivedResponse was not overridden we can avoid creating a state machine and return the task directly + this.OnSendingRequest(message); + return ProcessNextAsync(message, pipeline, currentIndex); + } + + return this.InnerProcessAsync(message, pipeline, currentIndex); + } + + private async ValueTask InnerProcessAsync(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) + { + this.OnSendingRequest(message); + await ProcessNextAsync(message, pipeline, currentIndex).ConfigureAwait(false); + this.OnReceivedResponse(message); + } + + /// + /// Method is invoked before the request is sent. + /// + /// The containing the request. + public virtual void OnSendingRequest(PipelineMessage message) { } + + /// + /// Method is invoked after the response is received. + /// + /// The containing the response. + public virtual void OnReceivedResponse(PipelineMessage message) { } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs new file mode 100644 index 000000000000..c304a2d2d564 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs @@ -0,0 +1,85 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.SemanticKernel.Embeddings; +using Microsoft.SemanticKernel.Services; +using OpenAI; + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +/// +/// OpenAI text embedding service. +/// +[Experimental("SKEXP0010")] +public sealed class OpenAITextEmbeddingGenerationService : ITextEmbeddingGenerationService +{ + private readonly ClientCore _core; + private readonly int? _dimensions; + + /// + /// Create an instance of the OpenAI text embedding connector + /// + /// Model name + /// OpenAI API Key + /// OpenAI Organization Id (usually optional) + /// Custom for HTTP requests. + /// The to use for logging. If null, no logging will be performed. + /// The number of dimensions the resulting output embeddings should have. Only supported in "text-embedding-3" and later models. + public OpenAITextEmbeddingGenerationService( + string modelId, + string apiKey, + string? organization = null, + HttpClient? httpClient = null, + ILoggerFactory? loggerFactory = null, + int? dimensions = null) + { + this._core = new( + modelId: modelId, + apiKey: apiKey, + organization: organization, + httpClient: httpClient, + logger: loggerFactory?.CreateLogger(typeof(OpenAITextEmbeddingGenerationService))); + + this._core.AddAttribute(AIServiceExtensions.ModelIdKey, modelId); + + this._dimensions = dimensions; + } + + /// + /// Create an instance of the OpenAI text embedding connector + /// + /// Model name + /// Custom for HTTP requests. + /// The to use for logging. If null, no logging will be performed. + /// The number of dimensions the resulting output embeddings should have. Only supported in "text-embedding-3" and later models. + public OpenAITextEmbeddingGenerationService( + string modelId, + OpenAIClient openAIClient, + ILoggerFactory? loggerFactory = null, + int? dimensions = null) + { + this._core = new(modelId, openAIClient, loggerFactory?.CreateLogger(typeof(OpenAITextEmbeddingGenerationService))); + this._core.AddAttribute(AIServiceExtensions.ModelIdKey, modelId); + + this._dimensions = dimensions; + } + + /// + public IReadOnlyDictionary Attributes => this._core.Attributes; + + /// + public Task>> GenerateEmbeddingsAsync( + IList data, + Kernel? kernel = null, + CancellationToken cancellationToken = default) + { + this._core.LogActionDetails(); + return this._core.GetEmbeddingsAsync(data, kernel, this._dimensions, cancellationToken); + } +} From 27e5b42cb880dccd463500a20d5d72858defcd0c Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 10:12:37 +0100 Subject: [PATCH 04/16] Adding descriptions + small improvements --- .../Connectors.OpenAIV2.UnitTests.csproj | 7 +- .../Connectors.OpenAIV2/Core/ClientCore.cs | 83 ++++++++++--------- .../Core/ClientResultExceptionExtensions.cs | 6 ++ .../Core/Models/AddHeaderRequestPolicy.cs | 2 +- .../Core/Models/CustomHostPipelinePolicy.cs | 22 ----- .../Core/Models/HttpPipelinePolicy.cs | 6 ++ .../Models/HttpPipelineSynchronousPolicy.cs | 6 ++ 7 files changed, 71 insertions(+), 61 deletions(-) delete mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj index 046b5999bee6..95862f006edb 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj @@ -1,4 +1,4 @@ - + SemanticKernel.Connectors.OpenAI.UnitTests @@ -35,5 +35,10 @@ + + + + + diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs index da8b0e5fd4fe..8eb967fe9294 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs @@ -1,5 +1,11 @@ // Copyright (c) Microsoft. All rights reserved. +/* +Phase 01 : This class was created adapting and merging ClientCore and OpenAIClientCore classes. +System.ClientModel changes were added and adapted to the code as this package is now used as a dependency over OpenAI package. +All logic from original ClientCore and OpenAIClientCore were preserved. +*/ + using System; using System.ClientModel; using System.ClientModel.Primitives; @@ -24,10 +30,13 @@ namespace Microsoft.SemanticKernel.Connectors.OpenAI; internal class ClientCore { /// - /// Model Id or Deployment Name + /// Model Id /// - internal string ModelName { get; set; } = string.Empty; + internal string ModelId { get; set; } = string.Empty; + /// + /// Non-default endpoint for OpenAI API. + /// internal Uri? Endpoint { get; set; } = null; /// @@ -35,17 +44,15 @@ internal class ClientCore /// internal ILogger Logger { get; set; } - private const string DefaultPublicEndpoint = "https://api.openai.com/v1"; - /// - /// Gets the attribute name used to store the organization in the dictionary. + /// OpenAI / Azure OpenAI Client /// - public static string OrganizationKey => "Organization"; + internal OpenAIClient Client { get; } /// - /// OpenAI / Azure OpenAI Client + /// Storage for AI service attributes. /// - internal OpenAIClient Client { get; } + internal Dictionary Attributes { get; } = []; /// /// Initializes a new instance of the class. @@ -67,26 +74,19 @@ internal ClientCore( Verify.NotNullOrWhiteSpace(modelId); this.Logger = logger ?? NullLogger.Instance; - this.ModelName = modelId; - - var options = GetOpenAIClientOptions(httpClient); - - if (!string.IsNullOrWhiteSpace(organization)) - { - options.AddPolicy(new AddHeaderRequestPolicy("OpenAI-Organization", organization!), PipelinePosition.PerCall); - } + this.ModelId = modelId; // Accepts the endpoint if provided, otherwise uses the default OpenAI endpoint. - var providedEndpoint = endpoint ?? httpClient?.BaseAddress; - if (providedEndpoint is null) + this.Endpoint = endpoint ?? httpClient?.BaseAddress; + if (this.Endpoint is null) { Verify.NotNullOrWhiteSpace(apiKey); // For Public OpenAI Endpoint a key must be provided. - this.Endpoint = new Uri(DefaultPublicEndpoint); } - else + + var options = GetOpenAIClientOptions(httpClient, this.Endpoint); + if (!string.IsNullOrWhiteSpace(organization)) { - options.AddPolicy(new CustomHostPipelinePolicy(providedEndpoint), PipelinePosition.PerTry); - this.Endpoint = providedEndpoint; + options.AddPolicy(new AddHeaderRequestPolicy("OpenAI-Organization", organization!), PipelinePosition.PerCall); } this.Client = new OpenAIClient(apiKey ?? string.Empty, options); @@ -109,7 +109,7 @@ internal ClientCore( Verify.NotNull(openAIClient); this.Logger = logger ?? NullLogger.Instance; - this.ModelName = modelId; + this.ModelId = modelId; this.Client = openAIClient; } @@ -121,15 +121,10 @@ internal void LogActionDetails([CallerMemberName] string? callerMemberName = def { if (this.Logger.IsEnabled(LogLevel.Information)) { - this.Logger.LogInformation("Action: {Action}. OpenAI Model ID: {ModelId}.", callerMemberName, this.ModelName); + this.Logger.LogInformation("Action: {Action}. OpenAI Model ID: {ModelId}.", callerMemberName, this.ModelId); } } - /// - /// Storage for AI service attributes. - /// - internal Dictionary Attributes { get; } = []; - /// /// Generates an embedding from the given . /// @@ -153,7 +148,7 @@ internal async Task>> GetEmbeddingsAsync( Dimensions = dimensions }; - ClientResult response = await RunRequestAsync(() => this.Client.GetEmbeddingClient(this.ModelName).GenerateEmbeddingsAsync(data, embeddingsOptions, cancellationToken)).ConfigureAwait(false); + ClientResult response = await RunRequestAsync(() => this.Client.GetEmbeddingClient(this.ModelId).GenerateEmbeddingsAsync(data, embeddingsOptions, cancellationToken)).ConfigureAwait(false); var embeddings = response.Value; if (embeddings.Count != data.Count) @@ -169,6 +164,12 @@ internal async Task>> GetEmbeddingsAsync( return result; } + + /// + /// Allows adding attributes to the client. + /// + /// Attribute key. + /// Attribute value. internal void AddAttribute(string key, string? value) { if (!string.IsNullOrEmpty(value)) @@ -179,26 +180,34 @@ internal void AddAttribute(string key, string? value) /// Gets options to use for an OpenAIClient /// Custom for HTTP requests. + /// Endpoint for the OpenAI API. /// An instance of . - private static OpenAIClientOptions GetOpenAIClientOptions(HttpClient? httpClient) + private static OpenAIClientOptions GetOpenAIClientOptions(HttpClient? httpClient, Uri? endpoint) { - OpenAIClientOptions options = new() - { - ApplicationId = HttpHeaderConstant.Values.UserAgent, - }; + // As the options Endpoint is an init property and I can't set it afterwards, + // I need an if statement to create the options for a custom endpoint. + OpenAIClientOptions options = (endpoint is null) + ? new OpenAIClientOptions() { ApplicationId = HttpHeaderConstant.Values.UserAgent } + : new OpenAIClientOptions() { ApplicationId = HttpHeaderConstant.Values.UserAgent, Endpoint = endpoint }; options.AddPolicy(new AddHeaderRequestPolicy(HttpHeaderConstant.Names.SemanticKernelVersion, HttpHeaderConstant.Values.GetAssemblyVersion(typeof(ClientCore))), PipelinePosition.PerCall); if (httpClient is not null) { options.Transport = new HttpClientPipelineTransport(httpClient); - options.RetryPolicy = new ClientRetryPolicy(maxRetries: 0); // Disable SDK retry policy if and only if a custom HttpClient is provided. - options.NetworkTimeout = Timeout.InfiniteTimeSpan; // Disable SDK default timeout + options.RetryPolicy = new ClientRetryPolicy(maxRetries: 0); // Disable retry policy if and only if a custom HttpClient is provided. + options.NetworkTimeout = Timeout.InfiniteTimeSpan; // Disable default timeout } return options; } + /// + /// Invokes the specified request and handles exceptions. + /// + /// Type of the response. + /// Request to invoke. + /// Returns the response. private static async Task RunRequestAsync(Func> request) { try diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs index d53ec355d12b..59134dbfb511 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs @@ -1,5 +1,11 @@ // Copyright (c) Microsoft. All rights reserved. +/* +Phase 01: +This class is introduced in exchange for the original RequestExceptionExtensions class of Azure.Core to the new ClientException from System.ClientModel, +Preserved the logic as is. +*/ + using System.ClientModel; using System.Net; diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs index bbdfd07219a8..518bc170f739 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs @@ -5,7 +5,7 @@ namespace Microsoft.SemanticKernel.Connectors.OpenAI; /// -/// Helper class to inject headers into Azure SDK HTTP pipeline +/// Helper class to inject headers into System ClientModel Http pipeline /// internal sealed class AddHeaderRequestPolicy(string headerName, string headerValue) : HttpPipelineSynchronousPolicy { diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs deleted file mode 100644 index 37a51d5d1927..000000000000 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/CustomHostPipelinePolicy.cs +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System; -using System.ClientModel.Primitives; - -namespace Microsoft.SemanticKernel.Connectors.OpenAI; - -internal sealed class CustomHostPipelinePolicy : HttpPipelineSynchronousPolicy -{ - private readonly Uri _endpoint; - - internal CustomHostPipelinePolicy(Uri endpoint) - { - this._endpoint = endpoint; - } - - public override void OnSendingRequest(PipelineMessage message) - { - // Update current host to provided endpoint - message.Request.Uri = this._endpoint; - } -} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs index 14d256e3849f..ddf72ff0fc93 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs @@ -1,5 +1,11 @@ // Copyright (c) Microsoft. All rights reserved. +/* +Phase 1 +As SystemClient model does not have any specialization or extension ATM, introduced this class with the adapted to use System.ClientModel abstractions. +https://github.com/Azure/azure-sdk-for-net/blob/8bd22837639d54acccc820e988747f8d28bbde4a/sdk/core/Azure.Core/src/Pipeline/HttpPipelinePolicy.cs +*/ + using System; using System.ClientModel.Primitives; using System.Threading.Tasks; diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs index b94d6403a592..68cd92583c19 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs @@ -1,5 +1,11 @@ // Copyright (c) Microsoft. All rights reserved. +/* +Phase 1 +As SystemClient model does not have any specialization or extension ATM, introduced this class with the adapted to use System.ClientModel abstractions. +https://github.com/Azure/azure-sdk-for-net/blob/8bd22837639d54acccc820e988747f8d28bbde4a/sdk/core/Azure.Core/src/Pipeline/HttpPipelineSynchronousPolicy.cs +*/ + using System; using System.ClientModel.Primitives; using System.Collections.Generic; From 80ff25ab504cbcbb426b30551c367196c50f60b4 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 13:00:25 +0100 Subject: [PATCH 05/16] UT + Policies Improvements --- .../Connectors.OpenAIV2.UnitTests.csproj | 1 - .../Models/AddHeaderRequestPolicyTests.cs | 43 +++++++++++++++ .../Models/PipelineSynchronousPolicyTests.cs | 52 +++++++++++++++++++ .../Core/Models/AddHeaderRequestPolicy.cs | 2 +- .../Core/Models/HttpPipelinePolicy.cs | 39 -------------- ...Policy.cs => PipelineSynchronousPolicy.cs} | 26 +++++++--- 6 files changed, 115 insertions(+), 48 deletions(-) create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/AddHeaderRequestPolicyTests.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs delete mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs rename dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/{HttpPipelineSynchronousPolicy.cs => PipelineSynchronousPolicy.cs} (73%) diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj index 95862f006edb..ea7cd8028f12 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj @@ -37,7 +37,6 @@ - diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/AddHeaderRequestPolicyTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/AddHeaderRequestPolicyTests.cs new file mode 100644 index 000000000000..83ec6a20568d --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/AddHeaderRequestPolicyTests.cs @@ -0,0 +1,43 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.ClientModel.Primitives; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using Xunit; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core.Models; + +public class AddHeaderRequestPolicyTests +{ + [Fact] + public void ItCanBeInstantiated() + { + // Arrange + var headerName = "headerName"; + var headerValue = "headerValue"; + + // Act + var addHeaderRequestPolicy = new AddHeaderRequestPolicy(headerName, headerValue); + + // Assert + Assert.NotNull(addHeaderRequestPolicy); + } + + [Fact] + public void ItOnSendingRequestAddsHeaderToRequest() + { + // Arrange + var headerName = "headerName"; + var headerValue = "headerValue"; + var addHeaderRequestPolicy = new AddHeaderRequestPolicy(headerName, headerValue); + var pipeline = ClientPipeline.Create(); + var message = pipeline.CreateMessage(); + + // Act + addHeaderRequestPolicy.OnSendingRequest(message); + + // Assert + message.Request.Headers.TryGetValue(headerName, out var value); + Assert.NotNull(value); + Assert.Equal(headerValue, value); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs new file mode 100644 index 000000000000..3868d4c3bf22 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.ClientModel.Primitives; +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using Xunit; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core.Models; +public class PipelineSynchronousPolicyTests +{ + [Fact] + public async Task ItProcessAsyncWhenSpecializationHasReceivedResponseOverrideShouldCallIt() + { + var first = new MyHttpPipelinePolicyWithoutOverride(); + var last = new MyHttpPipelinePolicyWithOverride(); + + IReadOnlyList policies = [first, last]; + await policies[0].ProcessAsync(ClientPipeline.Create().CreateMessage(), policies, 0); + + Assert.True(first.CalledProcess); + Assert.True(last.CalledProcess); + Assert.True(last.CalledOnReceivedResponse); + } + + private class MyHttpPipelinePolicyWithoutOverride : PipelineSynchronousPolicy + { + public bool CalledProcess { get; private set; } + + public override void Process(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) + { + this.CalledProcess = true; + base.Process(message, pipeline, currentIndex); + } + + public override ValueTask ProcessAsync(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) + { + this.CalledProcess = true; + return base.ProcessAsync(message, pipeline, currentIndex); + } + } + + private sealed class MyHttpPipelinePolicyWithOverride : MyHttpPipelinePolicyWithoutOverride + { + public bool CalledOnReceivedResponse { get; private set; } + + public override void OnReceivedResponse(PipelineMessage message) + { + this.CalledOnReceivedResponse = true; + } + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs index 518bc170f739..607f5012d829 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs @@ -7,7 +7,7 @@ namespace Microsoft.SemanticKernel.Connectors.OpenAI; /// /// Helper class to inject headers into System ClientModel Http pipeline /// -internal sealed class AddHeaderRequestPolicy(string headerName, string headerValue) : HttpPipelineSynchronousPolicy +internal sealed class AddHeaderRequestPolicy(string headerName, string headerValue) : PipelineSynchronousPolicy { private readonly string _headerName = headerName; private readonly string _headerValue = headerValue; diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs deleted file mode 100644 index ddf72ff0fc93..000000000000 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelinePolicy.cs +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -/* -Phase 1 -As SystemClient model does not have any specialization or extension ATM, introduced this class with the adapted to use System.ClientModel abstractions. -https://github.com/Azure/azure-sdk-for-net/blob/8bd22837639d54acccc820e988747f8d28bbde4a/sdk/core/Azure.Core/src/Pipeline/HttpPipelinePolicy.cs -*/ - -using System; -using System.ClientModel.Primitives; -using System.Threading.Tasks; - -namespace Microsoft.SemanticKernel.Connectors.OpenAI; - -internal abstract class HttpPipelinePolicy : PipelinePolicy -{ - /// - /// Invokes the next in the . - /// - /// The next policy would be applied to. - /// The set of to execute after next one. - /// Current index in the pipeline - /// The representing the asynchronous operation. - protected static ValueTask ProcessNextAsync(PipelineMessage message, ReadOnlyMemory pipeline, int currentIndex) - { - return pipeline.Span[0].ProcessAsync(message, pipeline.Slice(1).Span.ToArray(), currentIndex); - } - - /// - /// Invokes the next in the . - /// - /// The next policy would be applied to. - /// The set of to execute after next one. - /// Current index in the pipeline - protected static void ProcessNext(PipelineMessage message, ReadOnlyMemory pipeline, int currentIndex) - { - pipeline.Span[0].Process(message, pipeline.Slice(1).Span.ToArray(), currentIndex); - } -} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/PipelineSynchronousPolicy.cs similarity index 73% rename from dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs rename to dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/PipelineSynchronousPolicy.cs index 68cd92583c19..b7690ead8b7f 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/HttpPipelineSynchronousPolicy.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/PipelineSynchronousPolicy.cs @@ -15,18 +15,18 @@ Phase 1 namespace Microsoft.SemanticKernel.Connectors.OpenAI; /// -/// Represents a that doesn't do any asynchronous or synchronously blocking operations. +/// Represents a that doesn't do any asynchronous or synchronously blocking operations. /// -internal class HttpPipelineSynchronousPolicy : HttpPipelinePolicy +internal class PipelineSynchronousPolicy : PipelinePolicy { private static readonly Type[] s_onReceivedResponseParameters = new[] { typeof(PipelineMessage) }; private readonly bool _hasOnReceivedResponse = true; /// - /// Initializes a new instance of + /// Initializes a new instance of /// - protected HttpPipelineSynchronousPolicy() + protected PipelineSynchronousPolicy() { var onReceivedResponseMethod = this.GetType().GetMethod(nameof(OnReceivedResponse), BindingFlags.Instance | BindingFlags.Public, null, s_onReceivedResponseParameters, null); if (onReceivedResponseMethod != null) @@ -39,7 +39,11 @@ protected HttpPipelineSynchronousPolicy() public override void Process(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) { this.OnSendingRequest(message); - ProcessNext(message, pipeline, currentIndex); + if (pipeline.Count > currentIndex + 1) + { + // If there are more policies in the pipeline, continue processing + ProcessNext(message, pipeline, currentIndex); + } this.OnReceivedResponse(message); } @@ -50,7 +54,11 @@ public override ValueTask ProcessAsync(PipelineMessage message, IReadOnlyList currentIndex + 1) + { + // If there are more policies in the pipeline, continue processing + return ProcessNextAsync(message, pipeline, currentIndex); + } } return this.InnerProcessAsync(message, pipeline, currentIndex); @@ -59,7 +67,11 @@ public override ValueTask ProcessAsync(PipelineMessage message, IReadOnlyList pipeline, int currentIndex) { this.OnSendingRequest(message); - await ProcessNextAsync(message, pipeline, currentIndex).ConfigureAwait(false); + if (pipeline.Count > currentIndex + 1) + { + // If there are more policies in the pipeline, continue processing + await ProcessNextAsync(message, pipeline, currentIndex).ConfigureAwait(false); + } this.OnReceivedResponse(message); } From 485d2eceeea68cb2d3be1b91d20a571afcb412ff Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 13:36:54 +0100 Subject: [PATCH 06/16] Update unit tests --- .../Models/PipelineSynchronousPolicyTests.cs | 4 + .../ClientResultExceptionExtensionsTests.cs | 53 ++++++ .../Utils/MockPipelineResponse.cs | 158 ++++++++++++++++++ .../Utils/MockResponseHeaders.cs | 32 ++++ .../ClientResultExceptionExtensions.cs | 0 5 files changed, 247 insertions(+) create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs rename dotnet/src/Connectors/Connectors.OpenAIV2/{Core => Extensions}/ClientResultExceptionExtensions.cs (100%) diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs index 3868d4c3bf22..cae4b32b4283 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/Models/PipelineSynchronousPolicyTests.cs @@ -12,12 +12,16 @@ public class PipelineSynchronousPolicyTests [Fact] public async Task ItProcessAsyncWhenSpecializationHasReceivedResponseOverrideShouldCallIt() { + // Arrange var first = new MyHttpPipelinePolicyWithoutOverride(); var last = new MyHttpPipelinePolicyWithOverride(); IReadOnlyList policies = [first, last]; + + // Act await policies[0].ProcessAsync(ClientPipeline.Create().CreateMessage(), policies, 0); + // Assert Assert.True(first.CalledProcess); Assert.True(last.CalledProcess); Assert.True(last.CalledOnReceivedResponse); diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs new file mode 100644 index 000000000000..2a9c78be6c88 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs @@ -0,0 +1,53 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel; +using System.ClientModel.Primitives; +using System.Net.Http; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using Xunit; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests.Extensions; + +public class ClientResultExceptionExtensionsTests +{ + [Fact] + public void ItCanRecoverFromResponseErrorAndConvertsToHttpOperationExceptionWithDefaultData() + { + // Arrange + var exception = new ClientResultException("message", ClientPipeline.Create().CreateMessage().Response); + + // Act + var httpOperationException = exception.ToHttpOperationException(); + + // Assert + Assert.NotNull(httpOperationException); + Assert.Equal(exception, httpOperationException.InnerException); + Assert.Equal(exception.Message, httpOperationException.Message); + Assert.Null(httpOperationException.ResponseContent); + Assert.Null(httpOperationException.StatusCode); + } + + [Fact] + public void ItCanProvideResponseContentAndStatusCode() + { + // Arrange + var pipelineMessage = ClientPipeline.Create().CreateMessage(); + + response.Response + + pipelineMessage.Content = "content"; + pipelineMessage.StatusCode = 200; + var exception = new ClientResultException("message", pipelineMessage); + + // Act + var httpOperationException = exception.ToHttpOperationException(); + + // Assert + Assert.NotNull(httpOperationException); + Assert.Equal(exception, httpOperationException.InnerException); + Assert.Equal(exception.Message, httpOperationException.Message); + Assert.Equal(pipelineMessage.Content, httpOperationException.ResponseContent); + Assert.Equal(pipelineMessage.StatusCode, httpOperationException.StatusCode); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs new file mode 100644 index 000000000000..bc907ad710b1 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs @@ -0,0 +1,158 @@ +// Copyright (c) Microsoft. All rights reserved. +/* Phase 01 +This class was imported and adapted from the System.ClientModel Unit Tests. +https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs +*/ + +using System; +using System.ClientModel.Primitives; +using System.IO; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests; + +public class MockPipelineResponse : PipelineResponse +{ + private int _status; + private string _reasonPhrase; + private Stream? _contentStream; + private BinaryData? _bufferedContent; + + private readonly PipelineResponseHeaders _headers; + + private bool _disposed; + + public MockPipelineResponse(int status = 0, string reasonPhrase = "") + { + this._status = status; + this._reasonPhrase = reasonPhrase; + this._headers = new MockResponseHeaders(); + } + + public override int Status => this._status; + + public void SetStatus(int value) => this._status = value; + + public override string ReasonPhrase => this._reasonPhrase; + + public void SetReasonPhrase(string value) => this._reasonPhrase = value; + + public void SetContent(byte[] content) + { + this.ContentStream = new MemoryStream(content, 0, content.Length, false, true); + } + + public MockPipelineResponse SetContent(string content) + { + this.SetContent(Encoding.UTF8.GetBytes(content)); + return this; + } + + public override Stream? ContentStream + { + get => this._contentStream; + set => this._contentStream = value; + } + + public override BinaryData Content + { + get + { + if (this._contentStream is null) + { + return new BinaryData(Array.Empty()); + } + + if (this.ContentStream is not MemoryStream memoryContent) + { + throw new InvalidOperationException($"The response is not buffered."); + } + + if (memoryContent.TryGetBuffer(out ArraySegment segment)) + { + return new BinaryData(segment.AsMemory()); + } + else + { + return new BinaryData(memoryContent.ToArray()); + } + } + } + + protected override PipelineResponseHeaders HeadersCore + => this._headers; + + public sealed override void Dispose() + { + this.Dispose(true); + + GC.SuppressFinalize(this); + } + + protected void Dispose(bool disposing) + { + if (disposing && !this._disposed) + { + Stream? content = this._contentStream; + if (content != null) + { + this._contentStream = null; + content.Dispose(); + } + + this._disposed = true; + } + } + + public override BinaryData BufferContent(CancellationToken cancellationToken = default) + { + if (this._bufferedContent is not null) + { + return this._bufferedContent; + } + + if (this._contentStream is null) + { + this._bufferedContent = new BinaryData(Array.Empty()); + return this._bufferedContent; + } + + MemoryStream bufferStream = new(); + this._contentStream.CopyTo(bufferStream); + this._contentStream.Dispose(); + this._contentStream = bufferStream; + + // Less efficient FromStream method called here because it is a mock. + // For intended production implementation, see HttpClientTransportResponse. + this._bufferedContent = BinaryData.FromStream(bufferStream); + return this._bufferedContent; + } + + public override async ValueTask BufferContentAsync(CancellationToken cancellationToken = default) + { + if (this._bufferedContent is not null) + { + return this._bufferedContent; + } + + if (this._contentStream is null) + { + this._bufferedContent = new BinaryData(Array.Empty()); + return this._bufferedContent; + } + + MemoryStream bufferStream = new(); + + await this._contentStream.CopyToAsync(bufferStream, cancellationToken).ConfigureAwait(false); + await this._contentStream.DisposeAsync().ConfigureAwait(false); + + this._contentStream = bufferStream; + + // Less efficient FromStream method called here because it is a mock. + // For intended production implementation, see HttpClientTransportResponse. + this._bufferedContent = BinaryData.FromStream(bufferStream); + return this._bufferedContent; + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs new file mode 100644 index 000000000000..8bebf27b6045 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs @@ -0,0 +1,32 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel.Primitives; +using System.Collections.Generic; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests; + +public class MockResponseHeaders : PipelineResponseHeaders +{ + private readonly Dictionary _headers; + + public MockResponseHeaders() + { + this._headers = new Dictionary(); + } + + public override IEnumerator> GetEnumerator() + { + throw new NotImplementedException(); + } + + public override bool TryGetValue(string name, out string? value) + { + return this._headers.TryGetValue(name, out value); + } + + public override bool TryGetValues(string name, out IEnumerable? values) + { + throw new NotImplementedException(); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Extensions/ClientResultExceptionExtensions.cs similarity index 100% rename from dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientResultExceptionExtensions.cs rename to dotnet/src/Connectors/Connectors.OpenAIV2/Extensions/ClientResultExceptionExtensions.cs From 932d364ffb27c1a7bb938229586c4519e31a03a7 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 13:48:23 +0100 Subject: [PATCH 07/16] Update UTs --- .../ClientResultExceptionExtensionsTests.cs | 36 +++++++++++++++---- .../Utils/MockPipelineResponse.cs | 6 ++-- .../Utils/MockResponseHeaders.cs | 5 +++ .../ClientResultExceptionExtensions.cs | 2 +- 4 files changed, 37 insertions(+), 12 deletions(-) diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs index 2a9c78be6c88..6bb142519760 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs @@ -32,22 +32,44 @@ public void ItCanRecoverFromResponseErrorAndConvertsToHttpOperationExceptionWith public void ItCanProvideResponseContentAndStatusCode() { // Arrange - var pipelineMessage = ClientPipeline.Create().CreateMessage(); + using var pipelineResponse = new MockPipelineResponse(); - response.Response + pipelineResponse.SetContent("content"); + pipelineResponse.SetStatus(200); - pipelineMessage.Content = "content"; - pipelineMessage.StatusCode = 200; - var exception = new ClientResultException("message", pipelineMessage); + var exception = new ClientResultException("message", pipelineResponse); // Act var httpOperationException = exception.ToHttpOperationException(); // Assert Assert.NotNull(httpOperationException); + Assert.NotNull(httpOperationException.StatusCode); Assert.Equal(exception, httpOperationException.InnerException); Assert.Equal(exception.Message, httpOperationException.Message); - Assert.Equal(pipelineMessage.Content, httpOperationException.ResponseContent); - Assert.Equal(pipelineMessage.StatusCode, httpOperationException.StatusCode); + Assert.Equal(pipelineResponse.Content.ToString(), httpOperationException.ResponseContent); + Assert.Equal(pipelineResponse.Status, (int)httpOperationException.StatusCode!); + } + + [Fact] + public void ItProvideStatusForResponsesWithoutContent() + { + // Arrange + using var pipelineResponse = new MockPipelineResponse(); + + pipelineResponse.SetStatus(200); + + var exception = new ClientResultException("message", pipelineResponse); + + // Act + var httpOperationException = exception.ToHttpOperationException(); + + // Assert + Assert.NotNull(httpOperationException); + Assert.NotNull(httpOperationException.StatusCode); + Assert.Empty(httpOperationException.ResponseContent!); + Assert.Equal(exception, httpOperationException.InnerException); + Assert.Equal(exception.Message, httpOperationException.Message); + Assert.Equal(pipelineResponse.Status, (int)httpOperationException.StatusCode!); } } diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs index bc907ad710b1..1070620f4b95 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs @@ -1,4 +1,5 @@ // Copyright (c) Microsoft. All rights reserved. + /* Phase 01 This class was imported and adapted from the System.ClientModel Unit Tests. https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs @@ -74,10 +75,7 @@ public override BinaryData Content { return new BinaryData(segment.AsMemory()); } - else - { - return new BinaryData(memoryContent.ToArray()); - } + return new BinaryData(memoryContent.ToArray()); } } diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs index 8bebf27b6045..fceef64e4bae 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockResponseHeaders.cs @@ -1,5 +1,10 @@ // Copyright (c) Microsoft. All rights reserved. +/* Phase 01 +This class was imported and adapted from the System.ClientModel Unit Tests. +https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockResponseHeaders.cs +*/ + using System; using System.ClientModel.Primitives; using System.Collections.Generic; diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Extensions/ClientResultExceptionExtensions.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Extensions/ClientResultExceptionExtensions.cs index 59134dbfb511..7da92e5826ba 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Extensions/ClientResultExceptionExtensions.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Extensions/ClientResultExceptionExtensions.cs @@ -29,7 +29,7 @@ public static HttpOperationException ToHttpOperationException(this ClientResultE try { - responseContent = exception.GetRawResponse()?.Content?.ToString(); + responseContent = exception.GetRawResponse()?.Content.ToString(); } #pragma warning disable CA1031 // Do not catch general exception types catch { } // We want to suppress any exceptions that occur while reading the content, ensuring that an HttpOperationException is thrown instead. From 2a1adbb694670a06e9ec02f968c69d4a0847d9d4 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 15:00:27 +0100 Subject: [PATCH 08/16] Add changes to ClientCore and UTs --- .../Core/ClientCoreTests.cs | 63 ++++++++++++++++++ .../Core/ClientCore.Embeddings.cs | 64 +++++++++++++++++++ .../Connectors.OpenAIV2/Core/ClientCore.cs | 58 ++++------------- .../Core/Models/AddHeaderRequestPolicy.cs | 4 ++ 4 files changed, 142 insertions(+), 47 deletions(-) create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.Embeddings.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs new file mode 100644 index 000000000000..087e63f737ef --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.ClientModel; +using System.Net.Http; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using OpenAI; +using Xunit; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core; +public class ClientCoreTests +{ + [Fact] + public void ItCanBeInstantiated() + { + // Act + var client = new OpenAIClient(new ApiKeyCredential("key")); + + var clientCoreModelConstructor = new ClientCore("model1"); + var clientCoreOpenAIClientConstructor = new ClientCore("model1", client); + + // Assert + Assert.NotNull(clientCoreModelConstructor); + Assert.NotNull(clientCoreOpenAIClientConstructor); + + Assert.Equal("model1", clientCoreModelConstructor.ModelId); + Assert.Equal("model1", clientCoreOpenAIClientConstructor.ModelId); + + Assert.NotNull(clientCoreModelConstructor.Client); + Assert.NotNull(clientCoreOpenAIClientConstructor.Client); + Assert.Equal(client, clientCoreOpenAIClientConstructor.Client); + } + + + [Theory] + [InlineData(null, null)] + [InlineData("http://localhost", null)] + [InlineData(null, "http://localhost")] + [InlineData("http://localhost", "http://localhost")] + public void ItUsesEndpointAsExpected(string? clientBaseAddress, string? providedEndpoint) + { + // Arrange + Uri? endpoint = null; + HttpClient? client = null; + if (providedEndpoint is not null) + { + endpoint = new Uri(providedEndpoint); + } + + if (clientBaseAddress is not null) + { + client = new HttpClient { BaseAddress = new Uri(clientBaseAddress) }; + } + + // Act + var clientCore = new ClientCore("model", endpoint: endpoint, httpClient: client); + + // Assert + Assert.Equal(endpoint ?? client?.BaseAddress ?? new Uri("https://api.openai.com/v1"), clientCore.Endpoint); + + client?.Dispose(); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.Embeddings.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.Embeddings.cs new file mode 100644 index 000000000000..d11e2799addd --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.Embeddings.cs @@ -0,0 +1,64 @@ +// Copyright (c) Microsoft. All rights reserved. + +/* +Phase 01 + +This class was created to simplify any Text Embeddings Support from the v1 ClientCore +*/ + +using System; +using System.ClientModel; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using OpenAI.Embeddings; + +#pragma warning disable CA2208 // Instantiate argument exceptions correctly + +namespace Microsoft.SemanticKernel.Connectors.OpenAI; + +/// +/// Base class for AI clients that provides common functionality for interacting with OpenAI services. +/// +internal partial class ClientCore +{ + /// + /// Generates an embedding from the given . + /// + /// List of strings to generate embeddings for + /// The containing services, plugins, and other state for use throughout the operation. + /// The number of dimensions the resulting output embeddings should have. Only supported in "text-embedding-3" and later models. + /// The to monitor for cancellation requests. The default is . + /// List of embeddings + internal async Task>> GetEmbeddingsAsync( + IList data, + Kernel? kernel, + int? dimensions, + CancellationToken cancellationToken) + { + var result = new List>(data.Count); + + if (data.Count > 0) + { + var embeddingsOptions = new EmbeddingGenerationOptions() + { + Dimensions = dimensions + }; + + ClientResult response = await RunRequestAsync(() => this.Client.GetEmbeddingClient(this.ModelId).GenerateEmbeddingsAsync(data, embeddingsOptions, cancellationToken)).ConfigureAwait(false); + var embeddings = response.Value; + + if (embeddings.Count != data.Count) + { + throw new KernelException($"Expected {data.Count} text embedding(s), but received {embeddings.Count}"); + } + + for (var i = 0; i < embeddings.Count; i++) + { + result.Add(embeddings[i].Vector); + } + } + + return result; + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs index 8eb967fe9294..f68c8b2585b1 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs @@ -18,7 +18,6 @@ All logic from original ClientCore and OpenAIClientCore were preserved. using Microsoft.Extensions.Logging.Abstractions; using Microsoft.SemanticKernel.Http; using OpenAI; -using OpenAI.Embeddings; #pragma warning disable CA2208 // Instantiate argument exceptions correctly @@ -27,8 +26,13 @@ namespace Microsoft.SemanticKernel.Connectors.OpenAI; /// /// Base class for AI clients that provides common functionality for interacting with OpenAI services. /// -internal class ClientCore +internal partial class ClientCore { + /// + /// Default OpenAI API endpoint. + /// + private const string OpenAIV1Endpoint = "https://api.openai.com/v1"; + /// /// Model Id /// @@ -81,6 +85,7 @@ internal ClientCore( if (this.Endpoint is null) { Verify.NotNullOrWhiteSpace(apiKey); // For Public OpenAI Endpoint a key must be provided. + this.Endpoint = new Uri(OpenAIV1Endpoint); } var options = GetOpenAIClientOptions(httpClient, this.Endpoint); @@ -125,46 +130,6 @@ internal void LogActionDetails([CallerMemberName] string? callerMemberName = def } } - /// - /// Generates an embedding from the given . - /// - /// List of strings to generate embeddings for - /// The containing services, plugins, and other state for use throughout the operation. - /// The number of dimensions the resulting output embeddings should have. Only supported in "text-embedding-3" and later models. - /// The to monitor for cancellation requests. The default is . - /// List of embeddings - internal async Task>> GetEmbeddingsAsync( - IList data, - Kernel? kernel, - int? dimensions, - CancellationToken cancellationToken) - { - var result = new List>(data.Count); - - if (data.Count > 0) - { - var embeddingsOptions = new EmbeddingGenerationOptions() - { - Dimensions = dimensions - }; - - ClientResult response = await RunRequestAsync(() => this.Client.GetEmbeddingClient(this.ModelId).GenerateEmbeddingsAsync(data, embeddingsOptions, cancellationToken)).ConfigureAwait(false); - var embeddings = response.Value; - - if (embeddings.Count != data.Count) - { - throw new KernelException($"Expected {data.Count} text embedding(s), but received {embeddings.Count}"); - } - - for (var i = 0; i < embeddings.Count; i++) - { - result.Add(embeddings[i].Vector); - } - } - - return result; - } - /// /// Allows adding attributes to the client. /// @@ -184,11 +149,10 @@ internal void AddAttribute(string key, string? value) /// An instance of . private static OpenAIClientOptions GetOpenAIClientOptions(HttpClient? httpClient, Uri? endpoint) { - // As the options Endpoint is an init property and I can't set it afterwards, - // I need an if statement to create the options for a custom endpoint. - OpenAIClientOptions options = (endpoint is null) - ? new OpenAIClientOptions() { ApplicationId = HttpHeaderConstant.Values.UserAgent } - : new OpenAIClientOptions() { ApplicationId = HttpHeaderConstant.Values.UserAgent, Endpoint = endpoint }; + OpenAIClientOptions options = new() { + ApplicationId = HttpHeaderConstant.Values.UserAgent, + Endpoint = endpoint + }; options.AddPolicy(new AddHeaderRequestPolicy(HttpHeaderConstant.Names.SemanticKernelVersion, HttpHeaderConstant.Values.GetAssemblyVersion(typeof(ClientCore))), PipelinePosition.PerCall); diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs index 607f5012d829..2279d639c54e 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/Models/AddHeaderRequestPolicy.cs @@ -1,5 +1,9 @@ // Copyright (c) Microsoft. All rights reserved. +/* Phase 1 +Added from OpenAI v1 with adapted logic to the System.ClientModel abstraction +*/ + using System.ClientModel.Primitives; namespace Microsoft.SemanticKernel.Connectors.OpenAI; From 78290c599bfa6fba69c4f7e38c345f9933fe6791 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 17:09:05 +0100 Subject: [PATCH 09/16] ClientCore UTs --- .../Connectors.OpenAIV2.UnitTests.csproj | 3 +- .../Core/ClientCoreTests.cs | 89 +++++++++++++++++-- .../Utils/MockPipelineMessage.cs | 28 ++++++ .../Utils/MockPipelineRequest.cs | 71 +++++++++++++++ .../Utils/MockRequestHeaders.cs | 59 ++++++++++++ .../Connectors.OpenAIV2/Core/ClientCore.cs | 8 +- .../OpenAITextEmbbedingGenerationService.cs | 2 +- 7 files changed, 247 insertions(+), 13 deletions(-) create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj index ea7cd8028f12..c0af719e873e 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj @@ -29,7 +29,8 @@ - + + diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs index 087e63f737ef..9c190d280d23 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs @@ -2,8 +2,13 @@ using System; using System.ClientModel; +using System.ClientModel.Primitives; +using System.Linq; using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; using Microsoft.SemanticKernel.Connectors.OpenAI; +using Microsoft.SemanticKernel.Http; using OpenAI; using Xunit; @@ -14,10 +19,10 @@ public class ClientCoreTests public void ItCanBeInstantiated() { // Act - var client = new OpenAIClient(new ApiKeyCredential("key")); + var openAIClient = new OpenAIClient(new ApiKeyCredential("key")); - var clientCoreModelConstructor = new ClientCore("model1"); - var clientCoreOpenAIClientConstructor = new ClientCore("model1", client); + var clientCoreModelConstructor = new ClientCore("model1", "apiKey"); + var clientCoreOpenAIClientConstructor = new ClientCore("model1", openAIClient); // Assert Assert.NotNull(clientCoreModelConstructor); @@ -28,15 +33,14 @@ public void ItCanBeInstantiated() Assert.NotNull(clientCoreModelConstructor.Client); Assert.NotNull(clientCoreOpenAIClientConstructor.Client); - Assert.Equal(client, clientCoreOpenAIClientConstructor.Client); + Assert.Equal(openAIClient, clientCoreOpenAIClientConstructor.Client); } - [Theory] [InlineData(null, null)] [InlineData("http://localhost", null)] [InlineData(null, "http://localhost")] - [InlineData("http://localhost", "http://localhost")] + [InlineData("http://localhost-1", "http://localhost-2")] public void ItUsesEndpointAsExpected(string? clientBaseAddress, string? providedEndpoint) { // Arrange @@ -53,11 +57,82 @@ public void ItUsesEndpointAsExpected(string? clientBaseAddress, string? provided } // Act - var clientCore = new ClientCore("model", endpoint: endpoint, httpClient: client); + var clientCore = new ClientCore("model", "apiKey", endpoint: endpoint, httpClient: client); // Assert Assert.Equal(endpoint ?? client?.BaseAddress ?? new Uri("https://api.openai.com/v1"), clientCore.Endpoint); client?.Dispose(); } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task ItAddOrganizationHeaderWhenProvidedAsync(bool organizationIdProvided) + { + using HttpMessageHandlerStub handler = new(); + using HttpClient client = new(handler); + handler.ResponseToReturn = new HttpResponseMessage(System.Net.HttpStatusCode.OK); + + // Act + var clientCore = new ClientCore( + modelId: "model", + apiKey: "test", + organizationId: (organizationIdProvided) ? "organization" : null, + httpClient: client); + + var pipelineMessage = clientCore.Client.Pipeline.CreateMessage(); + pipelineMessage.Request.Method = "POST"; + pipelineMessage.Request.Uri = new Uri("http://localhost"); + pipelineMessage.Request.Content = BinaryContent.Create(new BinaryData("test")); + + // Assert + await clientCore.Client.Pipeline.SendAsync(pipelineMessage); + + if (organizationIdProvided) + { + Assert.True(handler.RequestHeaders!.Contains("OpenAI-Organization")); + Assert.Equal("organization", handler.RequestHeaders.GetValues("OpenAI-Organization").FirstOrDefault()); + } + else + { + Assert.False(handler.RequestHeaders!.Contains("OpenAI-Organization")); + } + } + + [Theory] + [InlineData(true, Skip = "Semantic Kernel header is not provided when using specific OpenAI client")] + [InlineData(false)] + public async Task ItAddSemanticKernelHeadersOnEachRequestAsync(bool useOpenAIClient) + { + using HttpMessageHandlerStub handler = new(); + using HttpClient client = new(handler); + handler.ResponseToReturn = new HttpResponseMessage(System.Net.HttpStatusCode.OK); + + // Act + var clientCore = (!useOpenAIClient) + ? new ClientCore(modelId: "model", apiKey: "test", httpClient: client) + : new ClientCore(modelId: "model", openAIClient: new OpenAIClient( + new ApiKeyCredential("test"), + new OpenAIClientOptions() + { + Transport = new HttpClientPipelineTransport(client), + RetryPolicy = new ClientRetryPolicy(maxRetries: 0), + NetworkTimeout = Timeout.InfiniteTimeSpan + })); + + var pipelineMessage = clientCore.Client.Pipeline.CreateMessage(); + pipelineMessage.Request.Method = "POST"; + pipelineMessage.Request.Uri = new Uri("http://localhost"); + pipelineMessage.Request.Content = BinaryContent.Create(new BinaryData("test")); + + // Assert + await clientCore.Client.Pipeline.SendAsync(pipelineMessage); + + Assert.True(handler.RequestHeaders!.Contains(HttpHeaderConstant.Names.SemanticKernelVersion)); + Assert.Equal(HttpHeaderConstant.Values.GetAssemblyVersion(typeof(ClientCore)), handler.RequestHeaders.GetValues(HttpHeaderConstant.Names.SemanticKernelVersion).FirstOrDefault()); + + Assert.True(handler.RequestHeaders.Contains("User-Agent")); + Assert.Contains(HttpHeaderConstant.Values.UserAgent, handler.RequestHeaders.GetValues("User-Agent").FirstOrDefault()); + } } diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs new file mode 100644 index 000000000000..f299b3340f10 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs @@ -0,0 +1,28 @@ +// Copyright (c) Microsoft. All rights reserved. + +/* Phase 01 +This class was imported and adapted from the System.ClientModel Unit Tests. +https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineMessage.cs +*/ + +using System.ClientModel.Primitives; +using System.Threading; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests; + +public class MockPipelineMessage : PipelineMessage +{ +#pragma warning disable CA2000 // Dispose objects before losing scope + public MockPipelineMessage() : this(new MockPipelineRequest()) +#pragma warning restore CA2000 // Dispose objects before losing scope + { + } + + public MockPipelineMessage(PipelineRequest request) : base(request) + { + this.NetworkTimeout ??= Timeout.InfiniteTimeSpan; + } + + public void SetResponse(PipelineResponse response) + => this.Response = response; +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs new file mode 100644 index 000000000000..facdaaf32c7f --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs @@ -0,0 +1,71 @@ +// Copyright (c) Microsoft. All rights reserved. + +/* Phase 01 +This class was imported and adapted from the System.ClientModel Unit Tests. +https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs +*/ + +using System; +using System.ClientModel; +using System.ClientModel.Primitives; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests; + +public class MockPipelineRequest : PipelineRequest +{ + private string _method; + private Uri? _uri; + private BinaryContent? _content; + private readonly PipelineRequestHeaders _headers; + + private bool _disposed; + + public MockPipelineRequest() + { + this._headers = new MockRequestHeaders(); + this._method = "GET"; + } + + protected override BinaryContent? ContentCore + { + get => this._content; + set => this._content = value; + } + + protected override PipelineRequestHeaders HeadersCore + => this._headers; + + protected override string MethodCore + { + get => this._method; + set => this._method = value; + } + + protected override Uri? UriCore + { + get => this._uri; + set => this._uri = value; + } + + public sealed override void Dispose() + { + this.Dispose(true); + + GC.SuppressFinalize(this); + } + + protected void Dispose(bool disposing) + { + if (disposing && !this._disposed) + { + var content = this._content; + if (content != null) + { + this._content = null; + content.Dispose(); + } + + this._disposed = true; + } + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs new file mode 100644 index 000000000000..77995b1d8235 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs @@ -0,0 +1,59 @@ +// Copyright (c) Microsoft. All rights reserved. + +/* Phase 01 +This class was imported and adapted from the System.ClientModel Unit Tests. +https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockRequestHeaders.cs +*/ + +using System; +using System.ClientModel.Primitives; +using System.Collections.Generic; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests; + +public class MockRequestHeaders : PipelineRequestHeaders +{ + private readonly Dictionary _headers; + + public MockRequestHeaders() + { + this._headers = new Dictionary(); + } + + public override void Add(string name, string value) + { + if (this._headers.ContainsKey(name)) + { + this._headers[name] += string.Concat(",", value); + } + else + { + this._headers[name] = value; + } + } + + public override IEnumerator> GetEnumerator() + { + throw new NotImplementedException(); + } + + public override bool Remove(string name) + { + return this._headers.Remove(name); + } + + public override void Set(string name, string value) + { + this._headers[name] = value; + } + + public override bool TryGetValue(string name, out string? value) + { + return this._headers.TryGetValue(name, out value); + } + + public override bool TryGetValues(string name, out IEnumerable? values) + { + throw new NotImplementedException(); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs index f68c8b2585b1..4911c2bec161 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs @@ -64,14 +64,14 @@ internal partial class ClientCore /// Model name. /// OpenAI API Key. /// OpenAI compatible API endpoint. - /// OpenAI Organization Id (usually optional). + /// OpenAI Organization Id (usually optional). /// Custom for HTTP requests. /// The to use for logging. If null, no logging will be performed. internal ClientCore( string modelId, string? apiKey = null, Uri? endpoint = null, - string? organization = null, + string? organizationId = null, HttpClient? httpClient = null, ILogger? logger = null) { @@ -89,9 +89,9 @@ internal ClientCore( } var options = GetOpenAIClientOptions(httpClient, this.Endpoint); - if (!string.IsNullOrWhiteSpace(organization)) + if (!string.IsNullOrWhiteSpace(organizationId)) { - options.AddPolicy(new AddHeaderRequestPolicy("OpenAI-Organization", organization!), PipelinePosition.PerCall); + options.AddPolicy(new AddHeaderRequestPolicy("OpenAI-Organization", organizationId!), PipelinePosition.PerCall); } this.Client = new OpenAIClient(apiKey ?? string.Empty, options); diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs index c304a2d2d564..3e2b8d8d1eed 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs @@ -42,7 +42,7 @@ public OpenAITextEmbeddingGenerationService( this._core = new( modelId: modelId, apiKey: apiKey, - organization: organization, + organizationId: organization, httpClient: httpClient, logger: loggerFactory?.CreateLogger(typeof(OpenAITextEmbeddingGenerationService))); From cb53ccb777e15af077ae77416c60daf9daecb114 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 17:10:09 +0100 Subject: [PATCH 10/16] Removing not used Utils --- .../Utils/MockPipelineMessage.cs | 28 -------- .../Utils/MockPipelineRequest.cs | 71 ------------------- .../Utils/MockRequestHeaders.cs | 59 --------------- 3 files changed, 158 deletions(-) delete mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs delete mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs delete mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs deleted file mode 100644 index f299b3340f10..000000000000 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineMessage.cs +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -/* Phase 01 -This class was imported and adapted from the System.ClientModel Unit Tests. -https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineMessage.cs -*/ - -using System.ClientModel.Primitives; -using System.Threading; - -namespace SemanticKernel.Connectors.OpenAI.UnitTests; - -public class MockPipelineMessage : PipelineMessage -{ -#pragma warning disable CA2000 // Dispose objects before losing scope - public MockPipelineMessage() : this(new MockPipelineRequest()) -#pragma warning restore CA2000 // Dispose objects before losing scope - { - } - - public MockPipelineMessage(PipelineRequest request) : base(request) - { - this.NetworkTimeout ??= Timeout.InfiniteTimeSpan; - } - - public void SetResponse(PipelineResponse response) - => this.Response = response; -} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs deleted file mode 100644 index facdaaf32c7f..000000000000 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineRequest.cs +++ /dev/null @@ -1,71 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -/* Phase 01 -This class was imported and adapted from the System.ClientModel Unit Tests. -https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockPipelineResponse.cs -*/ - -using System; -using System.ClientModel; -using System.ClientModel.Primitives; - -namespace SemanticKernel.Connectors.OpenAI.UnitTests; - -public class MockPipelineRequest : PipelineRequest -{ - private string _method; - private Uri? _uri; - private BinaryContent? _content; - private readonly PipelineRequestHeaders _headers; - - private bool _disposed; - - public MockPipelineRequest() - { - this._headers = new MockRequestHeaders(); - this._method = "GET"; - } - - protected override BinaryContent? ContentCore - { - get => this._content; - set => this._content = value; - } - - protected override PipelineRequestHeaders HeadersCore - => this._headers; - - protected override string MethodCore - { - get => this._method; - set => this._method = value; - } - - protected override Uri? UriCore - { - get => this._uri; - set => this._uri = value; - } - - public sealed override void Dispose() - { - this.Dispose(true); - - GC.SuppressFinalize(this); - } - - protected void Dispose(bool disposing) - { - if (disposing && !this._disposed) - { - var content = this._content; - if (content != null) - { - this._content = null; - content.Dispose(); - } - - this._disposed = true; - } - } -} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs deleted file mode 100644 index 77995b1d8235..000000000000 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockRequestHeaders.cs +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -/* Phase 01 -This class was imported and adapted from the System.ClientModel Unit Tests. -https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/System.ClientModel/tests/TestFramework/Mocks/MockRequestHeaders.cs -*/ - -using System; -using System.ClientModel.Primitives; -using System.Collections.Generic; - -namespace SemanticKernel.Connectors.OpenAI.UnitTests; - -public class MockRequestHeaders : PipelineRequestHeaders -{ - private readonly Dictionary _headers; - - public MockRequestHeaders() - { - this._headers = new Dictionary(); - } - - public override void Add(string name, string value) - { - if (this._headers.ContainsKey(name)) - { - this._headers[name] += string.Concat(",", value); - } - else - { - this._headers[name] = value; - } - } - - public override IEnumerator> GetEnumerator() - { - throw new NotImplementedException(); - } - - public override bool Remove(string name) - { - return this._headers.Remove(name); - } - - public override void Set(string name, string value) - { - this._headers[name] = value; - } - - public override bool TryGetValue(string name, out string? value) - { - return this._headers.TryGetValue(name, out value); - } - - public override bool TryGetValues(string name, out IEnumerable? values) - { - throw new NotImplementedException(); - } -} From 23b71a1c738d9242dde0b08a7dda7dadd688a25c Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 17:20:05 +0100 Subject: [PATCH 11/16] Update UT --- .../Core/ClientCoreTests.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs index 9c190d280d23..9e450c2d18e4 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs @@ -7,8 +7,11 @@ using System.Net.Http; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.SemanticKernel.Connectors.OpenAI; using Microsoft.SemanticKernel.Http; +using Moq; using OpenAI; using Xunit; @@ -16,13 +19,14 @@ namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core; public class ClientCoreTests { [Fact] - public void ItCanBeInstantiated() + public void ItCanBeInstantiatedAndPropertiesSetAsExpected() { // Act + var logger = new Mock>().Object; var openAIClient = new OpenAIClient(new ApiKeyCredential("key")); var clientCoreModelConstructor = new ClientCore("model1", "apiKey"); - var clientCoreOpenAIClientConstructor = new ClientCore("model1", openAIClient); + var clientCoreOpenAIClientConstructor = new ClientCore("model1", openAIClient, logger: logger); // Assert Assert.NotNull(clientCoreModelConstructor); @@ -34,6 +38,8 @@ public void ItCanBeInstantiated() Assert.NotNull(clientCoreModelConstructor.Client); Assert.NotNull(clientCoreOpenAIClientConstructor.Client); Assert.Equal(openAIClient, clientCoreOpenAIClientConstructor.Client); + Assert.Equal(NullLogger.Instance, clientCoreModelConstructor.Logger); + Assert.Equal(logger, clientCoreOpenAIClientConstructor.Logger); } [Theory] From 276340249cfde01c1f8910c06ddadc0bcbc22422 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 17:58:18 +0100 Subject: [PATCH 12/16] Adding UTs --- .../Core/ClientCore.EmbeddingsTests.cs | 24 +++++++ .../Core/ClientCoreTests.cs | 70 ++++++++++++++++--- .../Connectors.OpenAIV2/Core/ClientCore.cs | 5 +- 3 files changed, 86 insertions(+), 13 deletions(-) create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs new file mode 100644 index 000000000000..c9ac624d0af2 --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Threading; +using System.Threading.Tasks; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using Xunit; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core; + +public partial class ClientCoreTests +{ + [Fact] + public async Task ItGetEmbeddingsAsyncReturnsEmptyWhenProvidedDataIsEmpty() + { + // Arrange + var clientCore = new ClientCore("model", "apikey"); + + // Act + var result = await clientCore.GetEmbeddingsAsync([], null, null, CancellationToken.None); + + // Assert + Assert.Empty(result); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs index 9e450c2d18e4..cd2816f53778 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs @@ -16,7 +16,7 @@ using Xunit; namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core; -public class ClientCoreTests +public partial class ClientCoreTests { [Fact] public void ItCanBeInstantiatedAndPropertiesSetAsExpected() @@ -106,19 +106,46 @@ public async Task ItAddOrganizationHeaderWhenProvidedAsync(bool organizationIdPr } } - [Theory] - [InlineData(true, Skip = "Semantic Kernel header is not provided when using specific OpenAI client")] + [Fact] + // [InlineData(true, + // Skip = "Semantic Kernel header is not provided when using specific + // OpenAI client because once the client is created we can't add policies using plugic APIs")] [InlineData(false)] - public async Task ItAddSemanticKernelHeadersOnEachRequestAsync(bool useOpenAIClient) + public async Task ItAddSemanticKernelHeadersOnEachRequestAsync() + { + using HttpMessageHandlerStub handler = new(); + using HttpClient client = new(handler); + handler.ResponseToReturn = new HttpResponseMessage(System.Net.HttpStatusCode.OK); + + // Act + var clientCore = new ClientCore(modelId: "model", apiKey: "test", httpClient: client); + + var pipelineMessage = clientCore.Client.Pipeline.CreateMessage(); + pipelineMessage.Request.Method = "POST"; + pipelineMessage.Request.Uri = new Uri("http://localhost"); + pipelineMessage.Request.Content = BinaryContent.Create(new BinaryData("test")); + + // Assert + await clientCore.Client.Pipeline.SendAsync(pipelineMessage); + + Assert.True(handler.RequestHeaders!.Contains(HttpHeaderConstant.Names.SemanticKernelVersion)); + Assert.Equal(HttpHeaderConstant.Values.GetAssemblyVersion(typeof(ClientCore)), handler.RequestHeaders.GetValues(HttpHeaderConstant.Names.SemanticKernelVersion).FirstOrDefault()); + + Assert.True(handler.RequestHeaders.Contains("User-Agent")); + Assert.Contains(HttpHeaderConstant.Values.UserAgent, handler.RequestHeaders.GetValues("User-Agent").FirstOrDefault()); + } + + [Fact] + public async Task ItDoNotAddSemanticKernelHeadersWhenOpenAIClientIsProvidedAsync() { using HttpMessageHandlerStub handler = new(); using HttpClient client = new(handler); handler.ResponseToReturn = new HttpResponseMessage(System.Net.HttpStatusCode.OK); // Act - var clientCore = (!useOpenAIClient) - ? new ClientCore(modelId: "model", apiKey: "test", httpClient: client) - : new ClientCore(modelId: "model", openAIClient: new OpenAIClient( + var clientCore = new ClientCore( + modelId: "model", + openAIClient: new OpenAIClient( new ApiKeyCredential("test"), new OpenAIClientOptions() { @@ -135,10 +162,31 @@ public async Task ItAddSemanticKernelHeadersOnEachRequestAsync(bool useOpenAICli // Assert await clientCore.Client.Pipeline.SendAsync(pipelineMessage); - Assert.True(handler.RequestHeaders!.Contains(HttpHeaderConstant.Names.SemanticKernelVersion)); - Assert.Equal(HttpHeaderConstant.Values.GetAssemblyVersion(typeof(ClientCore)), handler.RequestHeaders.GetValues(HttpHeaderConstant.Names.SemanticKernelVersion).FirstOrDefault()); + Assert.False(handler.RequestHeaders!.Contains(HttpHeaderConstant.Names.SemanticKernelVersion)); + Assert.DoesNotContain(HttpHeaderConstant.Values.UserAgent, handler.RequestHeaders.GetValues("User-Agent").FirstOrDefault()); + } - Assert.True(handler.RequestHeaders.Contains("User-Agent")); - Assert.Contains(HttpHeaderConstant.Values.UserAgent, handler.RequestHeaders.GetValues("User-Agent").FirstOrDefault()); + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("value")] + public void ItAddAttributesButDoesNothingIfNullOrEmpty(string? value) + { + // Arrange + var clientCore = new ClientCore("model", "apikey"); + // Act + + clientCore.AddAttribute("key", value); + + // Assert + if (string.IsNullOrEmpty(value)) + { + Assert.False(clientCore.Attributes.ContainsKey("key")); + } + else + { + Assert.True(clientCore.Attributes.ContainsKey("key")); + Assert.Equal(value, clientCore.Attributes["key"]); + } } } diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs index 4911c2bec161..14089a5f7548 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs @@ -98,7 +98,7 @@ internal ClientCore( } /// - /// Initializes a new instance of the class using the specified OpenAIClient. + /// Initializes a new instance of the class using the specified OpenAIClient. /// Note: instances created this way might not have the default diagnostics settings, /// it's up to the caller to configure the client. /// @@ -149,7 +149,8 @@ internal void AddAttribute(string key, string? value) /// An instance of . private static OpenAIClientOptions GetOpenAIClientOptions(HttpClient? httpClient, Uri? endpoint) { - OpenAIClientOptions options = new() { + OpenAIClientOptions options = new() + { ApplicationId = HttpHeaderConstant.Values.UserAgent, Endpoint = endpoint }; From 6789067b1c21be55703873ffa2d8384969da9e80 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 19:15:22 +0100 Subject: [PATCH 13/16] All Unit Tests Added --- .../Connectors.OpenAIV2.UnitTests.csproj | 9 +- .../Core/ClientCore.EmbeddingsTests.cs | 24 ------ .../Core/ClientCoreTests.cs | 4 - .../ClientResultExceptionExtensionsTests.cs | 2 - ...enAITextEmbeddingGenerationServiceTests.cs | 86 +++++++++++++++++++ .../text-embeddings-multiple-response.txt | 20 +++++ .../TestData/text-embeddings-response.txt | 15 ++++ 7 files changed, 128 insertions(+), 32 deletions(-) delete mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Services/OpenAITextEmbeddingGenerationServiceTests.cs create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-multiple-response.txt create mode 100644 dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-response.txt diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj index c0af719e873e..0d89e02beb21 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Connectors.OpenAIV2.UnitTests.csproj @@ -7,7 +7,7 @@ true enable false - $(NoWarn);SKEXP0001;SKEXP0070;CS1591;IDE1006;RCS1261;CA1031;CA1308;CA1861;CA2007;CA2234;VSTHRD111 + $(NoWarn);SKEXP0001;SKEXP0070;SKEXP0010;CS1591;IDE1006;RCS1261;CA1031;CA1308;CA1861;CA2007;CA2234;VSTHRD111 @@ -38,7 +38,12 @@ - + + Always + + + Always + diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs deleted file mode 100644 index c9ac624d0af2..000000000000 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCore.EmbeddingsTests.cs +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System.Threading; -using System.Threading.Tasks; -using Microsoft.SemanticKernel.Connectors.OpenAI; -using Xunit; - -namespace SemanticKernel.Connectors.OpenAI.UnitTests.Core; - -public partial class ClientCoreTests -{ - [Fact] - public async Task ItGetEmbeddingsAsyncReturnsEmptyWhenProvidedDataIsEmpty() - { - // Arrange - var clientCore = new ClientCore("model", "apikey"); - - // Act - var result = await clientCore.GetEmbeddingsAsync([], null, null, CancellationToken.None); - - // Assert - Assert.Empty(result); - } -} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs index cd2816f53778..a3415663459a 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Core/ClientCoreTests.cs @@ -107,10 +107,6 @@ public async Task ItAddOrganizationHeaderWhenProvidedAsync(bool organizationIdPr } [Fact] - // [InlineData(true, - // Skip = "Semantic Kernel header is not provided when using specific - // OpenAI client because once the client is created we can't add policies using plugic APIs")] - [InlineData(false)] public async Task ItAddSemanticKernelHeadersOnEachRequestAsync() { using HttpMessageHandlerStub handler = new(); diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs index 6bb142519760..0b95f904d893 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Extensions/ClientResultExceptionExtensionsTests.cs @@ -1,9 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. -using System; using System.ClientModel; using System.ClientModel.Primitives; -using System.Net.Http; using Microsoft.SemanticKernel.Connectors.OpenAI; using Xunit; diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Services/OpenAITextEmbeddingGenerationServiceTests.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Services/OpenAITextEmbeddingGenerationServiceTests.cs new file mode 100644 index 000000000000..25cdc4ec61aa --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Services/OpenAITextEmbeddingGenerationServiceTests.cs @@ -0,0 +1,86 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.ClientModel; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.SemanticKernel; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using Microsoft.SemanticKernel.Services; +using OpenAI; +using Xunit; + +namespace SemanticKernel.Connectors.OpenAI.UnitTests.Services; +public class OpenAITextEmbeddingGenerationServiceTests +{ + [Fact] + public void ItCanBeInstantiatedAndPropertiesSetAsExpected() + { + // Arrange + var sut = new OpenAITextEmbeddingGenerationService("model", "apiKey", dimensions: 2); + var sutWithOpenAIClient = new OpenAITextEmbeddingGenerationService("model", new OpenAIClient(new ApiKeyCredential("apiKey")), dimensions: 2); + + // Assert + Assert.NotNull(sut); + Assert.NotNull(sutWithOpenAIClient); + Assert.Equal("model", sut.Attributes[AIServiceExtensions.ModelIdKey]); + Assert.Equal("model", sutWithOpenAIClient.Attributes[AIServiceExtensions.ModelIdKey]); + } + + [Fact] + public async Task ItGetEmbeddingsAsyncReturnsEmptyWhenProvidedDataIsEmpty() + { + // Arrange + var sut = new OpenAITextEmbeddingGenerationService("model", "apikey"); + + // Act + var result = await sut.GenerateEmbeddingsAsync([], null, CancellationToken.None); + + // Assert + Assert.Empty(result); + } + + [Fact] + public async Task IGetEmbeddingsAsyncReturnsEmptyWhenProvidedDataIsWhitespace() + { + using HttpMessageHandlerStub handler = new() + { + ResponseToReturn = new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(File.ReadAllText("./TestData/text-embeddings-response.txt")) + } + }; + using HttpClient client = new(handler); + + // Arrange + var sut = new OpenAITextEmbeddingGenerationService("model", "apikey", httpClient: client); + + // Act + var result = await sut.GenerateEmbeddingsAsync(["test"], null, CancellationToken.None); + + // Assert + Assert.Single(result); + Assert.Equal(4, result[0].Length); + } + + [Fact] + public async Task ItThrowsIfNumberOfResultsDiffersFromInputsAsync() + { + using HttpMessageHandlerStub handler = new() + { + ResponseToReturn = new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent(File.ReadAllText("./TestData/text-embeddings-multiple-response.txt")) + } + }; + using HttpClient client = new(handler); + + // Arrange + var sut = new OpenAITextEmbeddingGenerationService("model", "apikey", httpClient: client); + + // Act & Assert + await Assert.ThrowsAsync(async () => await sut.GenerateEmbeddingsAsync(["test"], null, CancellationToken.None)); + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-multiple-response.txt b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-multiple-response.txt new file mode 100644 index 000000000000..46a9581cf0cc --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-multiple-response.txt @@ -0,0 +1,20 @@ +{ + "object": "list", + "data": [ + { + "object": "embedding", + "index": 0, + "embedding": "zcyMP83MDEAzM1NAzcyMQA==" + }, + { + "object": "embedding", + "index": 1, + "embedding": "zcyMP83MDEAzM1NAzcyMQA==" + } + ], + "model": "text-embedding-ada-002", + "usage": { + "prompt_tokens": 7, + "total_tokens": 7 + } +} diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-response.txt b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-response.txt new file mode 100644 index 000000000000..c715b851b78c --- /dev/null +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/TestData/text-embeddings-response.txt @@ -0,0 +1,15 @@ +{ + "object": "list", + "data": [ + { + "object": "embedding", + "index": 0, + "embedding": "zcyMP83MDEAzM1NAzcyMQA==" + } + ], + "model": "text-embedding-ada-002", + "usage": { + "prompt_tokens": 7, + "total_tokens": 7 + } +} From 36c4b3a85d58e39431ad0b5f790a37a97b8cf44d Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 19:43:56 +0100 Subject: [PATCH 14/16] Integration Tests added and passing --- .../Utils/MockPipelineResponse.cs | 2 +- dotnet/src/IntegrationTestsV2/.editorconfig | 6 ++ .../OpenAI/OpenAITextEmbeddingTests.cs | 63 +++++++++++++++++++ .../IntegrationTestsV2.csproj | 8 +-- .../TestSettings/AzureOpenAIConfiguration.cs | 26 ++++++++ .../TestSettings/OpenAIConfiguration.cs | 15 +++++ 6 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 dotnet/src/IntegrationTestsV2/.editorconfig create mode 100644 dotnet/src/IntegrationTestsV2/Connectors/OpenAI/OpenAITextEmbeddingTests.cs create mode 100644 dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs create mode 100644 dotnet/src/IntegrationTestsV2/TestSettings/OpenAIConfiguration.cs diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs index 1070620f4b95..6fe18b9c1684 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2.UnitTests/Utils/MockPipelineResponse.cs @@ -68,7 +68,7 @@ public override BinaryData Content if (this.ContentStream is not MemoryStream memoryContent) { - throw new InvalidOperationException($"The response is not buffered."); + throw new InvalidOperationException("The response is not buffered."); } if (memoryContent.TryGetBuffer(out ArraySegment segment)) diff --git a/dotnet/src/IntegrationTestsV2/.editorconfig b/dotnet/src/IntegrationTestsV2/.editorconfig new file mode 100644 index 000000000000..394eef685f21 --- /dev/null +++ b/dotnet/src/IntegrationTestsV2/.editorconfig @@ -0,0 +1,6 @@ +# Suppressing errors for Test projects under dotnet folder +[*.cs] +dotnet_diagnostic.CA2007.severity = none # Do not directly await a Task +dotnet_diagnostic.VSTHRD111.severity = none # Use .ConfigureAwait(bool) is hidden by default, set to none to prevent IDE from changing on autosave +dotnet_diagnostic.CS1591.severity = none # Missing XML comment for publicly visible type or member +dotnet_diagnostic.IDE1006.severity = warning # Naming rule violations diff --git a/dotnet/src/IntegrationTestsV2/Connectors/OpenAI/OpenAITextEmbeddingTests.cs b/dotnet/src/IntegrationTestsV2/Connectors/OpenAI/OpenAITextEmbeddingTests.cs new file mode 100644 index 000000000000..6eca1909a546 --- /dev/null +++ b/dotnet/src/IntegrationTestsV2/Connectors/OpenAI/OpenAITextEmbeddingTests.cs @@ -0,0 +1,63 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Threading.Tasks; +using Microsoft.Extensions.Configuration; +using Microsoft.SemanticKernel.Connectors.OpenAI; +using Microsoft.SemanticKernel.Embeddings; +using SemanticKernel.IntegrationTests.TestSettings; +using Xunit; + +namespace SemanticKernel.IntegrationTests.Connectors.OpenAI; + +public sealed class OpenAITextEmbeddingTests +{ + private const int AdaVectorLength = 1536; + private readonly IConfigurationRoot _configuration = new ConfigurationBuilder() + .AddJsonFile(path: "testsettings.json", optional: true, reloadOnChange: true) + .AddJsonFile(path: "testsettings.development.json", optional: true, reloadOnChange: true) + .AddEnvironmentVariables() + .AddUserSecrets() + .Build(); + + [Theory]//(Skip = "OpenAI will often throttle requests. This test is for manual verification.")] + [InlineData("test sentence")] + public async Task OpenAITestAsync(string testInputString) + { + // Arrange + OpenAIConfiguration? openAIConfiguration = this._configuration.GetSection("OpenAIEmbeddings").Get(); + Assert.NotNull(openAIConfiguration); + + var embeddingGenerator = new OpenAITextEmbeddingGenerationService(openAIConfiguration.ModelId, openAIConfiguration.ApiKey); + + // Act + var singleResult = await embeddingGenerator.GenerateEmbeddingAsync(testInputString); + var batchResult = await embeddingGenerator.GenerateEmbeddingsAsync([testInputString, testInputString, testInputString]); + + // Assert + Assert.Equal(AdaVectorLength, singleResult.Length); + Assert.Equal(3, batchResult.Count); + } + + [Theory]//(Skip = "OpenAI will often throttle requests. This test is for manual verification.")] + [InlineData(null, 3072)] + [InlineData(1024, 1024)] + public async Task OpenAIWithDimensionsAsync(int? dimensions, int expectedVectorLength) + { + // Arrange + const string TestInputString = "test sentence"; + + OpenAIConfiguration? openAIConfiguration = this._configuration.GetSection("OpenAIEmbeddings").Get(); + Assert.NotNull(openAIConfiguration); + + var embeddingGenerator = new OpenAITextEmbeddingGenerationService( + "text-embedding-3-large", + openAIConfiguration.ApiKey, + dimensions: dimensions); + + // Act + var result = await embeddingGenerator.GenerateEmbeddingAsync(TestInputString); + + // Assert + Assert.Equal(expectedVectorLength, result.Length); + } +} diff --git a/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj b/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj index cbfbfe9e4df3..f3c704a27307 100644 --- a/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj +++ b/dotnet/src/IntegrationTestsV2/IntegrationTestsV2.csproj @@ -1,7 +1,7 @@ - + IntegrationTests - SemanticKernel.IntegrationTests + SemanticKernel.IntegrationTestsV2 net8.0 true false @@ -16,7 +16,7 @@ - + @@ -44,7 +44,6 @@ - @@ -64,4 +63,5 @@ Always + \ No newline at end of file diff --git a/dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs b/dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs new file mode 100644 index 000000000000..e530110f9322 --- /dev/null +++ b/dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs @@ -0,0 +1,26 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Diagnostics.CodeAnalysis; + +namespace SemanticKernel.IntegrationTests.TestSettings; + +[SuppressMessage("Performance", "CA1812:Internal class that is apparently never instantiated", + Justification = "Configuration classes are instantiated through IConfiguration.")] +internal sealed class AzureOpenAIConfiguration(string serviceId, string deploymentName, string endpoint, string apiKey, string? chatDeploymentName = null, string? modelId = null, string? chatModelId = null, string? embeddingModelId = null) +{ + public string ServiceId { get; set; } = serviceId; + + public string DeploymentName { get; set; } = deploymentName; + + public string ModelId { get; set; } = modelId ?? deploymentName; + + public string? ChatDeploymentName { get; set; } = chatDeploymentName ?? deploymentName; + + public string ChatModelId { get; set; } = chatModelId ?? deploymentName; + + public string EmbeddingModelId { get; set; } = embeddingModelId ?? "text-embedding-ada-002"; + + public string Endpoint { get; set; } = endpoint; + + public string ApiKey { get; set; } = apiKey; +} diff --git a/dotnet/src/IntegrationTestsV2/TestSettings/OpenAIConfiguration.cs b/dotnet/src/IntegrationTestsV2/TestSettings/OpenAIConfiguration.cs new file mode 100644 index 000000000000..cb3884e3bdfc --- /dev/null +++ b/dotnet/src/IntegrationTestsV2/TestSettings/OpenAIConfiguration.cs @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Diagnostics.CodeAnalysis; + +namespace SemanticKernel.IntegrationTests.TestSettings; + +[SuppressMessage("Performance", "CA1812:Internal class that is apparently never instantiated", + Justification = "Configuration classes are instantiated through IConfiguration.")] +internal sealed class OpenAIConfiguration(string serviceId, string modelId, string apiKey, string? chatModelId = null) +{ + public string ServiceId { get; set; } = serviceId; + public string ModelId { get; set; } = modelId; + public string? ChatModelId { get; set; } = chatModelId; + public string ApiKey { get; set; } = apiKey; +} From 1c559a74a39014cb4f44f1057d643a31f5913f60 Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Fri, 21 Jun 2024 19:56:35 +0100 Subject: [PATCH 15/16] Remove Azure related file --- .../TestSettings/AzureOpenAIConfiguration.cs | 26 ------------------- 1 file changed, 26 deletions(-) delete mode 100644 dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs diff --git a/dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs b/dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs deleted file mode 100644 index e530110f9322..000000000000 --- a/dotnet/src/IntegrationTestsV2/TestSettings/AzureOpenAIConfiguration.cs +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System.Diagnostics.CodeAnalysis; - -namespace SemanticKernel.IntegrationTests.TestSettings; - -[SuppressMessage("Performance", "CA1812:Internal class that is apparently never instantiated", - Justification = "Configuration classes are instantiated through IConfiguration.")] -internal sealed class AzureOpenAIConfiguration(string serviceId, string deploymentName, string endpoint, string apiKey, string? chatDeploymentName = null, string? modelId = null, string? chatModelId = null, string? embeddingModelId = null) -{ - public string ServiceId { get; set; } = serviceId; - - public string DeploymentName { get; set; } = deploymentName; - - public string ModelId { get; set; } = modelId ?? deploymentName; - - public string? ChatDeploymentName { get; set; } = chatDeploymentName ?? deploymentName; - - public string ChatModelId { get; set; } = chatModelId ?? deploymentName; - - public string EmbeddingModelId { get; set; } = embeddingModelId ?? "text-embedding-ada-002"; - - public string Endpoint { get; set; } = endpoint; - - public string ApiKey { get; set; } = apiKey; -} From 34c8d7ca81033e7fd8c1258fc5203c1db52fd8ef Mon Sep 17 00:00:00 2001 From: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com> Date: Sat, 22 Jun 2024 14:16:15 +0100 Subject: [PATCH 16/16] Address PR comments --- .../src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs | 8 ++++---- .../Services/OpenAITextEmbbedingGenerationService.cs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs index 14089a5f7548..12ca2f3d92fe 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs @@ -34,19 +34,19 @@ internal partial class ClientCore private const string OpenAIV1Endpoint = "https://api.openai.com/v1"; /// - /// Model Id + /// Identifier of the default model to use /// - internal string ModelId { get; set; } = string.Empty; + internal string ModelId { get; init; } = string.Empty; /// /// Non-default endpoint for OpenAI API. /// - internal Uri? Endpoint { get; set; } = null; + internal Uri? Endpoint { get; init; } /// /// Logger instance /// - internal ILogger Logger { get; set; } + internal ILogger Logger { get; init; } /// /// OpenAI / Azure OpenAI Client diff --git a/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs b/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs index 3e2b8d8d1eed..49915031b7fc 100644 --- a/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs +++ b/dotnet/src/Connectors/Connectors.OpenAIV2/Services/OpenAITextEmbbedingGenerationService.cs @@ -14,7 +14,7 @@ namespace Microsoft.SemanticKernel.Connectors.OpenAI; /// -/// OpenAI text embedding service. +/// OpenAI implementation of /// [Experimental("SKEXP0010")] public sealed class OpenAITextEmbeddingGenerationService : ITextEmbeddingGenerationService @@ -23,7 +23,7 @@ public sealed class OpenAITextEmbeddingGenerationService : ITextEmbeddingGenerat private readonly int? _dimensions; /// - /// Create an instance of the OpenAI text embedding connector + /// Create an instance of /// /// Model name /// OpenAI API Key