fix(types): JSON-quote string-shaped built-in terms inside containers - #2015
bharadwaj-pendyala wants to merge 1 commit into
Conversation
|
The empty checks on this PR are the first-time-contributor workflow-approval gate, not a problem with the branch. Worth saying here so nobody reads the blank list as a red CI. Both workflows on head Since CI cannot speak for the branch yet, here is what it does locally. On The generator accepts the string JSON rejects and rejects the string JSON accepts. On this branch, both flip: One scope note, since #1960 has come up as a candidate for a combined Happy to rebase whenever, though per the above that will not start CI on its own. |
|
Correcting an omission in the description above, since it matters for how this gets reviewed. @feiiiiii5 opened #1975 on 1 August with the same title and the same goal, sixteen days before this PR. I reviewed that branch on 10 August, found three terms falling through its allowlist, and feiiiiii5 fixed all three the next day in The "allowlist fails open" argument in my description above is weaker than I wrote it. That closes the hole I said an allowlist could not close, at least for terms exported from Where the two branches actually differ. Same probe on The first five are coverage this branch has and #1975 does not. The sixth row is the cost of getting the other five, and it goes the other way. Matching on the pattern string means a What I would suggest. #1975 has been running CI since 1 August and this branch has never run any, since both workflows on Either outcome is fine by me. I would rather that be decided in the open than have two branches sitting here doing the same job. |
list[types.email] generated [a@b.com], which does not parse as JSON. _ensure_json_quoted only quoted a bare Regex for Dict keys, so every built-in term reaching a list item, tuple item or dict value came back unquoted. Quote every built-in outlines.types term except the JSON scalars (integer, number, boolean, digit, latitude, longitude) and string, which carries its own quotes. Terms are matched on their pattern, so a copy or a rebuilt term is treated the same as the module-level one, and the outlines.types.locale.us terms are covered as well.
6beef9d to
d3120fb
Compare
ErenAta16
left a comment
There was a problem hiding this comment.
Your 24 August comment gave away more than it had to. The retraction is wrong, and
since it is the argument a maintainer would weigh when choosing between the two
branches, it is worth putting straight.
You credited test_every_builtin_regex_term_is_classified with closing the hole.
It does not, because of what it asserts:
assert not builtins - (set(BARE_TERMS) | set(QUOTED_TERMS) | {"string"})That is a membership check. It proves no built-in is left unclassified. It says
nothing about whether a term landed in the right tuple, so a string-shaped term
placed in BARE_TERMS passes it exactly as cleanly as a correct one. No
completeness test over dir(types) can catch a wrong classification, only a
missing one.
That distinction is not academic here. I called _ensure_json_quoted on every
built-in Regex across main, #1975 and this branch:
main #1975 #2015
integer number boolean digit lat long bare bare bare
whitespace newline sentence paragraph bare bare quoted
date time datetime bare bare quoted
email uuid4 ipv4 isbn semver slug hex e164 bare quoted quoted
Seven terms are classified bare on #1975 while passing its own completeness test.
Four of them are the control-character group, which that branch leaves bare
deliberately and documents in
test_e2e_control_char_builtin_terms_left_bare_in_containers, so that part is a
judgement call rather than a miss, and I have said so on #1975. The temporal row
is not:
bare quoted
2024-01-01 JSONDecodeError ['2024-01-01']
12:30:00 JSONDecodeError ['12:30:00']
So your original framing was right, just for a different reason than the one you
gave. An allowlist does not fail by leaving a term out, that test does close that
door. It fails by putting one on the wrong side, and nothing on that branch checks
for it.
This also changes the shape of the offer at the end of your comment. Handing
#1975 the locale submodule and the reconstruction cases is a few lines, as you
say, but whoever takes that route also has to move date, time and datetime
across, and that is #1960 rather than a tidy-up. Worth naming, because "a few
lines of follow-up" and "and it closes a second open issue" are different asks.
Your reachability rows and this classification row are independent, incidentally.
Yours is about which objects get recognised as a built-in, mine is about what
happens once one is. A branch can be right on either and wrong on the other, so a
maintainer picking between these two is choosing on both axes, not one.
On the omission you corrected: noted, and I would not have raised it. Saying it
yourself before anyone found it is the reason this thread is easy to read.
|
You're right, I retracted too much. The test closes the gap I claimed it left, and |
|
@RobinPicard CI has never run here: both check suites on d3120fb are at #1975 was closed in favor of this one on Sep 2, so it's the only open branch covering the date/time/datetime family. It closes #1960 alongside #1962 since the denylist quotes all three. |
ErenAta16
left a comment
There was a problem hiding this comment.
Confirming the CI reading independently, since it is the kind of claim a
maintainer should not have to take on trust before spending a click on it.
On d3120fb:
check-suites: 2
GitHub Actions status=completed conclusion=action_required latest_check_runs_count=0
GitHub Actions status=completed conclusion=action_required latest_check_runs_count=0
check-runs total: 0
action_required with zero runs is the first-time-contributor workflow gate, not
a failure and not something a push can clear. Nothing has executed on this branch,
so there is no green or red to read yet, and no amount of rebasing changes that.
Two other things worth having in one place, both checked rather than recalled.
#1975 was closed unmerged on 2 September, so this is now the only open branch
covering the string-shaped terms. The row is no longer a choice between two.
And the coverage is what the description claims. I called _ensure_json_quoted
on every built-in Regex in outlines.types at this head:
quoted (15) whitespace newline sentence paragraph date time datetime
email uuid4 ipv4 isbn semver slug hex_color e164
bare (6) integer number boolean digit latitude longitude
The six left bare are the number-shaped ones, which is correct, and the temporal
row is quoted, which is what #1960 asks for. #1960 and #1962 are both still open,
so approving the workflow here is the step that lets one branch close two issues.
@RobinPicard the only thing needed to move this is the workflow approval. Until
that happens neither the author nor a reviewer can tell you anything about this
branch that was not read by hand.
Closes #1962. Supersedes #1961, which fixed
date/time/datetimeonly.Problem
list[types.email]generates[a@b.com]. That is not JSON._ensure_json_quotedquotes a bareRegexonly when called withquote_regex=True, which happens forDictkeys and nowhere else (src/outlines/types/dsl.py:884before this change). So every built-in term reaching a list item, a tuple item or a dict value came back unquoted. Same fordict[str, types.email], which generates{"k":a@b.com}.The quiet case is worse than the loud one.
json.loadsreads a lone control character between brackets as whitespace, solist[types.newline]generates text a parser accepts as an empty array:The caller asked for one element, the generation produced one, the parse yields zero, and nothing raises.
Solution
Quote every built-in
outlines.typesterm except the ones JSON reads as scalars:integer,number,boolean,digit,latitude,longitude, plusstring, whose pattern carries the quotes already.Listing the exceptions rather than the terms to quote is a deliberate choice, and I would rather state it than leave it open. An allowlist has to be extended every time a term is added and fails open when someone forgets, which is how the missing quotes arrived. A denylist fails the other way: a new term gets quoted like the rest, and if it turns out to need more than quoting that is a visible bug instead of silent invalid JSON. @ErenAta16 argued the same case on the issue and did the work of separating the escaping treatments; @Sanjays2402 spotted the gap during review of #1961.
Terms are matched on their pattern, not by identity. Identity looks tighter but breaks under
pickle,copy.deepcopyand any reconstruction, which would silently restore the old output. Matching the pattern also means aRegex(r"\w")the caller wrote is treated astypes.charand quoted, since nothing distinguishes them;test_regex_colliding_with_a_builtin_is_quotedpins that. Any other user-suppliedRegexkeeps its current bare behaviour, because nothing records whether it stands for a JSON string or a number.The
outlines.types.locale.usterms (zip_code,phone_number) are included. They have the same bug and live in a submodule.24 built-in terms are now quoted. 17 are fully correct. The rest need escaping on top of quoting and are out of scope here:
newline,whitespacesentence,paragraph.*admits the backslash and a bare quote, so they wantjson.dumps, not a hand-written escape listdatetime\s, so a tab passesemail"For
newlineandwhitespacequoting still helps, because it converts the silent bad parse into a raised error:isbnis quoted with the rest and nothing observable changes: its four$anchors sit inside lookaheads, so once the term is embedded in\[(...)\]the alternation is unsatisfiable andlist[types.isbn]matches nothing either way. Dropping the anchors changes what bareisbnaccepts, so it needs its own call.Scope
Left out on purpose:
isbnanchors. Each needs a different treatment andsentence/paragraphwantjson.dumpsrather than the mechanism used here.latitudeandlongitudeemit a leading+that JSON rejects, andbooleanemitsTruerather thantrue. Those are separate bugs from the quoting one and the denylist keeps all three bare either way._ensure_json_quotedstill does not recurse intoSequence,Optionalor the quantifier nodes, solist[optional(types.email)]stays unquoted. That predates this change andtest_ensure_json_quoted_sequence_passthroughfreezes it.Validation
Base commit 7d06847.
pytest tests/types: 389 passed, up from 346 on 7d06847. 43 tests added; 34 of them fail against the 7d06847 source and pass here, and the 9 that pass on both pin the scalar and user-supplied-Regexbehaviour that must not change.ruff check --config=pyproject.toml src/outlines/types/dsl.py tests/types/test_dsl.py(0.9.1, the pinned pre-commit version): all checks passed.mypy --allow-redefinition src/outlines/types/dsl.py(1.14.1): no issues found.test_container_of_builtin_generates_parseable_jsonruns the generated regex against a real sample for 13 terms and assertsjson.loadsaccepts the quoted form and the regex rejects the bare one, so the tests check the output rather than restating the wrapper.pickle.loads(pickle.dumps(list[types.email]))andcopy.deepcopy(types.email)both quote correctly here and did not under an identity check.The rest of the suite needs
transformers, which I could not install locally;tests/test_exceptions.pyandtests/test_cache.pyreport 2 failures and 1 error on 7d06847 unchanged, from a missing async pytest plugin rather than this change.AI disclosure
This change was AI-assisted, per CONTRIBUTING.md. I reviewed every line, ran every command quoted above myself, and can explain the whole diff.