Skip to content

Conversation

@chaunceyjiang
Copy link
Collaborator

@chaunceyjiang chaunceyjiang commented Oct 24, 2025

Purpose

fix test_named_tool_use

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang chaunceyjiang added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 24, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to fix test_named_tool_use by making it more deterministic with temperature=0.0 and modifying a user prompt. While the intent is clear, there is a critical issue in the construction of the user prompt that will likely lead to test failures. A space is missing between two concatenated strings, which will create a malformed prompt for the model. My review provides a specific code suggestion to correct this.

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang chaunceyjiang force-pushed the fix_test_named_tool_use branch from f9e97c1 to d1ee9b8 Compare October 24, 2025 09:00
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang
Copy link
Collaborator Author

/cc @DarkLight1337 PTAL

@DarkLight1337 DarkLight1337 merged commit 41a6256 into vllm-project:main Oct 24, 2025
19 checks passed
@DarkLight1337
Copy link
Member

Thanks for fixing!

atalhens pushed a commit to atalhens/vllm that referenced this pull request Oct 24, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang chaunceyjiang deleted the fix_test_named_tool_use branch October 24, 2025 14:08
kingsmad pushed a commit to kingsmad/vllm that referenced this pull request Oct 25, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
rohin-garg pushed a commit to rohin-garg/vllm that referenced this pull request Oct 25, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
0xrushi pushed a commit to 0xrushi/vllm that referenced this pull request Oct 26, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: 0xrushi <6279035+0xrushi@users.noreply.github.com>
0xrushi pushed a commit to 0xrushi/vllm that referenced this pull request Oct 26, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Signed-off-by: 0xrushi <6279035+0xrushi@users.noreply.github.com>
ilmarkov pushed a commit to neuralmagic/vllm that referenced this pull request Nov 7, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

2 participants