-
Notifications
You must be signed in to change notification settings - Fork 2k
GH-4596: Handle candidates containing both text and tool calls in VertexAiGeminiChatModel #4599
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
Conversation
…ol calls in VertexAiGeminiChatModel Signed-off-by: NathanGrand <nathangrand@quantexa.com>
String text = parts.stream() | ||
.filter(part -> part.hasText() && !part.getText().isEmpty()) | ||
.map(Part::getText) | ||
.collect(Collectors.joining(" ")); |
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 didn't make sense to me to be turning one candidate into multiple generations, but perhaps I'm misunderstanding?
Previous behaviour collected all parts containing function tools and combined into one AssistantMessage; idea here was to take the same approach here if multiple parts contain text.
Just testing this further manually |
Seems to be working well |
Hi @q-nathangrand, thanks for taking the time to submit a PR for this issue. Could you provide a test case that exhibits the initial problem (ie a response that contains a mixture of FunctionCall and non-FunctionCall parts). Either as maybe a prompt that triggers such a response, or even better as an integration test. Also, could you confirm that this issue could arise in both the streaming and non-streaming cases? |
Hey, I've only been testing it in the non-streaming version, but the Integration test wise, not sure if I'll get time this week. But the general idea is to encourage gemini to explain why it it calling tools as it does. Here's a snippet (sorry for the scala/java mix)
Here's a couple of grabs from debugger showing the issue ![]() ![]() |
Thanks for the extensive response, that should help us reproduce the issue and create an appropriate test! |
Merged to main as 8e8654e |
See: #4596