Skip to content

Conversation

WangErXiao
Copy link
Contributor

@WangErXiao WangErXiao commented Mar 9, 2025

Remove the mutual exclusion restriction between tool calling and reasoning parser, as models like QwQ-32B can support both functionalities simultaneously. Additionally, having tool calling parse only from the content rather than the reasoning_content can improve the accuracy and performance of tool calling.

This PR resolves the issue #14490

Copy link

github-actions bot commented Mar 9, 2025

👋 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 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 either: Add ready label to the PR or enable auto-merge.

🚀

@mergify mergify bot added the frontend label Mar 9, 2025
@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch from 3cef121 to df45cb1 Compare March 9, 2025 07:06
@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch from df45cb1 to d1a57e7 Compare March 9, 2025 07:57
@gaocegege
Copy link
Contributor

Thanks for the PR. I will help review it. FYI I had a refactor for the reasoning support #14428, not sure if you can use is_reasoning_end instead of a new func here.

@WangErXiao
Copy link
Contributor Author

Thanks for the PR. I will help review it. FYI I had a refactor for the reasoning support #14428, not sure if you can use is_reasoning_end instead of a new func here.

Thanks very much. I'll take a look at #14428. If you add is_reasoning_end, I will update the PR accordingly.

@WangErXiao
Copy link
Contributor Author

Thanks for the PR. I will help review it. FYI I had a refactor for the reasoning support #14428, not sure if you can use is_reasoning_end instead of a new func here.

I have reviewed PR #14428, and I think we can reuse the is_reasoning_end function. After your review, I will proceed with the refactoring.

Copy link
Contributor

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The streaming logic looks good to me, but does it also support full generation?

Besides this, we could also add some example code and tests

@gaocegege
Copy link
Contributor

And we should:

  • Remove It is not compatible with [tool_calling](https://docs.vllm.ai/en/latest/features/tool_calling.html). in reasoning outputs docs
  • Add a new column in models section in reasoning outputs docs to show which model supports tool calling

@WangErXiao
Copy link
Contributor Author

The streaming logic looks good to me, but does it also support full generation?

Besides this, we could also add some example code and tests

Yes, It support full generation, I will add some example code and tests.

@WangErXiao
Copy link
Contributor Author

And we should:

  • Remove It is not compatible with [tool_calling](https://docs.vllm.ai/en/latest/features/tool_calling.html). in reasoning outputs docs
  • Add a new column in models section in reasoning outputs docs to show which model supports tool calling

OK

@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch from d1a57e7 to 96558b1 Compare March 10, 2025 14:10
@mergify mergify bot added the documentation Improvements or additions to documentation label Mar 10, 2025
@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch 3 times, most recently from 01e43db to 0b9cc13 Compare March 10, 2025 14:41
Copy link
Contributor

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge this before the refactor, I could rebase in that PR.

@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch from 0b9cc13 to 62043bd Compare March 11, 2025 12:06
@WangErXiao
Copy link
Contributor Author

I think this pr can be ready to merge. @gaocegege @russellb

@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch from 62043bd to 0cc007f Compare March 11, 2025 23:30
@szpnygo
Copy link

szpnygo commented Mar 12, 2025

Will this request be included in version 0.7.4? When will it be released?

@gaocegege
Copy link
Contributor

Will this request be included in version 0.7.4? When will it be released?

@szpnygo I hope so.

@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch 3 times, most recently from 16e984e to 4b96d9c Compare March 12, 2025 13:17
@aarnphm
Copy link
Collaborator

aarnphm commented Mar 16, 2025

can you change the title to "[Frontend] Support tool calling and reasoning parser"

@WangErXiao WangErXiao changed the title [Frontend] Support both tool calling and reasoning parser for reasoni… [Frontend] Support tool calling and reasoning parser Mar 16, 2025
@WangErXiao
Copy link
Contributor Author

can you change the title to "[Frontend] Support tool calling and reasoning parser"

OK, done

@WangErXiao
Copy link
Contributor Author

cc @DarkLight1337 @simon-mo @mgoin

@simon-mo simon-mo added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 18, 2025
@WangErXiao WangErXiao force-pushed the tool_calling_for_reasoning_model branch from 2e87f90 to 47a7c16 Compare March 18, 2025 23:45
@gaocegege
Copy link
Contributor

@robertgshaw2-redhat @mgoin Could you please take a look? It blocks some other PRs about the reasoning parser.

@WangErXiao
Copy link
Contributor Author

cc @simon-mo

@simon-mo simon-mo merged commit d6cd59f into vllm-project:main Mar 23, 2025
57 checks passed
Copy link

@fwensen fwensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when streaming request model the output tool message will be incompleted

@WangErXiao
Copy link
Contributor Author

when streaming request model the output tool message will be incompleted

#15177

It's maybe tool parser problem.
You can change

                # Coalesce any additional queued outputs
                while not q.empty():
                    next_out = q.get_nowait()
                    if sampling_params.output_kind == RequestOutputKind.DELTA:
                        out.add(next_out)
                    else:
                        out = next_out

to

                # Coalesce any additional queued outputs
               if not q.empty():
                    next_out = q.get_nowait()
                    if sampling_params.output_kind == RequestOutputKind.DELTA:
                        out.add(next_out)
                    else:
                        out = next_out

erictang000 pushed a commit to erictang000/vllm that referenced this pull request Mar 25, 2025
wrmedford pushed a commit to wrmedford/vllm that referenced this pull request Mar 26, 2025
lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
shreyankg pushed a commit to shreyankg/vllm that referenced this pull request May 3, 2025
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/build documentation Improvements or additions to documentation frontend ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.