Commit Graph

1 Commits

Author SHA1 Message Date
JustVugg 6de32c55f6 Don't trust a converted checkpoint's config for the stop set
The engine armed its stop tokens from config.json's eos_token_id and nothing
else. That trusts metadata written by third-party conversion tooling, which is
a thing we already know goes wrong: the README documents a mirror shipping
int4 MTP heads that silently give 0% draft acceptance. GLM-5.2 declares THREE
eos ids (<|endoftext|>, <|user|>, <|observation|>); a converter that rewrites
config.json with a reduced list leaves the engine stopping on fewer tokens
than the model emits, and the missed ones get detokenized and printed into the
chat as literal text while generation runs past the end of the turn.

Two independent defenses:

  - eos_token_id is now unioned with generation_config.json, which is
    HuggingFace's authority for generation (config.json often carries a
    partial legacy copy). An extra stop is harmless; a missing one is not.

  - every added-token the TOKENIZER marks "special":true is armed as a stop,
    whatever the configs say. Those are control tokens (<|user|>, <|assistant|>,
    <sop>, [gMASK], the image/video/audio markers) and are never legitimate
    content in a reply -- GLM itself lists three of them as official eos.
    <think>/<tool_call>/<arg_key> are "special":false and are deliberately NOT
    swept up: they are real output. tok.h was parsing added_tokens but throwing
    the "special" flag away, so the distinction wasn't available to anyone.

On the real per-row checkpoint this takes the armed set from 3 to 18:
  [stop] 18 stop tokens: 154820 154827 154829 154821 ... (15 from the
  tokenizer's special set)

Honesty about scope: this is hygiene for a class of bug, NOT a fix for the
trailing-junk report on #298 that prompted it. I hypothesised @woolcoxm's g64
checkpoint had lost eos ids in conversion; he checked, and it hadn't -- his
config arms all three correctly. The emit path is also innocent: is_stop() is
checked BEFORE emit() at every one of the four call sites (4215, 4256, 4908,
4987), so a correctly-armed stop cannot be printed. His trailing junk is still
unexplained and is more likely quantization noise. What this commit buys is
that a checkpoint we don't control cannot leak control tokens into a reply,
which was true before and is not now.

tests/test_stops.c covers both defenses: the union, a missing
generation_config.json, BOTH configs mutilated (the tokenizer still stops all
five control tokens while leaving <think> alone), and T=NULL (the validation
path keeps config-only behaviour).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-07-16 17:59:34 +02:00