fix: preserve temperature=0.0 and max_tokens=0 in subgrammar() factory#1459
fix: preserve temperature=0.0 and max_tokens=0 in subgrammar() factory#1459alvinttang wants to merge 1 commit into
Conversation
The factory function in guidance._grammar.subgrammar used a truthiness
check ("if temperature:") to decide whether to apply a temperature to
the generated RuleNode. This silently dropped the canonical greedy
setting temperature=0.0, meaning that
subgrammar(body, temperature=0.0)
produced a grammar with no temperature constraint at all — so sampling
was not forced to deterministic on the server side. The same truthiness
check was applied to max_tokens, which would have discarded max_tokens=0
as well (less impactful, but equally surprising).
Switch both checks to "is not None", matching the convention used by
guidance.json() and guidance._grammar.gen() throughout the codebase.
Regression tests walk the resulting grammar tree and confirm that
temperature=0.0 is reachable on a RuleNode.temperature field, and that
non-zero and None cases behave as expected.
ae79052 to
f49660d
Compare
riedgar-ms
left a comment
There was a problem hiding this comment.
I'm OK with this. @hudson-ai @nking-1 ?
ErenAta16
left a comment
There was a problem hiding this comment.
Confirmed this is the same falsy-truthiness class as the metrics trimming bug in #1474. Checked token_limit() and with_temperature() in _grammar.py: both take the value as-is via dataclasses.replace(...max_tokens=max_tokens) / temperature=temperature, no special casing for 0. So subgrammar()'s old if max_tokens: / if temperature: guards silently dropped exactly the two values (max_tokens=0, temperature=0.0) that callers most plausibly pass on purpose, greedy decoding and a zero-token stop condition are both legitimate.
The new tests walk the actual grammar tree rather than asserting on subgrammar()'s return type directly, which is the right level to test at since RuleNode wrapping can nest. test_temperature_none_sets_nothing and test_max_tokens_none_sets_nothing are good inclusions too, they confirm the None branch still short-circuits instead of accidentally wrapping with a sentinel.
ErenAta16
left a comment
There was a problem hiding this comment.
Reproduced the drop independently before looking at the fix: walking the RuleNode tree built by subgrammar(lexeme(r"\d+"), temperature=0.0) on main gives an empty temperature set, confirming 0.0 never reaches the node. Same for max_tokens=0. The is not None swap matches how guidance.json/gen already handle these two kwargs, so this also removes the one inconsistent path rather than adding a new convention.
Ran the new test file after checking out the branch — 14 passed, including the nonzero and None cases so the guard cannot silently regress to is not None catching too much either.
Summary
guidance._grammar.subgrammar()usedif temperature:/if max_tokens:to decide whether to attach those kwargs to the builtRuleNode. Because0.0and0are falsy in Python, callers passingtemperature=0.0(the canonical way to ask for deterministic / greedy sampling) ormax_tokens=0had their values silently dropped — the resulting grammar had noRuleNode.temperatureorRuleNode.max_tokensset.Every other factory in the codebase (
guidance.json,guidance._grammar.gen) already usesis not Nonefor the same kwargs, sosubgrammar()was the only inconsistent path.Root cause
Python treats
0and0.0as falsy, so the guards silently drop the valid-but-falsy values before they reach theRuleNode.Fix
Two-line change in
guidance/_grammar.py: swap both guards foris not None, matching the rest of the codebase:Tests
tests/unit/library/test_subgrammar.pyadds two parallel test classes:TestSubgrammarFactoryTemperature: verifiestemperature=0.0propagates (RED onmain),temperature=0.5still works,temperature=Nonesets nothing.TestSubgrammarFactoryMaxTokens: same three cases formax_tokens=0/max_tokens=16/max_tokens=None.A helper walks the resulting grammar tree via
GrammarNode.children()and collects everyRuleNode.temperature(ormax_tokens) that is notNone.Locally on
darwin/arm64:pytest tests/unit/library/test_subgrammar.py→ 14 passed.ruff checkclean.Risk notes
temperature=0.0ormax_tokens=0expecting them to be ignored will now see them forwarded tollguidance.llguidanceacceptsmax_tokens=0cleanly (verified locally viaLLMatchersmoke test) —max_tokens=0is also already the honoured value ingen()/json()/lark()per_ast.pyserialization, so this just bringssubgrammar()in line.None(the documented default) still means "unset" and sets no limit. That invariant is covered by the_none_sets_nothingtests.Related to the long-standing observation in #600 that temperature can be silently dropped on certain call paths; this PR addresses the
subgrammar()path specifically.