-
Notifications
You must be signed in to change notification settings - Fork 548
feat: add exchange rates currency sample #434
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @cmtoan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive new sample agent written in Java, designed to handle currency exchange rate queries. The agent exemplifies advanced features of the A2A protocol, such as managing multi-turn conversations where it can request additional user input, and providing real-time updates through streaming responses. It integrates with an external API for live data and uses an LLM for intelligent processing, offering a robust example for developing interactive and data-driven agents. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new sample agent for currency conversion, which is a great addition to demonstrate multi-turn dialogue and streaming responses. The implementation using Quarkus and LangChain4j is well-structured. My review focuses on improving code clarity, robustness, and adherence to best practices. I've included suggestions for making configurations more flexible, enhancing logging, improving error handling in the API client, and refactoring for better readability.
| public static void main(String[] args) { | ||
| String serverUrl = DEFAULT_SERVER_URL; | ||
|
|
||
| try { | ||
| System.out.println("Connecting to currency agent at: " + serverUrl); | ||
|
|
||
| // Fetch the public agent card | ||
| AgentCard publicAgentCard = | ||
| new A2ACardResolver(serverUrl).getAgentCard(); | ||
| System.out.printf(""" | ||
| Successfully fetched public agent card: | ||
| %s | ||
| Using public agent card for client initialization.%n | ||
| """, objectMapper.writeValueAsString(publicAgentCard)); | ||
|
|
||
| ClientConfig clientConfig = new ClientConfig.Builder() | ||
| .setAcceptedOutputModes(List.of("text")) | ||
| .build(); | ||
|
|
||
| // Create and send the message without INPUT_REQUIRED | ||
| StreamingClient streamingClient = getStreamingClient(publicAgentCard, clientConfig); | ||
| sendMessage(streamingClient.client(), "how much is 10 USD in INR?", null, null, streamingClient.messageResponse()); | ||
|
|
||
| // Create and send the message with INPUT_REQUIRED | ||
| StreamingClient streamingClient1 = getStreamingClient(publicAgentCard, clientConfig); | ||
| StreamingClient streamingClient2 = getStreamingClient(publicAgentCard, clientConfig); | ||
|
|
||
| MessageResponse message1 = sendMessage(streamingClient1.client(), "How much is the exchange rate for 1 USD?", null, null, streamingClient1.messageResponse()); | ||
|
|
||
| // Resubscribe to a task to send additional information for this task | ||
| streamingClient2.client().resubscribe(new TaskIdParams(message1.taskId()), | ||
| streamingClient2.consumers(), streamingClient2.streamingErrorHandler()); | ||
| sendMessage(streamingClient2.client(), "CAD", message1.contextId(), message1.taskId(), streamingClient2.messageResponse()); | ||
| } catch (Exception e) { | ||
| System.err.println("An error occurred: " + e.getMessage()); | ||
| e.printStackTrace(); | ||
| } | ||
| } |
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 main method is quite long and handles multiple scenarios (single-turn and multi-turn conversations). To improve readability and maintainability, consider refactoring it by extracting the logic for each scenario into separate private methods, for example runSingleTurnDemo() and runMultiTurnDemo().
| if (!data.containsKey("rates")) { | ||
| Log.error("Invalid API response format"); | ||
| } |
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.
This validation only logs an error but still returns the potentially invalid data map. This could confuse the LLM, as the tool output would not match its expectations. It's better to throw a RuntimeException here to signal that the tool execution has failed. LangChain4j will catch this and can potentially retry or inform the LLM about the failure.
| if (!data.containsKey("rates")) { | |
| Log.error("Invalid API response format"); | |
| } | |
| if (data == null || !data.containsKey("rates")) { | |
| throw new RuntimeException("Invalid API response format from Frankfurter API"); | |
| } |
| throw new RuntimeException("Request exchange rate failed with status " + response.statusCode()); | ||
| } | ||
|
|
||
| Map<String, Object> data = mapper.readValue(response.body(), Map.class); |
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.
Deserializing the API response to a generic Map<String, Object> is not type-safe and can lead to runtime errors if the API response structure changes. It's a better practice to create a dedicated DTO (Data Transfer Object) class that models the expected JSON response. This improves code readability and robustness.
For example:
public record FrankfurterResponse(java.math.BigDecimal amount, String base, String date, java.util.Map<String, java.math.BigDecimal> rates) {}Then you can deserialize to this record: FrankfurterResponse data = mapper.readValue(response.body(), FrankfurterResponse.class);
| String serverUrl = DEFAULT_SERVER_URL; | ||
|
|
||
| try { | ||
| System.out.println("Connecting to currency agent at: " + serverUrl); |
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.
| ```` | ||
| ollama pull qwen2.5:7b | ||
| ollama run qwen2.5:7b | ||
| ```` |
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 code block uses four backticks (````), which is not standard Markdown and may not render correctly on all platforms. Please use three backticks and specify the language for syntax highlighting.
| ```` | |
| ollama pull qwen2.5:7b | |
| ollama run qwen2.5:7b | |
| ```` | |
| ```bash | |
| ollama pull qwen2.5:7b | |
| ollama run qwen2.5:7b | |
| ``` |
| return new AgentCard.Builder() | ||
| .name("Currency Agent") | ||
| .description("Assistant for currency conversions") | ||
| .url("http://localhost:" + getHttpPort()) |
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.
|
|
||
| // call the currency agent with the message | ||
| ResponseFormat response = agent.handleRequest(message); | ||
| System.out.printf("Response: %s %n", response); |
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.
It's better to use a proper logging framework (like the one provided by Quarkus, io.quarkus.logging.Log) instead of System.out.printf. This provides more control over log levels and output formats. You would need to add import io.quarkus.logging.Log; to the file.
| System.out.printf("Response: %s %n", response); | |
| Log.infof("Response: %s", response); |
|
|
||
| ## How It Works | ||
|
|
||
| This agent uses Ollama LLM (for example qwen2.5:7b) to provide currency exchange information. |
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.
A minor grammar fix would improve readability. It's more natural to say "an Ollama LLM" and add a comma after "for example".
| This agent uses Ollama LLM (for example qwen2.5:7b) to provide currency exchange information. | |
| This agent uses an Ollama LLM (for example, qwen2.5:7b) to provide currency exchange information. |
| @@ -0,0 +1,144 @@ | |||
| # Currency Exchange Rates | |||
|
|
|||
| This sample demonstrates a currency conversion agent and exposed through the A2A protocol. | |||
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's a minor grammatical error here. It should be "...agent exposed through..." instead of "...agent and exposed through...".
| This sample demonstrates a currency conversion agent and exposed through the A2A protocol. | |
| This sample demonstrates a currency conversion agent exposed through the A2A protocol. |
| public ResponseFormat(String message) { | ||
| this(Status.INPUT_REQUIRED, message); | ||
| } |
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.
967d256 to
9f768f9
Compare
063b2cc to
b669e42
Compare
|
@fjuma When you have time, could you review this PR? |
…ulti-turn dialogue and streaming responses.
f8b1892 to
14b151a
Compare
…ulti-turn dialogue and streaming responses.
…ulti-turn dialogue and streaming responses.
Description
Add sample agent to provide currency conversion. This agent demonstrates multi-turn dialogue (input-required) and streaming responses.