Repository navigation
Fix the BCO encoder-decoder path - #6999
Closed
behroozazarkhalili wants to merge 11 commits into
Closed
behroozazarkhalili wants to merge 11 commits into
behroozazarkhalili wants to merge 11 commits into
Conversation
BCOTrainer failed before the first optimizer step on any encoder-decoder model, at two
independent sites. The first masked the second, so one run only ever showed one of them.
`_tokenize` adds `answer_input_ids`, and `embedding_input_ids` when an embedding tokenizer is
given. The encoder-decoder branch of `DPODataCollatorWithPadding` assigned a padding value to
the prompt, to any attention mask, and to the chosen, rejected, completion and decoder keys,
so both of those columns fell through to `raise ValueError("Unexpected key in batch")`. The
decoder-only branch of the same collator already pads every `*_input_ids` column with the pad
token, which is why the failure was specific to encoder-decoder models. Match on the suffix
here as well rather than listing prefixes: it keeps the two branches in step and covers the
next column someone adds. Reordering the checks leaves every key that already worked on its
old padding value.
`forward()` then read `batch["completion_input_ids"]` unconditionally, but `_process_tokens`
builds that key only on its decoder-only branch. The encoder reads the prompt and the
completion arrives as labels, so the encoder-decoder branch builds `completion_labels` and no
concatenated input ids. Read the prompt on that branch, which is what the sibling
`compute_reference_log_probs` already does.
Two tests cover the path. One is plain. The other takes the UDM path, which reaches the
collator from `_get_sample_prompt_embeddings` during init rather than from the training loop,
and is the only test that catches the `embedding_*` column. Both fail on main.
Resolves #6996
|
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. |
The two encoder-decoder training tests only show that a run completes. A collator that padded the new answer and embedding ids with -100 passed both of them, because those ids feed the reward embedding and the UDM classifier, where a wrong pad token changes a number rather than raising. A direct collator test now pins the values on a ragged batch: answer and embedding ids take the pad token, their masks take 0, completion labels keep -100. Against main's collator it fails on the ValueError the PR removes. The padding comment also claimed the decoder keys reach the model as labels. They share the -100 rule but are only built when a model is passed to the tokenizing step, which no trainer does, so they never reach a model. The comment now says that.
Member
|
Closing in favour of #7136, which removes |
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.
Resolves #6996
BCOTrainerfails before the first optimizer step on any encoder-decoder model. Two independentdefects sit on that path, and the first masks the second, so one run only ever shows one of them.
Defect 1: the collator has no case for the
answer_input_idscolumnMeasured on
mainat d9e880c:_tokenize(bco_trainer.py:228-233) addsanswer_input_idsandanswer_attention_maskto everyexample, and
embedding_input_idstoo when an embedding tokenizer is given. The encoder-decoderbranch of
DPODataCollatorWithPadding(utils.py:81-94) assigned a padding value toprompt*_input_ids, to any*_attention_mask, and to keys starting withchosen,rejectedorcompletionor containingdecoder. Both new columns matched none of those and reached the finalraise.
The decoder-only branch of the same collator (
utils.py:96-113) already pads every*_input_idscolumn with the pad token, which is why the failure was specific to encoder-decoder models. This
matches on the suffix there as well rather than listing prefixes, so the two branches stay in step
and the next column someone adds is covered. Reordering the checks leaves every key that already
worked on its old padding value.
Defect 2:
forward()reads a key the encoder-decoder path never buildsPatch past defect 1 and the same script reaches:
_process_tokensbuildscompletion_input_idsonly on its decoder-only branch(
bco_trainer.py:304). Its encoder-decoder branch (bco_trainer.py:344-352) buildsprompt_input_ids,prompt_attention_mask,completion_labels,completion_attention_maskandcompletion_decoder_input_ids, and no concatenated input ids, because the encoder reads the promptand the completion arrives as labels.
forward()now reads the prompt on that branch, which is whatthe sibling
compute_reference_log_probs(bco_trainer.py:1088-1114) already does.Tests
tests/experimental/test_bco_trainer.pyhad no encoder-decoder case at all. The one test thatpassed an
is_encoder_decoderkwarg read it off a decoder-only trainer, so the value was alwaysFalse.Two tests now cover the path.
test_train_encoder_decoderruns it plainly.test_train_encoder_decoder_udmruns it with an embedding function, which reaches the collator from_get_sample_prompt_embeddingsduring init rather than from the training loop, and is the only testthat catches the
embedding_*column.Both training tests only show that a run completes: a collator that padded the
answer_*andembedding_*ids with-100passes them, because those ids feed the reward embedding and the UDMclassifier, where a wrong pad token changes a number rather than raising.
test_collator_pads_answer_and_embedding_ids_with_pad_tokencalls the collator on a raggedencoder-decoder batch and pins the values: those ids take the pad token, their masks take
0, andcompletion_labelskeeps-100.Verification
ValueError: Unexpected key in batch 'answer_input_ids'forward()fixKeyError: 'completion_input_ids'("prompt", "answer")allowlistembedding_input_idsValueError: Unexpected key in batch 'answer_input_ids'answer_*andembedding_*with-100checkandformat --checkmax_len119The third mutant is the one that shows the UDM test is not redundant: it is the only case that
distinguishes a rule covering
answer_*from one covering every token column.Scope
Neither defect depends on the checkpoint. Both sites are gated only on
self.is_encoder_decoder, soboth fire for every encoder-decoder model. I measured one,
trl-internal-testing/tiny-T5ForConditionalGeneration.DPODataCollatorWithPaddingis shared with CPO, ORPO, OnlineDPO, NashMD and XPO. Those trainers keeptheir
answer_*values as locals rather than dataset columns, so none of them reached the raise, andtheir suites were run to confirm the reorder leaves them alone.
The
*_decoder_input_idskeys share the-100rule, and a model would reject-100as a token id.They are only built when a model is passed to
_process_tokens(and to CPO's and ORPO'stokenize_row), which no trainer does, so on every trainer path the key is absent and the modelderives its decoder inputs from the labels. That is pre-existing and unchanged here; the comment on
the rule now says so.
Note
Medium Risk
Touches shared
DPODataCollatorWithPaddingencoder-decoder logic and BCO training forward; behavior is gated onis_encoder_decoderand other DPO-family trainers were checked for regressions.Overview
BCOTrainer could not train encoder-decoder models (e.g. T5): batching failed on
answer_input_ids/embedding_input_ids, andforward()then looked forcompletion_input_ids, which the encoder-decoder tokenization path never creates.The encoder-decoder branch of
DPODataCollatorWithPaddingnow assigns padding like the decoder-only path: masks use0, completion/label fields use-100, and remaining*_input_ids(prompt,answer_*,embedding_*) use the tokenizer pad id—so BCO batches no longer hitUnexpected key.BCOTrainer.forwardon encoder-decoder runs the model onprompt_input_idswithcompletion_labels(and optionaldecoder_input_ids), matchingcompute_reference_log_probsinstead of concatenated completion ids.New tests cover a full T5 BCO train step, UDM with embeddings at init-time collator use, and explicit collator padding assertions for
answer_*/embedding_*.Reviewed by Cursor Bugbot for commit f2eb458. Bugbot is set up for automated code reviews on this repo. Configure here.