-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
[Bugfix] Catch and log invalid token ids in detokenizer #2 #26445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is an update to the "workaround" added in vllm-project#24351. That PR insulates against occasional negative token ids that can be produced occasionally, though we still don't know the root cause. With the update to tokenizers 0.22.1, this error manifests as a TypeError rather than an OverflowError, so the patch needs to be updated to account for this. Signed-off-by: Nick Hill <nhill@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
I've reviewed your pull request. The change to catch TypeError
is correct based on the updated behavior of the tokenizers
library. I've found one high-severity issue related to this change that could cause problems in the exception handling logic. Please see my detailed comment below.
Signed-off-by: Nick Hill <nhill@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the work!
…to loader * 'loader' of https://github.com/dsxsteven/vllm_splitPR: (778 commits) [torchao] Add support for ModuleFqnToConfig using regex (vllm-project#26001) Add: Support for multiple hidden layers in Eagle3 (vllm-project#26164) Enable `RMSNorm` substitution for Transformers backend (vllm-project#26353) [Model] Gemma3: Fix GGUF loading and quantization (vllm-project#26189) Bump Flashinfer to v0.4.0 (vllm-project#26326) Update Dockerfile and install runai-model-streamer[gcs] package (vllm-project#26464) [Core] Relax the LoRA max rank (vllm-project#26461) [CI/Build] Fix model nightly tests (vllm-project#26466) [Hybrid]: Decouple Kernel Block Size from KV Page Size (vllm-project#24486) [Core][KVConnector] Propagate all tokens on resumed preemptions (vllm-project#24926) [MM][Doc] Add documentation for configurable mm profiling (vllm-project#26200) [Hardware][AMD] Enable FlexAttention backend on ROCm (vllm-project#26439) [Bugfix] Incorrect another MM data format in vllm bench throughput (vllm-project#26462) [Bugfix] Catch and log invalid token ids in detokenizer #2 (vllm-project#26445) [Minor] Change warning->warning_once in preprocess (vllm-project#26455) [Bugfix] Set the minimum python version for gpt-oss (vllm-project#26392) [Misc] Redact ray runtime env before logging (vllm-project#26302) Separate MLAAttention class from Attention (vllm-project#25103) [Attention] Register FLASHMLA_SPARSE (vllm-project#26441) [Kernels] Modular kernel refactor (vllm-project#24812) ...
vllm-project#26445) Signed-off-by: Nick Hill <nhill@redhat.com> Signed-off-by: xuebwang-amd <xuebwang@amd.com>
vllm-project#26445) Signed-off-by: Nick Hill <nhill@redhat.com> Signed-off-by: Dhruvil Bhatt <bhattdbh@amazon.com>
vllm-project#26445) Signed-off-by: Nick Hill <nhill@redhat.com>
This is an update to the "workaround" added in #24351.
That PR insulates against occasional negative token ids that can be produced occasionally, though we still don't know the root cause (see #21951).
With the update to
tokenizers
0.22.1, this error manifests as aTypeError
rather than anOverflowError
, so the patch needs to be updated to account for this.Mitigates #26438, #26071, #25821.