Repository navigation
[https://nvbugs/6891162][fix] Stop test_generate_with_stop_words on the token Qwen3 generates for N - #20673
Conversation
…he token Qwen3 generates for N
test_generate_with_stop_words derived its stop id with
tokenizer.encode("N")[-1]. With TinyLlama's SentencePiece tokenizer that
is the same token the model generates after "M". With the Qwen3-0.6B
default model, BPE encodes a bare "N" as id 45. The model generates the
space-prefixed " N" (id 451), so end_id/stop_token_ids never matched
and generation ran to the full length. Derive the id from " N". Every
existing expectation in the test then holds, and its waiver is removed.
test_generate_with_bad_words stays waived: its first reference string
was captured with TinyLlama and needs a test-owner decision.
Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe stop-word test now derives ChangesStop-word test
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title identifies the NVBugs issue, the fix type, and the specific stop-token test change. It is clear and directly related to the primary change. Description check The description explains the failure, root cause, fix, test coverage, waiver removal, and remaining scope. It includes the required Description, Test Coverage, and PR Checklist sections. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
mikeiovine
left a comment
mikeiovine
left a comment
There was a problem hiding this comment.
Stamp on behalf of runtime devs
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
Bug engineer ownership @mikeiovine, as the engineer assigned to the linked bug, you are responsible for driving this bot PR to resolution. Please coordinate review, address feedback and required code changes, get CI passing, and verify the original failure before driving it to merge. If the PR is obsolete or superseded, close it with a reason. Keep this PR updated with blockers and the next step. |
Resolve the waives.txt conflict with NVIDIA#20758, which removed the adjacent test_generate_with_bad_words waiver line. No change to this PR's fix. Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
|
/bot run --disable-fail-fast --extra-stage "A100X-PyTorch-Post-Merge-1" |
|
Merged CI is started with |
|
PR_Github #80129 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
In the previous run, |
|
PR_Github #80188 [ run ] triggered by Bot. Commit: |
|
PR_Github #80129 [ run ] completed with state |
|
PR_Github #80188 [ run ] completed with state |
Description
unittest/llmapi/test_llm.py::test_generate_with_stop_wordshas failed since the default model oftest_llm.pychanged from TinyLlama-1.1B-Chat to Qwen3-0.6B (#19178).Root cause. The test derives its stop token with
tokenizer.encode("N", add_special_tokens=False)[-1]. TinyLlama's SentencePiece tokenizer gives the same token the model generates after"... L M". Qwen3's BPE tokenizer encodes a bare"N"as id 45, but continuing the prompt"A B C"the model generates the space-prefixed" N", id 451. Soend_id/stop_token_idsnever match, and generation runs to its length limit:Fix. Derive the stop id from
" N", the token the model actually generates. The test's expectations, criteria and other cases are unchanged. This test's waiver is removed.test_generate_with_bad_words(same bug) is not changed here and stays waived. Its first reference was captured with TinyLlama, and itsbad="I J"/bad=["F E", "I J"]strings do not match the space-prefixed tokens Qwen3 generates. Updating them is left to the test owner.Test Coverage
On an A100 80GB with the CI image,
unittest/llmapi/test_llm.py::test_generate_with_stop_words:The neighbouring
test_generate_with_detokenization_stop_wordsandtest_generate_with_detokenization_stop_words_streamingalso pass.Fixes https://nvbugs/6891162 (partially:
test_generate_with_stop_words)PR Checklist
🤖 Generated with Claude Code
Dev Engineer Review
test_generate_with_stop_wordsnow derives the stop-token ID from" N", matching the space-prefixed token generated by the model. The change removes only this test’s waiver.test_generate_with_bad_wordsremains waived and unchanged.QA Engineer Review
Modified
tests/unittest/llmapi/test_llm.pyandtests/integration/test_lists/waives.txt. The test now checks stopping on the space-prefixed token. The waiver entry for this test was removed. No matching entry was found in the searched CI or manual-QA lists. The PR description reports that this test and two detokenization stop-word tests passed on an A100 80GB. Coverage verdict: sufficient for this targeted change.Per-File QA Perspective
tests/unittest/llmapi/test_llm.py: Covers stop-word handling when the generated token is" N". No matching CI or manual-QA list entry was found in the searched lists.tests/integration/test_lists/waives.txt: Removes the waiver forunittest/llmapi/test_llm.py::test_generate_with_stop_words, allowing the test to run rather than be skipped.