[Bugfix] Fix inverted condition causing thinking_token_budget to be silently ignored - #41674
Conversation
…g_token_budget to be silently ignored When a request uses `thinking_token_budget` but has no penalty/bad-words settings (the common case), the previous code computed `needs_output_token_ids = False` due to an inverted boolean condition (`or not thinking_budget_tracks_reqs` instead of `or thinking_budget_tracks_reqs`). This caused `SamplingMetadata.output_token_ids` to be an empty list, so every call to `ThinkingBudgetStateHolder.update_state()` saw `seq_idx >= len([]) == 0` for all requests and skipped `_update_think_state()` entirely. As a result `in_end` was never set to `True`, `apply_to_logits()` never forced the end token, and the `thinking_token_budget` parameter had no effect. Fix: remove the erroneous `not` so that output_token_ids is populated whenever the thinking-budget state holder has any tracked requests. Regression test added in test_correctness.py that exercises ThinkingBudgetStateHolder directly, demonstrating the empty-list failure path and verifying the corrected live-list path raises `in_end` after the budget is exhausted. Co-authored-by: Claude Signed-off-by: JasonKeyiL <likey6688@gmail.com> Signed-off-by: Keyi Li <likey6688@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in gpu_input_batch.py where the thinking budget was not being enforced when no penalties or bad words were set. The logic for determining if output token IDs are needed was corrected by removing an incorrect negation, ensuring the thinking budget state holder receives the necessary data. Additionally, a regression test was added to verify that the budget is correctly enforced under these conditions. I have no feedback to provide.
|
Hi @njhill , could you please add the 'verified' label so CI can run? Thanks! |
| @@ -883,7 +876,7 @@ def _make_sampling_metadata(self) -> SamplingMetadata: | |||
| not self.no_penalties | |||
| or bool(self.bad_words_token_ids) | |||
| or self.logitsprocs_need_output_token_ids | |||
There was a problem hiding this comment.
Hey @JasonKeyiL , Thanks for catching this and for the fix. I was wondering: with reasoning mode on, ReasoningConfig is not None, so I’d expect self.logitsprocs_need_output_token_ids to be set, since that was verified with the E2E test as well. How did we end up with thinking_budget_tracks_reqs being set to True while self.logitsprocs_need_output_token_ids was False?
There was a problem hiding this comment.
Thanks for reply and follow up! @rishitdholakia13
sorry I missed it, you are right, self.logitsprocs_need_output_token_ids is already True at first whenever reasoning_config is not None, output_token_ids shouldn't be empty in practice
but isn't the or not thinking_budget_tracks_reqs logically inverted? but given it's masked, is it worth fixing or should I close this?
There was a problem hiding this comment.
Yes, definitely we can merge this fix.
There was a problem hiding this comment.
Thank you, Hi @njhill Nick, would you be able to take a look and put a verify label? thanks!
Signed-off-by: Keyi Li <likey6688@gmail.com>
Head branch was pushed to by a user without write access
|
@njhill looks like a globe infra issue, CI failed with |
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Liuweixiong0118 <lwx34158427@gmail.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Cherry-pick 62 bugfix/security PRs from upstream vllm-project/vllm main (2026-05-03 to 2026-06-17), covering scheduler, engine core, model runner, worker, attention, KV cache, compilation, and structured output fixes. Security (4): vllm-project#43286 vllm-project#44744 vllm-project#45118 vllm-project#45252 Bugfix (56): vllm-project#35536 vllm-project#36616 vllm-project#38895 vllm-project#39155 vllm-project#39324 vllm-project#39562 vllm-project#39805 vllm-project#40398 vllm-project#40726 vllm-project#40727 vllm-project#40737 vllm-project#40749 vllm-project#40961 vllm-project#41119 vllm-project#41133 vllm-project#41233 vllm-project#41237 vllm-project#41411 vllm-project#41496 vllm-project#41549 vllm-project#41674 vllm-project#41873 vllm-project#41895 vllm-project#42040 vllm-project#42112 vllm-project#42289 vllm-project#42479 vllm-project#42585 vllm-project#42692 vllm-project#42706 vllm-project#42709 vllm-project#42739 vllm-project#42967 vllm-project#43001 vllm-project#43079 vllm-project#43125 vllm-project#43160 vllm-project#43616 vllm-project#43669 vllm-project#43719 vllm-project#43768 vllm-project#43808 vllm-project#43961 vllm-project#43982 vllm-project#43988 vllm-project#43998 vllm-project#44057 vllm-project#44560 vllm-project#44574 vllm-project#44568 vllm-project#44603 vllm-project#44744 vllm-project#45195 vllm-project#45345 vllm-project#45383 vllm-project#45487 vllm-project#45564 vllm-project#45673 Runner fix (2): vllm-project#44568 vllm-project#44603 Skipped: vllm-project#43781 (ROCm-specific, not applicable to Ascend NPU) Conflict resolutions: - Manual merge: vllm-project#43286 vllm-project#45118 vllm-project#42112 vllm-project#43160 vllm-project#43719 vllm-project#44560 - Upstream-preferred (-X theirs): vllm-project#43808 vllm-project#43988 vllm-project#42967 vllm-project#35536 vllm-project#45195 - Test files (--theirs): vllm-project#44744 vllm-project#41895 vllm-project#42040 vllm-project#41233 vllm-project#45345 vllm-project#43982 Co-authored-by: GitHub Copilot Signed-off-by: MingqiWang-coder <mingqiwang@hust.edu.cn>
Cherry-pick 62 bugfix/security PRs from upstream vllm-project/vllm main (2026-05-03 to 2026-06-17), covering scheduler, engine core, model runner, worker, attention, KV cache, compilation, and structured output fixes. Security (4): vllm-project#43286 vllm-project#44744 vllm-project#45118 vllm-project#45252 Bugfix (56): vllm-project#35536 vllm-project#36616 vllm-project#38895 vllm-project#39155 vllm-project#39324 vllm-project#39562 vllm-project#39805 vllm-project#40398 vllm-project#40726 vllm-project#40727 vllm-project#40737 vllm-project#40749 vllm-project#40961 vllm-project#41119 vllm-project#41133 vllm-project#41233 vllm-project#41237 vllm-project#41411 vllm-project#41496 vllm-project#41549 vllm-project#41674 vllm-project#41873 vllm-project#41895 vllm-project#42040 vllm-project#42112 vllm-project#42289 vllm-project#42479 vllm-project#42585 vllm-project#42692 vllm-project#42706 vllm-project#42709 vllm-project#42739 vllm-project#42967 vllm-project#43001 vllm-project#43079 vllm-project#43125 vllm-project#43160 vllm-project#43616 vllm-project#43669 vllm-project#43719 vllm-project#43768 vllm-project#43808 vllm-project#43961 vllm-project#43982 vllm-project#43988 vllm-project#43998 vllm-project#44057 vllm-project#44560 vllm-project#44574 vllm-project#44568 vllm-project#44603 vllm-project#44744 vllm-project#45195 vllm-project#45345 vllm-project#45383 vllm-project#45487 vllm-project#45564 vllm-project#45673 Runner fix (2): vllm-project#44568 vllm-project#44603 Skipped: vllm-project#43781 (ROCm-specific, not applicable to Ascend NPU) Conflict resolutions: - Manual merge: vllm-project#43286 vllm-project#45118 vllm-project#42112 vllm-project#43160 vllm-project#43719 vllm-project#44560 - Upstream-preferred (-X theirs): vllm-project#43808 vllm-project#43988 vllm-project#42967 vllm-project#35536 vllm-project#45195 - Test files (--theirs): vllm-project#44744 vllm-project#41895 vllm-project#42040 vllm-project#41233 vllm-project#45345 vllm-project#43982 Co-authored-by: GitHub Copilot Signed-off-by: MingqiWang-coder <mingqiwang@hust.edu.cn>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
…ilently ignored (vllm-project#41674) Signed-off-by: Keyi Li <likey6688@gmail.com> Co-authored-by: Keyi Li <likey6688@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Summary
Fixes #41672.
Root cause: an inverted boolean condition (
or not thinking_budget_tracks_reqsinstead ofor thinking_budget_tracks_reqs) ingpu_input_batch.py. The condition is logically backwards: it setsneeds_output_token_ids = Truewhen there are no budget-tracking requests, and fails to contributeTruewhen there are budget-tracking requests.In practice the bug is currently masked —
self.logitsprocs_need_output_token_idsis set toTrueat init wheneverreasoning_config is not None(introduced in the same commit), sooutput_token_idsis never empty and the budget works correctly end-to-end. The fix corrects the latent logical error and removes the unnecessary population ofoutput_token_idswhen no budget-tracking requests are in the batch.Fix: remove the erroneous
not(single-token change ingpu_input_batch.py).Changes
vllm/v1/worker/gpu_input_batch.py: removenotfromor not thinking_budget_tracks_reqstests/v1/logits_processors/test_correctness.py: add regression testtest_thinking_budget_enforced_without_penaltiesthat directly exercisesThinkingBudgetStateHolderwith an empty vs. liveoutput_token_idslist, verifyingin_endis raised after the budget is exhaustedTest commands run
pre-commit run ruff-check --files vllm/v1/worker/gpu_input_batch.py tests/v1/logits_processors/test_correctness.py pre-commit run ruff-format --files vllm/v1/worker/gpu_input_batch.py tests/v1/logits_processors/test_correctness.py .venv/bin/python -m pytest tests/v1/logits_processors/test_correctness.py::test_thinking_budget_enforced_without_penalties -v # PASSED