-
Notifications
You must be signed in to change notification settings - Fork 609
[DRAFT] FEAT migrate to openai SDK for openai API targets #1194
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?
[DRAFT] FEAT migrate to openai SDK for openai API targets #1194
Conversation
| logger.info("Authenticating with AzureAuth") | ||
| scope = get_default_scope(self._endpoint) | ||
| self._azure_auth = AzureAuth(token_scope=scope) | ||
| # For SDK-based targets: auth is handled via azure_ad_token_provider parameter |
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.
Note: This PR so far only includes the required changes for response and chat completions APIs. If we like this approach, I'll migrate the rest as well and there won't be any more "non-SDK targets" which simplifies this class a bit.
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.
One of the issues we've had in the past is when certain endpoints aren't supported in the SDK (e.g. DALLE). Not a blocker, but it's likely worth thinking through how we'd handle those, or if it would be more work
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.
Indeed. But whenever that happens we usually have to write code from scratch anyway (?)
For example, with Sora the differences were so significant that we couldn't use the existing target at all. In hindsight, I would wager not wasting time on making it compatible with the existing target would have been preferable. That way, we can quickly put something custom together for the op and afterwards the openai sdk supports it anyway.
If there are truly minor differences then we have these options:
- change headers using
with_options(fairly lightweight change)or globally for all requests with that clientfrom openai import OpenAI client = OpenAI(api_key="sk-...") # For a single request (or a scoped sub-client), add custom headers or swap base_url. exp_client = client.with_options( headers={ "X-Experimental-Feature": "1", "X-Customer-Context": "PyRIT" # any custom header you need }, base_url="https://api.openai.com/v1" # or a proxy if you’re routing traffic ) resp = exp_client.responses.create( model="pre-release-model-name", # you can pass model strings not yet in SDK enums input=[{"role": "user", "content": "Test new model"}] )client = OpenAI( api_key="sk-...", base_url="https://your-proxy.example.com/v1", # You can also pass default headers here if you prefer: # default_headers={"X-Experimental-Feature": "1"} ) - inject your own httpx client!
import httpx from openai import OpenAI def add_experimental_bits(request: httpx.Request): # Add/override headers request.headers["X-Experimental-Feature"] = "1" # Optionally append a query param request.url = request.url.copy_add_param("beta", "true") http_client = httpx.Client( timeout=httpx.Timeout(connect=5.0, read=60.0), event_hooks={"request": [add_experimental_bits]}, ) client = OpenAI(api_key="sk-...", http_client=http_client) resp = client.responses.create( model="pre-release-model-name", input=[{"role": "user", "content": "Trying experimental settings"}], )
Also maybe @bashirpartovi has thoughts since he has thought about this before me 🙂
| Azure has multiple endpoint formats: | ||
| 1. Old format: https://{resource}.openai.azure.com/openai/deployments/{deployment}/...?api-version=... | ||
| Uses AsyncAzureOpenAI client with api-version parameter (supports both API key and Entra auth) | ||
| 2. New format: https://{resource}.openai.azure.com/openai/v1 OR https://{resource}.models.ai.azure.com/... | ||
| Uses standard AsyncOpenAI client (no api-version needed) | ||
| - With API key: pass api_key parameter | ||
| - With Entra: pass token provider function as api_key parameter |
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 related to #1173
I ran into this quite a bit and figured we might as well support the GA-style new format while we're at it. It does make things easier actually BUT for the time being both formats are allowed which definitely complicates things 😆
| azure_ad_token_provider=azure_ad_token_provider, | ||
| **httpx_kwargs, | ||
| ) | ||
| else: |
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.
Needless to say, this should be broken up into smaller methods.
| # Use the Azure SDK's async get_bearer_token_provider for proper token management | ||
| # This returns an async callable that the OpenAI SDK can await natively | ||
| scope = get_default_scope(self._endpoint) | ||
| api_key_value = get_async_token_provider_from_default_azure_credential(scope) |
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.
One more thing related to #1173 is that api_key can actually be a token provider now! So I suggest we tackle that here as well and redo this portion.
Currently, we have an arg use_entra_auth which adds a bearer token based on the default scope as indicated by the endpoint URL. Scope here means that we get the token for ml.azure.com or cognitiveservices.azure.com. It's possible that there could be other scopes, too, but we just don't support that right now.
My proposal:
- Remove
use_entra_auth - If no
api_keyis provided then attempt to use default scope based on the URL and do Entra auth. - If
api_keyis provided as key, use it that way - If
api_keyis provided as token provider, use that token provider and pass it to theAsyncOpenAIconstructor
More customizable ✅
Solves #1173 ✅
Removes an input arg to simplify ✅
The other thing going on here is that Azure had its own client class AsyncAzureOpenAI but with the new GA format we just use AsyncOpenAI which greatly simplifies things, too, at least once we can ignore the old-style formats. I suggest we don't stop supporting them until we don't use them anymore.
Description
So far this is an exploration to see if it helps, to be continued...
Tests and Documentation