fix(tokenizer): do not strip a BOS token the tokenizer does not have - #1629
Merged
jlarson4 merged 1 commit intoAug 10, 2026
Merged
Conversation
get_tokens_with_bos_removed assumed a bos_token_id exists. Callers gate it on cfg.tokenizer_prepends_bos, which detect_tokenizer_bos_eos only sets when the tokenizer has one, but the flag goes stale on a bridge built via build_bridge_from_module(tokenizer=None) and given a tokenizer afterwards: the setter re-runs configure_tokenizer only on reassignment, so the config default of True survives. Trusting it then does damage in both directions. Under right padding, the default, the helper drops the first token unconditionally, silently removing [CLS] from a BERT tokenizer's output and returning a plausible-looking wrong result. Under left padding it evaluates (tokens == None).int() and raises AttributeError: 'bool' object has no attribute 'int', which names neither the tokenizer nor the flag. Return the tokens unchanged when there is no bos_token_id: with no BOS there is nothing to remove, which is correct however the config got out of sync. Reproduced with two off-the-shelf tokenizers, bert-base-cased and t5-small, both of which have bos_token_id None. The normal boot_transformers path is unaffected, since detection runs there. The root cause is the reassignment test in bridge_core.py, left alone deliberately. Correcting the flag would route to_tokens(prepend_bos=True) into the manual-prepend branch at transformer_bridge.py:716, which calls get_input_with_manually_prepended_bos(tokenizer.bos_token, ...) and raises TypeError on a None bos_token. The stale flag currently masks that, so the root-cause fix needs the prepend path hardened first. Fixes TransformerLensOrg#1628 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
10 tasks
Collaborator
|
Looks good @sohv, thanks for filing and tackling this issue. Feel free to file the follow-up and work on that at your convenience. |
Collaborator
|
@sohv It looks like @Chinmayrawat15 has already addressed that follow up in #1634. |
Author
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
2 participants
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.
get_tokens_with_bos_removedassumed the tokenizer has abos_token_id. Whencfg.tokenizer_prepends_bosgoes stale the helper is reached anyway, and it then does damage in two directions.Under right padding, which is the default, it drops the first token unconditionally. For a BERT tokenizer that silently removes
[CLS]and returns a plausible looking wrong result. Under left padding it evaluates(tokens == None).int(), which is a Python bool rather than a tensor, and raisesAttributeError: 'bool' object has no attribute 'int'.What I changed
With no
bos_token_idthere is nothing to remove, so the helper returns the tokens unchanged. This is correct however the config got out of sync, it fixes both symptoms, and it leaves theboot_transformerspath untouched.What I did not change and why
The root cause is the reassignment test at
bridge_core.py:113. I left it alone, and not only because it alters build and attach semantics. Correcting the flag would routeto_tokens(prepend_bos=True)into the manual prepend branch attransformer_bridge.py:716, which callsget_input_with_manually_prepended_bos(tokenizer.bos_token, ...)and raisesTypeError: unsupported operand type(s) for +: 'NoneType' and 'str'when there is no BOS token. The stale flag is currently masking that, so the root cause fix needs the prepend path hardened first. Happy to take that on as a follow up if you want it.Verification
Six unit tests, four of which are red on
dev-4.x. The two that pass either way are controls confirming a real BOS is still removed fordistilgpt2on both padding sides.Reproduced with two off the shelf tokenizers,
google-bert/bert-base-casedandgoogle-t5/t5-small, both of which havebos_token_idset toNone.