Repository navigation
Conversation
`_process_tokens` truncates the response with `answer[: max_length - len(prompt_input_ids)]`.
Nothing caps the prompt, so that bound goes negative as soon as the prompt is
longer than the (BOS/EOS adjusted) `max_length`. A negative bound slices from the
*end* of the response instead of the start, which empties the response outright
whenever it is shorter than the overflow, and the sequence stays over `max_length`
regardless because the prompt was never shortened:
prompt= 10 answer= 10 max_length= 32 -> completion_len= 22 answer tokens: 11
prompt= 28 answer= 10 max_length= 32 -> completion_len= 32 answer tokens: 3
prompt= 60 answer= 10 max_length= 32 -> completion_len= 62 answer tokens: 1 (EOS only)
prompt=1000 answer= 10 max_length= 32 -> completion_len=1002 answer tokens: 1 (EOS only)
The last two carry no real completion tokens, so the example contributes nothing
but still costs a full forward pass at 1000+ tokens with `max_length=32`.
This is what huggingface#4964 left behind. Before that PR the branch truncated the prompt to
`max_prompt_length` first and only then gave the response what was left; removing
`max_prompt_length` dropped the first step, and the replacement bound has nothing
bounding the prompt any more.
Restore the two-step order with `max_length` as the prompt cap, so the response
budget can no longer be negative and the sequence honours `max_length`. This is
what `KTOTrainer` does when it truncates the concatenation with
`full_ids[: max_length]`.
An example whose prompt fills `max_length` still ends up with no completion tokens,
same as KTO. `DPOTrainer` additionally drops those rows from the dataset; doing the
same here would change the desirable/undesirable balance BCO relies on, so I left it
out of this change.
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?
_process_tokensin the experimental BCO trainer truncates the response withanswer[: max_length - len(prompt_input_ids)]. Nothing caps the prompt inBCOTrainer, so thatbound goes negative as soon as the prompt is longer than the (BOS/EOS adjusted)
max_length.answer[:negative]slices from the end of the response, which empties it outright whenever theresponse is shorter than the overflow, and the sequence stays over
max_lengthanyway because theprompt was never shortened.
Calling
_process_tokensdirectly with the tiny test tokenizer:answer_tokens: 1is the appended EOS and nothing else, so those rows carry no completion tokensat all and contribute no learning signal, while the 1000-token one still costs a full forward pass
under a
max_lengthof 32.Where it came from. #4964 removed
max_prompt_lengthfrom BCO. Before it the branch was twosteps: truncate the prompt to
max_prompt_length, then give the response whatever was left. Theresponse bound got the better
- len(prompt_input_ids)form, but the step that kept the promptbounded went away with
max_prompt_lengthand nothing replaced it. That PR's description says"only the response is truncated if the total sequence exceeds
max_length", which cannot hold whenthe prompt on its own is already past
max_length.The change. Restore the two-step order, with
max_lengthas the prompt cap instead of theremoved
max_prompt_length.answer_budgetcan no longer be negative and the sequence honoursmax_lengthin every case. This is the policyKTOTrainerapplies when it truncates theconcatenated sequence with
full_ids[: self.max_length].Same table after the change:
Rows 3 to 5 still end up with no completion tokens, because there is no way to honour
max_lengthand keep completion tokens when the prompt alone fills the budget. That matches KTO. What changes
is that the sequence is now within budget and the response is never cut from the end.
Question for maintainers.
DPOTrainergoes further and drops the rows where truncation leavesnothing behind (
dpo_trainer.py, the.filter()after tokenization). Adding the same filter to BCOwould be a two-line change next to the existing desirable/undesirable filters, but it changes the row
count and therefore the desirable-to-undesirable ratio BCO relies on, so it felt like your call. Happy
to add it here or separately.
Tests.
test_process_tokens_truncation_respects_max_lengthintests/experimental/test_bco_trainer.py, parametrized over five prompt/answer length combinations.It asserts the sequence stays within
max_length, that both halves remain prefixes of what went in(a negative bound keeps a suffix instead), and that the response survives intact when it fits.
Reverting only
bco_trainer.pyfails the[60-10]and[1000-10]cases:The three that pass either way are guards against over-truncating the cases that already worked.
CPO and ORPO have a different flavour of the same problem in
tokenize_row(they size the bound offlonger_response_lengthinstead of the prompt), tracked in #6548. That one is being picked upseparately; this PR does not touch either file.
Fixes # (issue)
Before submitting
AI writing disclosure
Who can review?
@qgallouedec @albertvillanova — flagging you since #4964 is the change this follows up on.
Note
Medium Risk
Changes tokenization for long prompts in experimental BCO training, which can alter which examples contribute completion tokens but fixes incorrect sequences and wasted forward passes.
Overview
Fixes overlong sequences in the experimental BCO trainer’s
_process_tokenswhen the prompt alone exceedsmax_length.Previously only the answer was capped with
answer[: max_length - len(prompt)]. If the prompt was longer than the budget, that slice bound went negative, Python kept a suffix of the answer (often wiping real completion tokens) while the prompt was never shortened, socompletion_input_idscould still exceedmax_length.The change truncates the prompt prefix first (to
max_length), then applies the remaining budget to the answer—aligned with KTOTrainer’s concatenated truncation. Comments document the old failure mode.Adds
test_process_tokens_truncation_respects_max_length, parametrized over five prompt/answer lengths, checking total length ≤max_length, prefix-only truncation for prompt and answer, and intact answers when everything fits.Reviewed by Cursor Bugbot for commit 81c9e86. Bugbot is set up for automated code reviews on this repo. Configure here.