Skip to content

fix: don't count a discarded sample as generated text - #776

Open
manueldomke wants to merge 1 commit into
ROCm:mainfrom
manueldomke:fix-discarded-sample
Open

manueldomke wants to merge 1 commit into
ROCm:mainfrom
manueldomke:fix-discarded-sample

Conversation

@manueldomke

@manueldomke manueldomke commented Oct 12, 2026 •

Copy link
Copy Markdown

Motivation

Qwen3_5VL::insert() and Qwen3_6_MoE::insert() call _shared_insert(), which
prefills and draws one token into last_token. Their generate() then
force-feeds the think block and samples again -- the drawn token is never used,
and the only reference to it is an assert.

sample() records every drawn token in the repetition and frequency windows
first. A token that was thrown away therefore counts as generated text. For a
model that answers without a thinking block the token predicted after
assistant IS the answer letter, so it is pushed down by
count * freq_penalty + pre_penalty at exactly the position where it is
needed -- 1.0 + 1.5 = 2.5 logits with this model's defaults.

The visible effect is that the model answers against its own preference. On
Write one sentence about the sea. it says A vast, ever-changing blue expanse... where it says The sea is a vast, ever-changing blue mirror...
once the discarded sample is no longer counted. Both are fine sentences; the
second is the one the model chose.

Three Qwen3.5 fine-tunes converted for the NPU are affected directly: they
answer with a single letter, and that letter is the one being pushed down.

Technical Details

Sampler::sample() and sample_greedy() take a record_in_history flag,
defaulting to true, so no other model changes behaviour. sample() forwards
it to sample_greedy() on the top_k == 1 path, and _shared_insert() passes
it through from its caller. Qwen3_5VL::insert() and Qwen3_6_MoE::insert()
pass false for the draw they discard.

Both paths have to honour the flag. With only one of them in place the same
prompt answers differently depending on a sampling knob:

top_k=1    B     (greedy path)
top_k=40   A     (sampled path)

Scope, and what it does not claim. Two models are covered, because they are
the two where the draw is provably discarded: in both, last_token appears
only in an assert and is read nowhere. The flag defaults to true, so every
other model keeps its behaviour -- which also means a model with the same
pattern would keep the bug. Several of them call _shared_insert() more than
once per request (qwen3 four times, gemma4e three, qwen3vl three, qwen3_8mtp
three); whether any of those calls throws its draw away needs checking one by
one, and this PR does not claim to have done it.

Test Plan

Qwen3.5-2B, two prompts that differ in one clause, temperature: 0, the same
build settings before and after, both sampling paths:

  • top_k=40, temperature=0 -- the sampled path
  • top_k=1, temperature=0 -- the greedy path, through sample_greedy()

Test Result

Before, both paths and both prompts answered B. After, all four answer A:

        before   after
Q k=1     B        A
Q k=40    B        A
R k=1     B        A
R k=40    B        A

A is the argmax of the unpenalised distribution (0.339 against 0.299 for
B), so the answer is no longer the penalised one.

For text generation the change is visible but harmless: see the two sentences
in the motivation. Nothing else moves -- record_in_history defaults to
true, and only the two Qwen3.5-VL paths pass false.

Submission Checklist

🤖 Generated with Claude Code

Qwen3_5VL::insert() and Qwen3_6_MoE::insert() call _shared_insert(), which
prefills and draws one token into `last_token`. Their generate() then
force-feeds the think block and samples again; the drawn token is never used,
and the only reference to it is an assert.

sample() records every drawn token in the repetition and frequency windows
first. So a token that was thrown away counts as generated text. For a model
that answers without a thinking block, the token predicted after `assistant`
IS the answer letter -- it is pushed down by count * freq_penalty +
pre_penalty at exactly the position where it is needed, which for a single
occurrence is 1.0 + 1.5 = 2.5 logits (Qwen3.5-VL's defaults; its rep_penalty
is 1.0 and does nothing).

Measured on Qwen3.5-2B, temperature 0, default penalties, before and after,
nothing else changed:

    prompt Q   before B   after A
    prompt R   before B   after A

A is the argmax of the unpenalised distribution (0.339 against 0.299 for B),
so the answer is no longer the penalised one.

Sampler::sample() and sample_greedy() take a record_in_history flag,
defaulting to true, so no other model changes behaviour. sample() forwards it
to sample_greedy() on the top_k == 1 path, and _shared_insert() passes it
through -- both paths have to honour it, or the same prompt answers
differently depending on a sampling knob:

    top_k=1    B     (greedy path)
    top_k=40   A     (sampled path)   before both flags were in place

