Skip to content

fix: honor skip_special_tokens from extra_body on the vLLM completion path#1478

Closed
DongjiGao wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
DongjiGao:fix/preserve-special-tokens
Closed

fix: honor skip_special_tokens from extra_body on the vLLM completion path#1478
DongjiGao wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
DongjiGao:fix/preserve-special-tokens

Conversation

@DongjiGao

@DongjiGao DongjiGao commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

What

The vLLM text-completion request builder hardcoded skip_special_tokens: False at the top level, while _build_request_body could simultaneously carry a user-supplied skip_special_tokens inside extra_body. The field was sourced twice, so any extra_body.skip_special_tokens was silently dropped on the completion path.

This pops skip_special_tokens out of extra_body into the single top-level field (default False), so inference.extra_body.skip_special_tokens is honored on the completion path — matching how the chat path already behaves.

Why

skip_special_tokens=false keeps special tokens (e.g. speaker tags) in the decoded output, which multispeaker ASR needs. The chat endpoint already respects this via extra_body; this makes the completion endpoint consistent, without adding any new config surface.

Usage (just extra_body, no new options):

++inference.extra_body.skip_special_tokens=false

Test

tests/test_vllm_completion.py:

  • default → top-level skip_special_tokens=False, absent from extra_body
  • extra_body.skip_special_tokens=True → lifted to the single top-level field, not duplicated

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Refactor vLLM completion request parameter building to extract skip_special_tokens from merged extra_body payload to the top-level request field, replacing hardcoded defaults. Add unit tests verifying default behavior and override lifting.

Changes

vLLM Detokenization Parameter Support

Layer / File(s) Summary
vLLM completion request builder refactoring
nemo_skills/inference/model/vllm.py
_build_completion_request_params now builds extra_body into a local variable, extracts skip_special_tokens via pop() with False default, and uses the modified payload as request extra_body instead of inlining _build_request_body(...) and hardcoding skip_special_tokens to False.
Completion request builder unit tests
tests/test_vllm_completion.py
New test module with _completion_request helper that mocks VLLMModel construction and calls _build_completion_request_params directly. Tests verify skip_special_tokens defaults to False when absent and is properly lifted to top-level without duplication when provided in extra_body.

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately and specifically describes the main change: honoring skip_special_tokens from extra_body on the vLLM completion path, which aligns with the core modification in VLLMModel._build_completion_request_params.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

… path

The text-completion request hardcoded `skip_special_tokens: False` at the top
level while `_build_request_body` could also carry a user-supplied
`skip_special_tokens` inside `extra_body`, so the field was sourced twice and
the extra_body value was silently dropped. Pop it from extra_body into the single
top-level field (default False) so `inference.extra_body.skip_special_tokens` is
honored on the completion path, matching how the chat path already behaves
(e.g. `=false` keeps speaker tags for multispeaker ASR).

Signed-off-by: Dongji Gao <dongjig@nvidia.com>
@DongjiGao
DongjiGao force-pushed the fix/preserve-special-tokens branch from ba8c38e to 35bf527 Compare June 9, 2026 04:47
@DongjiGao DongjiGao changed the title Add inference.skip_special_tokens / spaces_between_special_tokens options fix: honor skip_special_tokens from extra_body on the vLLM completion path Jun 9, 2026
@Kipok

Kipok commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

@DongjiGao please recreate from a branch

@DongjiGao

Copy link
Copy Markdown
Collaborator Author

Recreated as in-repo branch PR #1488 so CI can run (fork PRs don't receive CI secrets). Closing this fork PR in favor of #1488.

@DongjiGao DongjiGao closed this Jun 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants