.Net: Removed Azure and OpenAI text generation services #4246

dmytrostruk · 2023-12-13T21:05:30Z

Motivation and Context

Resolves: #3154

Removed Azure and OpenAI text generation services in favor of chat completion services.

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

dotnet/src/IntegrationTests/Connectors/OpenAI/OpenAICompletionTests.cs

This reverts commit d974885.

markwallace-microsoft

Blocking until I've had a chance to review

RogerBarreto

Suggestion

RogerBarreto · 2023-12-14T10:00:43Z

dotnet/src/IntegrationTests/Connectors/OpenAI/OpenAICompletionTests.cs

@@ -435,7 +438,7 @@ public async Task MultipleServiceLoadPromptConfigTestAsync()
            @"{
                ""name"": ""FishMarket2"",
                ""execution_settings"": {
-                    ""azure-open-ai"": {
+                    ""azure-text-davinci-003"": {


Do we need this?

RogerBarreto · 2023-12-14T10:01:39Z

dotnet/src/IntegrationTests/PromptTests.cs

@@ -101,6 +101,7 @@ private void ConfigureAzureOpenAI(IKernelBuilder kernelBuilder)

        kernelBuilder.AddAzureOpenAIChatCompletion(
            deploymentName: azureOpenAIConfiguration.DeploymentName,
+            modelId: azureOpenAIConfiguration.ModelId,


Suggested change

modelId: azureOpenAIConfiguration.ModelId,

modelId: azureOpenAIConfiguration.ChatModelId,

RogerBarreto · 2023-12-14T10:02:20Z

dotnet/src/IntegrationTests/TestSettings/AzureOpenAIConfiguration.cs

@@ -12,16 +12,25 @@ internal sealed class AzureOpenAIConfiguration

    public string DeploymentName { get; set; }

+    public string ModelId { get; set; }


Suggested change

public string ModelId { get; set; }

RogerBarreto · 2023-12-14T10:02:33Z

dotnet/src/IntegrationTests/TestSettings/AzureOpenAIConfiguration.cs

    {
        this.ServiceId = serviceId;
        this.DeploymentName = deploymentName;
+        this.ModelId = modelId ?? deploymentName;


Suggested change

this.ModelId = modelId ?? deploymentName;

RogerBarreto · 2023-12-14T10:02:46Z

dotnet/src/IntegrationTests/TestSettings/OpenAIConfiguration.cs

@@ -10,12 +10,14 @@ internal sealed class OpenAIConfiguration
 {
    public string ServiceId { get; set; }
    public string ModelId { get; set; }


Suggested change

public string ModelId { get; set; }

@RogerBarreto this change won't work because there is another settings for embeddings which uses ModelId and we use the same C# class for all the settings:

"OpenAIEmbeddings": { "ServiceId": "text-embedding-ada-002", "ModelId": "text-embedding-ada-002", "ApiKey": "" },

That's why I removed Chat prefix initially, reverted it back and proposed to keep it as it is for now as alternative, because settings and testing infrastructure need to be revisited completely.

RogerBarreto · 2023-12-14T10:02:53Z

dotnet/src/IntegrationTests/TestSettings/OpenAIConfiguration.cs

    public string ApiKey { get; set; }

-    public OpenAIConfiguration(string serviceId, string modelId, string apiKey)
+    public OpenAIConfiguration(string serviceId, string modelId, string apiKey, string chatModelId)
    {
        this.ServiceId = serviceId;
        this.ModelId = modelId;


Suggested change

this.ModelId = modelId;

RogerBarreto · 2023-12-14T10:03:06Z

dotnet/src/IntegrationTests/testsettings.json

@@ -1,12 +1,14 @@
 {
  "OpenAI": {
    "ServiceId": "open-ai",
-    "ModelId": "gpt-3.5-turbo",
+    "ModelId": "text-davinci-003",


Suggested change

"ModelId": "text-davinci-003",

RogerBarreto · 2023-12-14T10:03:13Z

dotnet/src/IntegrationTests/testsettings.json

    "ApiKey": ""
  },
  "AzureOpenAI": {
    "ServiceId": "azure-open-ai",
-    "DeploymentName": "gpt-35-turbo",
+    "DeploymentName": "text-davinci-003",


Suggested change

"DeploymentName": "text-davinci-003",

dmytrostruk · 2023-12-14T10:54:52Z

Closing this PR as text generation services can work with gpt-3.5-turbo-instruct model which is not going to be deprecated.

stephentoub · 2023-12-14T11:36:41Z

Closing this PR as text generation services can work with gpt-3.5-turbo-instruct model which is not going to be deprecated.

The ITextGenerationService implementation in OpenAIChatCompletionService doesn't work with it? Isn't it identical to the implementation used by OpenAITextGenerationService?

dmytrostruk · 2023-12-14T12:02:33Z

The ITextGenerationService implementation in OpenAIChatCompletionService doesn't work with it? Isn't it identical to the implementation used by OpenAITextGenerationService?

When you use kernel with AddOpenAIChatCompletion extention method and use gpt-3.5-turbo-instruct model - it will throw following exception:

Microsoft.SemanticKernel.Http.HttpOperationException: 'This is not a chat model and thus not supported in the v1/chat/completions endpoint. Did you mean to use v1/completions?
Status: 404 (Not Found)

That's because OpenAIChatCompletionService implements both IChatCompletionService and ITextGenerationService and we check for chat interface first in our logic:

semantic-kernel/dotnet/src/SemanticKernel.Core/Functions/KernelFunctionFromPrompt.cs

Lines 145 to 157 in d3773b2

    
           if (aiService is IChatCompletionService chatCompletion) 
        
           { 
        
               var chatContent = await chatCompletion.GetChatMessageContentAsync(renderedPrompt, executionSettings, kernel, cancellationToken).ConfigureAwait(false); 
        
               this.CaptureUsageDetails(chatContent.ModelId, chatContent.Metadata, this._logger); 
        
               return new FunctionResult(this, chatContent, kernel.Culture, chatContent.Metadata); 
        
           } 
        
           if (aiService is ITextGenerationService textGeneration) 
        
           { 
        
               var textContent = await textGeneration.GetTextContentWithDefaultParserAsync(renderedPrompt, executionSettings, kernel, cancellationToken).ConfigureAwait(false); 
        
               this.CaptureUsageDetails(textContent.ModelId, textContent.Metadata, this._logger); 
        
               return new FunctionResult(this, textContent, kernel.Culture, textContent.Metadata); 
        
           }

So, in order to use gpt-3.5-turbo-instruct, the service should implement just ITextGenerationService, as it is in case of OpenAITextGenerationService and usage of AddOpenAITextGeneration extension method.

But I think ideally OpenAIChatCompletionService should implement just IChatCompletionService and OpenAITextGenerationService should implement just ITextGenerationService for clear differentiation. Users shouldn't pass text generation AI model to chat completion service class or extension method.

dmytrostruk · 2023-12-14T12:12:22Z

The reason why OpenAIChatCompletionService implements both IChatCompletionService and ITextGenerationService interfaces is because previously kernel and functions worked only with ITextGenerationService interface and it was a limitation. Now, since we have this differentiation between interfaces inside a core logic, I don't think that OpenAIChatCompletionService still needs to implement ITextGenerationService.

stephentoub · 2023-12-14T15:52:28Z

I see, thanks. Yeah, that's a fairly unfortunate combination, but I get it.

Removed Azure and OpenAI text generation models

9a93331

dmytrostruk self-assigned this Dec 13, 2023

dmytrostruk requested a review from a team as a code owner December 13, 2023 21:05

shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Dec 13, 2023

dmytrostruk added the PR: ready for review All feedback addressed, ready for reviews label Dec 13, 2023

RogerBarreto approved these changes Dec 13, 2023

View reviewed changes

SergeyMenshykh approved these changes Dec 14, 2023

View reviewed changes

dotnet/src/IntegrationTests/Connectors/OpenAI/OpenAICompletionTests.cs Outdated Show resolved Hide resolved

dmytrostruk added 3 commits December 14, 2023 09:06

Small fix

75e2ff0

Merge branch 'main' into remove-open-ai-text-generation

ca617eb

Fixed tests

bbf8eb7

RogerBarreto approved these changes Dec 14, 2023

View reviewed changes

dmytrostruk added this pull request to the merge queue Dec 14, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 14, 2023

dmytrostruk added 3 commits December 14, 2023 09:40

Small refactoring

d974885

Revert "Small refactoring"

d0bb6c9

This reverts commit d974885.

Fixed service id

b3f2146

markwallace-microsoft requested changes Dec 14, 2023

View reviewed changes

RogerBarreto reviewed Dec 14, 2023

View reviewed changes

dmytrostruk closed this Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.Net: Removed Azure and OpenAI text generation services #4246

.Net: Removed Azure and OpenAI text generation services #4246

dmytrostruk commented Dec 13, 2023

markwallace-microsoft left a comment

RogerBarreto left a comment

RogerBarreto Dec 14, 2023

RogerBarreto Dec 14, 2023

RogerBarreto Dec 14, 2023

RogerBarreto Dec 14, 2023

RogerBarreto Dec 14, 2023

dmytrostruk Dec 14, 2023

RogerBarreto Dec 14, 2023

RogerBarreto Dec 14, 2023

RogerBarreto Dec 14, 2023

dmytrostruk commented Dec 14, 2023

stephentoub commented Dec 14, 2023

dmytrostruk commented Dec 14, 2023

dmytrostruk commented Dec 14, 2023

stephentoub commented Dec 14, 2023

	modelId: azureOpenAIConfiguration.ModelId,
	modelId: azureOpenAIConfiguration.ChatModelId,

		@@ -12,16 +12,25 @@ internal sealed class AzureOpenAIConfiguration

		public string DeploymentName { get; set; }

		public string ModelId { get; set; }

.Net: Removed Azure and OpenAI text generation services #4246

.Net: Removed Azure and OpenAI text generation services #4246

Conversation

dmytrostruk commented Dec 13, 2023

Motivation and Context

Contribution Checklist

markwallace-microsoft left a comment

Choose a reason for hiding this comment

RogerBarreto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmytrostruk commented Dec 14, 2023

stephentoub commented Dec 14, 2023

dmytrostruk commented Dec 14, 2023

dmytrostruk commented Dec 14, 2023

stephentoub commented Dec 14, 2023