BREAKING FEAT migrate to openai SDK for openai API targets #1194
Conversation
| # 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.
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.
There was a problem hiding this comment.
Have to punt this to another PR because this is already a lot...
…consistent_error_handling
…consistent_error_handling
…consistent_error_handling
romanlutz
changed the title
…small PR suggestions, text fixes, response target notebook tests working
romanlutz
changed the title
rlundeen2
left a comment
rlundeen2
left a comment
There was a problem hiding this comment.
I'd wait a day for other folks to take a look, but great work addressing all my concerns. It looks great!
| self._validate_duration() | ||
| self._size = self._validate_resolution(resolution_dimensions=resolution_dimensions) | ||
|
|
||
| def _set_openai_env_configuration_vars(self) -> None: |
There was a problem hiding this comment.
| def _set_openai_env_configuration_vars(self) -> None: | |
| def _set_openai_env_configuration_vars(self) : |
There was a problem hiding this comment.
It's a technically correct type annotation, isn't it (?)
hannahwestra25
left a comment
hannahwestra25
left a comment
There was a problem hiding this comment.
minor comments but looks good :)
…consistent_error_handling
3 participants
Description
Migrating (back) to the openai SDK since it appears to have matured to a point where we can actually use it. This simplifies some things for us as we don't have to manually craft HTTP messages anymore.
The PR handles this change and adds more structure for error handling. This can be observed in the
OpenAITargetabstract base class with_handle_openai_requestwhich is called by each target. In turn, it calls the provided API (this is specific per target, e.g., responses API, or chat completions API), catches any exceptions (e.g., input content filters, bad requests, rate limit error, and others), and if it succeeds runs_check_content_filter(for output filters) followed by_handle_content_filter_responsein case of filters. It also validates the response with_validate_responseand finally returns the result (which may be an errorMessageor a successfulMessage).Note: this is pending another thorough re-review by myself and then a review from other maintainers.
Note: This does not yet add token provider capabilities instead of the API key. That will require a follow-up PR.
Also done here:
Tests and Documentation