Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

odedia
Copy link

@odedia odedia commented Oct 9, 2024

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.

image

…ts listing vets, listing owners, adding owners and adding pets to owners
@arey
Copy link
Member

arey commented Oct 9, 2024

Hi @odedia, thanks for this contribution I have to review.

Since my last email to you and @showpune, I've reworked a bit your spring-ai branch and published it in a brand new repository: https://github.com/spring-petclinic/spring-petclinic-springai
The chatbot is always active. I will push other improvements trough Pull Request.

Over the next few months I plan to make a version with Lanchain4j.

@odedia
Copy link
Author

odedia commented Oct 9, 2024

Thanks!
The purpose of this exercise is to show spring ai naturally integrating with a Microservices architecture.
the chatbot will not do anything if the genai microservice is not running.

Copy link

sonarcloud bot commented Oct 11, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
6 Security Hotspots
3.1% Duplication on New Code (required ≤ 3%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Copy link
Member

@arey arey left a 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:
Copy link
Member

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
Copy link
Member

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.

Copy link
Author

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.

Copy link
Member

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?

Copy link
Author

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:
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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 {
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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")
Copy link
Member

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

Copy link
Author

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
Copy link
Member

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 ?

Copy link
Author

Choose a reason for hiding this comment

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

I began with using Llama 3.1, but the chat client became incredibly verbose and not as useful as OpenAI, that's why I switched to it.
image

args:
retries: 1
statuses: SERVICE_UNAVAILABLE
methods: GET, POST
Copy link
Member

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?

Copy link
Author

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>
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

@arey
Copy link
Member

arey commented Oct 12, 2024

There are also somme issues detected by the Sonar Quality Gate.

@odedia
Copy link
Author

odedia commented Oct 13, 2024

Sonarqube is mostly complaining about Code duplication, because I copied the data classes from existing microservices to the new one.
However this is the correct approach in a microservices architecture, you don't want to have dependencies on other microservices.
A better approach would be to use contract-based DTO generation but that's a rather large undertaking for the entire project.

@odedia
Copy link
Author

odedia commented Oct 13, 2024

I don't see a develop branch in the upstream project, is that exposed to all?

@arey
Copy link
Member

arey commented Oct 14, 2024

Sonarqube is mostly complaining about Code duplication, because I copied the data classes from existing microservices to the new one. However this is the correct approach in a microservices architecture, you don't want to have dependencies on other microservices. A better approach would be to use contract-based DTO generation but that's a rather large undertaking for the entire project.

Putting code duplication to one side.
We can focus on the 12 issues. I can help you if you want.
image

@arey
Copy link
Member

arey commented Oct 14, 2024

I don't see a develop branch in the upstream project, is that exposed to all?

I got confused. I meant the main branch

@odedia
Copy link
Author

odedia commented Oct 14, 2024

Any help would be appreciated, I will try to implement the other changes towards the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants