-
Notifications
You must be signed in to change notification settings - Fork 239
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 AzureOpenAIEncoder #73
Conversation
the openai==1 uses a client. like so: from openai import AzureOpenAI client = AzureOpenAI( response = client.embeddings.create( print(response.model_dump_json(indent=2)) I think this PR is for the <1. source: https://learn.microsoft.com/en-us/azure/ai-services/openai/how-to/embeddings?tabs=python is there a particular reason why you're using this version? @mckeown12 I need this as well but I think we should go with the version 1 api. |
Hey @mckeown12 this is great, thanks for the PR! I'm not super familiar w/ best practices for Azure OpenAI — but I'd also prefer whatever the most up-to-date approach is, if that is what you're recommending @stepanogil ? |
Thanks all for looking at this so quickly! @stepanogil I believe my PR is using the pinned openai library version 1.5.0 from the poetry.lock file so I guess I'm not sure where you're seeing <1 code. The init of AzureOpenAIEncoder instantiates a client in almost exactly like you showed. As a side note, we've been using my fork in an internal company project and are loving semantic-router so far! |
@mckeown12 assert (
deployment_name is no longer a required field to instantiate an Azure OpenAI client in OpenAI Python 1.x. api_key check the link I've provided and scroll down a little bit. https://learn.microsoft.com/en-us/azure/ai-services/openai/how-to/embeddings?tabs=python-new |
So I guess |
@jamescalam @simjak - my two cents. I think there will be more PRs to add custom embeddings (same will go with llm for dynamic routes too). Would it make sense to integrate with langchain embeddings? ie Add a LangChainEncoder and inject a langchain embeddings and delegate the real embed action to it - like below
Usage
I think it will help you put your effort in 'semantic' |
it's my guess as well. i haven't tried deploying 2 instances of the same model in the same resource. |
@mckeown12 I'm not sure about the circular imports. We should find a better way of handling that. I will test, in the current state does this PR for AzureOpenAI work for you both @mckeown12 and @stepanogil ?
@szelok I think you have a good point here. We can add support for the "essential" encoders, that being openai+azureopenai, cohere, HF, and fastembed, and the rest we can support indirectly via langchain. We'll get that in soon |
@jamescalam i think @mckeown12 is still making adjustments to make |
Just finished this and updated the PR. Let me know how y'all want to handle the circular imports issue with the ordering in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mckeown12 nice! I ran the tests and linters, could you get those working? @ashraq1455 is going to look into the circular import issue and see if we can find a good solution, thanks :)
@jamescalam, thanks for reviewing! I just merged main again, ran the linter, and got the tests passing locally. Not sure how to rerun the workflows though... Is that something y'all have to trigger? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
+ Coverage 88.62% 89.03% +0.41%
==========================================
Files 23 24 +1
Lines 967 1040 +73
==========================================
+ Hits 857 926 +69
- Misses 110 114 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay we made it! @mckeown12 this is epic thankyou for the PR 🔥
When I run the Dynamic Routes it does not work with AzureOpenAI it appears that only OpenAILLM is supported there can we have a fix for this? you can reproduce the issue with running example notebook for Dynamic Routes. Thank you. |
Azure's OpenAI deployments require a few extra parameters in order to initialize.