fix(types): JSON-quote date/time/datetime inside DSL containers - #1961
fix(types): JSON-quote date/time/datetime inside DSL containers#1961bharadwaj-pendyala wants to merge 5 commits into
Conversation
list[date], dict[str, date] and tuple[date, ...] generated regexes that only matched a bare 2024-01-01, which json.loads rejects. Dict keys of the same type were already quoted by _handle_dict's quote_regex path, so dict[date, str] and dict[str, date] disagreed about the same type. Quote the built-in temporal terms in container contexts too. They are matched by identity rather than by pattern, because Regex equality is pattern-based and would also capture a user's own term that happens to reuse types.date.pattern. Standalone date/time/datetime are unchanged. Fixes dottxt-ai#1960
The docstring described a general "JSON has no literal syntax for it" rule while the code only covers the temporal terms python_types_to_terms returns for a Python type. Built-in terms passed straight through, such as list[types.email], take the isinstance(ptype, Term) shortcut and stay bare. Point at dottxt-ai#1962 rather than imply they are handled.
ErenAta16
left a comment
There was a problem hiding this comment.
Checked this by running both sides rather than reading the diff. Fresh clone of main at be2cd15, then the same script against 08ba48a, comparing re.fullmatch(to_regex(python_types_to_terms(T)), sample) for a set of samples I picked so that the JSON-valid and JSON-invalid form of each is tested separately.
Before, on main:
List[date] ["2024-01-01"] match=False (valid JSON, rejected)
List[date] [2024-01-01] match=True (invalid JSON, accepted)
List[time] ["20:20:20"] match=False
List[datetime] ["2024-01-01 20:20:20"] match=False
Dict[str, date] {"k":"2024-01-01"} match=False
Dict[date, str] {"2024-01-01":"x"} match=True (already worked via quote_regex)
After, on this branch, every one of those flips the right way and Dict[date, str] stays where it was. So the asymmetry the description points at, keys quoted but values not, is real and this closes it.
The part I wanted to be sure about was the condition change from if quote_regex and isinstance(term, Regex) to if isinstance(term, Regex), since read quickly that looks like it drops the guard and would start quoting every bare Regex in list context. It does not, the guard moved into the inner if quote_regex or any(...), but it is worth saying out loud that this is what the nesting does. I pinned it anyway:
List[int] [1] match=True ["1"] match=False
List[float] [1.5] match=True
List[str] ["a"] match=True
No movement there, which is the outcome you want.
Identity matching is the right call and the comment explaining why belongs there. python_types_to_terms returns the module-level singletons directly (return types.date at line 764 and its neighbours), so term is types.date holds, and Regex.__eq__ being pattern-based really would have swept up a user term that happened to share the pattern. I also checked the recursion, since Alternatives passes the member through untouched and identity survives that:
List[Optional[date]] ["2024-01-01"] match=True
List[List[date]] [["2024-01-01"]] match=True
List[Union[date, time]] ["2024-01-01"] match=True
All three were False before. Good that test_e2e_temporal_types_quoted_in_containers pins the standalone case staying bare in the same test, because that is the boundary this change could plausibly have crossed and it did not.
One thing to correct before merge, in the new docstring paragraph:
Built-in terms passed straight through (e.g.
list[types.email]) are not covered: they reach this function via theisinstance(ptype, Term)shortcut inpython_types_to_termsand stay bare. See issue #1962.
That generalisation is not true for the three types this PR is about. Identity matching does not care which route the term took to get here, so a built-in temporal term passed straight through still gets quoted:
list[types.date] ["2024-01-01"] quoted in output: True
list[types.time] ["20:20:20"] quoted in output: True
list[types.datetime] ["2024-01-01 20:20:20"] quoted in output: True
list[types.email] ["a@b.co"] quoted in output: False
list[types.isbn] ["978-0-596-52068-7"] quoted in output: False
So the carve-out holds for email and isbn and everything else in types, and not for date, time and datetime. As written, someone reading it would conclude list[types.date] is still broken and would scope #1962 to include a case this PR already fixed. Narrowing it to something like "other built-in terms (e.g. list[types.email]) are not covered; the temporal terms are, since they are matched by identity regardless of how they reach this function" would say what the code does.
Worth adding a test for that route too, list[types.date] alongside list[datetime.date], since right now nothing pins it and it is the kind of thing a later refactor away from identity matching would silently drop.
Smaller point on test_e2e_temporal_types_quoted_in_containers: it asserts the container pattern matches ["2024-01-01"] but not that it stops matching [2024-01-01]. The positive assertion is enough to fail on old code so it is a real RED test, but the negative one is what guards against the quoting being added as an extra alternative rather than replacing the bare form. Cheap to add next to the line that is already there.
Nothing blocking from my side beyond the docstring sentence.
The paragraph added in 08ba48a said built-in terms reaching `_ensure_json_quoted` through the `isinstance(ptype, Term)` shortcut stay bare. That holds for `email`, `isbn` and the rest, but not for the three terms this branch quotes: identity matching doesn't look at how the term arrived, so `list[types.date]` is quoted just like `list[datetime.date]`. As written the note would have sent a reader to dottxt-ai#1962 for a case already fixed here. The first paragraph carried the same route-scoped phrasing and is narrowed too. The inline comment claimed `types` was read at call time "rather than at module level", which contradicts the `import outlines.types as types` at the top of the file. What can't happen at module level is the attribute lookup: `outlines/types/__init__.py` imports this module before it binds `date`/`time`/`datetime`, so hoisting the tuple to a constant would read names that don't exist yet. Reworded to say that. Add a parametrized test for the direct-term route across list, dict value and tuple. Nothing covered it before, and it's what would fail first if the identity check were swapped for pattern equality. Dict keys are left out on purpose, since `quote_regex` quotes those either way and the test would still pass with identity matching gone.
bb8b946 to
2ea6c61
Compare
ErenAta16
left a comment
There was a problem hiding this comment.
On the denylist proposal: it's the right shape, but there's a fifth term that belongs with your four rather than in the "quote it" group. isbn can't be made to work by quoting, for a different reason than the control characters.
Measured on main:
to_regex(python_types_to_terms(isbn)) vs '9783161484100' -> True
to_regex(python_types_to_terms(isbn)) vs '978-3-16-148410-0' -> True
to_regex(python_types_to_terms(List[isbn])) vs '[9783161484100]' -> False
to_regex(python_types_to_terms(List[isbn])) vs '["9783161484100"]' -> False
to_regex(python_types_to_terms(List[isbn])) vs '[]' -> False
Nothing matches list[isbn] at all — not the bare form, not the quoted form, not even the empty list. The term carries four $ anchors inside its lookaheads:
(?:ISBN(?:-1[03])?:? )?(?=[0-9X]{10}$|(?=(?:[0-9]+[- ]){3})[- 0-9X]{13}$|97[89][0-9]{10}$|...
and once that sits inside \[(...)\] the $ can't match, so the whole alternation is unsatisfiable. Adding quotes around it doesn't change that.
Worth separating from a "does it contain $" rule, because that would flag the wrong set: email also has two $ characters, but they're inside a character class rather than acting as anchors, and list[email] matches [a@b.com] fine on main. So the predictor is anchor semantics, not the character.
That makes the split five-and-eleven rather than four-and-twelve for the denylist version, and isbn needs the anchors dropped from the term itself — which changes what bare isbn accepts, so it's the same "maintainer's call first" bucket as newline/whitespace/paragraph/sentence.
Keeping this PR scoped to the reported date/time/datetime bug still seems right to me; the above is for whoever picks up #1962.
ErenAta16
left a comment
There was a problem hiding this comment.
Sanjays2402's reading is right — measured on this branch:
List[datetime.date] (Python type) bare=False quoted=True
List[types.date] (term passed in) bare=False quoted=True
List[types.email] (term passed in) bare=True quoted=False
So the identity check doesn't distinguish "reached here from a Python type" from "handed in as a term". python_types_to_terms(datetime.date) returns the same types.date object the user would pass directly, so term is types.date is True either way, and list[types.date] comes out quoted while list[types.email] — same shape, same isinstance(ptype, Term) shortcut — stays bare.
That means the note about built-in terms passed straight through staying bare doesn't hold for the three this PR names, and the split between types.date and types.email isn't one a caller can predict from anything visible.
It's not fixable by tightening the identity check, because the two paths are genuinely indistinguishable at that point — same object, no provenance. Which I think is the argument for the denylist shape you proposed on #1962: with "quote everything except the JSON scalars", types.date and types.email land in the same bucket by construction and the question stops existing.
Worth noting the current behaviour isn't wrong for list[types.date] — quoting is what it needs. The problem is only that list[types.email] doesn't get the same treatment, so this is an under-reach rather than a regression, and scoping this PR to the reported bug still holds up. The inconsistency argues for #1962 landing after it rather than for changing anything here.
The carve-out paragraph blamed the `isinstance(ptype, Term)` shortcut for `list[types.email]` staying bare, but the same paragraph then said the temporal terms take that route and are quoted anyway. The route is not the reason; the identity tuple is. Say that instead.
|
All three are in. Docstring. 2ea6c61 narrowed it but it was still wrong, it kept blaming the Direct-term test. Negative assertion.
Unsatisfiable with them, fine without. And your point about One correction, on the evidence not the conclusion. The
Checking the buckets turned up a second thing wrong in #1962, mine this time: the expected-result block says Nothing else outstanding here. |
ErenAta16
left a comment
There was a problem hiding this comment.
All three land, and the docstring is now saying the right thing rather than a narrower version of the wrong thing.
The sentence that matters is the one you rewrote: the match is on identity and carries no notion of how the term arrived, so list[types.date] and list[datetime.date] produce the same output. That is the fact I measured and could not reconcile with the earlier wording, and attributing it to the identity tuple rather than to the isinstance(ptype, Term) route is what makes #1962 scopeable, since it says exactly which built-ins are still uncovered and why.
test_e2e_temporal_term_passed_directly_quoted_in_containers, parametrized across the three terms and covering list, dict value and tuple, pins the route that had no coverage at all. And the negatives are in beside the positives:
assert not _re.fullmatch(list_pattern, f"[{value}]")
assert not _re.fullmatch(dict_pattern, f'{{"a":{value}}}')
assert not _re.fullmatch(tuple_pattern, f"({value})")which is the assertion that catches quoting being added as an extra alternative rather than replacing the bare form. A positive-only test passes either way.
Keeping the identity-versus-equality comment next to the check is worth it for the same reason as before: Regex.__eq__ is pattern-based, so a later "simplification" to == would silently start catching a user's own term that happens to share a pattern, and nothing in the suite would fail.
Nothing further from me.
|
Review here has closed out, so this is ready when you have a slot. Sanjays2402 and ErenAta16 both went through it by running the branch against Scope is unchanged: Branch is 5 ahead of |
Fixes #1960.
What broke
date,timeanddatetimeinside a DSL container came out unquoted, so the generated regex only matched a string JSON can't parse.Dict made the inconsistency obvious.
_handle_dictpassesquote_regex=Truefor keys, sodict[date, str]produced{"2024-01-01":"x"}, whiledict[str, date]produced{"d":2024-01-01}. Same type, opposite treatment, one object. The key half landed in #1917 and the value half was missed.The fix
_ensure_json_quoted, alongside theStringliterals that already get quoted there.Regexis a dataclass so equality compares pattern text, which would also grab a user's ownRegex(types.date.pattern). There's a test pinning that.date/time/datetimeare untouched, same as the deliberate Python-styletypes.boolean.Identity carries no provenance, so
list[types.date](handed in as a term) andlist[datetime.date](from the annotation) produce the same quoted output. That is deliberate and pinned by a test.Out of scope, and unchanged by this PR:
Regexterms.list[types.email]still generates[a@b.com]. That is the wider gap in Built-in Regex terms are not JSON-quoted inside containers (list[types.email] generates [a@b.com]) #1962, which needs a maintainer call on the shape before it can be written, so it isn't folded in here.Literal[datetime.date(2024, 1, 1)]and enums with date values still raiseTypeErrorfrompython_types_to_terms. That's the separate unsupported-instance path, and it fails loudly rather than generating bad output.Test plan
pytest tests/typespasses (296 tests)mainand pass here; the tenth is the user-Regexguard, which passes on both by designlist/dict/tuplecovered for all three temporal types, parametrizedlist[types.date]) covered separately from the annotation route (list[datetime.date]), since a move away from identity matching would drop the first while the second kept passingOptional[date]in a container quotes the date, leaves bareNonealoneRegexsharingtypes.date.patternstill renders baredate/time/datetimestill match unquotedpre-commit run --files src/outlines/types/dsl.py tests/types/test_dsl.pyclean (mypy, ruff)Note
tests/test_generator.py::test_steerable_generator_init_valid_processorerrors on a clean checkout ofmaintoo (aDeprecationWarningfromtokenizersBPE), unrelated to this change.