Skip to content

Restore tokenizer padding after callback generation - #6664

Open
RerankerGuo wants to merge 1 commit into
huggingface:mainfrom
RerankerGuo:fix/restore-callback-padding-side
Open

Restore tokenizer padding after callback generation#6664
RerankerGuo wants to merge 1 commit into
huggingface:mainfrom
RerankerGuo:fix/restore-callback-padding-side

Conversation

@RerankerGuo

@RerankerGuo RerankerGuo commented Aug 4, 2026

Copy link
Copy Markdown

What does this PR do?

LogCompletionsCallback and WeaveCallback currently set their shared tokenizer's padding_side to "left" and leave it there after completion generation. Because the tokenizer belongs to the trainer, this changes padding behavior for later training and evaluation batches.

This change moves the temporary setting into _generate_completions, which is shared by both callbacks:

  • generation still observes left padding;
  • the previous padding side is restored after successful generation;
  • the previous padding side is also restored when tokenization or generation raises;
  • callback generation arguments and outputs are unchanged.

Fixes #6663

PR #5625 also touches callback generation, but it adds generation_kwargs support and does not address tokenizer state restoration.

Verification

The regression coverage verifies three behaviors:

  1. _generate_completions uses left padding during successful generation and restores the original value;
  2. the original value is restored when generation raises;
  3. LogCompletionsCallback no longer changes the trainer tokenizer after returning.

Before the fix:

3 failed in 47.20s

After the fix:

3 passed in 15.67s
9 passed, 2 skipped in 66.26s

Commands:

pytest -q tests/test_callbacks.py::TestGenerateCompletions tests/test_callbacks.py::TestLogCompletionsCallback::test_does_not_change_tokenizer_padding_side
pytest -q tests/test_callbacks.py
pre-commit run --files trl/trainer/callbacks.py tests/test_callbacks.py
git diff --check

The two skipped full-file tests require optional W&B and Comet integrations. Six warnings report that pinned memory is unavailable on MPS and are unrelated. Pre-commit passed Ruff check, Ruff format, and doc-builder style.

Before submitting

AI writing disclosure

We welcome the use of AI tools to help with contributions. For transparency and to help us improve our review process, please indicate the level of AI involvement in this PR.

  • No AI usage: the PR was written entirely by a human.
  • AI-assisted: some parts were suggested or improved by AI, but the PR was written and reviewed by a human.
  • AI-generated: the PR was mostly or fully generated by an AI tool.

Who can review?

Anyone familiar with trainer callbacks or generation padding can review this focused change.


Note

Low Risk
Scoped state-restoration fix in callback generation with tests; no change to generation outputs or training logic beyond fixing tokenizer side effects.

Overview
Fixes a bug where LogCompletionsCallback and WeaveCallback set the trainer tokenizer’s padding_side to "left" for generation and never put it back, which could affect later train/eval batches.

Generation now applies left padding only inside shared _generate_completions via a _temporary_padding_side context manager. The previous padding_side is restored on success and if tokenization or generation raises. The callbacks no longer mutate the tokenizer directly.

Regression tests cover temporary left padding in _generate_completions (including error paths) and that LogCompletionsCallback leaves padding_side unchanged after logging.

Reviewed by Cursor Bugbot for commit 344676b. Bugbot is set up for automated code reviews on this repo. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Completion callbacks leave tokenizer in left-padding mode

1 participant