Repository navigation
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
… and tokenizer boundary fixes Preserve the upstream Qwen parser/streaming-engine and tokenizer corrections as attributed source patches. Keep parser state, configuration and token handling together so the integration can validate the complete compatible source set. Provenance Upstream PRs and authors: - vllm-project/vllm#56113 — Svliverse-1 - vllm-project/vllm#56661 — JordiPosthumus - huggingface/transformers#45422 — qgallouedec - huggingface/transformers#47855 — ameerhmz - huggingface/transformers#47990 — Ilya-a-sergeyev-ger - huggingface/transformers#48708 — albertvillanova
… and tokenizer boundary fixes Preserve the upstream Qwen parser/streaming-engine and tokenizer corrections as attributed source patches. Keep parser state, configuration and token handling together so the integration can validate the complete compatible source set. Provenance Upstream PRs and authors: - vllm-project/vllm#56113 — Svliverse-1 - vllm-project/vllm#56661 — JordiPosthumus - huggingface/transformers#45422 — qgallouedec - huggingface/transformers#47855 — ameerhmz - huggingface/transformers#47990 — Ilya-a-sergeyev-ger - huggingface/transformers#48708 — albertvillanova
…s-keep-declared-ids
|
Merged main: #48847 landed in the meantime and moves the comparison to Measured downstream: on TRL's suite the realignment message goes from 457 lines to 145, with @SunMarc you endorsed this direction on #48592, would you have a moment? @lewtun tagging you for visibility, since the original alignment came out of your report. |
SunMarc
left a comment
There was a problem hiding this comment.
Thanks, just a nit that will improve the readability of the code
| # A token the tokenizer does not define is not a statement that the model should drop the value the config | ||
| # declares: `_special_tokens_map` is initialized with `None` for every special token, so an absent attribute | ||
| # and a deliberately cleared one are indistinguishable. Only values the tokenizer defines are propagated. |
There was a problem hiding this comment.
we should warn/inform about this no somehow ? maybe we already do but just checking
There was a problem hiding this comment.
I don't think we can, and I think it would be counterproductive.
The two cases are indistinguishable at this point: _special_tokens_map is initialized with None for every special token, so a tokenizer that never declared one looks exactly like a tokenizer whose token was deliberately cleared. The case worth informing about is the second, and it is the one we cannot detect.
What we can detect is the first, and it is the common case: every Qwen model defines no BOS, so informing there would fire on every train() for a very large share of users, which is the noise this PR removes. If someone does want the removal, they can set it on the config directly, which is the explicit opt-in you suggested on #48592.
The previous behaviour was that warning: when the tokenizer did not define a token, the config value was overwritten with None and reported as an update. On a TRL CI run that produced 311 Updated tokens: {'bos_token_id': None} lines out of 457 warning lines, one per model whose tokenizer defines no BOS, which is most of them.
Now nothing is changed in that case, so there is nothing to report. The warning still covers every token that is actually modified.
| # 3 - Align PAD | ||
| if tokenizer.pad_token_id is not None: | ||
| tokenizer_has_new_pad = tokenizer.pad_token_id != getattr(config, "pad_token_id", None) | ||
| if model_has_generation_config: | ||
| model.generation_config.pad_token_id = tokenizer.pad_token_id | ||
| tokenizer_has_new_pad |= tokenizer.pad_token_id != model.generation_config.pad_token_id | ||
|
|
||
| if tokenizer_has_new_pad: | ||
| updated_tokens["pad_token_id"] = tokenizer.pad_token_id | ||
| config.pad_token_id = tokenizer.pad_token_id | ||
| if model_has_generation_config: | ||
| model.generation_config.pad_token_id = tokenizer.pad_token_id |
There was a problem hiding this comment.
small idea, let's create a helper function so that we clean this redundant code that is copied pasted three times with minimal changes. in the helper function, it should also deal with the case if the generation config may hold more than one token.
There was a problem hiding this comment.
Done. The helper takes the token name and looks it up on the tokenizer, the config and the generation config, so the three calls are identical apart from several=True for EOS, which is the case where the generation config may hold more than one token: there the tokenizer's id is prepended to the ones already defined instead of replacing them.
One behaviour change worth flagging: the old code turned generation_config.eos_token_id from an int into a one-element list as a side effect of the comparison, even when the tokenizer's EOS was already there and nothing was aligned. The helper only writes when a token actually changes, so that no-op normalization is gone. I compared the old and the new implementation over every combination of tokenizer, config and generation config values, and this is the only difference between them.
It should be safe: eos_token_id is documented as Union[int, list[int]], generate normalizes it once in _prepare_special_tokens and reads _eos_token_tensor everywhere afterwards, and the other direct readers branch on the type. The gain is that a checkpoint shipping "eos_token_id": 2 keeps that value after training instead of being rewritten to [2].
| self.assertEqual(text_config.bos_token_id, tokenizer.bos_token_id) | ||
| self.assertEqual(text_config.pad_token_id, tokenizer.pad_token_id) | ||
|
|
||
| def test_special_token_alignment_keeps_tokens_the_tokenizer_does_not_define(self): |
CI recapDashboard: View test results in Grafana |
What does this PR do?
align_special_tokenscopies the tokenizer's BOS/EOS/PAD ids ontomodel.configandmodel.generation_config. When the tokenizer does not define one of them, theNoneis copied over the value the config declares, andsave_pretrainedpersists the removal.This PR propagates only the values the tokenizer actually defines, as suggested in #48592. Removing a token stays an explicit action on the config.
Fixes #48592
Why
_special_tokens_mapis initialized withNonefor every special token, so a tokenizer that never declared one is indistinguishable from a tokenizer whose token was deliberately cleared:Resolving that ambiguity in favour of removal means any model family whose tokenizer simply does not manage a token loses it from the config on the first
train()call. Qwen is the widest case:config.jsondeclaresbos_token_id=151643whiletokenizer_config.jsondeclares nobos_token, so every Qwen fine-tune was saved withbos_token_id: null.Before this PR, on
Qwen/Qwen2.5-0.5B-Instruct:and the nulled value was written to
config.json. After this PR the declared id survives,generate()keeps working, and the alignment still reports the one genuine change:The same failure mode was reported for Whisper in #45584, closed as stale, and worked around in the still-open #45570 and #45917.
What this does not change
Values the tokenizer does define are still propagated, including onto a config that lacks them. A tokenizer that adds an end-of-turn token at fine-tuning time, the case #38441 was written for, behaves exactly as before.
The diff is 82 lines but only three are substantive: one guard per token, plus a comment.
git diff -wshows the rest is indentation.Tests
test_special_token_alignmentsets all three tokens to real values, so it is unaffected and still passes. Addedtest_special_token_alignment_keeps_tokens_the_tokenizer_does_not_definenext to it, asserting that a config-declared BOS survives a tokenizer that does not define one.Who can review?
@SunMarc