fix(types): JSON-quote string-shaped built-in terms inside containers - #1975
fix(types): JSON-quote string-shaped built-in terms inside containers#1975feiiiiii5 wants to merge 4 commits into
Conversation
|
📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1975/ Preview updates automatically with each commit. |
Built-in Regex terms handed in directly (list[types.email], dict[str, types.uuid4], ...) take the isinstance(ptype, Term) shortcut in python_types_to_terms and come back bare from _ensure_json_quoted, so containers generated [a@b.com] instead of ["a@b.com"] - output json.loads rejects (dottxt-ai#1962). Invert the rule in _ensure_json_quoted: identity-match the built-in JSON scalars (integer, number, boolean, digit) and the terms that need escaping before they can be quoted (newline, whitespace, paragraph, sentence - follow-up work), keep temporals scoped to dottxt-ai#1961, and quote every other string-shaped built-in term (uuid4, ipv4, ipv6, mac_address, semver, slug, hex_color, hex_str, credit_card, char). User-supplied Regex terms are matched by identity only, so a user regex that happens to share a built-in pattern is left alone. Closes dottxt-ai#1962 Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
|
Small mechanical note rather than a review of the logic — this and #1974 overlap in a way that will bite whichever lands second. Both branches carry the same if isinstance(term, NoneLiteral):
json_null = Regex("null")
if quote_regex:
return Sequence([String('"'), json_null, String('"')])
return json_nulland both are open against So this branch started from #1974 and has since drifted — #1974 has one commit this one doesn't. Practical consequences:
Either marking this one as depending on #1974 and rebasing it on top, or pulling the Worth flagging that |
fa6cb81 to
d344eb8
Compare
|
Thanks for the note — you're right that the stacking would bite whoever lands second, so I've un-stacked this PR: the branch is now based on |
ErenAta16
left a comment
There was a problem hiding this comment.
Thanks for un-stacking it — confirmed on d344eb8: one commit, no NoneLiteral block, +115/-0. The two PRs now carry disjoint changes.
Three things from reading it on its own.
They still collide, but only in the tests
I merged the two branches locally to check whether "any order" holds in practice:
$ git merge p1975 (onto p1974)
Auto-merging src/outlines/types/dsl.py
Auto-merging tests/types/test_dsl.py
CONFLICT (content): Merge conflict in tests/types/test_dsl.py
dsl.py auto-merges cleanly now, which is the part that mattered. What's left is that both PRs append their new tests at the same point in test_dsl.py, so whichever lands second still needs a trivial rebase. Not worth restructuring for — just flagging so it isn't a surprise.
Coverage is complete
I checked the two name tuples against every module-level Regex singleton in types/__init__.py rather than trusting the count in #1962:
Regex singletons in types/__init__.py : 24
covered by the two tuples : 23
not covered : ['string']
named in the PR but not in types : []
string is the one omission and it's already handled — the term.pattern != types.string.pattern guard sits above both _is_builtin_term calls, so it never reaches them. Nothing falls through to the bare path.
Using identity (term is getattr(types, name, None)) rather than pattern equality is also the right call, and the docstring saying so is worth keeping: a user's own Regex(r"[a-z0-9]+(?:-[a-z0-9]+)*") should not silently start behaving like types.slug.
The email exclusion looks wrong, and it's the case the issue was opened about
The test docstring says:
types.email/types.isbnare excluded from the parameter set because their patterns contain^/$anchors that cannot be embedded in a container regex at all
That holds for isbn, which really does carry $ inside its lookaheads. It doesn't hold for email. Stripping character classes before looking for anchors:
email ^=False $=False
isbn ^=False $=True
Every ^ and $ in the email pattern lives inside a character class — [a-z0-9!#$%&'*+/=?^_{|}~-]` — where they're literal characters, not anchors.
And on your branch it works:
list[types.email] quoted='["a@b.com"]' -> True bare='[a@b.com]' -> False
list[types.uuid4] quoted -> True bare -> False
list[types.isbn] quoted -> False bare -> False
email behaves exactly like the types that are in the parameter set. isbn is genuinely broken and the exclusion is right.
This matters more than a missing test row, because list[types.email] producing [a@b.com] is the example in the title of #1962. As it stands the PR fixes the motivating case and then leaves it out of its own regression coverage, so a future change could break it without failing anything. I'd move (types.email, "a@b.com") into the parametrize list and narrow the docstring to isbn alone.
…oting types.email carries no top-level ^/$ anchors (its ^/$ live inside character classes), so it is quoteable like uuid4/slug/etc. Add it to the parametrized set and narrow the docstring exclusion note to types.isbn, whose lookaheads really do contain top-level $. Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
|
Addressed in
Local run: |
ErenAta16
left a comment
There was a problem hiding this comment.
Anchors and parametrize set both look right now.
(types.email, "a@b.com") being in the set means the motivating case from #1962 is pinned directly rather than by analogy, and excluding types.isbn with the reason written down beats silently leaving it out.
The test I want to single out is this one:
def test_e2e_user_regex_term_still_bare_in_containers():
user_regex = Regex(r"[a-z]+@[a-z]+\.[a-z]+")
list_pattern = to_regex(python_types_to_terms(list[user_regex]))
assert _re.fullmatch(list_pattern, "[a@b.com]")That is the safety net for the whole approach. Since Regex.__eq__ is pattern-based, matching built-ins by equality rather than identity would catch a user's own term that happens to share a pattern and start quoting their output. This test fails the moment someone makes that simplification, and it is the failure that would otherwise be reported months later as "outlines corrupts my custom regex". Worth a comment above it saying that is what it is for, because it reads like a redundant negative case.
Same for test_e2e_control_char_builtin_terms_left_bare_in_containers: excluding newline/whitespace/paragraph/sentence rather than quoting them is the right call, and the docstring gives the reason (quoting would look fixed while still failing json.loads), which is the sort of thing that gets "tidied up" without it.
Nothing further from me.
|
I opened #1962, so I ran this branch rather than read it. Checked against current The branch is based on Copying only this branch's
Your test file passes unmodified on The thing that worries me more than the three names is that the fall-through case is "leave it bare" and it is silent, so every future type PR can quietly reopen #1962. Two did inside ten days. Lifting the two tuples to module level and adding a guard would fail the next one at test time instead: def test_every_builtin_regex_term_is_classified():
builtins = {
name
for name in dir(types)
if not name.startswith("_") and isinstance(getattr(types, name), Regex)
}
assert not builtins - (BARE_TERMS | QUOTED_TERMS | {"string"})Happy to push that as a follow-up on top of yours if you would rather keep this PR to the shape it already has. |
…tin-terms-in-containers
…s bare bharadwaj-pendyala found that three built-in terms added after this branch's base (iban, bic, e164 on main) fall through the container quoting allowlist and emit bare forms that fail json.loads, silently reopening dottxt-ai#1962. latitude/longitude are number-shaped, so bare is correct, but they reached it by accident rather than classification. Lift the allowlist tuples to module-level BARE_TERMS/QUOTED_TERMS, classify the five new terms explicitly, and add test_every_builtin_regex_term_is_classified so a future type PR fails at test time instead of quietly reopening dottxt-ai#1962. tests/types -> 368 passed; ruff 0.9.1 clean. Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
|
Thanks for running the branch against current main — the three fall-throughs are fixed in What changed:
Verified: You offered to push the guard as a follow-up — no need, it's in here. Happy to split it out if you'd rather review it separately. |
Root Cause
Built-in
Regexterms fromoutlines.typeshanded in directly to a container annotation (e.g.list[types.email],dict[str, types.uuid4]) take theisinstance(ptype, Term)shortcut inpython_types_to_termsand come back bare from_ensure_json_quoted. The generated regex matches[a@b.com]and rejects["a@b.com"]— output thatjson.loadscannot parse (issue #1962).Dictkeys already work (quote_regex=True); the gap is list items and dict values.Fix
Invert the quoting rule in
_ensure_json_quoted(per the issue's suggested design): identity-match the built-in terms and quote every string-shaped one, while leaving the special groups untouched:integer,number,boolean,digit.newline,whitespace,paragraph,sentence— quoting them would look fixed while still failingjson.loads(raw control chars / unescaped quotes); escaping them is follow-up work documented in the code.date,time,datetime).uuid4,ipv4,ipv6,mac_address,semver,slug,hex_color,hex_str,credit_card,char.Identity matching (not pattern equality) means a user-supplied
Regexthat happens to share a built-in pattern is left alone.Test
test_e2e_string_shaped_builtin_terms_quoted_in_containers— parametrized over the 10 string-shaped terms: list items and dict values now match the quoted spelling and reject the bare spelling; dict keys keep working. (types.email/types.isbnare excluded because their patterns contain^/$anchors that cannot be embedded in a container regex at all — a pre-existing limitation independent of quoting.)test_e2e_json_scalar_builtin_terms_stay_bare_in_containers—integer/number/digitstay bare.test_e2e_control_char_builtin_terms_left_bare_in_containers— the four control-char terms are unchanged.test_e2e_user_regex_term_still_bare_in_containers— a userRegexsharing a built-in pattern is untouched.tests/typessuite: 301 passed.Diff scope
2 files, +115/-0:
src/outlines/types/dsl.py,tests/types/test_dsl.py.AI Disclosure
AI-assisted root-cause analysis, initial draft, and test scaffolding; human review of the identity-matching design and the scalar/control-char carve-outs.
Not PR-related failures
None; the full types suite passes locally. (This PR deliberately avoids the temporal terms to stay disjoint from #1961.)