Co-Authored-By: DeepSeek V4 Flash (Default Thinking)
manueldomke added a commit to manueldomke/FastFlowLM that referenced this pull request Oct 21, 2026
Without thinking, the chat template ends with an empty think block.
Qwen3_5VL::insert() cut those four tokens off before the prefill, so the
checkpoint lands where the next turn of a conversation diverges, and
generate() then fed them in with one forward() each. Only after those four
decode steps came the draw for the answer's first token.

A single-turn model never returns to that checkpoint. For such a model the
block is now prefilled with the prompt, and the token _shared_insert() draws
from the prefill logits is the answer's first token. Multi-turn models keep
the old path, because there the checkpoint pays off. So does thinking, where
the model writes the block itself.

Under a decoding constraint the draw is kept as well. The constraint starts
from the prompt tail, which now ends behind the closed block, and it has
already accepted the token; nothing is rolled back.

The catalog's "single-turn" label now also sets AutoModel::single_turn. Until
now only three classes set the flag themselves, all with the label, so
nothing changes for them. It is only ever switched on, never off.

Measured on Qwen3.5-2B (NPU2) with the label set, 20 independent one-token
prompts, three alternating rounds against main: median wall clock 604 -> 486
ms per request. Prefill time is unchanged (430 vs 431 ms); the four tokens
it gains cost nothing measurable.

The numbers are not bit-identical. The template's tokens are exactly the
four ids generate() used to feed ([248068, 271, 248069, 271]), but they now
go through the prefill kernels instead of four decode steps. Against main
with the discarded-sample fix (ROCm#776) 1 of 20 answers differs, and that was an
exact tie (A and C both at logprob -1.939). One of two 60-token generations
diverges after 90 characters. With logprobs applied on top, the log-odds
between options move by 0.25 nats median, 0.5 at most.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
manueldomke added a commit to manueldomke/FastFlowLM that referenced this pull request Oct 21, 2026
Without thinking, the chat template ends with an empty think block.
Qwen3_5VL::insert() cut those four tokens off before the prefill, so the
checkpoint lands where the next turn of a conversation diverges, and
generate() then fed them in with one forward() each. Only after those four
decode steps came the draw for the answer's first token.

A single-turn model never returns to that checkpoint. For such a model the
block is now prefilled with the prompt, and the token _shared_insert() draws
from the prefill logits is the answer's first token. Multi-turn models keep
the old path, because there the checkpoint pays off. So does thinking, where
the model writes the block itself.

Under a decoding constraint the draw is kept as well. The constraint starts
from the prompt tail, which now ends behind the closed block, and it has
already accepted the token; nothing is rolled back.

The catalog's "single-turn" label now also sets AutoModel::single_turn. Until
now only three classes set the flag themselves, all with the label, so
nothing changes for them. It is only ever switched on, never off.

Measured on Qwen3.5-2B (NPU2) with the label set, 20 independent one-token
prompts, three alternating rounds against main: median wall clock 604 -> 486
ms per request. Prefill time is unchanged (430 vs 431 ms); the four tokens
it gains cost nothing measurable.

The numbers are not bit-identical. The template's tokens are exactly the
four ids generate() used to feed ([248068, 271, 248069, 271]), but they now
go through the prefill kernels instead of four decode steps. Against main
with the discarded-sample fix (ROCm#776) 1 of 20 answers differs, and that was an
exact tie (A and C both at logprob -1.939). One of two 60-token generations
diverges after 90 characters. With logprobs applied on top, the log-odds
between options move by 0.25 nats median, 0.5 at most.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
manueldomke added a commit to manueldomke/FastFlowLM that referenced this pull request Oct 21, 2026
…ck is prefilled

Builds on ROCm#777 (logprobs, which contains ROCm#776) and on the single-turn prefill
change. With the empty think block prefilled, the token _shared_insert() draws
is the answer's first token. It must be counted in the repetition window, and
a logprobs request must capture it. Until now generate() armed the capture
only on the old path, before the draw that follows the forced block, so a
single-turn model returned no logprobs at all.

_shared_insert() takes capture_logprobs and arms the sampler right before its
own draw, after the disarming at its top. Qwen3_5VL::insert() passes it,
together with record_sampled_token, exactly when the block was prefilled.

Measured on Qwen3.5-2B (NPU2), the same binary with and without the
"single-turn" label, 20 one-token prompts: 572 -> 455 ms median per request.
Both report one logprobs entry per answer token, the chosen entry matches the
content, and both return null without logprobs and with think:true.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@manueldomke

manueldomke commented Oct 21, 2026 •

Copy link
Copy Markdown
Author

Rebased onto main after #790: record_in_history now sits next to the
sampler's new allowed mask (_sample() -> sampler->sample()). No change in
behaviour; answers on Qwen3.5-2B (NPU2) are identical to the pre-rebase build,
and repeated identical requests pass.

This branch has not been deployed

No deployments
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.

1 participant