Skip to content

[Rust Frontend] Extend renderer/parser roundtrip tests to support token ids#47110

Merged
njhill merged 2 commits into
mainfrom
bz/roundtrip-token-ids-harmony
Jun 30, 2026
Merged

[Rust Frontend] Extend renderer/parser roundtrip tests to support token ids#47110
njhill merged 2 commits into
mainfrom
bz/roundtrip-token-ids-harmony

Conversation

@BugenZhao

@BugenZhao BugenZhao commented Jun 30, 2026

Copy link
Copy Markdown
Member

Purpose

Some model paths are token-id native: renderers may directly yield token ids, and output processors may directly consume token ids. This PR extends the roundtrip tests to support raw token ids in those cases, demonstrated by the GPT-OSS/Harmony path.

Test Plan

cargo fmt --all
cargo nextest run -p vllm-chat roundtrip_gpt_oss
cargo nextest run -p vllm-chat roundtrip_qwen3_reasoning_and_content
git diff --check

Test Result

All passed locally. I also verified that roundtrip_gpt_oss fails if generated chunk token ids are cleared, which confirms the test covers the token-id output path.


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.

…en ids

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@BugenZhao
BugenZhao requested a review from njhill as a code owner June 30, 2026 04:24

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@mergify mergify Bot added the rust label Jun 30, 2026
@BugenZhao BugenZhao added the ready ONLY add when PR is ready to merge/full CI is needed label Jun 30, 2026
@BugenZhao

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5870043ae

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


gemma4 => [tool_call_mix], // Gemma4 strips reasoning in history if there's no tool call
kimi_k25 => [tool_call_mix], // Kimi K2.5 strips reasoning in history
gpt_oss => [tool_call_mix], // Harmony strips reasoning in history if there's no tool call

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Take the Harmony cache lock for GPT-OSS roundtrip

This new GPT-OSS case loads the Harmony renderer/output path, which calls load_harmony_encoding; the Harmony output tests document that tests loading this encoding need the shared harmony_tiktoken_cache file lock so cargo nextest cannot populate the upstream tiktoken cache concurrently (rust/src/chat/src/output/harmony/tests.rs:1-4). Because this integration test only takes the per-model hf_gpt_oss lock, a full cargo nextest run -p vllm-chat can run it in parallel with the locked Harmony tests and reintroduce that cache race; add the same #[file_serial(harmony_tiktoken_cache)] attribute to this fixture.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch but not correct fix direction. I just realized after #46799 we don't even need existing harmony_tiktoken_cache anymore, since it embeds necessary vocab files directly into the binary and won't download at all.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
@njhill
njhill merged commit bdbd727 into main Jun 30, 2026
27 checks passed
@njhill
njhill deleted the bz/roundtrip-token-ids-harmony branch June 30, 2026 13:27
rjrock pushed a commit to rjrock/vllm that referenced this pull request Jul 1, 2026
lkk12014402 pushed a commit to lkk12014402/vllm that referenced this pull request Jul 8, 2026
noooop pushed a commit to noooop/vllm that referenced this pull request Jul 9, 2026
…en ids (vllm-project#47110)

Signed-off-by: Bugen Zhao <i@bugenzhao.com>
Signed-off-by: wang.yuqi <yuqi.wang@daocloud.io>
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 rust

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants