Fix assistant tokens mask when a generation span ends at token index 0 or on stripped whitespace - #47326
Closed
sohumt123 wants to merge 2 commits into
Closed
Fix assistant tokens mask when a generation span ends at token index 0 or on stripped whitespace#47326sohumt123 wants to merge 2 commits into
sohumt123 wants to merge 2 commits into
Conversation
…tripped whitespace
The assistant token mask builder in apply_chat_template computed the end of
each generation span with:
end_token = out.char_to_token(i, assistant_end_char - 1)
for token_id in range(start_token, end_token + 1 if end_token else len(input_ids[i])):
The ternary tested the truthiness of end_token, but end_token is legitimately
falsy in two non-truncation cases:
- end_token == 0, when the generation span is the first content in the
sequence (0 is a valid token index); and
- end_token is None, when char_to_token returns None because the last char
of the span has no aligned token, e.g. a trailing space stripped by a
WordPiece pre-tokenizer.
In both cases the code fell through to len(input_ids[i]) and masked every
remaining token, wrongly labelling later user/system turns as assistant and
silently corrupting SFT/RLHF loss masks.
Replace the truthiness check with a backward scan from assistant_end_char - 1
towards assistant_start_char, taking the first char that maps to a token as
end_token, and only fall back to masking to the end of the sequence when no
char in the span maps to a token (genuine truncation). This preserves the
mask-to-end truncation semantics covered by
test_chat_template_return_assistant_tokens_mask_truncated and fixes both the
end_token == 0 and whitespace-None cases.
These tokenizers only accept pretokenized word lists, so they can't run chat-template tests. I gave the new edge-case test the same per-model skips the existing assistant-mask tests already have.
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: layoutlmv2, layoutlmv3, layoutxlm, markuplm, tapas, udop |
Contributor
CI recapDashboard: View test results in Grafana |
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.
What does this PR do?
Fixes #47325.
apply_chat_template(..., return_assistant_tokens_mask=True)masks every token from the start of a{% generation %}span to the end of the sequence wheneverchar_to_token(assistant_end_char - 1)is falsy without the span being truncated, wrongly labelling later user/system turns as assistant tokens. This PR replaces the truthiness check with an explicitis Nonewalk-back so the mask stops at the last real assistant token, while keeping the existing mask-to-end behavior for genuinely truncated spans.Problem
The end of each generation span is computed as:
The ternary tests the truthiness of
end_token, butend_tokenis legitimately falsy in two non-truncation cases:end_token == 0— the span's last char maps to token index 0 because the assistant turn is the first content in the sequence.0is a valid token index.end_token is None— the span's last char has no aligned token, e.g. a trailing space inside{% generation %}that a WordPiece pre-tokenizer strips.In both cases the loop falls through to
len(input_ids[i])and masks the rest of the sequence. Sinceassistant_masksis typically used to build SFT/RLHF loss masks, this silently trains on user/system text.Minimal repro for case 1 (gpt2):
Case 2 with
bert-base-uncasedand a generation block renderingmessage['content'] + ' 'goes from[1, 1, 1, 1, 1, 1](everything masked) to[1, 1, 0, 0, 0, 0]. Full repro for both cases is in #47325.Fix
Instead of testing truthiness, scan backwards from
assistant_end_char - 1towardsassistant_start_charand take the first char that maps to a token asend_token. Only when no char in the span maps to a token (the span was truncated away entirely) fall back to masking to the end of the sequence — preserving the truncation semantics covered by the existingtest_chat_template_return_assistant_tokens_mask_truncated.Tests
Added
test_chat_template_return_assistant_tokens_mask_edge_casestotests/test_tokenization_common.pywith two subcases: an assistant-first span whose last token is index 0, and a generation block ending in a trailing space (with a comment on why byte-level BPE tokenizers make the boundary token ambiguous in the second case).Running the new test plus the existing truncation test against
main's source (new test, pre-fix library):On this branch:
The truncation test passes before and after, confirming the fallback semantics are unchanged.
Differentiation from existing PRs
ignore_merges] Fix offsets tokenizers#1640 fixed a start-tokenNonecase insidetokenizers; it does not touch this end-token truthiness check intransformers.processing_utils.py); no open PR addresses this tokenizer-path (tokenization_utils_base.py) bug.Note on AI assistance
Disclosure: this change was developed with AI assistance. I reviewed every changed line, reproduced the bug, and validated the fix and its tests locally, and I stand by the change. Coordination issue: #47325.
Who can review?
@ArthurZucker @itazap