-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Migration of the AzureOpenAiChatModel to use the Azure OpenAI SDK #328
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
|
wow if even rockstars start contributing here...... |
|
Ahah I've had a good look at your work here @clun and I'll try to do the same for Azure Cognitive Search... |
|
Really nice! The only thing I can think of with this one is that it breaks integrations using |
|
Thank you @geoand but why do you need this client? For Azure OpenAI, it's better to use the official client (and I'm not saying the client is better at all, just that it's the official one) |
|
There are a few reasons:
|
|
Thanks for insights, I had no idea this client was using some Quarkus-specific APIs. As the Azure SDK usually works very well with Spring, I'm guessing there will be the opposite argument for Spring users. |
|
👍🏼 Just to clarify, it would work in Quarkus too, it just won't have all the bells and whistles I mentioned above. |
|
@jdubois pardon me for the oversight, @geoand brought good points. Existing client does not use/depend on any Quarkus-specific APIs, but it has an SPI that allows Quarkus to use their network stack and have all the benefits mentioned above. It does not mean that users of Spring will see any downsides. I would propose to put this PR on hold for the moment and allow some time to explore options. Thanks for understanding! |
|
No it's indeed the same issue with Spring:
Both will work with the other framework, but they'll add more dependencies (and I'm guessing a second event loop), so it'll be less efficient for the "secondary" framework than for the "primary" framework. Now I don't want to enter the framework wars, here the issue for me is whether you should be using the official Azure SDK, from the Azure team, to connect to Azure OpenAI. Or use the client from https://github.com/ai-for-java which is a totally unknown organization (their org page is empty). |
I think this is a good way to go |
|
@langchain4j it's weird, so you're 1 person and coded everything by yourself? You should clarify it somewhere, because I didn't understand at all that LangChain4J and ai-for-java belonged to the same person - you're not using the same GitHub ID for both, and there's no name anywhere, so there's no way to know. Concerning using the official library:
Anyway, I get your point in having something specific for Quarkus, so I have 2 other proposals for you:
|
|
Thanks for elaborating @jdubois. Just to be clear, this repo does not have anything specific to Quarkus. All it has is an SPI that can be used by code to swap out various parts - this is what the Quarkus extension uses. |
|
@jdubois good point, I would indeed prefer using oficial SDK if I were Azure user. I think we can indeed use official Azure SDK for |
|
Oh yes totally agree @langchain4j ! |
|
Thank you so much for the detailed review @langchain4j !!! I fixed most of the comments, except 2 discussions that are still opened, could you check them? |
|
Argh I'm seeing the license compliance with Logback. That's really bad, I didn't know about this! Let me check. |
|
So I believe the Logback license issue is a false positive, as it is dual licensed ( https://logback.qos.ch/license.html ) and as many projects (including Spring Boot) use it by default. |
|
I was not sure about using logback due to unclear license and I am no legal expert to risk it, so I just use tinylog which is apache 2: But log4j also sounds good |
|
To fix it you need to add an exclusion to the license plugin, see example in |
|
I migrated to Log4J2, as it's the other supported framework by Spring Boot. Anyway, it shouldn't be very important. |
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.
@jdubois awesome, thank you so much for your work and patience!
|
Wonderful, thank you so much @langchain4j !! |
…ngchain4j#328) This PR fixes langchain4j#325 - [x] Migrate AzureOpenAiChatModel - [x] Migrate AzureOpenAiEmbeddingModel - [x] Migrate AzureOpenAiLanguageModel - [x] Migrate AzureOpenAiStreamingChatModel - [x] Migrate AzureOpenAiStreamingLanguageModel - [x] Add a full suite of tests
This PR fixes #325