Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add method to select the OpenAI component based on configuration #5789

Merged
merged 25 commits into from
Sep 24, 2024
Merged

Add method to select the OpenAI component based on configuration #5789

merged 25 commits into from
Sep 24, 2024

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Sep 19, 2024 •
edited
Loading

Fixes #5755
Contributes to #4567
Completes #5621

Microsoft Reviewers: Open in CodeFlow

sebastienros and others added 19 commits August 20, 2024 08:54
@sebastienros
Create OpenAI component project files
613f117
@sebastienros
Merge remote-tracking branch 'origin/main' into sebros/openai
4742464
@sebastienros
Keep Endpoint
314e86b
@sebastienros
Update README
8726de7
@sebastienros
Add tests
fe7333c
@sebastienros
Merge remote-tracking branch 'origin/main' into sebros/openai
d9d89fa 
# Conflicts:
#	Aspire.sln
@sebastienros
Update package version
9662840
@sebastienros
Clean project files
c645e7e
@sebastienros
Enable tracing tests
f3d62bc
@sebastienros
Fix component name
ccd4a95
@sebastienros
Update Azure.AI.OpenAI schema
1d31619
@sebastienros
Merge remote-tracking branch 'origin/main' into sebros/openai
d18e13e 
# Conflicts:
#	Aspire.sln
#	src/Components/Aspire.Azure.AI.OpenAI/ConfigurationSchema.json
@sebastienros
Merge branch 'main' into sebros/openai
daa9922
@sebastienros
Fix TFM variables
a6fd23b
@sebastienros @eerhardt
Update src/Components/Aspire.OpenAI/README.md
3fb8bd1 
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@sebastienros @eerhardt
Update src/Components/Aspire.OpenAI/README.md
7a778e4 
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@sebastienros
PR feedback
82844da
@sebastienros
Merge remote-tracking branch 'origin/sebros/openai' into sebros/openai
a10a215 
# Conflicts:
#	src/Components/Aspire.OpenAI/README.md
@sebastienros
Add method to select the OpenAI component based on configuration
dd1bfeb
@sebastienros
Copy link
Member Author

The method name is still an open discussion.

@sebastienros
Copy link
Member Author

Choosing the best name for a method is crucial for clarity and maintainability. Both names you provided have their merits, but let's break them down:
AddOpenAIClientFromConfiguration: This name clearly indicates that the method adds an OpenAI client based on some configuration settings. It's straightforward and tells the user exactly what the method does.
AddAzureOrDefaultOpenAIClient: This name suggests that the method adds an OpenAI client, either from Azure or a default configuration. It implies a fallback mechanism, which might be useful if the primary configuration is not available.
Given these options, I would recommend AddOpenAIClientFromConfiguration. This name is more explicit about the source of the configuration and avoids potential confusion about the fallback mechanism. It keeps the method's purpose clear and concise.

@sebastienros
Copy link
Member Author

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@davidfowl davidfowl added area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure and removed azure Issues associated specifically with scenarios tied to using Azure labels Sep 22, 2024
@eerhardt eerhardt added the azure Issues associated specifically with scenarios tied to using Azure label Sep 23, 2024
eerhardt
eerhardt reviewed Sep 23, 2024
View reviewed changes
tests/Aspire.Azure.AI.OpenAI.Tests/AspireRestClientOpenAIExtensionsTests.cs Outdated Show resolved Hide resolved
src/Components/Aspire.Azure.AI.OpenAI/AspireConfigurableOpenAIExtensions.cs Outdated Show resolved Hide resolved
src/Components/Aspire.Azure.AI.OpenAI/AspireConfigurableOpenAIExtensions.cs Show resolved Hide resolved
src/Components/Aspire.Azure.AI.OpenAI/AspireConfigurableOpenAIExtensions.cs Outdated Show resolved Hide resolved
src/Components/Aspire.Azure.AI.OpenAI/AspireConfigurableOpenAIExtensions.cs Outdated Show resolved Hide resolved
tests/Aspire.Azure.AI.OpenAI.Tests/AspireRestClientOpenAIExtensionsTests.cs Outdated Show resolved Hide resolved
src/Components/Aspire.Azure.AI.OpenAI/README.md Outdated Show resolved Hide resolved
@radical
Copy link
Member

radical commented Sep 23, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@sebastienros
Copy link
Member Author

I validated it's working in eShop.

Base automatically changed from sebros/openai to main September 24, 2024 01:49
@build-analysis build-analysis bot mentioned this pull request Sep 24, 2024
Open
@davidfowl davidfowl merged commit 3f38298 into main Sep 24, 2024
11 checks passed
@davidfowl davidfowl deleted the sebros/anyopenai branch September 24, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl approved these changes

@eerhardt eerhardt eerhardt approved these changes

@radical radical Awaiting requested review from radical radical is a code owner

Assignees
No one assigned
Labels
area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AddOpenAIRestApiClient to wrap both AzureOpenAI and OpenAI integrations
4 participants
@sebastienros @radical @davidfowl @eerhardt