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

Switched to AzureOpenAI for api_type=="azure" #1232

Merged
merged 16 commits into from
Jan 17, 2024
Merged

Switched to AzureOpenAI for api_type=="azure" #1232

merged 16 commits into from
Jan 17, 2024

Conversation

maxim-saplin
Copy link
Collaborator

With version 1 of openai client library there's a dedicated class AzureOpenAI to handle Azure endpoints. While the current version had a cryptic _process_for_azure workaround likely coming from pre v1 times it makes sense to remove the old code and hand over the responsibility of interacting with Azure straight to openai library/

Checks

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (acf81ac) 31.95% compared to head (d5caae1) 41.81%.
Report is 1 commits behind head on main.

Files Patch % Lines
autogen/oai/client.py 41.17% 9 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1232      +/-   ##
==========================================
+ Coverage   31.95%   41.81%   +9.85%     
==========================================
  Files          33       33              
  Lines        4431     4415      -16     
  Branches     1035     1085      +50     
==========================================
+ Hits         1416     1846     +430     
+ Misses       2897     2421     -476     
- Partials      118      148      +30     
Flag Coverage Δ
unittests 41.74% <41.17%> (+9.83%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@sonichi sonichi left a comment

Choose a reason for hiding this comment

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

Doing this PR right requires some deep understanding of the code. #868 tried it but also didn't succeed. If my comment is hard to understand, please let me know and I can do the PR and make you a reviewer.

autogen/oai/client.py Show resolved Hide resolved
autogen/oai/client.py Show resolved Hide resolved
autogen/oai/client.py Outdated Show resolved Hide resolved
autogen/oai/client.py Outdated Show resolved Hide resolved
@maxim-saplin
Copy link
Collaborator Author

Pushed a change that should fix this.

Looks good, tests pass

@sonichi sonichi added this pull request to the merge queue Jan 17, 2024
Merged via the queue into microsoft:main with commit 00dbcb2 Jan 17, 2024
76 of 84 checks passed
joshkyh pushed a commit that referenced this pull request Jan 17, 2024
* Switched to AzureOpenAI for api_type=="azure"

* Setting AzureOpenAI to empty object if no `openai`

* extra_ and openai_ kwargs

* test_client, support for Azure and "gpt-35-turbo-instruct"

* instruct/azure model in test_client_stream

* generalize aoai support (#1)

* generalize aoai support

* Null check, fixing tests

* cleanup test

---------

Co-authored-by: Maxim Saplin <[email protected]>

* Returning back model names for instruct

* process model in create

* None check

---------

Co-authored-by: Chi Wang <[email protected]>
@yiranwu0 yiranwu0 mentioned this pull request Mar 3, 2024
3 tasks
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* Switched to AzureOpenAI for api_type=="azure"

* Setting AzureOpenAI to empty object if no `openai`

* extra_ and openai_ kwargs

* test_client, support for Azure and "gpt-35-turbo-instruct"

* instruct/azure model in test_client_stream

* generalize aoai support (microsoft#1)

* generalize aoai support

* Null check, fixing tests

* cleanup test

---------

Co-authored-by: Maxim Saplin <[email protected]>

* Returning back model names for instruct

* process model in create

* None check

---------

Co-authored-by: Chi Wang <[email protected]>
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.

4 participants