fix(types): reject non-string values for DSL terms - #1944
Conversation
|
📚 Documentation preview: https://dottxt-ai.github.io/outlines/pr-preview/pr-1944/ Preview updates automatically with each commit. |
| pattern = to_regex(self) | ||
| compiled = re.compile(pattern) | ||
| if not compiled.fullmatch(str(value)): | ||
| if not is_str_instance(value) or not compiled.fullmatch(value): |
There was a problem hiding this comment.
matches() still does fullmatch(str(value)) a few lines below, so Regex("[0-9]+").matches(123) remains true while validate(123) now rejects it. Should these agree on rejecting non-strings?
ErenAta16
left a comment
There was a problem hiding this comment.
The defect is real and the reasoning holds. Ran it on a clone of main at be2cd15 against 4a05a12.
The declared schema is unambiguous about what the field is:
Regex("[0-9]+") as a pydantic field
-> {"pattern": "([0-9]+)", "title": "Field", "type": "string"}
and on main the model happily contradicts it:
Model(field="123") -> accepted, type=str, value='123'
Model(field=123) -> accepted, type=int, value=123
So a model that advertises "type": "string" hands back an int. On this branch the second case raises, non-matching strings are still rejected as before, and a str subclass still passes, which is the case a naive type(value) is str check would have broken:
Model(field=MyStr("123")) -> accepted, type=MyStr
Model(field="abc") -> ValidationError
is_str_instance being the guard rather than an exact type check is the right call there.
The thing I would want settled before merge is the blast radius, which is wider than the description implies. The same validate serves the built-in terms, so those stop accepting their natural Python values:
main this PR
types.integer 123 accepted (stays int) ValidationError
types.number 1.5 accepted (stays float) ValidationError
types.boolean True accepted (stays bool) ValidationError
types.integer "123" accepted accepted
Whether that is correct depends on what types.integer is meant to be. As a regex term whose schema is {"type": "string", "pattern": ...} it is self-consistent to take strings only. As a name that reads like an integer field, Model(field=123) raising is going to surprise people. I checked the docs and there is no user-facing example annotating a pydantic field with types.integer, it only shows up in architecture.md describing the internal mapping, so this is not breaking a documented pattern. It is still a public name in outlines.types though, and this is a behaviour change rather than a pure bug fix for anyone who found it.
Worth noting the original complaint was about the returned value, not about acceptance:
This allowed matching non-string values to pass validation while remaining non-strings in the resulting model.
Rejecting is one way to resolve that. Coercing is the other: keep the str(value) match and return the string when it matches, so the field ends up a str consistent with the declared schema and Model(field=123) yields "123" instead of raising. That fixes the stated defect, the model no longer contains a non-string, with a much smaller compatibility surface. The cost is a silent 123 -> "123" conversion, which is its own kind of surprise.
I lean toward the strict version you have written, because silent coercion in a validator is harder to debug than a rejection and the schema really does say string. But it is a deliberate choice rather than an obvious one, and the PR description currently presents it as only a bug fix. Saying explicitly that built-in numeric and boolean terms no longer accept their Python values, and getting a maintainer to confirm that is intended, would be the thing that unblocks this.
One factual point in favour of merging either way: this touches validation only, not generation. I confirmed the generated regex for these terms is unchanged, so nothing about constrained output moves. The change is scoped to what happens when you hand an already-produced value to a pydantic model.
Test looks right for the strict reading. If you keep it, a case pinning that a str subclass is still accepted would be worth adding, since that is the part of the guard most likely to be tightened by accident later.
|
Thanks — fair read, and you're right the blast radius belongs in the description. Reproduced your table independently at cb095ba. Two things keep me on the strict version rather than coercion: The bad value doesn't stay contained — the model serializes to JSON its own schema rejects. M(field=123).model_dump_json() gives {"field":123}, and jsonschema says 123 is not of type 'string'. That schema is what gets handed to providers. And it isn't only ints. str(value) admits anything with a cooperative str: class Sneaky: M2(field=Sneaky()) # main: accepted, type=Sneaky Also worth noting the terms already don't take natural Python values: types.string is Regex(r'"[^"]*"'), so Model(field="abc") raises on main today. Added the str subclass test and rewrote the description with the behaviour-change table. |
ErenAta16
left a comment
There was a problem hiding this comment.
Both points check out on my side, and they settle it. Dropping the coercion suggestion.
The containment argument is the one I had not followed through:
M(field=123) accepted on main, field stays int
M(field=123).model_dump_json() {"field":123}
jsonschema.validate(that, M.model_json_schema())
ValidationError: 123 is not of type 'string'
A model that serialises to something its own generated schema rejects is worse than I was treating it. I was thinking about the value sitting in an attribute; the schema is what goes to the provider, so the bad value does not stay local.
The Sneaky case is the part that actually kills coercion:
class Sneaky:
def __str__(self): return "123"
M(field=Sneaky()) # main: accepted, type = Sneakystr(value) is not a narrow int-to-str convenience, it accepts anything with a cooperative __str__. Coercing 123 to "123" is defensible in isolation; coercing an arbitrary object to "123" and storing that is not, and it would convert a case that today survives visibly into one that silently looks correct. So coercion does not just fail to fix the problem, it removes the evidence.
That leaves the compatibility surface as the only open question, and you have already agreed to put it in the description. With the reasoning above in there too, the built-in numeric and boolean terms no longer accepting their Python values reads as the intended consequence of a schema-conformance fix rather than as collateral damage.
Reproducing the table independently at cb095ba7 rather than taking mine is the right instinct, and it is why the Sneaky case surfaced at all. Nothing further from me.
What
DSL
Termobjects can be used directly as Pydantic field types. They generate a JSON Schema field of{"type": "string", "pattern": ...}, butTerm.validate()converted every input withstr(value)before applying the regex and then returned the original input unchanged.Matching non-string values therefore passed validation and stayed non-strings in the resulting model:
The consequence is not confined to the model instance. The field's declared schema says
string, so the model serializes to JSON that its own schema rejects:That same schema is what is handed to providers for structured output.
It also is not limited to numeric input. Because the guard was
str(value), any object with a cooperative__str__passed and was stored unconverted:Fix
Require values passed to
Term.validate()to be strings before applying the compiled regular expression, using the existingis_str_instancehelper so thatstrsubclasses remain valid.This aligns the runtime with the signature the method has carried since the DSL was introduced in
da1b0db5(def validate(self, value: str) -> str); thestr()call was added in that same commit and reads as defensive rather than as an intended coercion feature.Behaviour change
This is a behaviour change, not only a bug fix.
Term.validate()also backs the built-in terms inoutlines.types, which areRegexinstances. Terms whose pattern happens to match thestr()form of their Python counterpart previously accepted that value and now reject it:types.integer123intValidationErrortypes.number1.5floatValidationErrortypes.booleanTrueboolValidationErrortypes.integer"123"Anyone annotating a Pydantic field with
types.integer,types.number, ortypes.booleanand passing the corresponding Python value will see aValidationErrorwhere the value was previously accepted.Two notes on how large that surface actually is:
types.stringisRegex(r'"[^"]*"'), soModel(field="abc")raises onmaintoday and only'"abc"'passes.types.integeralready rejected123.0while accepting123. The leniency was incidental tostr(), not designed.docs/guide/architecture.md, describing the internal Python-type-to-term mapping.The alternative resolution is coercion: keep the
str(value)match and return the string, soPayload(value=123)yields"123". That fixes the stated defect with a smaller compatibility surface. I did not take it because the accepted inputs are not only numbers — coercing would silently replace an unrelated object with"123"in theSneakycase above, where the current behaviour at least leaves the object intact to be noticed. Happy to switch if maintainers prefer the coercing variant.Scope
Term.validate()is reachable only through__get_pydantic_core_schema__and__get_validator__. Nothing else insrc/calls it and no test calls it directly.Term.matches(),to_regex(), regex generation, and the model-provider integrations are unchanged — this affects only what happens when an already-produced value is handed to a Pydantic model, not constrained generation.Tests
Added a regression test verifying that:
ValidationErrorstrsubclass is still accepted, pinning that the guard is anisinstancecheck rather than an exact type checkValidation performed:
tests/types/test_dsl.py: 60 passedtests/types: 261 passedgit diff --checkpassedThe full provider and model integration suite was not run locally because it requires heavyweight and environment-specific dependencies.