Skip to content

.Net OpenAI SDK V2 - Phase 01 Embeddings + ClientCore (Feature Branch) - #6898

Merged
Roger Barreto (rogerbarreto) merged 18 commits into
microsoft:feature-connectors-openaifrom
rogerbarreto:features/openai-v2-phase001
Jun 24, 2024
Merged

.Net OpenAI SDK V2 - Phase 01 Embeddings + ClientCore (Feature Branch) #6898
Roger Barreto (rogerbarreto) merged 18 commits into
microsoft:feature-connectors-openaifrom
rogerbarreto:features/openai-v2-phase001

Conversation

@rogerbarreto

@rogerbarreto Roger Barreto (rogerbarreto) commented Jun 20, 2024

Copy link
Copy Markdown
Member

ClientCore + Foundation

This PR is the first and starts the foundation structure and classes for the V2 OpenAI Connector.

In this PR I also used the simpler TextEmbeddings service to wrap up a vertical slice of the Service + Client + UT + IT + Dependencies needed also to validate the proposed structure of folders and namespaces for internal and public components.

ClientCore

As part of this PR I'm also taking benefit of the partial keyword for ClientCore class dividing its implementation per Service. In the original V1 ClientCore the file was very big, and creating specific files PR service/modality will make it simpler and easier to maintain.
  

What Changed

This change includes a new update from previous Azure.Core Pipeline abstractions to the new System.ClientModel which is used by OpenAI package.

Those include the update and addition of the below files:

  • AddHeaderRequestPolicy - Adapted from previous AddHeaderRequestPolicy
  • ClientResultExceptionExtensions - Adapted from previous RequestExceptionExtensions
  • OpenAIClientCore - Merged with ClientCore (No more need for a specialized Azure and OpenAI clients)
  • ClientCore (Updated internals just with necessary for Text Embeddings), merged OpenAIClientCore also into this one and made it not as abstract class.
  • OpenAITextEmbbedingGenerationService (Updated to use ClientCore directly instead of OpenAIClientCore.

Whats New

  • PipelineSynchronousPolicy - Azure.Core/src/Pipeline/HttpPipelineSynchronousPolicy.cs
    This file didn't exist and was necessary to add as it is a dependency for AddHeaderRequestPolicy
  • Mockups added for System.ClientModel pipeline testing
  • Unit Tests Covering
    • ClientCore
    • OpenAITextEmbeddingsGenerationService
    • AddHeadersRequestPolicy
    • PipelineSynchronousPolicy
    • ClientResultExceptionExtensions
  • Integration Tests
    • OpenAITextEmbeddingsGenerationService (Moved from V1)

What was Removed

  • OpenAIClientCore - This class was merged in ClientCore
  • CustomHostPipelinePolicy - Removed as the new OpenAI SDK supports Non-Default OpenAI endpoints.

Unit & Integration Test

Differently from V1, this PR focus on individual UnitTest for the OpenAI connector only.

With the target of above 80% code converage the Unit Tests targets Services + Clients + Extensions & Utilities

The structure of folders and tested components on the UnitTests will follow the same structure defined in project under test.

@markwallace-microsoft Mark Wallace (markwallace-microsoft) added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Jun 20, 2024
@markwallace-microsoft

Mark Wallace (markwallace-microsoft) commented Jun 21, 2024

Copy link
Copy Markdown
Contributor

Roger Barreto (@rogerbarreto) I know this is a draft but a few comments

  • If files are just being moved (unchanged) from the V1 project please list these in the PR description so reviewers know.
  • If files are being changed as they move please provide some context for the changes.
  • More the moved files please move the corresponding unit tests at the same time, if there are no unit tests for the files please check code coverage in the old project to confirm they are fully tested by indirect unit tests.
  • For all new files please include unit tests in each PR so we don't build up tech debt, the expectation is that we will have at least 80% code coverage

FYI SergeyMenshykh

Comment thread dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs Outdated
Comment thread dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs Outdated
Comment thread dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs Outdated
Comment thread dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs
Comment thread dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs
@rogerbarreto
Roger Barreto (rogerbarreto) marked this pull request as ready for review June 21, 2024 18:44
@rogerbarreto
Roger Barreto (rogerbarreto) requested a review from a team as a code owner June 21, 2024 18:44
@rogerbarreto Roger Barreto (rogerbarreto) changed the title .Net WIP OpenAI SDK V2 - Phase 01 Embeddings + ClientCore (Feature Branch) .Net OpenAI SDK V2 - Phase 01 Embeddings + ClientCore (Feature Branch) Jun 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor stuff but LGTM

Comment thread dotnet/src/Connectors/Connectors.OpenAIV2/Core/ClientCore.cs Outdated
IList<string> data,
Kernel? kernel,
int? dimensions,
CancellationToken cancellationToken)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we allow a model id override when this method is called?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this method should have an execution settings TBH where the ModelId would be overriden per request, I will keep track on this ask to investigate further.

Investigation: Check if this would require rethinking on ITextEmbeddingsGeneration interface which could have bigger consequences, impacting other connectors implementations, as well as how this also relates to the work on the Vector abstractions westey (@westey-m) is doing.

/// </summary>
/// <param name="modelId">Model name</param>
/// <param name="apiKey">OpenAI API Key</param>
/// <param name="organization">OpenAI Organization Id (usually optional)</param>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Curious, when is it not optional?

@rogerbarreto Roger Barreto (rogerbarreto) Jun 22, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Organization Id by OpenAI

For users who belong to multiple organizations or are accessing their projects through their legacy user API key, you can pass a header to specify which organization and project is used for an API request. Usage from these API requests will count as usage for the specified organization and project.
To access the Default project in an organization, leave out the OpenAI-Project header

@rogerbarreto
Roger Barreto (rogerbarreto) merged commit af19aa7 into microsoft:feature-connectors-openai Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants