-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: adding support for Azure OpenAI in Semantic Kernel #505
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
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@swapydapy, could you please review? |
@DJ-os perhaps you could review as well? |
@didier-durand or perhaps you could review as you were the one who committed the semantic kernel sample. Thanks. |
Hi, sorry: I am not accredited to make reviews on this repo. Didier |
Hi @jland-redhat, this article may help to understand the differences between the Azure endpoint for OpenAI and the native one. https://learn.microsoft.com/en-us/azure/ai-services/openai/how-to/switching-endpoints |
Ok I missed the custom library. So still have a few suggestions I would like to run by y'all. There is a lot of commented code here which I am not in love with. What if instead we simplify the environment Variables. So instead of the current setup, what if we primarily stuck to Including And for the Azure/Non-Azure Logic from what I can tell, the only code difference is on lines 137 and 155, where What are y'alls thoughts on that? |
Sure. See the flag implementation in the latest commit. I tested it with Azure OpenAI as working successfully. However, I don't have an OpenAI api key to test with. If you could test it, that'd be great. Cheers. |
Hey Oliver, Validated the change:
I was also hoping that we could have provided a custom endpoint to the But this all LGTM! |
@jland-redhat this is easy. I could add that in if you like (but you'd have to retest it). This would add one extra variable in the upd: I see, google/a2a-eng needs to approve. |
@oliverlabs I think you need to get one of the repo owners to approve in order to move forward. Not sure the best way to poke them to take a look |
@moonbox3 @holtskinner @kthota-g @koverholt , perhaps one of you could merge? |
@moonbox3 would you be able to review this PR? |
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.
Looks like this needs a merge from main, as well as some other updates. Appreciate you adding this functionality.
Hi @oliverlabs, please let me know if you'd like some support getting the Azure OpenAI support added. I'm from the SK Python team and will be glad to help add the functionality. |
@moonbox3 hi Evan, thanks for reviewing this PR. Apologies, it's a bank holiday weekend in the UK and I was OoO. I will reach out to you tomorrow with some comments. |
…bs/A2A into feature/sk-aoai-support
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.
Implemented
@moonbox3 , UPD: fixed. I almost went mad searching for an error, but then noticed that |
Hi @oliverlabs, great progress. I would really prefer to have a more explicit approach to configuring the chat service, versus relying solely on if X number of env vars configured. Something like this (doesn't have to be that involved, but the enum is nice): https://github.com/microsoft/semantic-kernel/blob/main/python/samples/concepts/setup/chat_completion_services.py The method We can also link to SK's docs on configuring chat completion services in the README so devs can understand what needs to be included, if there are ever questions: https://learn.microsoft.com/en-us/semantic-kernel/concepts/ai-services/chat-completion/?tabs=csharp-AzureOpenAI%2Cpython-AzureOpenAI%2Cjava-AzureOpenAI&pivots=programming-language-python#creating-a-chat-completion-service along with this doc that spells out what env vars are required. |
@moonbox3, well, it looks like they removed the samples from this repository altogether. So this was all for nothing. Thanks for your feedback though. |
P.S. I rewrote the auth function to use the functions you mentioned @moonbox3. I will make a PR in the new repo, but I am not sure if you would have the maintainer rights there. I tested it and it works. |
Description
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTING
Guide.nox -s format
from the repository root to format)Fixes google-a2a/a2a-samples#45 🦕