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

feat(openai): add azure support #96

Merged
merged 3 commits into from
Nov 19, 2024
Merged

feat(openai): add azure support #96

merged 3 commits into from
Nov 19, 2024

Conversation

trojan-bumble-bee
Copy link
Contributor

@trojan-bumble-bee trojan-bumble-bee commented Nov 11, 2024 •
edited
Loading

Adds support for using Azure OpenAI endpoints

@kharvd
Copy link
Owner

kharvd commented Nov 17, 2024

Not inclined to merge this because we have custom URLs - would Azure work with that? https://github.com/kharvd/gpt-cli?tab=readme-ov-file#customize-openai-api-url

@trojan-bumble-bee
Copy link
Contributor Author

Annoyingly I don't think the custom_base_url will work for Azure Open AI... The Azure API is similar to the OpenAI one, but different enough to make things a bit painful. The main issue I found is that azure requires the api-version uri parameter (see here) which the standard OpenAI client does not support (hence dropping in the AzureOpenAI client).

The other complication is that that azure replaces model name with deployment name which:

  1. Can be any user defined name, making the get_completion_provider logic a bit less clean
  2. Is provided within the url of the completion request, as opposed to the body when using standard OpenAI (this is abstracted away by the AzureOpenAi client, but is another slight difference between the apis)

Happy for you to opt-out of supporting Azure Open AI if you think it'll keep things cleaner.

kharvd
kharvd requested changes Nov 17, 2024
View reviewed changes
Copy link
Owner

@kharvd kharvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to support Azure, but would like to suggest that we use model name prefixes instead of a config flag

gptcli/assistant.py Outdated
Comment on lines 87 to 88
elif gptcli.providers.openai.use_azure:
return OpenAICompletionProvider()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having Azure be essentially the default, I think we can add a prefix to Azure model names, similar to oai-compat: - maybe oai-azure:?

gptcli/config.py Outdated
@@ -21,6 +21,10 @@ class GptCliConfig:
api_key: Optional[str] = os.environ.get("OPENAI_API_KEY")
openai_api_key: Optional[str] = os.environ.get("OPENAI_API_KEY")
openai_base_url: Optional[str] = os.environ.get("OPENAI_BASE_URL")
# When using azure open ai, set your assistant's model to your deployment
# name
openai_use_azure: bool = False
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we have oai-azure: prefix, we probably don't need this

gptcli/providers/openai.py Outdated
@@ -15,10 +15,16 @@
UsageEvent,
)

use_azure: bool = False

class OpenAICompletionProvider(CompletionProvider):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably would want class AzureOpenAICompletionProvider(OpenAICompletionProvider): that overrides the client

@trojan-bumble-bee trojan-bumble-bee force-pushed the add-azure-open-ai-support branch from edb060b to b8425ee Compare November 18, 2024 21:13
@trojan-bumble-bee
Copy link
Contributor Author

Agree the oai-azure prefix is a much nicer way to do it. Had to force push as I committed with the wrong account previously, sorry.

kharvd
kharvd reviewed Nov 18, 2024
View reviewed changes
gptcli/providers/azure_openai.py Outdated
Comment on lines 7 to 8
def __init__(self):
self.client = AzureOpenAI(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's call super().__init__(self) here - in case we ever decide to do some initialization in the parent class' __init__ other than setting the client

kharvd
kharvd approved these changes Nov 19, 2024
View reviewed changes
Copy link
Owner

@kharvd kharvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kharvd kharvd merged commit d61cbeb into kharvd:main Nov 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kharvd kharvd kharvd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@trojan-bumble-bee @kharvd