-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Construct non-Azure OpenAI clients for default and EU endpoints #43228
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
base: main
Are you sure you want to change the base?
Conversation
With the release of OpenAI's EU endpoint, the client builder needs to create non-Azure OpenAI clients if the endpoint is null or if it matches the default API URL or the EU URL.
Thank you for your contribution @micah-press! We will review the pull request and get back to you soon. |
return endpoint == null || endpoint.startsWith(OPEN_AI_ENDPOINT); | ||
return endpoint == null || endpoint.startsWith(OPEN_AI_ENDPOINT) || endpoint.startsWith(EU_OPEN_AI_ENDPOINT); |
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.
This isn't the most extensible design. Might make more sense to check for whether the endpoint contains the OpenAI domain, but my security best practices are a bit shaky and I'm not sure how easily that could be exploited to direct requests somewhere they're not supposed to go.
public String getEndpoint() { | ||
return this.endpoint; | ||
} |
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.
Added this mostly to conform to the way the other implementation is written.
Field azureClient = OpenAIClient.class.getDeclaredField("serviceClient"); | ||
azureClient.setAccessible(true); | ||
Field nonAzureClient = OpenAIClient.class.getDeclaredField("openAIServiceClient"); | ||
nonAzureClient.setAccessible(true); |
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.
This is pretty brittle, but I'm not sure of a better way to test that the correct implementation is used. Maybe it's better to leave it un-unit-tested since it's all under the hood. Open to suggestions!
API change check API changes are not detected in this pull request. |
@microsoft-github-policy-service agree company="Everlaw" |
@micah-press Thank you for the contribution! |
@mssfang I'm using the EU-based endpoint for work. It hasn't been publicly announced by OpenAI yet, but if you use any normal OpenAI API key and send a request to If my approach is too strict considering there hasn't been a public announcement yet, maybe testing whether the |
… to use This commit generalizes the logic a bit more for deciding whether an Azure OpenAI client should be used or not. The regex allows for a single subdomain in the endpoint as long as the rest of it matches.
This PR contains the same exact changes as my PR targeting the upstream repo[1], but consolidated into a single commit for convenience. 1: Azure#43228
EU data residency has been announced! Link here with API documentation linked towards the end of the page. |
Light bump here -- happy to incorporate review comments! |
Hi @micah-press. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
I think this is still a change worth making, so I'd appreciate getting eyes back on it. |
Description
OpenAI is releasing an EU-based endpoint for their API. Users can set up a project that is regionally restricted so that API requests using a key for that project will fail if they don't hit the proper endpoint.
The current logic for choosing the client implementation to use with the
OpenAIClient
object checks for an unsetendpoint
value, or one that exactly matches"https://api.openai.com/v1"
. Using the EU endpoint,"https://eu.api.openai.com/v1"
, leads to runtime failures because an Azure-hosted OpenAI client implementation is used instead of a non-Azure implementation.This PR adds the EU endpoint as a new public static field of the non-Azure OpenAI client class and checks for its presence when deciding which implementation to create in
OpenAIClientBuilder
.Additional Testing
I wanted to make sure these changes did what I wanted them to do, so I created a POC project with both normal and EU-only API keys. I ran the following end to end tests:
null
and with a 404 if I set the endpoint to the EU-based URL)All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines