feat: Add HuggingFace-style skip_special_tokens and batch_decode support (#403) - #1186
Conversation
|
@lakshitsoni26 so there are some risks here, and decisions to be made, it's not a 'zero regression' case... there's also some shifting ground re other significant additions (e.g. #2713) so am going to defer tokenizer cleanup until a bit later |
|
Understood! Thanks for taking a look. I'll leave this PR open for now — just ping me whenever the dust settles on #2713 and you are ready to revisit the tokenizer cleanup. Happy to update this PR to resolve any conflicts then! |
ErenAta16
left a comment
There was a problem hiding this comment.
Checked the special-token handling across all four tokenizer classes since that's where a change like this usually breaks.
SimpleTokenizer.all_special_ids is built from ['<start_of_text>', '<end_of_text>'] + additional_special_tokens, and both are already present in self.decoder (they're added to vocab before the encoder/decoder are built), so skip_special_tokens=True's truncate-at-eot-then-filter-by-all_special_ids correctly strips both, including any user-supplied additional special tokens, not just the two built-in ones.
TikTokenTokenizer.decode's non-skip branch (the one that renders special tokens as literal strings like <|endoftext|> instead of dropping them) hardcodes exactly three ids via if/elif, I checked this against self.all_special_ids = [eot_token_id, pad_token_id, bos_token_id] from __init__ and it's the same three, so nothing falls through the elif chain and gets silently dropped.
The switch from self.decoder[token] to self.decoder.get(token, '') in SimpleTokenizer.decode changes a KeyError into a silent empty string for a truly out-of-vocab id. Given the new batch_decode is clearly meant to run on raw padded tensors, that's the right direction, an id you didn't expect (e.g., stray padding) shouldn't crash decode, but flagging it since it is a behavior change for any existing caller that relied on the KeyError to catch a bad token id.
clean_up_tokenization matches the standard HF replacement list exactly (checked against the reference set of ten replacements), and the test_clean_up_tokenization case traces correctly through the chain by hand.
Good, well-tested addition, no correctness issues found.
|
Exploring a different approach to unifying tokenizer interface, aligning options a bit closer to HF while keeping defaults that shouldn't break compat in #1196 ... still pondering aspects of both of these |
|
Thanks for the update and for linking #1196! I just took a look at the refactor, and unifying the backend decode helpers while strictly preserving the backwards-compatible defaults is definitely the right architectural approach here. Since #1196 comprehensively handles the HuggingFace-style decoding we were aiming for, I'm more than happy to close this PR in favor of yours once you are ready to merge it. Please let me know if you'd like me to help test any of the edge cases on #1196 in the meantime! |
batch_decode,skip_special_tokens, andclean_up_tokenization_spacesto the tokenizer API.clean_up_tokenizationhelper to exactly mimic HuggingFace's internal string-cleaning logic (stripping spurious spaces before punctuation).batch_decodeand the new decoding arguments toSimpleTokenizerandTikTokenTokenizer.HFTokenizerandSigLipTokenizer.SimpleTokenizer.tests/test_tokenizer_decode.pycovering all tokenizer variants (100% test coverage for the new features).Falseacross all signatures.