Pydantic package organization episode 3: "System update complete" - #403
Conversation
aa22c1c to
9a391b9
Compare
GitHub Actions
Inactive
… @require_if - NEED TESTS THOUGH build is passing but there's a unit test deficit growing around all the new classes
…eficit continues to incerase though
…f, but likely mypy issues still to fix
…Schema constraints working?
… - patternProperties in root
9a391b9 to
498d3f7
Compare
GitHub Actions
Inactive
| "patternProperties": { | ||
| "^ext_.*$": { | ||
| "description": "Additional top-level properties must be prefixed with `ext_`." | ||
| } | ||
| }, |
There was a problem hiding this comment.
This is one of the subtle 🐞 bugs addressed: patternProperties shouldn't exist at the top level because that would allow extra properties to be put into the root of the GeoJSON document, like this:
{
"type": "Feature",
"geometry": {},
"properties": {},
"ext_foo": "bar",
"ext_bar": "baz"
}The extension properties would then get lost by the model validator that imports the GeoJSON into the Pydantic model class instance.
There was a problem hiding this comment.
Mmm, so it wasn't getting pivoted into properties correctly. Good catch.
There was a problem hiding this comment.
It was getting cloned into properties, but was somehow also getting replicated at the top level.
| }, | ||
| "properties": { | ||
| "additionalProperties": false, | ||
| "not": { |
There was a problem hiding this comment.
This fixes another subtle 🐞 bug: if these properties were allowed in under "properties", then you'd be open to this scenario in a GeoJSON input:
{
"type": "Feature",
"id": "outer-id",
"bbox": [0, 0, 0, 0],
"geometry": {},
"properties": {
"id": "inner-id",
"bbox": [1, 1, 1, 1],
"geometry": "anything"
}
}The model validator would then copy everything from "properties" up to the root (overwriting the values in the root).
There was a problem hiding this comment.
Does not.required achieve this? It seems more like it would say that those fields aren't required, which was already the case. additionalProperties: false will prevent it, but is contingent on the shape of properties filtering out references to id, bbox, geometry.
There was a problem hiding this comment.
required is a funny beast.
The positive version means "it must be the case that it is there" and the negative version means "it must not be the case that it is there".
So if you check out this mini-schema at https://www.jsonschemavalidator.net/...
{
"type": "object",
"not": { "required": ["foo" ] },
"required": ["bar"]
}You find that this passes: {"bar": 42}, but this fails: {"foo": "anything", "bar": 42}.
Edit: Fixed an example that wrongly ended in a bracket ] instead of a brace }.
There was a problem hiding this comment.
TIL, wow. That's worth capturing (if it's not already) in the code that produces expressions like that.
| "version" | ||
| ], | ||
| "type": "object", | ||
| "unevaluatedProperties": false |
There was a problem hiding this comment.
Redundant - because "additionalProperties": false is stricter...
There was a problem hiding this comment.
What do you mean by "stricter"?
IIRC, unevaluatedProperties applies to sub-schemas (and structures introduced through allOf, etc.), which is probably what we want at the OvertureFeature level (to prevent people from accidentally making their schemas more open than intended, as is the case in a few parts of our current JSON Schema).
There was a problem hiding this comment.
What I mean by stricter is that "additionalProperties": false will prohibit any properties from being included if they are not either explicitly listed in properties or covered by an allowed pattern in patternProperties. So the fact that it won't traverse sub-schemas introduced through aggregators allOf, anyOf, etc.) makes it stricter.
e.g. consider this JSON Schema:
{
"type": "object",
"additionalProperties": false,
"properties": {
"foo": { "type": "integer" }
},
"patternProperties": {
"bar.*": { }
},
"allOf": [{
"properties": {
"baz": { }
}
}]
}The above will work for {"foo":42,"bark": "anything"} but not {"foo":42,"bark": "anything", "baz":"anything else"}.
The reason this makes sense to me in the Pydantic context is that Pydantic models always have enumerated properties in the properties section. The one exception so far is ext_*, which is covered by patternProperties. All of our other applicable constraints don't add allowed properties, they just constrain the values the existing properties are allowed to have.
Edit: Added a missing not that was super confusing.
There was a problem hiding this comment.
Aie, that's confusing.
Given the behavior you're showing, we might do well to avoid ever creating aggregators that introduce sub-schemas.
| @model_validator(mode="after") | ||
| def validate_model(self) -> Self: |
There was a problem hiding this comment.
This is just applying the ext_* validation and can be dropped when we drop that.
| cls, | ||
| schema: core_schema.CoreSchema, | ||
| handler: GetJsonSchemaHandler, | ||
| ) -> JsonSchemaValue: |
There was a problem hiding this comment.
This is just applying the ext_* schema and can be dropped when we drop that.
| }, | ||
| { | ||
| "if": { | ||
| "properties": { | ||
| "subtype": { | ||
| "const": "country" | ||
| } | ||
| } | ||
| }, | ||
| "then": { | ||
| "not": { | ||
| "required": [ | ||
| "country" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
This is another bug 🐞 fix.
The old decoration was:
@not_required_if("subtype", PlaceType.COUNTRY, ["country"])
This only captured half the original constraint (if-then but not if-then-else).
There was a problem hiding this comment.
Not required is the default though, yes? I agree that creating explicit rules is good (particularly to override something that's been marked as required, but that feels like a modeling mistake depending on guidance for semi-required fields), but I don't see the bug.
There was a problem hiding this comment.
I think, IIRC, that @forbid_if won't let you override something that is required. Meaning, if it is required then @forbid_if will throw.
It's limited to the case of "this already optional field is also prohibited if this other condition holds".
There was a problem hiding this comment.
I think, IIRC, that
@forbid_ifwon't let you override something that is required. Meaning, if it is required then@forbid_ifwill throw.It's limited to the case of "this already optional field is also prohibited if this other condition holds".
Yup it inherits this behavior from OptionalFieldGroupConstraint: https://github.com/OvertureMaps/schema/pull/403/files#r2445351121.
There was a problem hiding this comment.
Based on ☝️, my understanding is that not.required is not the default (because it actively blocks). "Not required" (or optional) is. Following from that, this means that the not.required in the annotated code means "shouldn't be present" and that's the bug fix, correct?
The raising behavior is nice. Will it raise at definition time (runtime), so it'll pop up when you try to generate JSON Schema from it? I stuck a note on @forbid_if to document the "won't let you override something that is required" behavior.
There was a problem hiding this comment.
It raises at the time of applying the decorator, so basically you'll have to cure the error before you can go on to generate the JSON Schema from the model.
There was a problem hiding this comment.
Perfect!
| @@ -0,0 +1,319 @@ | |||
| from collections.abc import ( | |||
There was a problem hiding this comment.
The metadata is a formalization and generalization of the model_constraint metadata introduced in #400 track the model constraints applied on a model for code generation support.
| __validators__: dict[str, Callable[..., Any]] | None = None, | ||
| __cls_kwargs__: dict[str, Any] | None = None, | ||
| __qualname__: str | None = None, | ||
| __metadata__: Metadata | None = None, |
There was a problem hiding this comment.
#It's the addition of this line that makes our create_model different from Pydantic's.
| @@ -0,0 +1,146 @@ | |||
| from collections.abc import Callable | |||
There was a problem hiding this comment.
I'm expecting one more pass may be necessary to make @forbid_if, @radio_group, @require_any_of, and @require_if aware of Omitable and behave differently if the field is omitable or nullable (nullable being a union with None).
There was a problem hiding this comment.
This is the factored-out GeoJSON-enabled feature model.
Everything line 388 and below is in support of "on-the-fly refactoring" the JSON Schema to support model_constraint decorators to support three cases:
- Constraints that refer only to the top-level fields
"id","bbox". and"geometry"`. - Constraints that refer only to non-top-level fields.
- Constraints that refer to a mix of top-level and non-top-level fields.
Seth Fitzsimmons (sethfitz)
left a comment
Seth Fitzsimmons (sethfitz)
left a comment
There was a problem hiding this comment.
I'm going to approve and merge to tee you up for episode 4.
Please look through my comments though; there are some minor tweaks that deserve follow-up commits. I'm also unclear on how not.required fixes the bugs you flagged (this appears in a couple places).
I want to raise 2 pseudo-blocking discussions:
First, the use of additionalProperties vs. unevaluatedProperties (which only applies to JSON Schema validation, but has the potential to allow nested properties through if authors don't use @no_extra_fields).
The meta-issue is that I believe that all models that compose an OvertureFeature subclass should behave as if @no_extra_fields were applied, even if it's not explicit, due to the constraints that most format serializations introduce (fixed set of columns, explicit struct members). If you need a dict, use a dict. Apply patterns if you need to, but don't make it a hybrid.
This is somewhat similar to another implicit constraint where we disallow the mode where a JSON array or "map" (i.e., not modeled as a struct) can contain mixed value (or key) types. I can't remember the specifics, but I remember issues early on when we tried to translate JSON data sketches into Parquet.
Second is Omitable (beyond the potential typo). I don't think it's necessary. Outside of document-targeted modeling (i.e., JSON Schema for JSON), the distinction between omittable and nullable doesn't have meaning.
| "patternProperties": { | ||
| "^ext_.*$": { | ||
| "description": "Additional top-level properties must be prefixed with `ext_`." | ||
| } | ||
| }, |
There was a problem hiding this comment.
Mmm, so it wasn't getting pivoted into properties correctly. Good catch.
| "version" | ||
| ], | ||
| "type": "object", | ||
| "unevaluatedProperties": false |
There was a problem hiding this comment.
What do you mean by "stricter"?
IIRC, unevaluatedProperties applies to sub-schemas (and structures introduced through allOf, etc.), which is probably what we want at the OvertureFeature level (to prevent people from accidentally making their schemas more open than intended, as is the case in a few parts of our current JSON Schema).
| }, | ||
| "properties": { | ||
| "additionalProperties": false, | ||
| "not": { |
There was a problem hiding this comment.
Does not.required achieve this? It seems more like it would say that those fields aren't required, which was already the case. additionalProperties: false will prevent it, but is contingent on the shape of properties filtering out references to id, bbox, geometry.
| ) | ||
|
|
||
| # FIXME: Use of `default` on this "floating" type declaration results in a Pydantic warning that the | ||
| # default has no effect. Default value should be migrated to site usage in the actual models. |
There was a problem hiding this comment.
Or removed, since Pydantic defaults only apply to programmatic use of the models (see #354 (comment)). A default of 0 for an int field isn't weird though.
(This is present as a side-effect of the port from JSON Schema.)
| json_schema = get_static_json_schema(config) | ||
|
|
||
| try: | ||
| prev = json_schema["minProperties"] |
There was a problem hiding this comment.
I'm torn on the @min_fields_set name, but I think I'm persuaded that the presence of "set" as a way to more clearly indicate its purpose outweighs following JSON Schema terminology because assuming that schema authors have JSON Schema experience is likely incorrect (and for those that do, we can provide a mapping in the guide; I've left myself a note there).
There was a problem hiding this comment.
I used fields in lieu of properties because it is the Pydantic term and I think we should assume Pydantic-oriented authoring.
I think "set" is a Pydantic term. It's used in the property model_fields_set.
>>> from pydantic import BaseModel
>>> class Foo(BaseModel):
... bar: int | None = None
...
>>> Foo().model_fields_set
set()
>>> Foo(bar=42).model_fields_set
{'bar'}
>>> There was a problem hiding this comment.
I think we should assume Pydantic-oriented authoring.
I agree! (This feeds into the meta-issue about open vs. closed behavior, where we're falling into the trap of paying attention to the JSON Schema behavior over Pydantic (my bad).)
That's not the meaning of "set" that I was expecting ("make sure to set at least n fields"), but good that it matches the other meaning too!
| }, | ||
| { | ||
| "if": { | ||
| "properties": { | ||
| "subtype": { | ||
| "const": "country" | ||
| } | ||
| } | ||
| }, | ||
| "then": { | ||
| "not": { | ||
| "required": [ | ||
| "country" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Not required is the default though, yes? I agree that creating explicit rules is good (particularly to override something that's been marked as required, but that feels like a modeling mistake depending on guidance for semi-required fields), but I don't see the bug.
| - **`overture-schema-core`** - Base classes, geometry models, and common structures | ||
| shared across all themes | ||
| - **`overture-schema-validation`** - Validation system with constraints and mixins | ||
| - **`overture-schema-system`** - Foundational system of primitivef types and constraints |
There was a problem hiding this comment.
| - **`overture-schema-system`** - Foundational system of primitivef types and constraints | |
| - **`overture-schema-system`** - Foundational system of primitive types and constraints |
| def validate_class(self, model_class: type[BaseModel]) -> None: | ||
| super().validate_class(model_class) | ||
|
|
||
| required_fields = [ | ||
| f for f in self.field_names if model_class.model_fields[f].is_required() | ||
| ] | ||
| if required_fields: | ||
| raise TypeError( | ||
| f"`{self.name}` expects all the fields to be optional, but at least one is required in the model class `{model_class.__name__}`: {', '.join(required_fields)}" | ||
| ) |
There was a problem hiding this comment.
(Comment for linking purposes.)
Yeah. Let's keep up a conversation to try to resolve each of the threads.
|
| Decorates a Pydantic model class with a constraint forbidding any of the named fields from | ||
| holding an explicitly-assigned value, but only if a field value condition is true. | ||
|
|
||
| To ensure parity between Python and JSON Schema validation, a field's value must be explicitly | ||
| set to violate the constraint. This means in particular that fields whose value was set by | ||
| Pydantic using a default value do not count as having a set value, and fields containing the | ||
| value `None`, if this value was explicitly set rather than being inherited by default, do count. |
There was a problem hiding this comment.
Missing: a comment that this won't let you override something that is required (and will raise...when?).
There was a problem hiding this comment.
Yeah this is a good idea. Could even be a doctest example. I'll try to wedge it in the next iteration.
Yes, thanks for clarifying. Definitely confusing behavior on the JSON Schema side, but not something we need to surface to Pydantic builders (esp. if we get it right).
Re: I was referring to other Pydantic optimizes for the API definition use case, so it's open by default. I believe that our "flavo[u]r" of Pydantic should default to "closed", even when schema authors don't include
Fair enough ;-) This is from the New Oxford American Dictionary, which is the one that ships with macOS (and is neither Canadian nor old): 
Yep, that was the same reason I remember from the GitHub issue I can't find. Makes sense.
From a JSON/JSON Schema point of view, I agree with you. However, I think that's the wrong lens; coming from JSON Schema doesn't mean that we need to support its semantics, especially if they don't have value for the type of data we're describing. I think we should start from the Pydantic perspective (which is what people will be authoring, even if it doesn't fully support the richness(?) of JSON Schema) and allow that to influence us vs. letting an implementation that we're moving away from (in part because it's confusing) drive the bus. Alternately, I'd be ok saying that Overture only has optional fields (nothing can be omitted, and removing the custom serializer), which maps cleanly to tabular representations. It has the downside of producing more verbose JSON Schema (which also differs from our hand-written version) and making all of our JSON/YAML representations invalid, eliminating the convenience we've derived from them. I think the convenience wins though, since it doesn't change the actual validation behavior. What's a concrete example where |
There's an argument that we're over-thinking this some. When looking at code-generated tools it may be mostly a non-issue. For something like a Spark dataframe writer, we can just generate the code to ignore all the "extra" fields, and I would be tempted to have the generator emit an optional warning/error if the model allows extra fields. For a reader, again we can just ignore any extra columns (top-level model) or again have the code generator emit an optional warning/error if the model allows extra fields. I'd go this way: allow authors to do whatever they say they want, but warn them if what they say they want would produce unexpected results in, say, Spark reading.
The way I'm currently thinking about this is that JSON has two alternative models of optionality: you can go the omit route (which I have an aesthetic preference for) or the specify It also allows a middle ground where explicit Pydantic supports two of the three models of optionality: explicit For e.g., this Pydantic model does allow the >>> class Foo(BaseModel):
... bar: int | None = None
...
>>> Foo()
Foo(bar=None)And it produces this JSON Schema, which also allows the {
"title": "Foo",
"type": "object",
"properties": {
"bar": {
"anyOf": [{
"type": "integer"
}, {
"type": "null"
}],
"default": null,
"title": "Bar"
}
}
}The one thing Pydantic is missing is "full omit[t]ability" without a default, which is semantically the same as the others but just represented differently at the JSON level. This is what Finally getting to your point, it's true that for most of the non-JSON representations we're looking to serialize to, the different representations of omit[t]ability do not matter: the value is either there or it's Maybe it's just the laziness/need to move on speaking, but given we mostly have a workable solution without
|
Probably ;-)
Agreed. Spark Datasets will ignore columns that aren't present in the class definition (
As do I. It's downright convenient to be able to work with sparse JSON.
Aligned, other than switching to |
I think we have the path forward! And agreed that |
The destination is as described in PR #397 (1/N), PR #400, and PR #403. In this commit, the following value is added: 1. 🐞 Fix bug: The `not_required_if` mixin wasn't meeting the original schema intent, which result in the `DivisionBoundary` feature type having the wrong JSON Schema. The expected behavior is "if the boundary is at the country level, then the `country` pair should be omitted" but instead it did two variations: in the Python validator, it required the country pair to be present; and in the JSON Schema, it required the country pair to be present, but only if the condition was not true. Both were subtly wrong. The bug is fixed in this commit and the mixin is renamed to `forbid_if`, which I hope will make the semantics more obvious. 2. 🐞 Fix bug: Very subtle, but the mixin model constraint validators did not work correctly if added directly to a `Feature` model, because they aren't aware of the quirky multi-tiered GeoJSON JSON Schema where most properties get pushed down into "properties", but some do not. This is (hopefully ... 🤞) fixed in the new `Feature` model in the system package. 3. 🐞 Fix bug: Very subtle, but the previous `Feature` model's JSON Schema did not prohibit fields named `"id"`, `"bbox"`, and `"geometry"` within the properties object. This created a weird quirk where in a poisoned JSON object containing those values under properties, the properties version would overwrite the root version. This is fixed by explicitly prohibiting those fields in the JSON Schema for the `Feature` class' properties object. 4. 🐞 Fix bug: The `ExtensibleBaseModel`'s `patternProperties` got put both at the top level of the GeoJSON Feature schema and in the "properties" object schema. This would have allowed the input JSON to contain `ext_*` at two levels, which doesn't meet the original schema intent. As part of this bug fix, `ExtensibleBaseModel` is collapsed directly into `OvertureFeature` and the `ext_*` feature is documented as being on a deprecation path. 5. The `validation` package is officially deleted as its essential George functions have all been re-homed either into `system` or `core`. 6. All model constraint mixins have been refactored along the lines started in PR #400. They are now called `@forbid_if`, `@min_fields_set`, `@no_extra_fields`, `@radio_group`, `@require_any_of`, and `@require_if`. 7. A type hint, `Omitable[T]` has been added to the system package. This type hint allows a Pydantic model to "naturally" get JSON Schema semantics for omitted values - it de-conflates optionality and nullability, which Pydantic currently conflates. In an upcoming episode, I hope to replace the `X | None` unions in the existing model with `Omitable[X]`. 8. The existing `Feature` class from the `core` package has been split into two: (i) `Feature` in the `system` package now acts as a generic feature and owns all the funky GeoJSON logic, allowing George enthusiasts to work with a nice ergonomic feature type without having to use an "Overture"-flavored feature; (ii) `OvertureFeature` in the `core` package adds the Overture schema specific flavoring on top of `Feature`. 9. The `ref` module from `core` has been pushed down into `system` and the dependency on `(Overture)Feature` has been factored out in favor of a generic dependency on an `Identified` model, which is basically just a `BaseModel` with a mandatory ID. Stay tuned for episode 4, where we will re-organize the core package.
The destination is as described in PR #397 (1/N), PR #400, and PR #403. In this commit, the following value is added: 1. 🐞 Fix bug: The `not_required_if` mixin wasn't meeting the original schema intent, which result in the `DivisionBoundary` feature type having the wrong JSON Schema. The expected behavior is "if the boundary is at the country level, then the `country` pair should be omitted" but instead it did two variations: in the Python validator, it required the country pair to be present; and in the JSON Schema, it required the country pair to be present, but only if the condition was not true. Both were subtly wrong. The bug is fixed in this commit and the mixin is renamed to `forbid_if`, which I hope will make the semantics more obvious. 2. 🐞 Fix bug: Very subtle, but the mixin model constraint validators did not work correctly if added directly to a `Feature` model, because they aren't aware of the quirky multi-tiered GeoJSON JSON Schema where most properties get pushed down into "properties", but some do not. This is (hopefully ... 🤞) fixed in the new `Feature` model in the system package. 3. 🐞 Fix bug: Very subtle, but the previous `Feature` model's JSON Schema did not prohibit fields named `"id"`, `"bbox"`, and `"geometry"` within the properties object. This created a weird quirk where in a poisoned JSON object containing those values under properties, the properties version would overwrite the root version. This is fixed by explicitly prohibiting those fields in the JSON Schema for the `Feature` class' properties object. 4. 🐞 Fix bug: The `ExtensibleBaseModel`'s `patternProperties` got put both at the top level of the GeoJSON Feature schema and in the "properties" object schema. This would have allowed the input JSON to contain `ext_*` at two levels, which doesn't meet the original schema intent. As part of this bug fix, `ExtensibleBaseModel` is collapsed directly into `OvertureFeature` and the `ext_*` feature is documented as being on a deprecation path. 5. The `validation` package is officially deleted as its essential George functions have all been re-homed either into `system` or `core`. 6. All model constraint mixins have been refactored along the lines started in PR #400. They are now called `@forbid_if`, `@min_fields_set`, `@no_extra_fields`, `@radio_group`, `@require_any_of`, and `@require_if`. 7. A type hint, `Omitable[T]` has been added to the system package. This type hint allows a Pydantic model to "naturally" get JSON Schema semantics for omitted values - it de-conflates optionality and nullability, which Pydantic currently conflates. In an upcoming episode, I hope to replace the `X | None` unions in the existing model with `Omitable[X]`. 8. The existing `Feature` class from the `core` package has been split into two: (i) `Feature` in the `system` package now acts as a generic feature and owns all the funky GeoJSON logic, allowing George enthusiasts to work with a nice ergonomic feature type without having to use an "Overture"-flavored feature; (ii) `OvertureFeature` in the `core` package adds the Overture schema specific flavoring on top of `Feature`. 9. The `ref` module from `core` has been pushed down into `system` and the dependency on `(Overture)Feature` has been factored out in favor of a generic dependency on an `Identified` model, which is basically just a `BaseModel` with a mandatory ID. Stay tuned for episode 4, where we will re-organize the core package.
2 participants
tl;dr
This is part
#3. Part#4is expected early in the week of 10/20 if all goes according to plan.At this point the
systempackage (George) is pretty much finished, documented, and tested, and there's nothing more that can obviously move into it. It's my hope that the majority of the refactoring is done now. Part#4will focus on organizing thecorepackage.Squash Message
Journey
Destination
pyproject.tomlwill be standardized.make check.pydocstylewith the NumPy convention. (As of now, the tool reports too many complaints to add it intomake check.)pydocstylewill be enforced bymake check.docformatterwill be run bymake check.__init__.pydocstring.doctarget in theMakefilethat doesuv run pdocto a useful place.feature, which will contain the feature models and the main simple types that support them.perspectives.