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

Ensure deployment_id is set to provided deployment, required for Azure OpenAI. #5002

Merged
merged 10 commits into from Aug 11, 2023

Conversation

tot0
Copy link
Contributor

@tot0 tot0 commented May 19, 2023

Ensure deployment_id is set to provided deployment, required for Azure OpenAI.

Fixes #5001

Before submitting

Verified in my own code and by running this example notebook, works after the fix (as in it doesn't fail before trying to talk to the endpoint): https://github.com/hwchase17/langchain/blob/22d844dc0795e7e53a4cc499bf4974cb83df490d/docs/modules/models/text_embedding/examples/azureopenai.ipynb

Who can review?

@agola11 @hwchase17

@dev2049
Copy link
Contributor

dev2049 commented May 19, 2023

should we just revert #4915? seems like the api update they were referring to was specific to completion endpoints, so we shouldn't have changed in first place

@tot0
Copy link
Contributor Author

tot0 commented May 19, 2023 via email

@dev2049
Copy link
Contributor

dev2049 commented May 19, 2023

will revert for now and we can add tests in follow-up

@tot0
Copy link
Contributor Author

tot0 commented May 20, 2023

@dev2049 Updated this PR to only have unit test.


try:
query_result = embeddings.embed_query(text)
except error.InvalidRequestError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry what is this testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That deployment_id/engine is set on openai.Embeddings when the api_type is azure. Basically a regression test against a similar change being made as the one that was reverted in response to this PR.

@dosubot dosubot bot added Ɑ: embeddings Related to text embedding models module 🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature labels Jul 14, 2023
@vercel
Copy link

vercel bot commented Aug 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Aug 11, 2023 10:38pm

@baskaryan baskaryan added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Aug 11, 2023
@baskaryan baskaryan merged commit 1d3735a into langchain-ai:master Aug 11, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:bug Related to a bug, vulnerability, unexpected error with an existing feature Ɑ: embeddings Related to text embedding models module lgtm PR looks good. Use to confirm that a PR is ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure OpenAI Embeddings failed due to no deployment_id set.
4 participants