refactor(ChatModel): Standardize Non-blocking Behavior for ChatModelBase Implementations#642
Conversation
… scheduler to unify processing in chatmodel non-streaming mode.
Summary of ChangesHello @wuji1428, 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 refactors the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully standardizes the non-blocking behavior for non-streaming calls in DashScopeChatModel, OllamaChatModel, and OpenAIChatModel by using subscribeOn(Schedulers.boundedElastic()). The addition of ChatModelNonStreamingBlockingBehaviorTest is a great step to verify this behavior. My review includes suggestions to improve consistency in exception handling across the models and to enhance the new test file for better robustness and maintainability.
| new RuntimeException( | ||
| "DashScope API call failed: " + e.getMessage(), | ||
| e)); |
There was a problem hiding this comment.
For consistency with other models like OpenAIChatModel, it's better to use the more specific ModelException instead of a generic RuntimeException. This provides more structured error information.
| new RuntimeException( | |
| "DashScope API call failed: " + e.getMessage(), | |
| e)); | |
| new ModelException( | |
| "DashScope API call failed: " + e.getMessage(), | |
| e, modelName, "dashscope")); |
| try { | ||
| OllamaResponse response = httpClient.chat(request); | ||
| return Flux.just( | ||
| formatter.parseResponse(response, startTime)); | ||
| } catch (Exception e) { | ||
| return Flux.error(e); | ||
| } |
There was a problem hiding this comment.
The current exception handling is inconsistent with other models. It just re-throws the exception without logging or adding context. For better diagnostics and consistency, please log the error and wrap it in a ModelException, similar to OpenAIChatModel.
try {
OllamaResponse response = httpClient.chat(request);
return Flux.just(
formatter.parseResponse(response, startTime));
} catch (Exception e) {
log.error("Ollama API call failed: {}", e.getMessage(), e);
return Flux.error(new ModelException("Ollama API call failed: " + e.getMessage(), e, modelName, "ollama"));
}| return Flux.error( | ||
| new ModelException( | ||
| "Failed to call OpenAI API: " + e.getMessage(), | ||
| e, | ||
| modelName, | ||
| "openai")); |
There was a problem hiding this comment.
For better diagnostics and consistency with other models like DashScopeChatModel, it's good practice to log the exception before returning the error Flux.
log.error("Failed to call OpenAI API: {}", e.getMessage(), e);
return Flux.error(
new ModelException(
"Failed to call OpenAI API: " + e.getMessage(),
e,
modelName,
"openai"));| new MockResponse() | ||
| .setResponseCode(200) | ||
| .setBody("{\"request_id\":\"test\",\"output\":{\"choices\":[]}}") | ||
| .setHeader("Content-Type", "application/json")); |
There was a problem hiding this comment.
For consistency with the other tests in this file and to make the test more robust when testing asynchronous behavior, consider adding a body delay to the mock response.
| new MockResponse() | |
| .setResponseCode(200) | |
| .setBody("{\"request_id\":\"test\",\"output\":{\"choices\":[]}}") | |
| .setHeader("Content-Type", "application/json")); | |
| new MockResponse() | |
| .setResponseCode(200) | |
| .setBody("{\"request_id\":\"test\",\"output\":{\"choices\":[]}}") | |
| .setHeader("Content-Type", "application/json") | |
| .setBodyDelay(RESPONSE_DELAY_MS, TimeUnit.MILLISECONDS)); |
|
|
||
| DashScopeChatModel model = | ||
| DashScopeChatModel.builder().apiKey("test-key").modelName("qwen-max").stream(false) | ||
| .baseUrl(mockServer.url("/").toString().replaceAll("/$", "")) |
There was a problem hiding this comment.
The base URL construction logic mockServer.url("/").toString().replaceAll("/$", "") is duplicated in all three test methods. To improve maintainability and reduce duplication, consider extracting this logic into a baseUrl field initialized in the setUp() method.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.7, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
Add
subscribeOnto non-streaming calls to theOpenAIChatModel,DashScopeChatModel, andOllamaChatModelmodels. This maintains consistent blocking behavior for all non-streaming calls toChatModel.Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)Related Issues
Ref #571