-
Notifications
You must be signed in to change notification settings - Fork 707
fix for wrong OpenAIClientOptions used with keyed OpenAIClient(s) #11003
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.
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
Conversation
with keyed OpenAIClients
```
builder.AddKeyedOpenAIClient("openai");
builder.AddKeyedOpenAIClient("ionos");
```
assuming that at least one keyed OpenAIClient has a differnt endpoint,
```
{
"ConnectionStrings": {
"openai": "Key={account_key};"
"ionos": "Endpoint=https://{openai_rest_api_url};Key={account_key};"
}
}
```
getting the client with
```
var client = serviceProvider.GetRequiredKeyedService<OpenAIClient>("ionos")
```
the client uses the key from the connectionstring but it still has the default enpoint https://api.openai.com/v1
The issue is that ConfigureOpenAI ignores the serviceKey and always get's the default OpenAIClientOptions instead of the one created with optionsName = servicekey.
This makes using keyed OpenAIClients virtually impossible because the main reason having multiple OpenAIClients is having different endpoints.
|
@dotnet-policy-service agree |
|
Can you add tests for this? |
|
Fixing #9543 options = serviceProvider.GetRequiredService<IOptionsMonitor<OpenAIClientOptions>>()
.Get(serviceKey ?? Options.DefaultName);According to: openai/openai-dotnet#215 |
|
@eerhardt I added a test case that is failing without my changes and passing afterwards. As @Kumima mentioned, testing this is tricky because OpenAIClient doesn't expose the endpoint. I don't really like the test because I usually only test the public interface of a class but I could not figure out how to workaround this so I opted for using reflection to get the endpoint. This would work as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the fix!
with keyed OpenAIClients
assuming that at least one keyed OpenAIClient has a differnt endpoint,
getting the client with
the client uses the key from the connectionstring but it still has the default enpoint https://api.openai.com/v1
The issue is that ConfigureOpenAI ignores the serviceKey and always get's the default OpenAIClientOptions instead of the one created with optionsName = servicekey.
This makes using keyed OpenAIClients virtually impossible because the main reason having multiple OpenAIClients is having different endpoints.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate