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.

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

Fix azure openai support and remove redundant semicolon #67

Merged
merged 2 commits into from
Feb 11, 2024

Conversation

jackyu1996
Copy link
Contributor

Sorry, I intended to make two separate commits but removed their branches too soon.

return openai.AzureOpenAI(api_key=config["openai_api_key"])
return openai.AzureOpenAI(
api_key=config["openai_api_key"],
azure_deployment=os.environ.get("OPENAI_API_AZURE_ENGINE"),
Copy link

@ahernsean ahernsean Feb 7, 2024

Choose a reason for hiding this comment

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

This is unnecessary. If you set the env variable OPENAI_API_MODEL to the value of OPENAI_API_AZURE_ENGINE, you get the same results as the code change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is introduced because the official openai-python library requires initialization with deployment name for an azure client and such name doesn't seem to get picked up by chatblade when initializing it. Although error handling might become tricky.

Copy link

@ahernsean ahernsean Feb 9, 2024

Choose a reason for hiding this comment

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

I wonder why it's working for me on Azure without your change…hmm

Choose a reason for hiding this comment

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

I figured out why. The model I thought I was selecting isn't being used. I've been using the default deployment without realizing it. I need your patch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that was possible, but glad you have figured it out.

Copy link

@ahernsean ahernsean left a comment

Choose a reason for hiding this comment

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

The changes work well for me. I've confirmed the base_url is now computed properly with the OpenAI API.

@npiv npiv merged commit 2b6f2f5 into npiv:main Feb 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants