Skip to content

GH-868: Fix inconsistency in autoconfigured beans #1112

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

GH-868: Fix inconsistency in autoconfigured beans #1112

wants to merge 2 commits into from

Conversation

sobychacko
Copy link
Contributor

  • Fixing auto configured beans where they are missing @ConditionalOnMissingBean
  • Add matchIfMissing on @ConditionalOnProperty where it is missing with value true

Resolves #868

@tzolov
Copy link
Contributor

tzolov commented Jul 25, 2024

@sobychacko ,
The Bedrock models are disabled by default .
While the non-Bedrock model providers have a dedicated auto-configuration and a dedicated boot starter, the Bedrock is an umbrella of providers and the Bedrock auto-configuration and single Boot starter exposes the models from 5-6 providers. Therefore they are disabled by default.

Perhaps we should crate a dedicated boot-starter for each Bedrock provider and then we can enable the models for this provider?

tzolov
tzolov requested changes Jul 25, 2024
View reviewed changes
...pringframework/ai/autoconfigure/bedrock/anthropic/BedrockAnthropicChatAutoConfiguration.java Outdated
* @since 0.8.0
*/
@AutoConfiguration
@ConditionalOnClass(AnthropicChatBedrockApi.class)
@EnableConfigurationProperties({ BedrockAnthropicChatProperties.class, BedrockAwsConnectionProperties.class })
@ConditionalOnProperty(prefix = BedrockAnthropicChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true")
@ConditionalOnProperty(prefix = BedrockAnthropicChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true",
matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

...ingframework/ai/autoconfigure/bedrock/anthropic3/BedrockAnthropic3ChatAutoConfiguration.java Outdated
* @since 0.8.0
*/
@AutoConfiguration
@ConditionalOnClass(Anthropic3ChatBedrockApi.class)
@EnableConfigurationProperties({ BedrockAnthropic3ChatProperties.class, BedrockAwsConnectionProperties.class })
@ConditionalOnProperty(prefix = BedrockAnthropic3ChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true")
@ConditionalOnProperty(prefix = BedrockAnthropic3ChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true",
matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

.../org/springframework/ai/autoconfigure/bedrock/cohere/BedrockCohereChatAutoConfiguration.java Outdated
* @since 0.8.0
*/
@AutoConfiguration
@ConditionalOnClass(CohereChatBedrockApi.class)
@EnableConfigurationProperties({ BedrockCohereChatProperties.class, BedrockAwsConnectionProperties.class })
@ConditionalOnProperty(prefix = BedrockCohereChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true")
@ConditionalOnProperty(prefix = BedrockCohereChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true",
matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

...springframework/ai/autoconfigure/bedrock/cohere/BedrockCohereEmbeddingAutoConfiguration.java Outdated
* @since 0.8.0
*/
@AutoConfiguration
@ConditionalOnClass(CohereEmbeddingBedrockApi.class)
@EnableConfigurationProperties({ BedrockCohereEmbeddingProperties.class, BedrockAwsConnectionProperties.class })
@ConditionalOnProperty(prefix = BedrockCohereEmbeddingProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true")
@ConditionalOnProperty(prefix = BedrockCohereEmbeddingProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true",
matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

...gframework/ai/autoconfigure/bedrock/jurrasic2/BedrockAi21Jurassic2ChatAutoConfiguration.java Outdated
* @since 1.0.0
*/
@AutoConfiguration
@ConditionalOnClass(Ai21Jurassic2ChatBedrockApi.class)
@EnableConfigurationProperties({ BedrockAi21Jurassic2ChatProperties.class, BedrockAwsConnectionProperties.class })
@ConditionalOnProperty(prefix = BedrockAi21Jurassic2ChatProperties.CONFIG_PREFIX, name = "enabled",
havingValue = "true")
havingValue = "true", matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

...va/org/springframework/ai/autoconfigure/bedrock/llama/BedrockLlamaChatAutoConfiguration.java Outdated
@@ -44,7 +44,8 @@
@AutoConfiguration
@ConditionalOnClass(LlamaChatBedrockApi.class)
@EnableConfigurationProperties({ BedrockLlamaChatProperties.class, BedrockAwsConnectionProperties.class })
@ConditionalOnProperty(prefix = BedrockLlamaChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true")
@ConditionalOnProperty(prefix = BedrockLlamaChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true",
matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

...va/org/springframework/ai/autoconfigure/vertexai/gemini/VertexAiGeminiAutoConfiguration.java
@@ -74,6 +77,8 @@ public VertexAI vertexAi(VertexAiGeminiConnectionProperties connectionProperties

@Bean
@ConditionalOnMissingBean
@ConditionalOnProperty(prefix = VertexAiGeminiChatProperties.CONFIG_PREFIX, name = "enabled", havingValue = "true",
matchIfMissing = true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because, currently, we are using a single boot starter for all Bedrock providers, the Bedrock models should be disabled by default.

@sobychacko
spring-projectsGH-868: Fix inconsistency in autoconfigured beans
24283d6 
* Fixing auto configured beans where they are missing `@ConditionalOnMissingBean`
* Add `matchIfMissing` on `@ConditionalOnProperty` where it is missing with value `true`

Resolves spring-projects#868
@sobychacko
Addressing PR review
2ee503f
@sobychacko
Copy link
Contributor Author

@tzolov Removed the matchIfMissing from bedrock auto configuration.

@tzolov
Copy link
Contributor

tzolov commented Jul 30, 2024

LGTM

@tzolov
Copy link
Contributor

tzolov commented Jul 30, 2024

rebased, squashed and merged at ce59613

@tzolov tzolov closed this Jul 30, 2024
@tzolov tzolov added this to the 1.0.0-M2 milestone Jul 30, 2024
@tzolov tzolov self-assigned this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzolov tzolov tzolov requested changes

Assignees

@tzolov tzolov

Labels
None yet
Projects
None yet
Milestone
1.0.0-M2
Development

Successfully merging this pull request may close these issues.

Inconsistent Annotations on Bean Factory Methods for Chat Models
2 participants
@sobychacko @tzolov