Extracted OpenAI Spring Boot starter into a separate module #361
Conversation
|
@langchain4j the first thing I would do is upgrade to the latest Spring Boot 3.2.0 release, as the 2.7.x branch is end of life now. I can have a look if you want! |
|
@jdubois that would require minimum java 17... Please let's not start this discussion right now 😅 |
|
I've just tested this dependency on Spring 3.2.0 project, works good. It does not pull any 2.X dependencies, so no worries here. |
|
Ah ah OK, but we need to check if this will work fine with Spring Boot 3.2 |
|
Oh same comment at the same time :-) |
...in4j-open-ai-spring-boot-starter/src/main/java/dev/langchain4j/openai/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
spring-boot-starters/langchain4j-open-ai-spring-boot-starter/pom.xml
Outdated
Show resolved
Hide resolved
|
|
||
| @Bean | ||
| @Lazy | ||
| @ConditionalOnMissingBean |
There was a problem hiding this comment.
A @ConditionalOnProperty would be easier to configure when you'll have several beans (I'm guessing you'll have them all in the same package)
There was a problem hiding this comment.
Sorry the ConditionnalBean is what they are using in Spring AI (see https://github.com/spring-projects/spring-ai/blob/1a8fcac206070990f312986f34649bda9c6ac003/spring-ai-spring-boot-autoconfigure/src/main/java/org/springframework/ai/autoconfigure/azure/openai/AzureOpenAiAutoConfiguration.java#L43C3-L43C3 ), the trick is that they split their code in one starter per technology, and that's more work but that's probably the best idea
There was a problem hiding this comment.
I am splitting into one autoconfigure/starter per provider. I see that they mix all the autoconfigurations in one module, then have starters per each providers that references autoconfigure module. Not sure what's the point?
There was a problem hiding this comment.
We're looking at the same code with @SandraAhlgrimm :-)
Not sure why they did it, then yes it should be separated in one starter per provider.
There was a problem hiding this comment.
Hi @SandraAhlgrimm :)
I am changing @ConditionalOnMissingBean now to @ConditionalOnProperty(PREFIX + ".chat-model.api-key")
There was a problem hiding this comment.
@ConditionalOnProperty(PREFIX + ".chat-model.api-key") simplifies a bit, I don't need to check if user provided properties or not. User still can provide his own @Bean
There was a problem hiding this comment.
Not sure I fully understand your point. Let's say user added langchain4j-open-ai-spring-boot-starter to their dependencies. If he/she does not provide api key, bean is not created automatically. If they provide API key, it will be created automatically. If they wish to configure their own, they remove properties and provide their own @Bean. Seems good, no?
There was a problem hiding this comment.
Yes that works too. That's what's happening also quite frequently with some Spring Boot starters when you need to add a key like .enabled=true (for example management.endpoint.info.enabled=true).
But with @ConditionalOnMissingBean it works by magic (no key), as adding the starter is enough to tell you want that feature. And then if you want to create your own bean (because there's a feature you need that's not available in the starter), you can then re-use the keys, including the API key one.
There was a problem hiding this comment.
And if user defines both properties and his own @Bean, it should fail to avoid ambiguity because there are now 2 beans. This is not possible with @ConditionalOnMissingBean
There was a problem hiding this comment.
Every starter @Bean that we will provide will have at least one mandatory property (e.g. API key for managed services and base URL for local deployments), so user won't be able to just add a starter dependency and get a bean automagically, they will have to define at least one property. So I guess we can use the presence of this property to judge if the bean should be created automatically or not.
I could use @ConditionalOnMissingBean, but then it sohuld be either @Lazy or @ConditionalOnProperty. langchain4j-open-ai-spring-boot-starter provides 5 beans, if they will be just @ConditionalOnMissingBean, Spring Boot will attempt to create all of them and will fail because they have mandatory properties (API key).
...in4j-open-ai-spring-boot-starter/src/main/java/dev/langchain4j/openai/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...in4j-open-ai-spring-boot-starter/src/main/java/dev/langchain4j/openai/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...in4j-open-ai-spring-boot-starter/src/main/java/dev/langchain4j/openai/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...in4j-open-ai-spring-boot-starter/src/main/java/dev/langchain4j/openai/spring/Properties.java
Outdated
Show resolved
Hide resolved
...4j-open-ai-spring-boot-starter/src/test/java/dev/langchain4j/openai/spring/AutoConfigIT.java
Outdated
Show resolved
Hide resolved
|
@jdubois I have pushed updates that address your comments, could you please check? Thanks! One more thing: I anticipate that it will be common to use multiple models (with different configs) for different purposes in the same app, so langchain4j should support that. But I am not sure how to better implement this in auto configuration. Any ideas? |
|
is examples and starters not better put in the examples repo or separate module ? otherwise depmanagement in the pom and bom's now drag in a tail of dependencies that is not actually needed for langchain4j. |
|
@maxandersen I am not sure I understand the problem. Spring Boot starters will reside in their own maven modules/artifacts, so there should be no problem. BOM pulls only versions of langchain4j modules, so it makes sense to include all related dependencies there (that's the point of the BOM, right?). Another pom that is referencing spring boot starters is |
|
Reasons:
|
|
@maxandersen good point, I have moved it to https://github.com/langchain4j/langchain4j-spring |
No description provided.