Skip to content

[Misc] Enhance warning information to user-defined chat template #15408

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

No Sign up for GitHub

By clicking “No 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? No Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Mar 26, 2025
Merged

[Misc] Enhance warning information to user-defined chat template #15408

merged 6 commits into from
Mar 26, 2025

Conversation

wwl2755
Copy link
Contributor

@wwl2755 wwl2755 commented Mar 24, 2025 •
edited by github-actions bot
Loading

Enhance warning information to user-defined chat template.

FIX #15395

cc @xihajun @jeejeelee

@wwl2755
add warning information to user-defined chat template
9b676b7 
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
@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.

🚀

@mergify mergify bot added the frontend label Mar 24, 2025
@jeejeelee jeejeelee requested a review from DarkLight1337 March 25, 2025 07:06
@jeejeelee
Copy link
Collaborator

Hi, please address the CI failure then

@wwl2755
change to single logger.warning
27d6b36 
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
@wwl2755
add comparison with HF chat template
b118977 
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
@wwl2755
use resolve_hf_chat_template()
a325cde 
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
@wwl2755 wwl2755 requested a review from DarkLight1337 March 25, 2025 09:16
@wwl2755
move the logic to resolve_mistral_chat_template()
c35d7f9 
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
@wwl2755
Copy link
Contributor Author

wwl2755 commented Mar 25, 2025

Updated. PTAL.

Though after a second thought, maybe several warning lines added to resolve_hf_chat_template could be the simplest solution.

@DarkLight1337
Copy link
Member

Since resolve_hf_chat_template doesn't take in two chat templates for comparison, I think the current PR is fine. Thanks for adding this!

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) March 25, 2025 17:13
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 25, 2025
@wwl2755
Copy link
Contributor Author

wwl2755 commented Mar 25, 2025

Since resolve_hf_chat_template doesn't take in two chat templates for comparison, I think the current PR is fine. Thanks for adding this!

Yes. The thought was to add comparison logic into the resolve_hf_chat_template. Anyway, glad you are happy with it!

@DarkLight1337
Copy link
Member

Please fix the test

@wwl2755
fix test failure
4212f56 
Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
auto-merge was automatically disabled March 26, 2025 03:49

Head branch was pushed to by a user without write access

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) March 26, 2025 03:49
jeejeelee
jeejeelee approved these changes Mar 26, 2025
View reviewed changes
Copy link
Collaborator

@jeejeelee jeejeelee left a comment

Choose a reason for hiding this comment

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

Thank you for your improvement and patience.

@vllm-bot vllm-bot merged commit 99f536f into vllm-project:main Mar 26, 2025
33 of 35 checks passed
lengrongfu pushed a commit to lengrongfu/vllm that referenced this pull request Apr 2, 2025
@wwl2755
[Misc] Enhance warning information to user-defined chat template (vll…
71caaae 
…m-project#15408)

Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025
@wwl2755 @lulmer
[Misc] Enhance warning information to user-defined chat template (vll…
01484d1 
…m-project#15408)

Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
Signed-off-by: Louis Ulmer <ulmerlouis@gmail.com>
nishith-fujitsu pushed a commit to nishith-fujitsu/vllm that referenced this pull request Apr 9, 2025
@wwl2755 @nishith-fujitsu
[Misc] Enhance warning information to user-defined chat template (vll…
9d91bf2 
…m-project#15408)

Signed-off-by: wwl2755 <wangwenlong2755@gmail.com>
@ckhordiasma ckhordiasma mentioned this pull request Apr 17, 2025
Closed
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Reviewers

@jeejeelee jeejeelee jeejeelee 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
No one assigned
Labels
frontend ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: Add Warning for Chat Template Mismatches similar to SGLang
4 participants
@wwl2755 @jeejeelee @DarkLight1337 @vllm-bot