-
-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[BugFix] Fix malformed output with streaming responses API #29044
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
Signed-off-by: Nick Hill <[email protected]>
|
|
||
| def append_output(self, output: RequestOutput) -> None: | ||
| output_token_ids = output.outputs[0].token_ids | ||
| self.parser = get_streamable_parser_for_assistant() |
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.
@heheda12345 it looks like you added this line in #22512... wonder if you could check this and claude's logic when you have a chance?
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.
Actually in response to the new test failure below, claude suggested to reinstate this line.
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.
What's not clear to me is why the non-streaming case resets the parser between turns but the streaming case uses the same parser and tries to feed the missing tokens into it to catch it back up with the expected state. On the surface, it feels like both of these should take the same approach - either resetting the parser between turns or doing the extra logic the streaming side does down in streaming's render_for_completion.
But, for the sake of trying to get this going without digging deeper into things here, I think putting this back like it was with the added comment at least makes it clear why it's doing this.
vllm/entrypoints/context.py
Outdated
| self.last_tok = toks[-1] | ||
| # TODO: add tool_output messages to self._messages | ||
| # Add tool output messages to self._messages | ||
| self._messages.extend(output) |
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 looks like a key fix to any scenario involving server-side builtin tools executed via the Responses API. Without this, the logic in render_for_completion(self) of the StreamingHarmonyContext would backtrack much further than just the <|start|>assistant tokens it is trying to do and would go all the way back to before the tool output, leading to who knows what as far as next tokens and state.
|
I ran the CI on this change in #27614, unfortunately a different test failed before it reached |
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
fee81d7 to
4d59d5e
Compare
Signed-off-by: Nick Hill <[email protected]>
Attempting to fix issues described in #28635.
This was written by claude code, I haven't yet scrutinized the logic.