Repository navigation
[Bugfix] Reland #53020: tie lm_head.weight for Nemotron Parse when checkpoint omits it - #60272
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mergify
Bot
added
multi-modality
|
/ci run --allow-stale |
github-actions
Bot
added
the
ready
|
✅ Triggered Buildkite CI #93150 for commit |
|
@DarkLight1337 thanks! One thing: |
|
Hi @aniskumar-nv, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
CI selector (shadow): 5 test steps (28 jobs) instead of 62 (97 jobs)Shadow mode: this changes nothing about what CI runs. It shows what the evidence-based selector would pick for this PR, next to today's rules. How it works. Feedback welcome: reply here if it would skip a step this change needs, or runs something unrelated.
Selector would run (5)
Would skip (today's rules run them) (58)
Would add (today's rules do not run them) (1)
AMD mirrors: would skip (56)
AMD mirrors: would add (0)none 2 changed files · base |
|
The pre-commit failure is mypy on |
|
/ci run |
|
✅ Triggered Buildkite CI #93164 for commit |
|
The two |
Head branch was pushed to by a user without write access
0524569 to
17506d8
Compare
|
/ci run |
|
/ci run --allow-stale |
|
✅ Triggered Buildkite CI #93363 for commit
|
|
/ci retry |
|
✅ Queued 1 failed job(s) for retry in Buildkite CI #93363. |
|
✅ No failed, timed-out, or expired jobs need retrying: https://buildkite.com/vllm/ci/builds/93363 |
|
The remaining red is one |
|
/ci retry |
|
✅ Queued 1 failed job(s) for retry in Buildkite CI #93363. |
Head branch was pushed to by a user without write access
17506d8 to
6e2e9d6
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #93386 for commit |
|
CI on the rebased head: every NVIDIA step is green, including the |
|
@DarkLight1337 all NVIDIA steps are green here; the only red is the AMD mirror failing on |
…arse when checkpoint omits it Reverts vllm-project#59805 (7cfd233), restoring vllm-project#53020 (83cadd6) unchanged. nvidia/NVIDIA-Nemotron-Parse-2.0 sets tie_word_embeddings=True and ships no lm_head.weight; without the tie, NemotronParseForConditionalGeneration keeps a randomly initialized output head and every response is garbage while the server reports healthy. vllm-project#53020 was reverted because the Parse-2.0 test case it added failed in the nightly full CI run at tokenizer load (TokenizerFast.from_file -> DuplicatePatternError), before any model code ran. The cause was the CI pin tokenizers==0.23.1, which rejects added tokens that collide after NFKC normalization (14 such pairs in this tokenizer). vllm-project#59796 bumped the pin to 0.23.2, which loads the file, so the test case can run as written. Co-authored-by: Cyrus Leung <tlleungac@connect.ust.hk> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Anish Kumar <aniskumar@nvidia.com>
6e2e9d6 to
91b560f
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #93437 for commit |
|
CI is fully green on the rebased head (155/155, including the AMD mirror after #60462). Ready to merge; auto-merge was disabled by the branch update, so it needs re-enabling or a manual merge. |
|
@DarkLight1337 ready to merge? |
3 participants
Purpose
Re-lands #53020 (commit
83cadd65d9), reverted in #59805. The diff is byte-identical to the reverted commit (git patch-idmatches). Original bug report: #53019. Supersedes #60266, which carries the same diff; thanks @DiegoCao for preparing it.NemotronParseForConditionalGenerationbuilds a standaloneParallelLMHeadand only populates it fromlm_head.*weights. A checkpoint that setstie_word_embeddingsand ships nolm_head.weight, such asnvidia/NVIDIA-Nemotron-Parse-2.0, therefore keeps a randomly initialized output head. Nothing reports it: the server starts, reports healthy, and returns garbage.Why this can land now
#59805 did not revert because the fix was wrong. It reverted because the
NVIDIA-Nemotron-Parse-2.0case added totest_nemotron_parse.pyfailed in the nightly full CI run while loading that checkpoint's tokenizer, before any model code ran:The cause was the CI pin
tokenizers==0.23.1. The 2.0tokenizer.jsonuses an NFKC normalizer, and 14 pairs of its added tokens become identical after normalization (for exampleნU+10DC andჼU+10FC both normalize toe1 83 9c, the[225, 131, 156]in the error). 0.23.1 rejects the duplicates when it builds its matcher; 0.23.2 (daachorse 1.0.1 → 3.0.0, huggingface/tokenizers#2050) accepts them. #59796 bumped the pin.Tokenizer.from_fileon the 2.0tokenizer.json, bytokenizersversion:DuplicatePatternErrorUnder the current CI pins (
tokenizers==0.23.2,transformers==5.18.0),AutoTokenizer.from_pretrained("nvidia/NVIDIA-Nemotron-Parse-2.0")loads (TokenizersBackend, vocab 72,249).Test Plan
tests/models/multimodal/generation/test_nemotron_parse.py::test_models: thenvidia/NVIDIA-Nemotron-Parse-2.0parametrization is restored. It is the only coverage of the tied-head path and is the case that must be green before merge.Multimodal Models (Extended Generation 1), which isoptional: true, so it did not run on [Bugfix] Tie lm_head.weight for Nemotron Parse when checkpoint omits it #53020's PR CI and only surfaced in the nightly run. Could a maintainer include that step when triggering CI here?nvidia/NVIDIA-Nemotron-Parse-v1.2stays in the same parametrization and is unaffected (see below).Test Result
Out-of-CI validation on one RTX 6000 Ada, serving the 2.0 checkpoint with
vllm serve … --dtype bfloat16 --trust-remote-code --limit-mm-per-prompt '{"image": 1}', one greedy request (50 max tokens) with the model's documented control-token prompt on the deterministic test image from the model repo'stest_golden.py:<s><s><s>…(50 ×<s>)<x_0.3906><y_0.5969>\begin{tabular}{ccccc}\n\end{tabular}<x_0.6084><y_0.6367><class_Table>The patched output is byte-identical to what the model's own shipped workaround (
vllm_tied_patch/sitecustomize.pyin the HF repo) produces on the same input.Why v1.2 cannot be affected: the two checkpoints differ exactly where the new branch reads.
tie_word_embeddings(config /config.decoder)lm_head.weightin checkpointNVIDIA-Nemotron-Parse-v1.2false/falseNVIDIA-Nemotron-Parse-2.0true/trueThe tie goes through
ParallelLMHead.tie_weights(), so it routes viaquant_method.tie_weightsrather than assigning.weightdirectly.ruff checkandruff format --checkpass on both files.Notes
Signed-off-by: Anish Kumar,Co-authored-by: Cyrus Leung).cc @DarkLight1337 @Thangnguyenvn98 @hmellor @djramic
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.
🤖 Generated with Claude Code