fix(llguidance): raise a clear error for CFG literals matching special tokens - #1965
Conversation
…l tokens A grammar literal that exactly matches one of the tokenizer's special tokens (e.g. `<think>` on reasoning-model tokenizers) can make llguidance's parser fail with an opaque `ParserTooComplex` error at generation time. The model may emit the literal as a single atomic special token, which the parser can't match against a literal-string terminal the way it matches ordinary, byte-decomposable tokens. Detect this at grammar-compile time for the Transformers backend, where the tokenizer's special tokens are available, and raise a `ValueError` naming the conflicting token(s) instead of letting the confusing parser failure surface later during generation. Fixes dottxt-ai#1771
The special-token guard added in c471c4b only checked `all_special_tokens`, which misses tokens registered with `special=False`. Reasoning-model tokenizers commonly register their `<think>`/`</think>` tags this way (e.g. Qwen/Qwen3-4B-Thinking-2507) specifically so `skip_special_tokens=True` decoding doesn't strip them - but the tokenizer still emits them atomically, bypassing ordinary BPE merging exactly like a true special token does. Verified against the real Qwen3-4B-Thinking tokenizer: `<think>` is absent from `all_special_tokens`, so the existing guard silently let the original dottxt-ai#1771 grammar through to the opaque runtime `ParserTooComplex` error it was meant to catch. Broaden the check to the union of `all_special_tokens` and every token in `added_tokens_decoder`, since the `special` flag only governs decode-time stripping, not which tokens bypass BPE merging. Adds a regression test reproducing the exact gap via `add_tokens(..., special_tokens=False)` on the existing lightweight gpt2-mini fixture, so it doesn't need network access to the 4B-parameter model.
ErenAta16
left a comment
There was a problem hiding this comment.
The diagnosis reads right to me, and the docstring explaining why added_tokens_decoder is included alongside all_special_tokens — that special only governs decode-time stripping, not whether the tokenizer splits the text out before BPE — is the part I'd have got wrong. Worth keeping.
The literal extraction is where I'd look again. Running the regex over a few grammar shapes:
| grammar | literals found | conflict raised |
|---|---|---|
start: "<think>" WORD |
['<think>'] |
yes |
start: '<think>' WORD |
[] |
no |
start: "<think>" WORD |
['\\u003cthink\\u003e'] |
no |
// example: "<think>" + rule |
['<think>'] |
yes, from a comment |
start: "say\"hi" WORD |
['say\\"hi'] |
no |
Four things fall out of that.
Single-quoted literals are missed. Lark accepts '<think>' and "<think>" interchangeably, and the pattern only looks at double quotes. Someone hitting the original ParserTooComplex and switching quote style to see if it helps would get the opaque error back with no explanation. Adding '((?:[^'\\]|\\.)*)' as an alternation covers it.
Escapes aren't decoded before comparison. The capture keeps the raw text, so "<think>" and "say\"hi" are compared as their source form rather than the string they denote. Both are ways to write a conflicting literal that the check waves through. The escapes a grammar author would realistically use here are \" and \uXXXX, so decoding those two before the set intersection would close most of it.
A literal inside a comment raises. That's the direction that costs the user something — a valid grammar refuses to compile because a token name appears in a // comment. Stripping comments before the scan, or at least mentioning the limitation in the error, would avoid a confusing hard failure.
The check is exact-match only. "<think>abc" embeds the atomic token without equalling it, so it passes the check and presumably still confuses the parser. That may be deliberate scoping — matching substrings would produce false positives on any grammar containing < — but if so it's worth a line saying the check covers exact literals only, since the error message reads like a general guarantee.
None of these break the reported case; they're all ways the same bug reaches a user without the new error firing. Given the point of the PR is turning an opaque failure into an actionable one, the quote-style gap in particular seems worth closing before merge — it's the first thing someone debugging this would try.
Address review feedback on the literal-matching check added in the previous commits: it only scanned double-quoted literals (Lark accepts single-quotes too), compared raw source text instead of decoding backslash escapes (so a \uXXXX-escaped literal slipped through), and had no notion of comments, so a token's text appearing inside a `//` comment triggered a false-positive rejection. Replace the single find-all regex with a combined literal-or-comment scanner: string literals (either quote style) are extracted and escape-decoded, comment matches are discarded, and a `//` occurring inside a literal (e.g. a URL) is correctly kept as part of that literal rather than treated as a comment start. Also documents that the check is exact-literal-match only, not a substring check.
|
Thanks for the close read — the regex table made the gaps concrete. Pushed
On exact-match-only: kept that scoping as-is (matching substrings would indeed produce false positives on grammars containing Added regression tests for each of the four cases from your table (single-quote, |
ErenAta16
left a comment
There was a problem hiding this comment.
Re-ran the same table against 269a7f87. All five rows flip, and the //-inside-a-literal case the new comment mentions holds too:
| grammar | literals found | conflict |
|---|---|---|
start: "<think>" W |
['<think>'] |
yes |
start: '<think>' W |
['<think>'] |
yes (was missed) |
start: "<think>" W |
['<think>'] |
yes (was missed) |
// ornek: "<think>" + rule |
[] |
none (was a false positive) |
start: "say\"hi" W |
['say"hi'] |
yes (was missed) |
start: "http://foo" W |
['http://foo'] |
none |
The alternation ordering is the part worth keeping the comment on. Putting the string alternatives before //[^\n]* means a // inside a literal gets consumed by the string match, so "http://foo" survives intact instead of having the rest of the line swallowed as a comment — and because only dq/sq are read, a literal written inside a real comment never reaches the set. Two problems solved by one ordering decision, which isn't obvious from reading the regex alone.
Decoding \uXXXX alongside the simple escapes covers the case I'd worry about most in practice, since a grammar author working around the original ParserTooComplex is fairly likely to try escaping the angle brackets.
Substring matching still isn't covered ("<think>abc" passes), but you've said so explicitly in the docstring now, which was the ask — the error message no longer reads like a guarantee it can't make.
|
Ran the new scanner against the cases I was worried about rather than eyeballing it, and all three gaps are genuinely closed. Extracting with
The two that could plausibly have gone wrong both hold up. An unpaired Two observations, neither blocking: Lark regexp terminals aren't distinguished from string literals.
The |
The attribute was added in transformers 4.34; accessing it bare in the constructor raised AttributeError for any older tokenizer, taking down the entire LLGuidanceBackend (including JSON-schema and regex backends) for a CFG-only feature. Fall back to an empty dict so the atomic-token set degrades gracefully to all_special_tokens on older installations. Also adds a docstring note to _extract_grammar_literals clarifying that the scanner may extract quoted runs from inside Lark regexp terminals (e.g. the "[\^" portion of STR: /"[^"]*"/); these can never match a real token string and cannot produce false positives.
|
Pushed
The special/added token distinction in the docstring is unchanged, as requested. |
Problem
Fixes #1771.
A CFG literal that exactly matches one of the tokenizer's special tokens (e.g.
<think>on reasoning-model tokenizers like Qwen3-4B-Thinking) makes llguidance's parser fail with an opaqueParserTooComplexerror at generation time:Root cause: the grammar's literal terminal (
"<think>") is written for byte-level matching, but the tokenizer emits<think>as a single atomic token rather than as decomposable byte fragments. llguidance's parser can't reconcile an atomic token against a literal-string terminal, and bails out with a hard-to-diagnose error deep in generation.What this does
This doesn't attempt to fix llguidance's parser itself (that lives in a separate compiled dependency,
guidance-ai/llguidance, and reasoning-model support there looks like a bigger design effort — the maintainer's comment on the issue mentions this is being worked on). Instead, it detects the conflict at grammar-compile time, where we have both the grammar and the tokenizer's vocabulary available, and raises a clearValueErrornaming the conflicting literal(s) instead of letting the confusing runtime parser failure surface later during generation.Scoped to the
Transformersbackend (where the tokenizer's vocabulary is available) since that's what's reported and what I could verify.LlamaCppandMLXLMtokenizers expose this differently and aren't covered here.Update: the initial version of this fix only checked
all_special_tokens, which misses tokens registered withspecial=False. Reasoning-model tokenizers commonly register<think>/</think>exactly this way (verified against the realQwen/Qwen3-4B-Thinking-2507tokenizer —<think>is absent fromall_special_tokens) specifically soskip_special_tokens=Truedecoding doesn't strip them mid-generation, but the tokenizer still emits them atomically, bypassing ordinary BPE merging exactly like a true special token does. Without this, the guard would have silently let the original issue's exact grammar through to the same opaqueParserTooComplexfailure it was meant to catch. The check now covers the union ofall_special_tokensand every token inadded_tokens_decoder, since thespecialflag only governs decode-time stripping, not which tokens bypass BPE merging.Testing
Qwen/Qwen3-4B-Thinking-2507tokenizer (no model weights needed for the grammar-compile-time check — confirmed separately end-to-end with the actual model loaded too) and confirmed the exactParserTooComplexfailure from the issue, then confirmed the fix raises a clearValueErrorinstead.test_cfg_logits_processor_rejects_literal_matching_special_token, using the existing lightweighterwanf/gpt2-minifixture already in this test file (its<|endoftext|>special token stands in for<think>, so the test doesn't need network access to a 4B+ model).test_cfg_logits_processor_allows_literal_not_matching_special_tokento confirm ordinary grammars are unaffected.test_cfg_logits_processor_rejects_literal_matching_added_non_special_token, reproducing the added-but-not-special gap viatokenizer.add_tokens(["<think>"], special_tokens=False)on the same lightweight fixture — no network access to the 4B model needed here either.test_llguidance_backend[model_transformers-torch]parametrization (the one exercisingget_cfg_logits_processorwith the existingcfg_lark/cfg_ebnffixtures end-to-end through real generation) to confirm no regression.ruff checkclean on both changed files (matches the pinned pre-commit ruff version, 0.9.1).pre-commit runmypy reports two pre-existing, unrelatedurllib3stub errors insrc/outlines/exceptions.pypresent identically without this change (confirmed by re-running against the base commit) — not introduced by this PR.