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

feat: add support for azure openai #16

Merged
merged 6 commits into from
Apr 5, 2024

Conversation

cpendery
Copy link
Contributor

@cpendery cpendery commented Apr 3, 2024

Adds support for models hosted on Azure OpenAI under the model prefix azure:.

Part of #14 & #22

Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
Copy link
Member

@klieret klieret left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR.

This seems to introduce a lot of code duplication. Couldn't you just subclass OpenAIModel? Or perhaps even better to add some logic in OpenAIModel.__init__ to just initialize self.client according to the prefix of SHORTCUTS?

@cpendery
Copy link
Contributor Author

cpendery commented Apr 3, 2024

Thanks for opening this PR.

This seems to introduce a lot of code duplication. Couldn't you just subclass OpenAIModel? Or perhaps even better to add some logic in OpenAIModel.__init__ to just initialize self.client according to the prefix of SHORTCUTS?

I’m happy to do either, I was just trying to avoid changing the convention of one class extending the base per model provider. I’ll go with the updating the OpenAIModel

@klieret klieret linked an issue Apr 4, 2024 that may be closed by this pull request
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
@cpendery cpendery requested a review from klieret April 4, 2024 18:08
Copy link
Member

@klieret klieret left a comment

Choose a reason for hiding this comment

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

LGTM! If you confirm that you've tested it, I think we're good to merge :)

@cpendery
Copy link
Contributor Author

cpendery commented Apr 4, 2024

LGTM! If you confirm that you've tested it, I think we're good to merge :)

If someone else can validate it before we merge, that would be great. I'm only on Windows so all the other issues are blocking me for verifying this works for the whole pipeline

@klieret
Copy link
Member

klieret commented Apr 4, 2024

Let me ping @0xdevalias and @kim-borgen since they upvoted. Perhaps they can test this.

@0xdevalias
Copy link
Contributor

since they upvoted. Perhaps they can test this.

@klieret I'm not currently setup to be able to test this unfortunately. I just liked the idea of wider support.

@kim-borgen
Copy link

@klieret Unfortunately I only have the 8k context length available for testing now and the first prompt (messages) is 10,963. Other than that, it seems to work. Sorry but I can't dedicate more time, but if someone supplies a config that uses less than 6k tokens for example I can test quickly

Otherwise:

  • Requested change: add api_version to cfg keys
  • When trying to get the API working I get this error, so I had to delete the traj/kim folder every time, a bit annoying.
WARNING  **************************************************                                                                                                                                                                                                                                                                                                                                                              
WARNING  Found existing args.yaml with different arguments!                                                                                                                                                                                                                                                                                                                                                              
WARNING  **************************************************   

@klieret
Copy link
Member

klieret commented Apr 5, 2024

@kim-borgen Thanks for the quick check! Can you open issues with some more context for the "Otherwise" ;) (not sure if I fully understand the context of the second one yet)

sweagent/agent/models.py Outdated Show resolved Hide resolved
cpendery and others added 3 commits April 5, 2024 09:07
Co-authored-by: Massimiliano Pronesti <massimiliano.pronesti@gmail.com>
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
Signed-off-by: Chapman Pendery <cpendery@microsoft.com>
@klieret klieret merged commit 95deb5d into princeton-nlp:main Apr 5, 2024
1 check passed
@klieret
Copy link
Member

klieret commented Apr 5, 2024

Since this doesn't seem to impact any other models anyway, I've gone ahead and merged it :)

@klieret
Copy link
Member

klieret commented Apr 7, 2024

Note: We shouldn't override the --model_name, so I've removed AZURE_OPENAI_DEPLOYMENT in #127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please also add support for Azure OpenAI
5 participants