Skip to content
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

[pydanticgen] Simplify pydantic generator render method #2019

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link
Collaborator

@sneakers-the-rat sneakers-the-rat commented Mar 24, 2024

Part of a continuing series of refactoring the pydanticgen.

Should not be merged until:

When last we left it in #1927 i had made a mess at the bottom of the serialization method casting the previous serialization method result into the new template models.

This PR refactors the serialization method into a generate_class and generate_slot method, splitting up the logic that was all sorta piled up in the serialization method.

I also started transforming some of the get methods into properties, since we want to minimized repeated calls to induced_slots as much as we possibly can, and those should need to only be computed once and have a static result (ie. be properties not methods).

Quite a ways to go yet, but another incremental step there :)

@sneakers-the-rat
Copy link
Collaborator Author

In fixing the failing test, i both noticed that the test was a string comparison test and that the test seems to be incorrect.

Stems from #1094

The model in question has a slot has_bikes that is inlined as a dict and required.

formerly, the test produced a field annotation like this:

bikes: Dict[str, str] = Field(default_factory=dict)

but that would allow a model to be instantiated without bikes, ie Person() would incorrectly validate (id also generates a default of None even when it's required).

The annotation is also too general - it shouldn't be a dictionary of strings, but a dictionary of Bike instances. Looking through the _inline_as_simple_dict_with_value , it looks like the str annotation is from the situation where you have the id of the object, but the Dict would incorrectly fail to validate if it was a dict of Bike instances.

The correct annotation, i think, should be:

bikes: Dict[str, Union[str, Bike]] = Field(...)

so I updated the code and the test to match, and also added a test for list inlining.

This should generally be true, right?

  • if a slot is required, i would expect there to be no default or default factory (unless there is an if_absent or other precomputed value), and for models to fail validation is no value is passed.
  • if a slot is not required, the type annotation should be wrapped in Optional[] and the default should be None.

Optional is an alias for Union[X, None], so there should be a three way definitional equality
required: false === Optional[] === default = None

…issing pydantic compliance tests, make test fail if no condition present
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 94.18605% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 79.90%. Comparing base (24f08f3) to head (5c9e881).
Report is 1 commits behind head on main.

Files Patch % Lines
linkml/generators/pydanticgen/pydanticgen.py 95.31% 2 Missing and 4 partials ⚠️
linkml/generators/pydanticgen/build.py 80.00% 2 Missing and 1 partial ⚠️
linkml/generators/pydanticgen/array.py 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2019      +/-   ##
==========================================
+ Coverage   79.88%   79.90%   +0.02%     
==========================================
  Files         109      109              
  Lines       12249    12252       +3     
  Branches     3489     3486       -3     
==========================================
+ Hits         9785     9790       +5     
+ Misses       1874     1873       -1     
+ Partials      590      589       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sneakers-the-rat
Copy link
Collaborator Author

This PR also:

  • modifies the array generators to create a RangeResult rather than a SlotResult because they're just generating ranges, and the terminology is more uniform that way (they are called ArrayRangeGenerators).
  • implements a SlotResult as well as a ClassResult so that we can propagate side-effects of generating "lower level" objects up to the module, in this case imports and injected classes, in a uniform way. Still some work to do on that but another step towards a reasonable build system.
  • uses plain strings rather than Annotation objects, which were just a sort of perplexing carrier of a string anyway.
  • creates a _clean_injected_classes method, trying to keep clutter out of the render method.

@sneakers-the-rat
Copy link
Collaborator Author

tests pass - bump on reviewing this, this is a blocker for #2036 and other future planned improvements to pydantic generator. basically a tech debt PR that makes few if any changes to behavior, just cleaning up code, breaking it up into discrete sections, reducing the amount of implicit mutation in place, etc.

once this is merged i'll keep working on #2036 :)

@sneakers-the-rat sneakers-the-rat changed the title [pydanticgen] Simplify pydantic generator serialization method [pydanticgen] Simplify pydantic generator render method May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants