-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Generative AI support for Spring Petclinic Microservices #281
base: main
Are you sure you want to change the base?
Conversation
…ts listing vets, listing owners, adding owners and adding pets to owners
Hi @odedia, thanks for this contribution I have to review. Since my last email to you and @showpune, I've reworked a bit your Over the next few months I plan to make a version with Lanchain4j. |
Thanks! |
Quality Gate failedFailed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
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.
Thank you @odedia for your contribution to modernizing the architecture by adding a very hype GenAI service in 2024.
I've given you a lot of feedback. Topics are open for discussion.
In order to be able to merge this PR on the main
branch, to remain open to as many developers as possible, I think the sample application needs to start without an OpenAI or Azure account. The genai servide should be optional or start in LLM mode.
@@ -0,0 +1,43 @@ | |||
spring: |
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.
suggestion: most of the configuration properties are located to the spring-petclinic-microservices-config
repository, especially the docker profile. I suggest to create another PR to report it.
https://github.com/spring-petclinic/spring-petclinic-microservices-config/
active: production | ||
config: | ||
import: optional:configserver:${CONFIG_SERVER_URL:http://localhost:8888/},optional:classpath:/creds.yaml | ||
#These apply when using spring-ai-azure-openai-spring-boot-starter |
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.
question: how could be the credentials managed in a Docker image exported to a registry? We have to answer this question whether we leave the config here or centralize it on the server config.
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.
I'm not sure if we can reliably export the credentials in the config server. Since this is private sensitive data, and the config server is publicly available on Github, the creds.yaml should remain in the source code of the genai service. It is listed for ignore in .gitignore.
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.
The config repository could be cloned in a private repository. It's URL could be provided with the CONFIG_SERVER_URL
environment variable. I agree: it's not very practical.
I'm not very fan of adding the creds.yaml
in a Docker image. We could use a Docker volume. But I'll prefer to use an environment variable like AZURE_OPENAI_KEY
and AZURE_OPENAI_ENDPOINT
. See spring-petclinic/spring-petclinic-ai@fbaf8dc#diff-54eeffbae371fcd1398d4ca5e89a1b8118208b7bb2f8ddf55c1aa2f7d98ab136R36
Developers can pass those parameteres from their IDE or from the Docker command line. What do you think about?
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.
Yep, that would work.
chat: | ||
client: | ||
enabled: true | ||
azure: |
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.
suggestion : in the spring-petclinic-ai
, I used the defaultFunctions
method of the ChatClient
. Like this we don't depend of Azure OpenAI or OpenAPI specificities. Are you agree with this approach?
See spring-petclinic/spring-petclinic-ai#2
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.
I have no preference one way or the other, it can be in the code instead.
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.
I prefer this because it avoids having duplicate configuration lines/
spring.ai.azure.openai.chat.options.functions=xxx,yyy
spring.ai.openai.chat.options.functions=xxx,yyy
*/ | ||
@Data | ||
@NoArgsConstructor | ||
public class VisitDetails { |
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.
suggestion: I propose to user records. Is there any technical constraint?
Same question for PetDetails
and OwnerDetails
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.
Records would be great, I simply opted to use the same classes that already exist in other microservices for clarity and cosistency.
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.
Last year, we began to remove Lombok: #251
Could you please try to use record? If it's too complicated, it doesn't matter: we'll postpone the change.
@RestController | ||
public class FallbackController { | ||
|
||
@RequestMapping("/fallback") |
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.
suggestion: add POST
(and GET
?) in the method
attributes
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.
Agreed
@@ -93,7 +93,25 @@ Each service has its own specific role and communicates via REST APIs. | |||
|
|||
![Spring Petclinic Microservices architecture](docs/microservices-architecture-diagram.jpg) | |||
|
|||
## Integrating the Spring AI Chatbot |
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.
suggestion: we need to offer a solution to developers who don't have an Azure or OpenAI account and don't want to pay for it: either enable fallback by default, or disable chat, or have a local solution with ollama. What do you think @odedia ?
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.
args: | ||
retries: 1 | ||
statuses: SERVICE_UNAVAILABLE | ||
methods: GET, POST |
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.
The POST
looks like to be enough. Isn't it?
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.
Seems like it, yes.
|
||
|
||
<script th:src="@{/webjars/bootstrap/5.3.3/dist/js/bootstrap.bundle.min.js}"></script> | ||
<script src="https://cdn.jsdelivr.net/npm/marked/marked.min.js"></script> |
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.
suggestion: to be consistent with bootstrap
we could declare marked
as a webjar
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.
Can you share an example for that?
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.
There are also somme issues detected by the Sonar Quality Gate. |
Sonarqube is mostly complaining about Code duplication, because I copied the data classes from existing microservices to the new one. |
I don't see a |
I got confused. I meant the |
Any help would be appreciated, I will try to implement the other changes towards the weekend. |
This PR adds a new microservice to support a generative ai chatbot based on Spring AI.
It supports natural language for performing activities like listing vets, listing owners, adding owners and adding pets to owners.
If the microservice is not booted, the chatbot simply response that chat is not available.
Based on https://github.com/spring-projects/spring-petclinic/tree/spring-ai
The code was adapted to keep domain boundaries - the functions now call the relevant microservice to perform the various activities as needed.
It also include an example of using RAG in the listVets function.
Thank you.