Skip to content

Trim the shared state prefix to a real token boundary - #47

Open
pxtroniwnl wants to merge 2 commits into
TheoLeeCJ:masterfrom
pxtroniwnl:fix/shared-state-prefix-token-boundary
Open

pxtroniwnl wants to merge 2 commits into
TheoLeeCJ:masterfrom
pxtroniwnl:fix/shared-state-prefix-token-boundary

Conversation

@pxtroniwnl

Copy link
Copy Markdown

Fixes #31.

Summary

score_shared (and SerialPrefixScorer, which has the same defect and is not
mentioned in the issue) rejects a subset of states that validate_row accepts,
failing with:

ValueError: The fixed state prefix does not match every full prompt

What makes this a bug rather than a contract the caller should respect: the same
row succeeds with one question and raises with two, and which states fail is a
property of the tokenizer vocabulary. A caller cannot predict it, cannot work
around it, and gets no actionable signal — the validation error describes a
plumbing invariant, not a problem with the input.

The reported example is a state string of Did the deploy use the same parameters)?.
Deleting the stray ) makes the same state work.

Cause

Both copies of _state_prefix ended with:

return tokenizer.encode(text, add_special_tokens=False)[:-1]

text is the chat prompt truncated just before the closing } of the evidence
object, so the last token holds the trailing ?" of the evidence. The code
assumed that appending the next field's punctuation always merges with exactly
one final token, so the shared prefix is the encoding minus one token.

That is a property of the state text, not a guarantee. Against the pinned
Qwen/Qwen3.5-4B tokenizer at revision 851bf6e, a state ending in (, -,
=, ? or } merges across the boundary by two tokens. The prefix is then
one token too long, and so is not a token prefix of the full prompt at all. That
is 5 of the 100 printable ASCII characters, and it is why the issue's row fails
while ...parameters? succeeds.

serial.py carries a duplicated copy of this helper with the same defect, so
SerialPrefixScorer raised on precisely the same states. The issue only reports
score_shared because that is the mode the reporter used. Both are fixed.

Fix

Replace the guess with the longest common token prefix between the truncated
evidence and that text plus the separator that always follows it:

ids = tokenizer.encode(text, add_special_tokens=False)
extended = tokenizer.encode(text + payload[len(evidence):], add_special_tokens=False)
return [first for first, _ in takewhile(lambda pair: pair[0] == pair[1], zip(ids, extended))]

This is sound by construction: the result is a prefix of both encodings by
definition, so it is always a valid shared prefix. It is also the longest one
available, so it does not give up shared tokens where the old code happened to be
right.

The separator is read from the payload already in hand rather than hardcoded, so
this survives a change to the prompt wording.

I also considered a narrower fix — fall back to the common prefix only when
ids[:-1] fails the prefix check — and rejected it. It is unsound: the length
comparison it relies on is not a validity condition, and it passed the sweep only
by coincidence. The pure common prefix is both sound and simpler.

One behavioural consequence worth stating plainly: when the boundary token
merges forward, the shared prefix now stops just before it, so the prefix ends
mid-word rather than after a complete word. E.g. for the issue's state it ends
at ...same parameters rather than ...same parameters?". That is unavoidable
and correct — the merged token straddles the boundary, so it cannot be shared
and belongs to the suffix. It is a consequence of the fix, not a regression.

Verification

tests/test_state_prefix.py drives both copies through a byte tokenizer that
merges only at the evidence boundary, so the regression is covered with no
torch and no network
— it runs in the default suite. Four of its cases fail
against the previous code, and I confirmed that by reverting the fix and
re-running.

The fix is a no-op on every committed fixture. Audited with the pinned
tokenizer over all 217 distinct states in shape777, authored144 and
perturbations108:

  • zero prefix differences against the previous implementation
  • both copies now agree on all 217, where before they could diverge
  • 217/217 prefixes were valid before and 217/217 are valid after

So no committed evidence is affected, nothing needs regenerating, and the change
cannot alter any reported number.

pytest -q on this branch: 75 passed, 3 skipped.
python benchmarks/verify_published.py: 69 claims verified.
cd results/raw && sha256sum -c SHA256SUMS: 23 of 23 OK.

Limitations

No files under results/ are touched and no reported number changes.

validate_row declares state may be a string, object or array and score()
honours all three, but the prefix-sharing paths rejected a subset of valid
states with "The fixed state prefix does not match every full prompt". The same
row succeeded with one question and raised with two, and which states failed was
a property of the tokenizer vocabulary, so a caller could not predict or avoid it.

