Repository navigation
Conversation
added 2 commits
August 10, 2026 23:26
llama_detokenize() and llama_tokenize() both apply sequence-level space handling, so llguidance got wrong bytes in both directions. Assisted-by: Claude Opus 5
The behaviour was removed in ggml-org#4081. Assisted-by: Claude Opus 5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Review Guide: llguidance token-bytes and tokenize_fn byte-exactness
Branch:
llg-spm-tokenize(worktreerepos/llama-wt-spm), based on upstream030ebb55.Build used throughout:
cmake -B build -DGGML_CUDA=ON -DLLAMA_LLGUIDANCE=ON -DCMAKE_BUILD_TYPE=Release -DCMAKE_CUDA_ARCHITECTURES=89-real -DLLAMA_BUILD_TESTS=ON.Diff: 4 files, 152 insertions, 13 deletions. The fix itself is ~20 lines in
common/llguidance.cpp;the rest is the regression test and one stale-comment deletion.
1. One-paragraph summary
common/llguidance.cppis the bridge between llama.cpp's vocab and the llguidance constraintengine. llguidance needs an exact bytes <-> tokens mapping in both directions. Both of
llama.cpp's conversion calls apply sequence-level SentencePiece/HF decorations that are correct
for their normal callers and wrong here. Direction one (
tokens -> bytes, building llguidance'stoken table) used
llama_detokenize(), which strips a leading space; this is a live constraintviolation on every vocab type. Direction two (
bytes -> tokens, thetokenize_fncallback)used
llama_tokenize(), which prepends a space onadd_space_prefixvocabs; this kills theconstraint entirely on SentencePiece. The fix uses
llama_token_to_piece()for the first, and forthe second detects non-byte-exact tokenizers and hands llguidance its own token table instead.
2. The contract being violated
Verbatim from the vendored
llguidance.h(build/llguidance/source/parser/llguidance.h:196-225):token_bytesis "the exact bytes of each token".tokenize_fnmust "only tokenize the bytes andnot add any
<BOS>etc.". Both requirements were being broken.Note
tokenize_assumes_stringis alreadytruein llama.cpp. Its doc says it works around"tokenize_fn only accepting valid UTF-8 strings and possibly adding etc." -- but the
source says the
<BOS>half is a TODO, not implemented (llguidance.h:208-211). It onlyhandles invalid UTF-8 (
toktrie/src/toktree.rs:595-618,tokenize_with_greedy_fallback). So theflag does not rescue us.
3. Flow of one constrained token, end to end
Follow this to see where each bug lands.
3.1 Request to sampler
common/sampling.cpp:215is the only production caller. The other references are the sampler'sown
clone(common/llguidance.cpp:80) and the tests. Anything not starting with the literal%llguidancefalls through to GBNF atcommon/sampling.cpp:266-270, which is why thetoolsrequest path is unaffected by any of this --common/chat.cppbuilds tool grammars withbuild_grammar(), which emits raw GBNF.3.2 Sampler construction -- where both bugs are planted
llama_sampler_init_llg(common/llguidance.cpp:262) callsllama_sampler_llg_new_tokenizer(vocab)(:139), which does two things:token_bytes/token_lensby looping over alln_vocabtokens. Bug A liveshere.
tokenize_fn = llama_sampler_llg_tokenize_fn(:123). Bug B lives here.Then
llg_new_tokenizer(&tinit, ...)builds llguidance'sTokTriefromtoken_bytes, andllama_sampler_llg_new(:17) builds the matcher viallg_new_matcher.Worth knowing for review:
llg_new_matcher's doc (parser/src/ffi.rs:973-974) saysinit.ff_tokens_okis ignored;Matcher::new(parser/src/matcher.rs:24-40) callsstart_without_prompt(), neverprocess_prompt(). That matters in section 6.3.3 Per-token decode loop
Two things a reviewer should take from this:
compute_mask_innercallsff_tokens()whenevercan_force_bytes().ff_tokens()is notonly for jump-forward; it is on the ordinary mask path. So bug B breaks mask computation, in a
plain upstream build, with no feature flags.
can_force_bytes()(tokenparser.rs:639) is!lexer_spec().no_forcing && token_env.tokenize_is_canonical(). Andtokenize_is_canonical()is simplytokenize_fn.is_some()(parser/src/ffi.rs:73). This is thehinge the fix turns.
4. Bug A:
token_bytesbuilt withllama_detokenize()4.1 The code
4.2 Why a single-token detokenize is not a piece
llama_vocab::impl::detokenize(src/llama-vocab.cpp:3658):remove_spaceis passed astoken_to_piece'slstripargument, andlstripstrips leadingspaces (
:3549). Becauseremove_specialisfalse, theremove_space = falsereset neverhappens before the loop, so with
n_tokens == 1every token getslstrip = 1.Then, after the loop:
So there are two independent corruption mechanisms, and they hit different vocab families:
lstrip = remove_space = add_space_prefixadd_space_prefix:2376sets ittrue), UGM via KVclean_spacespasses over" ?!.,"clean_spaces:2119sets ittrue), WPM (:2383)Defaults, from
src/llama-vocab.cpp::2117):2119):2376):2383)tokenizer.ggml.add_space_prefixKV (:2401)This is the point most likely to be missed: BPE is not safe.
clean_spacesistrueforevery BPE vocab.
4.3 Measured: how many tokens got the wrong bytes
Scratch tool comparing
llama_token_to_piece(vocab, t, buf, n, 0, false)againstllama_detokenize(vocab, &t, 1, buf, n, false, false)for every token:models/ggml-vocab-llama-spm.ggufmodels/ggml-vocab-llama-bpe.ggufmodels/ggml-vocab-gemma-4.ggufmodels/ggml-vocab-bert-bge.ggufExamples:
4.4 Measured: the user-visible consequence
llguidance believes token 5354 is
.., so a grammar that requires..and forbids a space stillpermits it. Grammar
start: /[.a-z]{2,6}/ "!", counting allowed tokens at position 0 whose realpiece contains a space:
.,..,...,....,.....,......)So on Llama-3 and Gemma -- the common case, no SPM involved -- a grammar that forbids whitespace
can emit whitespace, and llguidance records the token as if it had not.
Why this stays hidden in casual testing: when
can_force_bytes()is true and the grammar has aunique continuation,
compute_mask_innerreturnssingleton_token_set(ff_tokens[0])(tokenparser.rs:459-463) computed fromtokenize_fn, whichis correct on BPE. The corrupted trie is only consulted at positions with genuine choice
(
compute_bias) and inconsume_tokenbyte accounting. That is exactly why the test grammar aboveneeds a
/regex/rather than a plain literal.4.5 The fix, and why it is behaviour-preserving
llama_detokenizeon one token istoken_to_piece(token, ..., lstrip=remove_space, special=unparse_special)plus optional
clean_spaces. Passinglstrip = 0and skippingclean_spacesis precisely "give methe raw piece", which is what
token_bytesis documented to hold.The
size == 0fallback branch is unchanged in behaviour.token_to_piecereturns 0 exactly when!special && (attr & (LLAMA_TOKEN_ATTR_UNKNOWN | LLAMA_TOKEN_ATTR_CONTROL))(
src/llama-vocab.cpp:3536-3540), and single-tokendetokenizereturns whatevertoken_to_piecereturned. So the same set of tokens takes the
\xffspecial-token-marker branch before andafter. The second call likewise changes only
lstrip(false->0) and dropsclean_spaces.5. Bug B:
tokenize_fn=llama_tokenize()5.1 The code and the state at failure
SPM branch of
llama_vocab::impl::tokenize(src/llama-vocab.cpp:3348-3366):is_prev_specialis initialisedtrueunconditionally, so the first raw-text fragment alwaysgets the space when
add_space_prefix.add_special = falsedoes not suppress it (it only gatesBOS/EOS). This matches HF:
tokenizer.encode("The", add_special_tokens=False)on Llama SPM returns[450]=_The. It is correct SentencePiece behaviour and a wrong callback for llguidance.Concrete state,
models/ggml-vocab-llama-spm.gguf:So for grammar
start: "hello world", position 0:Measured, pre-fix,
test_forced_mask_byteson llama-spm:The sampler is then forced to emit
" hello";llama_sampler_llg_accept_implcallsllg_matcher_consume_token(22172), which fails:The matcher enters an error state, the next
llg_matcher_compute_maskfails, andllama_sampler_llg_apply(common/llguidance.cpp:53-58) does:From then on
applyis a no-op: generation continues completely unconstrained, HTTP 200, noerror field. Measured on TinyLlama-1.1B with a grammar admitting exactly one string:
5.2 Why not fix the tokenizer instead
This is the first question a reviewer will ask, so it is worth having the answer ready.
is_prev_special = trueis unconditional; a special-token prefixre-arms it (
:3378setsis_prev_special = trueforFRAGMENT_BUFFER_VARIANT_TYPE_TOKEN), soprefixing
<s>does not help; andadd_space_prefixhas no public getter inllama.h(only
llama_vocab::get_add_space_prefix()insrc/llama-vocab.h:142, not reachable fromcommon/).bool is_prev_special = add_special;is a one-line change with a large blast radius: everyadd_special = falsecaller on an SPM vocab would tokenize differently. Small diff, big risk --the wrong shape for a bug fix.
vocab piece spans the boundary, which is false for large SPM vocabs (Gemma has multi-newline
pieces).
So the byte-exact route needs a vocab-level API decision, which does not belong in a bug fix. It is
listed as an open question in section 10.
5.3 The fix
Probe rationale: three probes rather than one, cheap (once per vocab, cached by
vocab_cache/tokenizer_cacheat:141-146)."The"catches the SPM prefix;"a b"catchesinterior whitespace handling;
"1\n2"catches newline mangling (theclean_spacesfamily). Anyfailure -- including a tokenize error or an oversized piece -- returns
false, so the probe failssafe: worst case a byte-exact vocab is misclassified and loses byte forcing, never gains a
wrong mask. Note the probe deliberately reassembles with
llama_token_to_piece, i.e. it tests theround trip through both fixed conversion points.
6. Why the greedy fallback does not degrade constraint enforcement
llguidance's own header calls greedy tokenization "often incorrect and may reduce accuracy", so
this needs justifying rather than asserting.
Where
tokenize_bytesis actually called. Exhaustive, from the vendored source:tokenparser.rs:235inprocess_promptMatcher::newcallsstart_without_prompt()(matcher.rs:32-34)tokenparser.rs:249inprocess_prompttokenparser.rs:683inff_tokensdo_force = ... && tokenize_is_canonical()(:680)tokenparser.rs:695inff_tokensSo on the greedy path llguidance never tokenizes bytes at all. The "reduced accuracy" warning
describes a code path llama.cpp does not take. (It also means
process_prompt'sassert!(tokenize_is_canonical())at:220is not a crash risk here.)Where the mask comes from instead.
compute_mask_inner's else-branch:compute_biaswalks theTokTrieagainst the Earley/lexer state. That is the real constraint andit does not involve
tokenize_bytes. Enforcement is exact.What is actually lost.
can_force_bytes()goes false, so:tokens that spell a legal continuation (see the 6-token result below);
llg_matcher_compute_ff_tokens()returns nothing.None of these affect correctness. Item 3 is the price: a jump-forward-style feature cannot fire
on SPM vocabs. That is graceful degradation and matches the pre-fix reality, where
ff_nwas 0because the matcher was dead.
Nothing changes for byte-exact vocabs. When the probe returns
true,tinitisbyte-identical to before, so BPE/gemma users keep canonical tokenization and forcing. Verified: on
llama-bpe,
test_forced_mask_bytesreports the same single allowed token (15339"hello") beforeand after.
7. Test design, and why the obvious test does not work
tests/test-grammar-llguidance.cpphas only ever been run against a BPE vocab(
tests/CMakeLists.txt:148). Three changes:7.1
test_no_space_tokens()-- catches bug A, on the vocab CI already usesGrammar
start: /[.a-z]{2,6}/ "!"; asserts no allowed token contains a space. A/regex/isrequired rather than a literal, because a literal triggers the singleton-forcing short-circuit and
never consults the trie (section 4.4).
.,.., ...)This is the strongest part of the test story: it fails today on
ggml-vocab-llama-bpe.gguf, thevocab already wired into CI. No new fixture needed to demonstrate the bug.
7.2
test_forced_mask_bytes()-- catches bug BGrammar admitting one literal; asserts every allowed token is a prefix of it.
"hello"(ok)" hello""hello"h,he,hel,hell,hello,hA weaker assertion would have proved nothing. My first attempt asserted only
0 < n_allowed < n_vocab-- "the mask is a real constraint". That passes on the buggy build,because the buggy mask allows exactly one token; it is just the wrong one. The assertion has to be
about bytes, not cardinality. Worth flagging to a reviewer as the reason the test looks the way it
does.
7.3
tokenizer_is_exact()guard and the SPM ctest entryThe pre-existing string tests call
common_tokenize()on their own expected strings, which appliesthe same space prefix, so they cannot express
"abc"on an SPM vocab and would fail spuriously.They are skipped there. This is a genuine limitation, not laziness: it is the same missing
capability described in section 5.2, and it is fair to say so in review.
Both ctest entries pass:
ctest -R llguidance-> 2/2.8. Reproduction, without and with the fix
Server level (
jf_eval/spm_fix_verify.py, three cases: forced literal,json_schemaenum, forcedskeleton around a free region):
Pre-fix outputs worth quoting, because they show the two failure modes are different:
The
json_enumline is the one to lead with for a maintainer: it is not an error, it isstructured-looking output with a corrupted key name.
9. The stale
llama.hcomment -- yes, it belongs hereinclude/llama.h:1156-1158, before:That last sentence is false, and has been since November 2023. History:
1a159553f"tokenizer : special token handling (tokenizer : special token handling ggml-org/llama.cpp#3538)", Oct 2023, introduced both the sentenceand the behaviour:
auto raw_text = (special ? "" : " ") + fragment.raw_text.substr(...). Soparse_special = truegenuinely suppressed the leading space.a6fc554e2"llama : restore prefix space in llama tokenizer (llama : restore prefix space in llama tokenizer ggml-org/llama.cpp#4081)", Nov 2023, deliberatelyreverted the behaviour:
if (&fragment == &fragment_buffer.front()) { raw_text = " " + raw_text; }.The comment was not updated.
Why it belongs in this PR rather than a separate one:
llama_tokenize'scontract will ask "why not pass
parse_special = true?" -- and the honest answer is "the doc iswrong". Leaving it in place guarantees that round trip.
behaviour", which is an invalid framing -- llama : restore prefix space in llama tokenizer ggml-org/llama.cpp#4081 was intentional. Checking history is what
killed that framing. A reviewer who does not check history will reach the same wrong conclusion.
llama_tokenize), andtrivially separable.
Scope choice: delete the false sentence, do not replace it. The change made is exactly:
The tempting alternative is to document what does happen, e.g. "SPM and UGM vocabs insert a
leading space when
tokenizer.ggml.add_space_prefixis set, independent of both flags". That istrue, but it is a new documented behaviour claim on a public API, and documenting it invites
exactly the "should it?" discussion that ggml-org#4081 already had. Deleting a false sentence needs no such
debate. If a maintainer asks for the positive statement, it is a one-line follow-up.
Recommendation: keep it as its own commit in this PR, so a maintainer who wants it split can
drop one commit without touching the fix.
10. What a reviewer should push on
tokenize_bytesis unreachable in llama.cpp's Matcher usage. But this is a maintainer preference, and it is the open question to raise.token_byteschange alter behaviour for BPE users?\xffspecial-token branch (section 4.5)./regex/needed intest_no_space_tokens?token_bytesloop for special-token attrs and keepsllama_detokenize, so it does not fix either bug. Worth referencing..github/workflows/build-3rd-party.yml:30builds-DLLAMA_LLGUIDANCE=ON, so it compiles; it just never tested an SPM vocab, and the BPE bug went unnoticed because no test asserted on token bytes.No existing issue or PR reports either bug (searched
llguidance,grammar ignored sentencepiece,add_space_prefix grammar,grammar not enforced llguidance).11. Files touched
common/llguidance.cppllama_token_to_piecefortoken_bytes; probe + greedy fallback fortokenize_fntests/test-grammar-llguidance.cpptest_no_space_tokens(bug A),test_forced_mask_bytes(bug B),tokenizer_is_exactguardtests/CMakeLists.txtggml-vocab-llama-spm.gguftooinclude/llama.hScratch tools used for the measurements in sections 4.3 and 4.4 are not part of the PR; they live
in the session scratchpad (
tokbytes.cpp,maskcheck.cpp) and are worth keeping locally to answerreview questions.