allenai / allenai/open-instruct

Thinker chat templates cannot be used for SFT: every row's labels are undecidable

Offen
#1,804 0 Kommentare 0 Reaktionen 1 zugewiesene Person Beansprucht von @farhatkevin Auf GitHub ansehen
Vorherrschende Sprache
Python
Sterne
3.9k
Forks
585
Ø Merge
5 T. 17 Std.
Gemergte PRs (30 T.)
16

Beschreibung

## Summary

The `*_thinker*` chat templates rewrite assistant content while rendering, so no span-based
derivation can locate the assistant turn. After the detection added for #1800, this no longer
corrupts labels silently — but it means **every conversation using these templates is dropped**,
so the templates are effectively unusable for SFT.

Affected (verified against the bundled test tokenizer, both `eos_token` variants, both
alternating and consecutive-assistant conversation shapes — 16 of 44 swept combinations):

- `olmo_thinker`
- `olmo_thinker_remove_intermediate_thinking`
- `tulu_thinker`
- `tulu_thinker_r1_style`

## Why derivation fails

Both available derivations locate an assistant turn by re-rendering conversation prefixes —
either as strings (char offsets into the full render) or as token counts. Both assume the turn
appears in the full render the same way it appears in a prefix render. The thinker templates
break that assumption directly: they split on ``, strip intermediate thinking, or inject
a `` opener into the generation prompt, so the rendered turn is not the message content.

Before the #1800 fix this was **silent**. The token-count path produced spans that started
mid-content:

```
olmo_thinker, alternating:
trained='<|im_end|>WO<|endoftext|>'
# ASSISTONE dropped entirely; only the fragment "WO" of ASSISTTWO trained

olmo_thinker, thinking content:
trained='THINKONEANSWERONE<|im_end|>THINKTWOANSWERTWO<|endoftext|>'
# the opening tag masked out of the loss
```

That second case is the one worth keeping in view: a thinking model that never sees `` in
its loss. It is now detected rather than trained on, but it is not *fixed*.

## Related: consecutive assistant turns

The same root cause makes consecutive assistant turns undecidable for **every** template
including `tulu` and `olmo`, because deriving the second turn's boundary requires rendering a
prefix that ends in an assistant turn — which templates special-casing the final turn render
differently from the full conversation. Rare in practice (15 rows of 939k in
`tulu-3-sft-olmo-2-mixture`) and currently dropped, but a `{% generation %}` fix would resolve
it too.

## Proposed fix

Mark assistant content in each template with `{% generation %}` blocks and read the mask from
`apply_chat_template(..., return_assistant_tokens_mask=True)`. This removes the offset
arithmetic entirely: no prefix re-rendering, no prefix-stability requirement, and content
rewriting stops mattering because the template itself declares which span is trainable.

Verified working on the pinned transformers (5.4.0): the returned `assistant_masks` aligns 1:1
with `input_ids` and survives `truncation=True, max_length=N`.

### Two constraints any patch must respect

1. **A template with no `{% generation %}` block does not raise.** It warns and returns an
all-zero mask. A partial migration would therefore train on nothing, silently — strictly
worse than today's behaviour, where undecidable rows are detected and counted. Any patch
needs an explicit non-empty-mask assertion. There is already a regression test pinning this
behaviour (`test_generation_blocks_yield_an_all_zero_mask_without_raising`).
2. **External templates have no generation blocks.** Qwen3's and Olmo-3's published templates
are used whenever `--chat_template_name` is unset or unrecognised, so the existing derivation
cannot simply be deleted; it has to stay as the path for templates the repo does not own.

## Test scaffolding already in place

`TestChatTemplateAssistantLabelSweep` in `open_instruct/test_dataset_transformation.py` sweeps
11 SFT templates × 2 `eos_token` variants × 2 conversation shapes, fully offline against the
bundled tokenizer fixture. `DERIVABLE_COMBINATIONS` lists the 18 of 44 that currently produce
correct labels; the other 26 are asserted to train nothing rather than train the wrong tokens.

Fixing this issue means moving entries into `DERIVABLE_COMBINATIONS` and watching the sweep go
green — the specification is already encoded, so the work is verifiable as it lands.

## Priority

Not urgent. These templates are currently unusable rather than dangerous, and no shipped Olmo 3
model was affected (the published SFT datasets were verified correctly masked — see the
resolution comment on #1800). But `olmo_thinker` is the natural template for thinking SFT, so
this blocks that path.

Refs #1800.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.