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

Enable azure openai engines #20

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Enable azure openai engines #20

merged 4 commits into from
Oct 16, 2023

Conversation

ecatkins
Copy link
Contributor

@ecatkins ecatkins commented Oct 14, 2023

Love the library!

I had to make modifications to get it to work with the Azure instances of OpenAI models that I use.

Primarily, I made it possible to pass an engine parameter to the LLMClassifier as well/instead of the model parameter which is what the Azure instance requires.

I tried to do it in a way that minimized the # of edits on the existing codebase - if this is something you'd be interested in including, happy to make any changes to fit with your design principles.

import openai
openai.api_type = "azure"
openai.api_version = "2023-07-01-preview"
openai.api_base = "https://azure-openai-xxxxx.openai.azure.com/"  # Your Azure OpenAI resource's endpoint value.
openai.api_key = "<AZURE-OPENAI_API-KEY>"
evaluator = Factuality(engine="<Azure-Deployment-Name>", model=None)

Copy link
Contributor

@ankrgyl ankrgyl left a comment

Choose a reason for hiding this comment

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

This looks great! Just left one comment. Really appreciate the contribution 🙏

Comment on lines 85 to 91
for m in SUPPORTED_MODELS:
# Prefixes are ok, because they are just time snapshots
if model.startswith(m):
found = True
break
if not found:
raise ValueError(f"Unsupported model: {model}. Currently only supports OpenAI chat models.")
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to just remove this check altogether — we definitely support more than the listed models, and "non-chat" flavored models are all deprecated at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@ankrgyl ankrgyl self-requested a review October 14, 2023 22:44
@ankrgyl
Copy link
Contributor

ankrgyl commented Oct 14, 2023

One more thing -- can you make the equivalent change in the JS implementation?

@ecatkins
Copy link
Contributor Author

One more thing -- can you make the equivalent change in the JS implementation?

JS isn't really my forte - I took a brief look - but chance I would do more harm than good :)

@ankrgyl
Copy link
Contributor

ankrgyl commented Oct 16, 2023

One more thing -- can you make the equivalent change in the JS implementation?

JS isn't really my forte - I took a brief look - but chance I would do more harm than good :)

No worries! I can follow up and do that.

@ankrgyl ankrgyl self-requested a review October 16, 2023 14:34
@ankrgyl
Copy link
Contributor

ankrgyl commented Oct 16, 2023

Hopefully last thing -- I ran the linter on your change and it appears to be failing (missing a comma after engine). I think you can fix this by running make fixup, or just add the comma in directly.

@ecatkins
Copy link
Contributor Author

Hopefully last thing -- I ran the linter on your change and it appears to be failing (missing a comma after engine). I think you can fix this by running make fixup, or just add the comma in directly.

Done - but it looked like the pytests were also failing?

@ankrgyl
Copy link
Contributor

ankrgyl commented Oct 16, 2023

Ah yep -- I think the way we cache parameters will be invalidated everytime a new one is added. Let me update the cached test results and push to your branch

@ankrgyl ankrgyl merged commit 6ac56c9 into braintrustdata:main Oct 16, 2023
3 checks passed
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.

2 participants