Skip to content

[Frontend] [Bugfix] Refactor tool parsers and simplify the tool parsing interface.#11554

Closed
elementary-particle wants to merge 1 commit intovllm-project:mainfrom
elementary-particle:main
Closed

[Frontend] [Bugfix] Refactor tool parsers and simplify the tool parsing interface.#11554
elementary-particle wants to merge 1 commit intovllm-project:mainfrom
elementary-particle:main

Conversation

@elementary-particle
Copy link

@elementary-particle elementary-particle commented Dec 27, 2024

This is the PR for the RFC #11522. Currently we are building a draft of simpler tool parsers using streaming JSON parsing libraries to reduce overhead and avoid bugs. Tests and commits will be added gradually.

FIX #11392.

…` for streaming outputs.

We only use `delta_token_ids` and `delta_text` in the streaming code.
This avoids overhead and errors.
Note that finish reasons other than end of stream aren't addressed yet.

Signed-off-by: elementary-particle <quantum.field@outlook.com>
@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 do one of these:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@marcelodiaz558
Copy link

marcelodiaz558 commented Jan 31, 2025

@elementary-particle I have been testing your version of the Hermes tool parser with ijson, and it is working nicely, great job! It solved the issue I was facing here: #11279. I tested it on v0.7.0 and V1 mode.

Also, sometimes I encountered an issue regarding the JSON decode of arguments in the postprocessing step, but this small modification proposed by wangluyi fixed it: #9874 (comment).

I pushed a v0.7.0 Docker image with ijson installed and your commit 2f77b7b here: https://hub.docker.com/repository/docker/marcelodiaz/vllm-openai-hermes-fix.

07-02 EDIT: While testing a few models, I actually discovered that the issue I was facing might not be directly related to the hermes tool parser, instead, it happens when I set the tool_choice parameter to any value different than "auto", and it is reproducible with commit 2f77b7b of this PR. This issue was introduced in v0.6.5, I assume that it is related to the guided decoding used for the tool_choice parameter to work.

Nevertheless, the hermes tool parser of this PR works nicely and looks way cleaner than the one currently in the main branch.

@paolovic
Copy link
Contributor

paolovic commented Apr 3, 2025

Hi @elementary-particle ,
do you want to fix the PR?

@Endebert
Copy link

Endebert commented Apr 4, 2025

It would be really awesome if this could be merged into main 🙏 . I don't know what's blocking it, but with the state of this PR the strange (malformed) function calls we receive during streaming are resolved.

Surprised this isn't impacting a lot more people. No one else is using Qwen with streaming + function calling?

@mergify
Copy link

mergify bot commented Apr 4, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @elementary-particle.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@npuichigo
Copy link

@Endebert Same issues here when trying to use vllm + Qwen with langgraph. The args in tool_calls becomes None, which make the whole chain fail to use tools.

@wangtingshuai
Copy link

wangtingshuai commented Apr 16, 2025

I ran into a number conversion problem when using this feature, and here’s how I fixed it

def decimal_default(obj): 
    if isinstance(obj, decimal.Decimal): 
        return float(obj)   
    raise TypeError 

and change
arguments = json.dumps(arguments)
to
arguments = json.dumps(arguments, default=decimal_default)

@paolovic
Copy link
Contributor

def decimal_default(obj): 
    if isinstance(obj, decimal.Decimal): 
        return float(obj)   
    raise TypeError 

Hi @wangtingshuai ,
as this is stale, I created an active version of this PR here.

Thank you for your input, I incorporated it.

@hmellor
Copy link
Member

hmellor commented Mar 4, 2026

Closing this PR as stale. It has had unresolved merge conflicts since April 2025 with no author activity. A continuation was attempted in #16096 but that was also closed without merging. The tool parser codebase has evolved significantly since then with many individual fixes merged, so this would need to be reimplemented against the current codebase. If the broader refactoring is still desired, please open a fresh PR.

@hmellor hmellor closed this Mar 4, 2026
@mergify mergify bot added the bug Something isn't working label Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug]: [v0.6.5] Streaming tool call responses with the hermes template is inconsistent with the non-stream version.

7 participants