Both copies of _state_prefix ended with encode(text)[:-1], assuming that
appending the next field's punctuation always merges with exactly one final
token. That is a property of the state text, not a guarantee. Against the pinned
Qwen/Qwen3.5-4B tokenizer at revision 851bf6e, a state ending in one of ( - = ? }
produces a prefix one token too long, so it is not a token prefix of the full
prompt at all: 5 of the 100 printable ASCII characters, and the state from the
issue report, "Did the deploy use the same parameters)?", is one of them.

serial.py carried the same duplicated helper with the same defect, so
SerialPrefixScorer raised on exactly the same states even though the issue only
reports score_shared. Both are fixed here.

Replace the guess with the longest common token prefix between the truncated
evidence and that text plus the separator that always follows it. The separator
is taken from the payload already in hand rather than hardcoded, so the fix
survives a change to the prompt wording. A consequence worth stating: when the
boundary token merges forward, the shared prefix now stops just before it, which
is the longest prefix that can be shared at all.

tests/test_state_prefix.py drives both copies through a byte tokenizer that
merges only at the evidence boundary, so the regression is covered without torch
and without network. Four of its cases fail against the previous code.

The fix is a no-op on every committed fixture. Audited with the pinned tokenizer
over all 217 distinct states in shape777, authored144 and perturbations108: zero
prefix differences against the previous implementation, and both copies now
agree on all of them, so no committed evidence is affected and nothing needs
regenerating. verify_published.py still reproduces all 69 claims and every
results/raw checksum still matches.
The stub tokenizer only reproduced the string-state variant, so the test proved
less than it appeared to: it would still have passed if the object case were
broken. Objects and arrays are the shapes that actually fail in practice, because
the value's own closing brace or quote sits right in front of the comma.

Model both extremes instead of one hand-picked triple. PlainTokenizer never merges
so the boundary costs exactly one token, which is the case the old code handled
correctly; MergingTokenizer lets a token straddle the comma, which is the case it
did not. Six states across both tokenizers and both copies: 48 cases, and 10 of
them fail against the previous implementation.

Verified against the pinned Qwen tokenizer that this is not a stub artefact: of 70
structured states ending in a merge-prone character, 11 produce an invalid prefix
with the old code and 0 with the fix.
@pxtroniwnl

Copy link
Copy Markdown
Author

Addendum after review of #35

@maximelefrancois86 found this bug
independently in #35 and left it unfixed on purpose, writing:

Cutting at the longest common token prefix would fix it.

That is exactly what this PR does, so this is not a competing fix — it is the
fix #35 asked for.
#35's diagnosis and this implementation agree on both the
mechanism and the remedy.

Two things this PR adds on top of that note:

  1. The same defect is in two places, not one. serial.py carries a duplicated
    copy of _state_prefix with the same [:-1], so SerialPrefixScorer raised on
    precisely the same states. Pluggable direct prompt: built-in en/fr wordings, prompt files, one prompt_version per wording #35 only mentions serial and shared; both are fixed
    here.

  2. The object/array variant is the one that bites in practice, and it is now
    covered.
    My first version of the regression test reproduced only the
    string-state variant, which meant it would have passed even with the object case
    broken — worth saying plainly, since that is what a hand-written stub tends to
    do. The test now models both extremes: a tokenizer that never merges (where the
    old code was correct) and one whose tokens straddle the comma (where it was not).
    48 cases, 10 of which fail against the previous code, in both copies.

Measured against the pinned tokenizer, over 70 structured states whose tail ends
in a merge-prone character: 11 produce an invalid prefix with the old code, 0
with this fix.
For context, #35 reports 10 of 300 structured states failing on
the private dataset — the same order of magnitude, so the fix should clear it.

The audit against all 217 committed states is unchanged by this addendum: still
zero prefix differences, so no committed evidence is affected.

Suggested integration

Since #35 is stacked on #34 and rewrites the prompt, whoever merges these should
rebase #35 onto this branch (or rebase this branch onto #35) rather than resolve
the shared hunks by hand — _state_prefix is touched by both. The one thing to
preserve is that the separator is read from payload[len(evidence):] instead of
being hardcoded, so the boundary stays correct for the French wording and for
prompt files.

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.

score_shared rejects string and object states that validate_row accepts

1 participant