-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix openai auto-configuration #4548
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
Signed-off-by: Issam El-atif <[email protected]>
I like your idea @ielatif to use centralization for auto-configurations. Same issue is in my PR #4535 and #4543 With one point I disagree,
Not in every auto-configuration test we need all classes from common, e.g. in I think we can start with
|
@ielatif Thanks for the PR! I like your suggestion on having the centralised test configuration. @centrumek Good point as well. Perhaps, we can still centralise the auto-configurations by importing auto-configuration specific to tool calling tests. While this may not the scope of this PR, we can have it addressed separately once the related epic #4494 is closed. |
@centrumek thanks for your feedback. The reason why i suggest to put all commons configurations in one place is because they are conditionnal auto-configurations, the beans will get loaded only if the condition match.
If VertexAiTextEmbedding use only SpringAiRetry classes like RestClientBuilder, WebClient and ChatModel shouldn't be in the classpath then auto-configurations RestClientAutoConfiguration, WebClientAutoConfiguration and ToolCallingAutoConfiguration will not be loaded in the applicationContext due to missing classes on the classpath @ilayaperumalg yes sure! I'll be happy to address this seperately once #4494 is closed. |
@ilayaperumalg i see that the build is falling for this PR with :
I run |
@ielatif The check style errors need to be fixed manually. I will take care of it when merging the PR. Thanks again. |
Rebased and merged as 0ae56f7 |
Fixes openai auto-configurations for #4494
Changes
Refactoring suggestion
I did not use a BaseIT class:
I suggest to isolate and centralise common auto-configurations in one place (may be in spring-ai-test).
Something like this
Then use it like this
@Kehrlann your thoughts