Repository navigation
Conversation
…mapping Map prompt-embedding placeholder IDs outside the model vocabulary to UNK without indexing outside the mapping table. Reuse the existing CPU/CUDA mapping tests for both directions and preserve the no-host-sync path. Co-authored-by: Codex <noreply@openai.com> Signed-off-by: 子华 <huaxi.shx@alibaba-inc.com>
shaohuaxi
requested review from
MatthewBonanni,
benchislett and
luccafong
as code owners
September 28, 2026 10:55
Contributor
Author
|
Hi @tomasruizt, since you reviewed the vocab mapping and CUDA sync checks in #57396, could you take a look at this PR when you have a chance? It maps out-of-range IDs to UNK in both directions while preserving the no-host-sync path. All 29 tests passed on A10, including the CUDA sync checks. I'd appreciate your feedback. Thanks! |
Contributor
|
@shaohuaxi I can review it next week |
Contributor
Author
|
Thanks! |
6 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Mixed prompt embeddings can leave placeholder token IDs outside the model vocabulary. These positions use the supplied embeddings in the target model, but heterogeneous speculative decoding still looks up their IDs in a mapping table sized from the model config. The lookup fails before the existing UNK fallback can run.
This change checks the ID range before lookup, uses bounded indices for the gather, and maps out-of-range IDs to UNK. It applies the same handling in both directions and preserves the existing fallback for tokens outside the shared vocabulary, without adding a tensor-dependent Python branch.
Duplicate-work check on September 28, 2026 found no existing fix for this case. #57396 removes CPU/GPU synchronization from these methods; #51564 addresses the separate local-argmax path that bypasses vocabulary mapping.
Test Plan
Using the CPU test environment with this checkout on
PYTHONPATH(TEST_PYTHONis its Python executable):The existing CPU/CUDA fixture now includes tokenizer entries beyond the model vocabulary. The added cases cover both mapping directions, negative IDs, and empty inputs, while retaining the existing dtype, device, and GPU synchronization assertions.
Also ran the unchanged test file on an NVIDIA A10 using an existing CUDA 13 image, with the committed mapping and GPU sync-check modules mounted read-only:
Test Result
50257to UNK50256in both directions, for int32 and int64 inputs. CUDA Graph capture and replay with changed inputs also passed for all four combinations.AI assistance: Codex assisted with the implementation and validation.