-
Notifications
You must be signed in to change notification settings - Fork 281
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
Add support to OpenAI API to use official or custom endpoints #65
Conversation
Signed-off-by: Daniel Fernández <daniel@dfer.io>
@microsoft-github-policy-service agree |
Thank you for your contribution, @friyin ! Can you add tests similar to what we have for other classes in the same module? From a brief glance, the code looks fine, but I'll take a closer look on Monday. |
Sure, I will add them. Regards! |
Signed-off-by: Daniel Fernández <daniel@dfer.io>
Signed-off-by: Daniel Fernández <daniel@dfer.io>
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.
Thank you for one of our first pull requests!
This is functionality we want, to talk to the OpenAI API. However, it would be more maintainable to generalize the functionality in the existing class and renaming it to something more general, so users of the class could use AzureOpenAI based on how it's initialized.
There are multiple ways to do this, but this is how I'd take a stab at it (and one of the ways we'd approve this). If you have other ideas please ask us!
- Create an abstract class,
OpenAIBase
that just has all the shared functions implemented - Create two subclasses,
AzureOpenAIChat
andOpenAIChat
, where they initialize the differentself._client
s appropriately
This would be more maintainable, we could reuse tests, etc. Please let me know if you have any questions, and thanks again!
Yes, I agree with that, I'm going to make the changes. |
Looking forward to the merge for OpenAI support |
Let us know if you're still planning on making this change! That would be great, but if not, we may do this later this week :) |
I´m still planning doing it, I will submit changes this week |
Awesome! Let us know if you get stuck on anything. Happy to help. |
eb2b766
to
ed1981e
Compare
…penAIBase Signed-off-by: Daniel Fernández <daniel@dfer.io>
It's done, now there are OpenAIChat and AzureOpenAIChat classes that extend from OpenAIBase. |
It's done |
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.
Great work! This looks good; merge conflicts need to be fixed.
I am moving this over to promptTarget
afterward, so just keep your version of the conflicts and I'll fix it up after we get this in
Description
Added support to use OpenAI API to communicate with official API or custom LLMs, you can change base URL of this chat engine to use your own OpenAI-like LLMs.
Tests
Documentation