[GPT-OSS] Add support for arrays at tool message content #25593
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
![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 aims to add support for array content in tool messages. The current implementation is partial, only handling the first element of a text array, which can lead to data loss and potential runtime errors for other array types. I've suggested a more robust implementation to correctly process all text parts in an array and ensure the content is always a string, preventing crashes and preserving all information.
luis5tb
force-pushed
the
tool-message-array
branch
from
bfa6f7e to
f3ac776
Compare
|
This directionally seems reasonable and looks like it would fix the issue of vLLM rejecting valid tool responses when they're in a list of content vs direct string. The Chat Completions API spec does not technically restrict the array of For the failing ruff pre-commit hook, see https://docs.vllm.ai/en/latest/contributing/#linting for how to run these linting checks locally. |
luis5tb
force-pushed
the
tool-message-array
branch
2 times, most recently
from
83482f0 to
da5e8eb
Compare
Ensure harmony utils can also handle arrays for tool message content Signed-off-by: Luis Tomas Bolivar <[email protected]>
luis5tb
force-pushed
the
tool-message-array
branch
from
da5e8eb to
520ac14
Compare
luis5tb
requested review from
DarkLight1337,
NickLucche,
robertgshaw2-redhat and
simon-mo
as code owners
|
The latest version of this looks good to me, with tests added for tool message content as a string, as a one-element list, and as a multi-element list. Thanks for helping find and fix these tool calling gaps! |
github-project-automation
bot
moved this from To Triage
to Ready
in gpt-oss Issues & Enhancements
github-actions
bot
added
the
ready
github-project-automation
bot
moved this from Ready
to Done
in gpt-oss Issues & Enhancements
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]> Signed-off-by: Dhruvil Bhatt <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]> Signed-off-by: bbartels <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]> Signed-off-by: 0xrushi <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]> Signed-off-by: 0xrushi <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]>
…t#25593) Signed-off-by: Luis Tomas Bolivar <[email protected]>
5 participants
Ensure harmony utils can also handle arrays for tool message content
This fixes the issue reported at #25001