Repository navigation
Fix packed sequences in BERT-like decoders: pass position_ids to create_causal_mask - #50696
Open
EvanZhuang wants to merge 1 commit into
Open
EvanZhuang wants to merge 1 commit into
EvanZhuang wants to merge 1 commit into
Conversation
BertModel._create_attention_masks did not pass position_ids to create_causal_mask, so BERT-like models used as decoders (is_decoder=True) did not detect packed (padding-free) sequences with eager and sdpa: each document in a packed row attended to the documents before it. Pass position_ids from forward through _create_attention_masks to create_causal_mask in BERT, in its "Copied from" copies (BertGeneration, BridgeTower, Electra, Esm, RobertaPreLayerNorm, RoCBert, Xmod) and in the modular models built on it (Camembert, Data2VecText, Ernie, Roberta, XLMRoberta, XLMRobertaXL). attention_mask_padding_matches_padding_free_with_position_ids built these models with is_decoder=False, so their LM heads ran with a bidirectional mask, which has no packed-sequence support. Set is_decoder=True as the other generation tests do, and remove the XLMRobertaXL skips of the eager/sdpa variants, which now pass.
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: bert, bert_generation, bridgetower, camembert, data2vec, electra, ernie, esm, roberta, roberta_prelayernorm, roc_bert, xlm_roberta, xlm_roberta_xl, xmod |
This branch has not been deployed
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?
BERT-like models used as decoders (
is_decoder=True, e.g.BertLMHeadModel,RobertaForCausalLM) build their self-attention mask in_create_attention_maskswithoutposition_ids, socreate_causal_maskcan't detect packed (padding-free) inputs. With eager and sdpa, each document in a packed row then attends to the documents before it. FA2 is not affected, since it getsposition_idsdirectly.Reproducer (CPU, float32):
On main this prints 1.34e+01 for eager and sdpa; with this PR, 6.82e-05.
This PR passes
position_idsfromforwardthrough_create_attention_masks(new optional argument) tocreate_causal_mask. The mask only changes for packed inputs (position_idsgiven, noattention_mask, no cache), as in the other decoder models.Models: the change is in
BertModel.utils/check_copies.py --fix_and_overwriteupdated the copies ofBertModel._create_attention_masks(BertGeneration, BridgeTower, Electra, Esm, RobertaPreLayerNorm, RoCBert, Xmod), and I passedposition_idsat their call sites.utils/modular_model_converter.pyregenerated Roberta and XLMRobertaXL (fromBertModel), Camembert, Data2VecText and XLMRoberta (fromRobertaModel), and Ernie (call site inmodular_ernie.py).Test:
attention_mask_padding_matches_padding_free_with_position_idsshould cover this, but it builds the model from the common tester config, where these models haveis_decoder=False. Their LM heads then ran with a bidirectional mask, which has no packed-sequence support, and the test only passed because of its loose tolerance. The test now setsconfig.is_decoder = True, like the other generation tests (encoder-decoders are already skipped). The XLM-RoBERTa-XL eager/sdpa variants, skipped with "doesn't work for some reason, FIXME", now pass, so I removed the skips.This is needed for the follow-up to the review of #50450 (a separate PR that scales this test's tolerance to the logits): with that tolerance, these tests fail without this PR.
Measurements
CPU, with the testers' own configs and inputs.
is_decoder=True, 160 of 180 fail with the current modeling code and 0 of 180 with this PR. Withoutis_decoder=Truethey fail in about 30% of runs whether or not the modeling code is fixed, because the bidirectional mask ignores packing. With the current tolerance they pass (180/180 with this PR).tests/modelsshow no new failures, and the FA2 variants of the 9 test classes pass on an H100.Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag members/contributors who may be interested in your PR.