Pydantic package organization episode 2: "The mess unfolds" - #400
Conversation
…...` and move to system
…tensionPrefixModelConstraint wasn't used anywhere
…es so make check passes Now I want to turn back to mixin/model constraints and implement require_field_from().
…e model_constraint
…destinations The "require one of" for destinations wasn't reflected in the JSON schema, fixed now: https://github.com/OvertureMaps/schema/blob/dev/schema/transportation/segment.yaml#L104-L106
|
|
||
|
|
||
| class Address(StrictBaseModel): | ||
| # TODO - vic - move this into Places |
There was a problem hiding this comment.
👍
FWIW, it's only here because this matches where it was located in the JSON Schema.
| def allow_extension_fields() -> Callable: | ||
| """Decorator to allow only ext_* prefixed extension fields.""" | ||
|
|
||
| def decorator(cls: type[BaseModel]) -> type[BaseModel]: | ||
| constraint = ExtensionPrefixValidator() | ||
| register_constraint(cls, constraint) | ||
| return cls | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
Very nice.
| @@ -0,0 +1,208 @@ | |||
| from collections.abc import ( | |||
There was a problem hiding this comment.
Should I come back and review this once it's wired up? I haven't spent the time to properly grok it, but I suspect that I have questions that are better answered by comments, how it's used by models.
| no_extra_fields, | ||
| ) | ||
|
|
||
| sys.path.insert(0, str(Path(__file__).parent)) # Needed to import `util` peer module. |
There was a problem hiding this comment.
This may be fixable within pyproject.toml, setting up tests/ to behave as modules... Did you try anything else?
There was a problem hiding this comment.
No. I took the lazy man's way out. (Well lazy and time-constrained.)
We should fix it for sure. But being a test I'm thinking it's a bit farther down for now.
|
|
||
| if __name__ == "__main__": | ||
| pytest.main([__file__]) |
There was a problem hiding this comment.
Whoops. This is probably extraneous too.
…ing.py Apply Seth's suggestion on color docs consistency. Co-authored-by: Seth Fitzsimmons <sethfitz@amazon.com>
…nit__.py Apply Seth's suggestion on grammar correction. Co-authored-by: Seth Fitzsimmons <sethfitz@amazon.com>
…nit__.py Apply Seth's suggestion on grammar correction. Co-authored-by: Seth Fitzsimmons <sethfitz@amazon.com>
Apply Seth's suggestion on confidence score precision. Co-authored-by: Seth Fitzsimmons <sethfitz@amazon.com>
0c0f7f9 to
86314a9
Compare
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.
Re-org. Pydantic packages (2/N) The destination is as described in PR #397 (1/N) and PR #400. In this commit, the following value is added: 1. 🐞 Fix bug: The `@any_of` decorator was not working with transporation destinations. The bug was discovered while refactoring the `mixin` module and the fix is reflected in the updated transportation schema baseline. 2. 🐞 Fix bug: The `make doctest` target exited with code 0 (success) even if it found doctest failures. This bug was added in #397 when the `doctest` target was added and is now fixed. 3. Apply multiple suggestions from PR #397 including renaming `CountryCode` to `CountryCodeAlpha2` and dropping an unnecessary attribute from `PatternConstraint`. 4. Rename the sources package to annex in alignment as shown in the destination package diagram. 5. Begin refactoring `mixin` module to fix some known issues and polish it for final release. Changes made here: - Underlying implementation is replaced with the Pydantic function `create_model` (as was done in the scoping POC) which eliminates the need for `ConstrainedBaseModel` and should also eliminate MRO issues. - Underlying implementation uses some basic "JSON Schema algebra" to ensure that multiple decorators can coexist without breaking each others' JSON Schema changes. - The module name `mixin` is replaced by `model_constraint`, which feels more use case-oriented. - Initially one main decorator is migrated from `mixin` in the validation package to `model_constraint` in the system package. The rest will come next PR. 6. Migrate much of the validation package `README.md` into the system package's main module `__init__.py` and migrate the examples into doctests. 7. Re-home a few other bits of functionality from validation to core. 8. Bring the https://github.com/vcschapp/overture-schema-pydantic scoping into core as `scoping.py` and fix up the linter errors. This is not wired up yet, but that's Coming Soon, likely in episode 4. 9. At Seth's suggestion, switch `ConfidenceScore` from `float64` to `float32` though we will have to double-check the actual value in the Parquet model here. Stay tuned for episode 3, where we will finish re-homing the validation 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.
Re-org. Pydantic packages (2/N) The destination is as described in PR #397 (1/N) and PR #400. In this commit, the following value is added: 1. 🐞 Fix bug: The `@any_of` decorator was not working with transporation destinations. The bug was discovered while refactoring the `mixin` module and the fix is reflected in the updated transportation schema baseline. 2. 🐞 Fix bug: The `make doctest` target exited with code 0 (success) even if it found doctest failures. This bug was added in #397 when the `doctest` target was added and is now fixed. 3. Apply multiple suggestions from PR #397 including renaming `CountryCode` to `CountryCodeAlpha2` and dropping an unnecessary attribute from `PatternConstraint`. 4. Rename the sources package to annex in alignment as shown in the destination package diagram. 5. Begin refactoring `mixin` module to fix some known issues and polish it for final release. Changes made here: - Underlying implementation is replaced with the Pydantic function `create_model` (as was done in the scoping POC) which eliminates the need for `ConstrainedBaseModel` and should also eliminate MRO issues. - Underlying implementation uses some basic "JSON Schema algebra" to ensure that multiple decorators can coexist without breaking each others' JSON Schema changes. - The module name `mixin` is replaced by `model_constraint`, which feels more use case-oriented. - Initially one main decorator is migrated from `mixin` in the validation package to `model_constraint` in the system package. The rest will come next PR. 6. Migrate much of the validation package `README.md` into the system package's main module `__init__.py` and migrate the examples into doctests. 7. Re-home a few other bits of functionality from validation to core. 8. Bring the https://github.com/vcschapp/overture-schema-pydantic scoping into core as `scoping.py` and fix up the linter errors. This is not wired up yet, but that's Coming Soon, likely in episode 4. 9. At Seth's suggestion, switch `ConfidenceScore` from `float64` to `float32` though we will have to double-check the actual value in the Parquet model here. Stay tuned for episode 3, where we will finish re-homing the validation 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.
tl;dr
This is part #2. Part #3 should drop by Friday 8/10 latest. I'm now expecting 5 total parts.
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.Our most foundationalI no longer think this is necessary as there's an evolving clean separation between fields and models (user-facing descriptions or docstrings) and system types (builder-facing docstrings).BaseModelbase class insystemwill be given an optionaldescriptionproperty that can be used to provide class-level documentation for the schema user persona. (See this conversation in the comments on this PR.)feature, which will contain the feature models and the main simple types that support them.perspectives.Packaging Deep Dive
Recapping the target package organization from part 1 (PR #397):