Skip to content

Initialize the delta tool call fields explicitly #17340

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
May 12, 2025
Merged

Initialize the delta tool call fields explicitly #17340

merged 7 commits into from
May 12, 2025

Conversation

maxdebayser
Copy link
Contributor

@maxdebayser maxdebayser commented Apr 29, 2025 •
edited by github-actions bot
Loading

Fixes #16340
Continuing the work of #16340

Previously the implementation relied on a combination of the default field values being and
model_dump_json(exclude_unset=True). This led to bugs such as the "type" field missing from the response.

The behavior of the OpenAI API is to set the "id" and "type" fields only in the first Delta object.

The purpose of this commit is to make all the places where these fields need to be set explicit.

@github-actions GitHub Actions
Copy link

👋 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.

🚀

@maxdebayser @iGmainC
Initialize the delta tool call fields explicitly
2fa00dd 
Previously the implementation relied on a combination
of the default field values being and
`model_dump_json(exclude_unset=True)`. This led to bugs such
as the "type" field missing from the response.

The behavior of the OpenAI API is to set the "id" and "type"
fields only in the first Delta object.

The purpose of this commit is to make all the places
where these fields need to be set explicit.

Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: igmainc <igmainc@icloud.com>
@maxdebayser maxdebayser force-pushed the fix_delta_tool_calls branch from 3fa8474 to 2fa00dd Compare April 29, 2025 01:32
@maxdebayser maxdebayser mentioned this pull request Apr 29, 2025
Closed
1 task
@DarkLight1337 DarkLight1337 self-assigned this Apr 29, 2025
@DarkLight1337
Copy link
Member

This looks good to me, let's have @iGmainC verify first though

@maxdebayser
Copy link
Contributor Author

Agreed

@maxdebayser
Merge branch 'upstream_main' into fix_delta_tool_calls
0f23a78 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@maxdebayser
Don't send fields that are None
1f7dc03 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@mergify Mergify
Copy link

mergify bot commented Apr 30, 2025

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

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

@mergify mergify bot added the needs-rebase label Apr 30, 2025
@maxdebayser
Merge branch 'upstream_main' into fix_delta_tool_calls
2baa1c0 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@mergify mergify bot removed the needs-rebase label Apr 30, 2025
maxdebayser added 2 commits May 1, 2025 13:10
@maxdebayser
Merge branch 'upstream_main' into fix_delta_tool_calls
c116d59
@maxdebayser
trigger ci
c206312 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@iGmainC
Copy link
Contributor

iGmainC commented May 2, 2025

This looks good to me, let's have @iGmainC verify first though

OK, I will test it as soon as possible.

@maxdebayser
Fix test
1428589 
The delta too call type can only be set once during the stream

Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
@aarnphm aarnphm self-requested a review May 2, 2025 18:57
aarnphm
aarnphm approved these changes May 2, 2025
View reviewed changes
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@DarkLight1337
Copy link
Member

Tests are failing though

@maxdebayser
Copy link
Contributor Author

@iGmainC , did you get a chance to test on your side? The CI tests are passing now.

@iGmainC
Copy link
Contributor

iGmainC commented May 12, 2025

@iGmainC , did you get a chance to test on your side? The CI tests are passing now.

I briefly tested my scene and it seemed to be ok.

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) May 12, 2025 11:52
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label May 12, 2025
@DarkLight1337 DarkLight1337 merged commit 05a4324 into vllm-project:main May 12, 2025
56 checks passed
mawong-amd pushed a commit to ROCm/vllm that referenced this pull request May 14, 2025
@maxdebayser @iGmainC @mawong-amd
Initialize the delta tool call fields explicitly (vllm-project#17340)
297d353 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: igmainc <igmainc@icloud.com>
@NickLucche NickLucche mentioned this pull request May 15, 2025
Closed
zzzyq pushed a commit to zzzyq/vllm that referenced this pull request May 24, 2025
@maxdebayser @iGmainC
Initialize the delta tool call fields explicitly (vllm-project#17340)
2c35547 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: igmainc <igmainc@icloud.com>
Signed-off-by: Yuqi Zhang <yuqizhang@google.com>
@cjackal cjackal mentioned this pull request Jun 14, 2025
Closed
1 task
minpeter pushed a commit to minpeter/vllm that referenced this pull request Jun 24, 2025
@maxdebayser @iGmainC @minpeter
Initialize the delta tool call fields explicitly (vllm-project#17340)
beb1fbf 
Signed-off-by: Max de Bayser <mbayser@br.ibm.com>
Co-authored-by: igmainc <igmainc@icloud.com>
Signed-off-by: minpeter <kali2005611@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aarnphm aarnphm aarnphm approved these changes

@DarkLight1337 DarkLight1337 DarkLight1337 approved these changes

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

@simon-mo simon-mo Awaiting requested review from simon-mo simon-mo is a code owner

Assignees

@DarkLight1337 DarkLight1337

Labels
frontend ready ONLY add when PR is ready to merge/full CI is needed tool-calling
Projects
Tool Calling
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Missing "type":"function" in OpenAI-Compatible Streaming Tool Calls with specific tool_choice
4 participants
@maxdebayser @DarkLight1337 @iGmainC @aarnphm