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: Added AzureOpenAILLM #112

Merged
merged 13 commits into from
Jan 23, 2024
Merged

Conversation

arashaga
Copy link

@arashaga arashaga commented Jan 16, 2024

The Dynamic routing only worked with OpenAILLM now I added the AzureOpenAILLM class based following the OpenAI one now it works with Azure as well.


@jamescalam
Copy link
Member

thanks for PR @arashaga — we'll review and get back to you soon

@jamescalam jamescalam self-requested a review January 18, 2024 01:28
@jamescalam jamescalam self-assigned this Jan 18, 2024
@jamescalam jamescalam changed the title Added AzureOpenAILLM feat: Added AzureOpenAILLM Jan 18, 2024
@jamescalam
Copy link
Member

@arashaga could you add lint tweaks? (can run w/ make lint and make format) thanks!

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

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

Comparison is base (45e7ca0) 91.25% compared to head (dfb7571) 91.14%.

Files Patch % Lines
semantic_router/llms/zure.py 86.84% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #112      +/-   ##
==========================================
- Coverage   91.25%   91.14%   -0.11%     
==========================================
  Files          25       26       +1     
  Lines        1086     1129      +43     
==========================================
+ Hits          991     1029      +38     
- Misses         95      100       +5     

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

@arashaga
Copy link
Author

Done. had a bit of issue using the make lint on Windows but finally figure it out.

@arashaga
Copy link
Author

I will add the tests as well.

@jamescalam
Copy link
Member

@arashaga that'd be great! I think you will be able to take a lot of the OpenAILLM tests and just adopt them for AzureOpenAILLM here, with some small tweaks

@arashaga
Copy link
Author

added the unit tests + make lint format. once I see the code coverage report from your end I will modify anything that might be left accordingly.

@arashaga
Copy link
Author

now llm/zure.py getting 87% coverage on my end

@jamescalam
Copy link
Member

Thanks @arashaga will do another review tomorrow morning and hopefully get this merged!

Copy link
Member

@jamescalam jamescalam left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @arashaga !

@jamescalam jamescalam merged commit 8597362 into aurelio-labs:main Jan 23, 2024
6 of 8 checks passed
@arashaga arashaga deleted the azure-dyn-route-fix branch January 23, 2024 16:16
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