alexandreroman / news-factory

Create AI-generated newsletters with Spring AI
Apache License 2.0
6 stars 5 forks source link

Add support for multiple AI providers #2

Closed alexandreroman closed 3 months ago

alexandreroman commented 4 months ago

When running unit tests, the process fails with the following error:

...
Caused by: org.springframework.beans.factory.NoUniqueBeanDefinitionException: No qualifying bean of type 'org.springframework.ai.chat.ChatClient' available: expected single matching bean but found 2: azureOpenAiChatClient,openAiChatClient
    at org.springframework.beans.factory.config.DependencyDescriptor.resolveNotUnique(DependencyDescriptor.java:218) ~[spring-beans-6.1.4.jar:6.1.4]
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1420) ~[spring-beans-6.1.4.jar:6.1.4]
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1353) ~[spring-beans-6.1.4.jar:6.1.4]
    at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:907) ~[spring-beans-6.1.4.jar:6.1.4]
    at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:785) ~[spring-beans-6.1.4.jar:6.1.4]
    ... 110 common frames omitted

This issue was introduced with 02fc2d8946f4160b8a3d946644be53aae77572ee.

alexandreroman commented 4 months ago

Reproduced in the GitHub Actions run.

alexandreroman commented 4 months ago

Having more than one Spring AI starter seems to be the root cause of this issue.

I disabled AWS Bedrock Lllama2 and Azure OpenAI by default by removing those starters in bb5ec558f6bdc39fcd05756ec382bcd2d63f389c: unit tests now work as expected (see GitHub Actions run for details).

We should be able to include the right dependencies using a Maven profile. Also, we may have to use a Spring Profile or a Conditional configuration to tune app properties depending on the AI provider.

alexandreroman commented 4 months ago

I'm exploring the use of Maven profiles to support multiple AI providers in 0aa108e011339325011d360972472d918c3c3baf.

odedia commented 4 months ago

I wonder if it's preferred to keep all 3 dependencies in the pom.xml and choose the right bean at runtime based on Spring profiles. Not entirely "clean" but perhaps makes more sense, kind of like having both H2 and MySQL dependencies in petclinic but choosing the correct repository based on config.

alexandreroman commented 4 months ago

I see your point. I'm afraid this strategy may not work with GraalVM and native-image though.

odedia commented 4 months ago

Seems like it should work when passing --spring.profiles.active to the executable, but I haven't validated it. Based on https://stackoverflow.com/questions/75242495/profile-specific-application-properties-not-picked-by-spring-boot-3-native-execu

odedia commented 4 months ago

AWS Bedrock supports many AI implementations so perhaps the the @Configuration class should be BedrockLlama2AIConfig.

odedia commented 4 months ago

Added a branch features/multi-ai-providers-all-mvn-deps to try out support for all 3 dependencies in pom.xml. In this version, the newsFactoryChatClient @Bean is instanciated based on a @ConditionalOnProperty settings in application.properties. For some reason, it works for OpenAI and AzureOpenAI, but fails with BedrockLlama2 @Configuration. If you can figure out what caused this to refuse to load the 3rd one it would be great.

odedia commented 4 months ago

features/multi-ai-providers-all-mvn-deps is now working with all 3 AI providers. Selected provider is based on property newsletter.ai.model in application.properties.

alexandreroman commented 4 months ago

That's great work! I'm trying to see if we can make a single ChatClient available, overriding instances coming from auto-configuration classes. I wish we could remove the @Qualifier in user code like this:

    AINewsletterFactory(ContentSummarizer summarizer, 
                        @Qualifier("newsFactoryChatClient") ChatClient cs, 
                        NewsletterProps np, 
                        AIResources aiResources) {
        this.summarizer = summarizer;
        this.cs = cs;
        this.np = np;
        this.aiResources = aiResources;
    }
alexandreroman commented 4 months ago

We're looking good: we don't need @Qualifier anymore with d426dec17baf7585f80eabb8467f2e50a9f3cb34!

odedia commented 4 months ago

For some reason the tests fail for me locally although they do pass with Github actions.

Caused by: org.springframework.beans.factory.NoUniqueBeanDefinitionException: No qualifying bean of type 'org.springframework.ai.chat.ChatClient' available: expected single matching bean but found 3: chatClient,azureOpenAiChatClient,openAiChatClient

Is AzureOpenAiChatClient the @Primary Bean for a reason or just an arbitrary choice?

odedia commented 4 months ago

Also running mvn spring-boot:run fails with the below:


***************************
APPLICATION FAILED TO START
***************************

Description:

Parameter 0 of constructor in com.broadcom.tanzu.newsfactory.impl.ContentSummarizer required a single bean, but 3 were found:
    - chatClient: defined by method 'chatClient' in class path resource [com/broadcom/tanzu/newsfactory/impl/openai/OpenAIConfig.class]
    - azureOpenAiChatClient: defined by method 'azureOpenAiChatClient' in class path resource [org/springframework/ai/autoconfigure/azure/openai/AzureOpenAiAutoConfiguration.class]
    - openAiChatClient: defined by method 'openAiChatClient' in class path resource [org/springframework/ai/autoconfigure/openai/OpenAiAutoConfiguration.class]

This may be due to missing parameter name information

Action:

Consider marking one of the beans as @Primary, updating the consumer to accept multiple beans, or using @Qualifier to identify the bean that should be consumed

Ensure that your compiler is configured to use the '-parameters' flag.
You may need to update both your build tool settings as well as your IDE.
(See https://github.com/spring-projects/spring-framework/wiki/Upgrading-to-Spring-Framework-6.x#parameter-name-retention)
alexandreroman commented 4 months ago

Weird. It works on my machine™. And GitHub Actions. Also: I have no issues with mvn spring-boot:run.

Regarding @Primary: you're right, this line of code is not required.

alexandreroman commented 3 months ago

@odedia Did you have a chance to validate that the branch is working fine on your side?

odedia commented 3 months ago

The application boots but it seems like it is always taking OpenAI bean. Were you able to verify it works with other AI providers?

alexandreroman commented 3 months ago

Not yet.

At this point I think we should merge this branch back into main, as we have pending issues depending on this new architecture model.

If any AI provider is broken then, let's create a new issue.

alexandreroman commented 3 months ago

The branch features/multi-ai-providers-all-mvn-deps has been merged into main. Closing this issue.