-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add oneOf
+const
JSON Schema Option for Literals
#9029
Changes from 3 commits
f8b8d26
9ab3272
22f0e3d
f9242a3
3bf1614
e02fb06
f3f0a24
7251376
7bdf02f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -732,10 +732,26 @@ def literal_schema(self, schema: core_schema.LiteralSchema) -> JsonSchemaValue: | |
# jsonify the expected values | ||
expected = [to_jsonable_python(v) for v in expected] | ||
|
||
result: dict[str, Any] = {'enum': expected} | ||
result: dict[str, Any] = {} | ||
if len(expected) == 1: | ||
result['const'] = expected[0] | ||
|
||
if self._config.json_schema_literal_type == 'enum': | ||
result['enum'] = expected | ||
elif self._config.json_schema_literal_type == 'oneof-const': | ||
# TODO (reesehyde): do we want this condition or not? why do we still produce 'enum' for single values? | ||
if len(expected) > 1: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems intentional that the {
"const": "SingleValue",
"oneOf": [{"const": "SingleValue"}]
} over just the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per this comment
|
||
descriptions = schema.get('metadata', {}).get('enum_case_descriptions', {}) | ||
members = [] | ||
for e in expected: | ||
member = {'const': e} | ||
if e in descriptions: | ||
member['description'] = descriptions[e] | ||
members.append(member) | ||
result['oneOf'] = members | ||
else: | ||
raise ValueError(f"Unknown literal type '{self._config.json_schema_literal_type}'") | ||
|
||
types = {type(e) for e in expected} | ||
if types == {str}: | ||
result['type'] = 'string' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd been following a great suggestion to look to the newly added opt-in attribute docstring support for inspiration here.
But I'm realizing that I followed that paradigm a bit too closely by putting the config option here, I think the best place for it is in a
BaseModel.model_json_schema()
parameter. Will refactor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmehyde,
Ah yeah, that makes sense re moving to
model_json_schema
. That being said, you should still be able to keep most / all of the docs / tests that you've written, which is nice. Ping me when you've refactored, and I'd be more than happy to review! Looks like great work so far :).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sydney-runkle, I've made that update!
There's one check failing: a segfault when running the Ubuntu 3.10 tests. I don't believe this is directly related to my changes, and I don't get that behavior running the command in a 3.10 environment on my own Ubuntu machine. But I did re-run the CI/CD and the behavior is consistent, let me know what the right next step there is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, that test is annoyingly flaky. I just reran it again, hopefully it passes this time 🍀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a first glance, looks great. I'll get back to you with a final confirmation tomorrow regarding if this is the best place to have this parameter. I think it is, but I suppose I could see an argument for just creating a custom instance of
GenerateJsonSchema
with this logic, and passing that to themodel_json_schema()
function.Could you re-add that comprehensive docs section that you added? Maybe to the JSON schema docs (instead of where you originally had it, in the config docs)