-
Notifications
You must be signed in to change notification settings - Fork 19
feat: support advanced non-incremental output #132
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: develop
Are you sure you want to change the base?
feat: support advanced non-incremental output #132
Conversation
Summary of ChangesHello @kevinlin09, 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 significantly improves the handling of non-incremental output in streaming calls for both 'Generation' and 'MultiModalConversation' APIs. It introduces an intelligent internal merging strategy that reconstructs complete responses, including complex tool call structures, from incremental streams. This allows users to request a single, consolidated output even when the underlying API streams data, enhancing usability and consistency. 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 support for non-incremental streaming output, which is particularly useful for features like tool calls. The implementation cleverly simulates this by internally using incremental streaming and accumulating the results. The changes are mostly in Generation.java
and MultiModalConversation.java
, with corresponding updates to sample files. My review focuses on the core merging logic, where I've identified a critical bug, some areas for performance improvement, and a potential issue with future extensibility. There is also significant code duplication between Generation.java
and MultiModalConversation.java
that should be addressed by refactoring the common logic into a shared component to improve maintainability.
if (existingCall instanceof ToolCallFunction && | ||
currentCall instanceof ToolCallFunction) { | ||
// Merge function calls | ||
ToolCallFunction existingFunctionCall = (ToolCallFunction) existingCall; | ||
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall; | ||
|
||
if (currentFunctionCall.getFunction() != null) { | ||
// Ensure existing function call has a function object | ||
if (existingFunctionCall.getFunction() == null) { | ||
existingFunctionCall.setFunction(existingFunctionCall.new CallFunction()); | ||
} | ||
|
||
// Accumulate arguments if present | ||
if (currentFunctionCall.getFunction().getArguments() != null) { | ||
String existingArguments = existingFunctionCall.getFunction().getArguments(); | ||
if (existingArguments == null) { | ||
existingArguments = ""; | ||
} | ||
String currentArguments = currentFunctionCall.getFunction().getArguments(); | ||
existingFunctionCall.getFunction().setArguments(existingArguments + currentArguments); | ||
} | ||
|
||
// Accumulate function name if present | ||
if (currentFunctionCall.getFunction().getName() != null) { | ||
String existingName = existingFunctionCall.getFunction().getName(); | ||
if (existingName == null) { | ||
existingName = ""; | ||
} | ||
String currentName = currentFunctionCall.getFunction().getName(); | ||
existingFunctionCall.getFunction().setName(existingName + currentName); | ||
} | ||
|
||
// Update function output if present | ||
if (currentFunctionCall.getFunction().getOutput() != null) { | ||
existingFunctionCall.getFunction().setOutput(currentFunctionCall.getFunction().getOutput()); | ||
} | ||
} | ||
|
||
// Update other fields with latest non-empty values | ||
if (currentFunctionCall.getIndex() != null) { | ||
existingFunctionCall.setIndex(currentFunctionCall.getIndex()); | ||
} | ||
if (currentFunctionCall.getId() != null && !currentFunctionCall.getId().isEmpty()) { | ||
existingFunctionCall.setId(currentFunctionCall.getId()); | ||
} | ||
if (currentFunctionCall.getType() != null) { | ||
existingFunctionCall.setType(currentFunctionCall.getType()); | ||
} | ||
} else { | ||
// Add new tool call (create a copy) | ||
if (currentCall instanceof ToolCallFunction) { | ||
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall; | ||
ToolCallFunction newFunctionCall = new ToolCallFunction(); | ||
newFunctionCall.setIndex(currentFunctionCall.getIndex()); | ||
newFunctionCall.setId(currentFunctionCall.getId()); | ||
newFunctionCall.setType(currentFunctionCall.getType()); | ||
|
||
if (currentFunctionCall.getFunction() != null) { | ||
ToolCallFunction.CallFunction newCallFunction = newFunctionCall.new CallFunction(); | ||
newCallFunction.setName(currentFunctionCall.getFunction().getName()); | ||
newCallFunction.setArguments(currentFunctionCall.getFunction().getArguments()); | ||
newCallFunction.setOutput(currentFunctionCall.getFunction().getOutput()); | ||
newFunctionCall.setFunction(newCallFunction); | ||
} | ||
|
||
accumulatedToolCalls.add(newFunctionCall); | ||
} else { | ||
// For other types of tool calls, add directly (assuming they are immutable or don't need merging) | ||
accumulatedToolCalls.add(currentCall); | ||
} | ||
} |
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 logic for merging tool calls is flawed. If a tool call with a given index already exists in accumulatedToolCalls
but is not of type ToolCallFunction
, the else
branch is taken, which incorrectly adds a new tool call with the same index. This leads to duplicate tool calls for the same index in the accumulated list.
The logic should be restructured to first check for the existence of a tool call with the given index, and then decide whether to merge with it (if it exists) or add a new one (if it doesn't).
if (existingCall != null) {
if (existingCall instanceof ToolCallFunction &&
currentCall instanceof ToolCallFunction) {
// Merge function calls
ToolCallFunction existingFunctionCall = (ToolCallFunction) existingCall;
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall;
if (currentFunctionCall.getFunction() != null) {
// Ensure existing function call has a function object
if (existingFunctionCall.getFunction() == null) {
existingFunctionCall.setFunction(existingFunctionCall.new CallFunction());
}
// Accumulate arguments if present
if (currentFunctionCall.getFunction().getArguments() != null) {
String existingArguments = existingFunctionCall.getFunction().getArguments();
if (existingArguments == null) {
existingArguments = "";
}
String currentArguments = currentFunctionCall.getFunction().getArguments();
existingFunctionCall.getFunction().setArguments(existingArguments + currentArguments);
}
// Accumulate function name if present
if (currentFunctionCall.getFunction().getName() != null) {
String existingName = existingFunctionCall.getFunction().getName();
if (existingName == null) {
existingName = "";
}
String currentName = currentFunctionCall.getFunction().getName();
existingFunctionCall.getFunction().setName(existingName + currentName);
}
// Update function output if present
if (currentFunctionCall.getFunction().getOutput() != null) {
existingFunctionCall.getFunction().setOutput(currentFunctionCall.getFunction().getOutput());
}
}
// Update other fields with latest non-empty values
if (currentFunctionCall.getIndex() != null) {
existingFunctionCall.setIndex(currentFunctionCall.getIndex());
}
if (currentFunctionCall.getId() != null && !currentFunctionCall.getId().isEmpty()) {
existingFunctionCall.setId(currentFunctionCall.getId());
}
if (currentFunctionCall.getType() != null) {
existingFunctionCall.setType(currentFunctionCall.getType());
}
}
} else {
// Add new tool call (create a copy)
if (currentCall instanceof ToolCallFunction) {
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall;
ToolCallFunction newFunctionCall = new ToolCallFunction();
newFunctionCall.setIndex(currentFunctionCall.getIndex());
newFunctionCall.setId(currentFunctionCall.getId());
newFunctionCall.setType(currentFunctionCall.getType());
if (currentFunctionCall.getFunction() != null) {
ToolCallFunction.CallFunction newCallFunction = newFunctionCall.new CallFunction();
newCallFunction.setName(currentFunctionCall.getFunction().getName());
newCallFunction.setArguments(currentFunctionCall.getFunction().getArguments());
newCallFunction.setOutput(currentFunctionCall.getFunction().getOutput());
newFunctionCall.setFunction(newCallFunction);
}
accumulatedToolCalls.add(newFunctionCall);
} else {
// For other types of tool calls, add directly (assuming they are immutable or don't need merging)
accumulatedToolCalls.add(currentCall);
}
}
if (existingCall instanceof ToolCallFunction && | ||
currentCall instanceof ToolCallFunction) { | ||
// Merge function calls | ||
ToolCallFunction existingFunctionCall = (ToolCallFunction) existingCall; | ||
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall; | ||
|
||
if (currentFunctionCall.getFunction() != null) { | ||
// Ensure existing function call has a function object | ||
if (existingFunctionCall.getFunction() == null) { | ||
existingFunctionCall.setFunction(existingFunctionCall.new CallFunction()); | ||
} | ||
|
||
// Accumulate arguments if present | ||
if (currentFunctionCall.getFunction().getArguments() != null) { | ||
String existingArguments = existingFunctionCall.getFunction().getArguments(); | ||
if (existingArguments == null) { | ||
existingArguments = ""; | ||
} | ||
String currentArguments = currentFunctionCall.getFunction().getArguments(); | ||
existingFunctionCall.getFunction().setArguments(existingArguments + currentArguments); | ||
} | ||
|
||
// Accumulate function name if present | ||
if (currentFunctionCall.getFunction().getName() != null) { | ||
String existingName = existingFunctionCall.getFunction().getName(); | ||
if (existingName == null) { | ||
existingName = ""; | ||
} | ||
String currentName = currentFunctionCall.getFunction().getName(); | ||
existingFunctionCall.getFunction().setName(existingName + currentName); | ||
} | ||
|
||
// Update function output if present | ||
if (currentFunctionCall.getFunction().getOutput() != null) { | ||
existingFunctionCall.getFunction().setOutput(currentFunctionCall.getFunction().getOutput()); | ||
} | ||
} | ||
|
||
// Update other fields with latest non-empty values | ||
if (currentFunctionCall.getIndex() != null) { | ||
existingFunctionCall.setIndex(currentFunctionCall.getIndex()); | ||
} | ||
if (currentFunctionCall.getId() != null && !currentFunctionCall.getId().isEmpty()) { | ||
existingFunctionCall.setId(currentFunctionCall.getId()); | ||
} | ||
if (currentFunctionCall.getType() != null) { | ||
existingFunctionCall.setType(currentFunctionCall.getType()); | ||
} | ||
} else { | ||
// Add new tool call (create a copy) | ||
if (currentCall instanceof ToolCallFunction) { | ||
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall; | ||
ToolCallFunction newFunctionCall = new ToolCallFunction(); | ||
newFunctionCall.setIndex(currentFunctionCall.getIndex()); | ||
newFunctionCall.setId(currentFunctionCall.getId()); | ||
newFunctionCall.setType(currentFunctionCall.getType()); | ||
|
||
if (currentFunctionCall.getFunction() != null) { | ||
ToolCallFunction.CallFunction newCallFunction = newFunctionCall.new CallFunction(); | ||
newCallFunction.setName(currentFunctionCall.getFunction().getName()); | ||
newCallFunction.setArguments(currentFunctionCall.getFunction().getArguments()); | ||
newCallFunction.setOutput(currentFunctionCall.getFunction().getOutput()); | ||
newFunctionCall.setFunction(newCallFunction); | ||
} | ||
|
||
accumulatedToolCalls.add(newFunctionCall); | ||
} else { | ||
// For other types of tool calls, add directly (assuming they are immutable or don't need merging) | ||
accumulatedToolCalls.add(currentCall); | ||
} | ||
} |
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 logic for merging tool calls is flawed. If a tool call with a given index already exists in accumulatedToolCalls
but is not of type ToolCallFunction
, the else
branch is taken, which incorrectly adds a new tool call with the same index. This leads to duplicate tool calls for the same index in the accumulated list.
The logic should be restructured to first check for the existence of a tool call with the given index, and then decide whether to merge with it (if it exists) or add a new one (if it doesn't).
if (existingCall != null) {
if (existingCall instanceof ToolCallFunction &&
currentCall instanceof ToolCallFunction) {
// Merge function calls
ToolCallFunction existingFunctionCall = (ToolCallFunction) existingCall;
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall;
if (currentFunctionCall.getFunction() != null) {
// Ensure existing function call has a function object
if (existingFunctionCall.getFunction() == null) {
existingFunctionCall.setFunction(existingFunctionCall.new CallFunction());
}
// Accumulate arguments if present
if (currentFunctionCall.getFunction().getArguments() != null) {
String existingArguments = existingFunctionCall.getFunction().getArguments();
if (existingArguments == null) {
existingArguments = "";
}
String currentArguments = currentFunctionCall.getFunction().getArguments();
existingFunctionCall.getFunction().setArguments(existingArguments + currentArguments);
}
// Accumulate function name if present
if (currentFunctionCall.getFunction().getName() != null) {
String existingName = existingFunctionCall.getFunction().getName();
if (existingName == null) {
existingName = "";
}
String currentName = currentFunctionCall.getFunction().getName();
existingFunctionCall.getFunction().setName(existingName + currentName);
}
// Update function output if present
if (currentFunctionCall.getFunction().getOutput() != null) {
existingFunctionCall.getFunction().setOutput(currentFunctionCall.getFunction().getOutput());
}
}
// Update other fields with latest non-empty values
if (currentFunctionCall.getIndex() != null) {
existingFunctionCall.setIndex(currentFunctionCall.getIndex());
}
if (currentFunctionCall.getId() != null && !currentFunctionCall.getId().isEmpty()) {
existingFunctionCall.setId(currentFunctionCall.getId());
}
if (currentFunctionCall.getType() != null) {
existingFunctionCall.setType(currentFunctionCall.getType());
}
}
} else {
// Add new tool call (create a copy)
if (currentCall instanceof ToolCallFunction) {
ToolCallFunction currentFunctionCall = (ToolCallFunction) currentCall;
ToolCallFunction newFunctionCall = new ToolCallFunction();
newFunctionCall.setIndex(currentFunctionCall.getIndex());
newFunctionCall.setId(currentFunctionCall.getId());
newFunctionCall.setType(currentFunctionCall.getType());
if (currentFunctionCall.getFunction() != null) {
ToolCallFunction.CallFunction newCallFunction = newFunctionCall.new CallFunction();
newCallFunction.setName(currentFunctionCall.getFunction().getName());
newCallFunction.setArguments(currentFunctionCall.getFunction().getArguments());
newCallFunction.setOutput(currentFunctionCall.getFunction().getOutput());
newFunctionCall.setFunction(newCallFunction);
}
accumulatedToolCalls.add(newFunctionCall);
} else {
// For other types of tool calls, add directly (assuming they are immutable or don't need merging)
accumulatedToolCalls.add(currentCall);
}
}
for (ToolCallBase accCall : accumulatedToolCalls) { | ||
if (accCall != null && accCall.getIndex() != null && | ||
accCall.getIndex().equals(index)) { | ||
existingCall = accCall; | ||
break; | ||
} | ||
} |
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 current implementation uses a linear search to find an existing tool call by its index within the accumulatedToolCalls
list. This is inefficient, with a time complexity that can be problematic if the number of tool calls is large.
For better performance, consider changing accumulatedToolCalls
from a List<ToolCallBase>
to a Map<Integer, ToolCallBase>
in the AccumulatedData
class. Using the tool call index as the key would allow for O(1) average time complexity for lookups. This would also simplify the mergeToolCalls
logic.
Example of the proposed change:
// In AccumulatedData class
private static class AccumulatedData {
StringBuilder content = new StringBuilder();
// Use a Map for efficient lookups by index.
// Use LinkedHashMap to preserve insertion order if needed.
Map<Integer, ToolCallBase> toolCalls = new LinkedHashMap<>();
}
// In mergeToolCalls method
private void mergeToolCalls(List<ToolCallBase> currentToolCalls, Map<Integer, ToolCallBase> accumulatedToolCalls) {
for (ToolCallBase currentCall : currentToolCalls) {
// ...
int index = currentCall.getIndex();
ToolCallBase existingCall = accumulatedToolCalls.get(index);
if (existingCall != null) {
// ... merge logic ...
} else {
// ... add new logic ...
accumulatedToolCalls.put(index, newCall);
}
}
}
// For other types of tool calls, add directly (assuming they are immutable or don't need merging) | ||
accumulatedToolCalls.add(currentCall); | ||
} |
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.
When adding a new tool call that is not a ToolCallFunction
, the currentCall
object is added directly to the accumulatedToolCalls
list. The comment mentions an assumption that other tool call types are immutable. This is a risky assumption and makes the code less robust for future extensions. If a new, mutable ToolCallBase
subclass is introduced, this could lead to unintended side effects where the object in the stream is modified.
It would be safer to enforce a copy/clone mechanism for all ToolCallBase
types to ensure that the accumulated data is always a deep copy.
for (ToolCallBase accCall : accumulatedToolCalls) { | ||
if (accCall != null && accCall.getIndex() != null && | ||
accCall.getIndex().equals(index)) { | ||
existingCall = accCall; | ||
break; | ||
} | ||
} |
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 current implementation uses a linear search to find an existing tool call by its index within the accumulatedToolCalls
list. This is inefficient, with a time complexity that can be problematic if the number of tool calls is large.
For better performance, consider changing accumulatedToolCalls
from a List<ToolCallBase>
to a Map<Integer, ToolCallBase>
in the AccumulatedData
class. Using the tool call index as the key would allow for O(1) average time complexity for lookups. This would also simplify the mergeToolCalls
logic.
Example of the proposed change:
// In AccumulatedData class
private static class AccumulatedData {
List<Map<String, Object>> content = new ArrayList<>();
// Use a Map for efficient lookups by index.
// Use LinkedHashMap to preserve insertion order if needed.
Map<Integer, ToolCallBase> toolCalls = new LinkedHashMap<>();
}
// In mergeToolCalls method
private void mergeToolCalls(List<ToolCallBase> currentToolCalls, Map<Integer, ToolCallBase> accumulatedToolCalls) {
for (ToolCallBase currentCall : currentToolCalls) {
// ...
int index = currentCall.getIndex();
ToolCallBase existingCall = accumulatedToolCalls.get(index);
if (existingCall != null) {
// ... merge logic ...
} else {
// ... add new logic ...
accumulatedToolCalls.put(index, newCall);
}
}
}
// For other types of tool calls, add directly (assuming they are immutable or don't need merging) | ||
accumulatedToolCalls.add(currentCall); | ||
} |
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.
When adding a new tool call that is not a ToolCallFunction
, the currentCall
object is added directly to the accumulatedToolCalls
list. The comment mentions an assumption that other tool call types are immutable. This is a risky assumption and makes the code less robust for future extensions. If a new, mutable ToolCallBase
subclass is introduced, this could lead to unintended side effects where the object in the stream is modified.
It would be safer to enforce a copy/clone mechanism for all ToolCallBase
types to ensure that the accumulated data is always a deep copy.
No description provided.