Fix three places where constrained JSON generation ignores the model - #180
Open
asaptf wants to merge 3 commits into
Open
Fix three places where constrained JSON generation ignores the model#180asaptf wants to merge 3 commits into
asaptf wants to merge 3 commits into
Conversation
ConstrainedJSONGenerator pre-filtered optional properties with a hash of
the field name XOR the token budget:
let hash = key.utf8.reduce(0) { ($0 &* 31) &+ Int($1) }
let combined = hash ^ backend.totalTokenBudget
return combined % 2 == 0
Because 31 is odd and the budget is typically even, this reduces to the
parity of the field name's bytes — identical for every document, decided
before the model is consulted. Optional inclusion must not be a function
of the key string.
Keys are now chosen by masked sampling under the JSON grammar. After each
property the mask permits any not-yet-emitted key, and permits `}` once
every required property has been emitted. A last-resort budget floor still
stops offering further optionals when remaining tokens are nearly exhausted
and all required keys are already present.
Visible behaviour change: for schemas with *no* required properties, `{}`
is now reachable. That is schema-valid JSON and is what the model asked for
when it samples the closing brace immediately.
ConstrainedJSONGenerator derived array element counts from the token budget
before generation:
let budgetBasedCount = backend.totalTokenBudget / arrayDefaultCountDivisor
...
let offset = rangeSize > 0 ? backend.totalTokenBudget % rangeSize : 0
count = minItems + offset
The loop then emitted exactly that many elements, with no way to stop early.
A three-item list was padded with invented elements; a ten-item list was
truncated. Same defect shape as pre-filtering optional object keys by name
hash: the length was a function of a constant budget, identical for every
document, decided before the model was consulted.
Array length is now chosen by masked sampling. After each element the mask
permits both `,` and `]`, honouring schema `minItems` / `maxItems` when
stated. Empty arrays (`minItems == 0` / unbounded) are reachable by sampling
`]` immediately after `[`. A last-resort budget floor still force-closes once
`minItems` is satisfied when remaining tokens are nearly exhausted.
Drops the `totalTokenBudget % rangeSize` arithmetic entirely — an arbitrary
count inside a valid range is still arbitrary.
buildValidDecimalTokens required every candidate token to contain a digit:
if text.allSatisfy({ $0.isNumber || $0 == "-" || $0 == "." }),
text.contains(where: { $0.isNumber })
Tokenizers that encode `.` and `-` as standalone tokens (Qwen2.5 does;
`473.00` is `4` `7` `3` `.` `0` `0`) therefore could not produce a decimal
point at all. Observed effect: the model emits the integer part, cannot
place the point, keeps emitting digits until the token cap, and the result
re-serialises as something like `4.73e+31`.
Include standalone `.` / `-` in the decimal mask (and standalone `-` in the
integer mask). Restrict accepted digits to ASCII `0`...`9` so fullwidth /
superscript forms that `Character.isNumber` admits cannot enter JSON numbers.
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.
Three defects in
ConstrainedJSONGeneratorwhere the shape of the generated JSON is decided by arbitrary constants rather than by the model. Each is wrong on any schema, independent of backend or tokenizer, and each has a test that fails without its fix.Found while trying to use constrained generation for document extraction with runtime-built schemas, on
main(f22b78e) as well as 0.8.0.1. Optional object keys are chosen by a hash of the field name
generateObjectpre-filters keys with this, so whether an optional property appears depends on its name and a constant budget — the same for every document, decided before the model is consulted. Since 31 is odd and the budget is typically even, it reduces to the parity of the field name's bytes.Measurable consequence: with a fixed budget, a given optional field is emitted for every input or for none. On a 30-document extraction benchmark, predicting that parity from the field names in advance correctly called which fields would be absent from all outputs (
currency,taxTotal,issueDate) and which would always be present (invoiceNumber,grandTotal).Keys are now chosen by masked sampling: the mask permits any not-yet-emitted key, and permits
}once every required property has been emitted.Visible behaviour change: for schemas with no required properties,
{}becomes reachable. That is schema-valid, and it is what the model asked for when it samples the closing brace — but it is a change worth knowing about, so it is called out in the commit message too.2. Array length is derived from the token budget
The element count is fixed before generation and the loop emits exactly that many, with no way to stop early — a three-item list gets padded with invented elements, a ten-item list is truncated. Same shape of defect as (1).
Length is now chosen by sampling: after each element the mask permits
,and], honouringminItems/maxItemswhen the schema states them. A budget floor may still force a close onceminItemsis satisfied.3. The decimal point is unreachable, so decimals cannot be generated
buildValidDecimalTokensrequires every candidate token to contain a digit. Tokenizers that encode.and-as standalone tokens are therefore unable to emit a decimal point at all — Qwen2.5 encodes473.00as473.00.Observed effect: the model emits the integer part, cannot place the point, keeps emitting digits until the token cap, and the result re-serialises as
4.73e+31. Every monetary value in an extraction came back like that.Standalone
./-are now included, with digits restricted to ASCII0–9so the fullwidth and superscript formsCharacter.isNumberadmits cannot enter JSON numbers.Verification
swift test: 328 tests. The 11OllamaLanguageModelTestsfailures are pre-existing and environmental (no local Ollama) —mainatf22b78efails the same 11 in the same environment, with 317 tests.StructuredGenerationTests: 25 tests pass, 11 of them new.Deliberately not included
We also carry an API change (letting a runtime
GenerationSchemareach the constrained generator — todayLanguageModelSession.respond(to:schema:)accepts one and then forwards toGeneratedContent, whose static schema is a placeholder) and a more opinionated change around all-optional objects (required keys with nullable values, the shape strict structured-output modes converged on). Those are design proposals rather than defects, and mixing them in would make this harder to review. Happy to open an issue for either if useful.One question: the
mattt/xgrammarbranch suggests structured output might move to XGrammar. It predates currentmainand does not touchShared/StructuredGeneration.swift, so these fixes apply to the code in use today — but if the hand-rolled generator is on its way out, say so and we will not push on it.