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

Update pydantic to ~=1.8 #731

Merged

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Mar 3, 2021

Closes #730.

It seems we do not need to support more of the special OpenAPI keys that we're already supporting. Only allOf is used in the first-level for the retrieving the "attributes" fields anyway.

This PR also closes #729, since it will update pydantic, adding proper support for Python 3.9.

dependabot bot and others added 3 commits March 3, 2021 14:22
The latter is needed due to an updated serialization of Enums used in
pydantic models in pydantic>=1.7.
@CasperWA
Copy link
Member Author

CasperWA commented Mar 3, 2021

Note, this will update the OpenAPI schemas, but this makes sense, as the Enum classes simply get the same treatment and representation as pydantic models have always had.

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #731 (bf54594) into master (c282454) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #731   +/-   ##
=======================================
  Coverage   92.81%   92.81%           
=======================================
  Files          67       67           
  Lines        3520     3520           
=======================================
  Hits         3267     3267           
  Misses        253      253           
Flag Coverage Δ
project 92.81% <ø> (ø)
validator 63.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/models/entries.py 97.50% <ø> (ø)
optimade/models/index_metadb.py 100.00% <ø> (ø)
optimade/models/links.py 100.00% <ø> (ø)
optimade/models/structures.py 96.38% <ø> (ø)
optimade/models/utils.py 91.56% <ø> (ø)
...made/server/entry_collections/entry_collections.py 98.85% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c282454...bf54594. Read the comment docs.

@CasperWA
Copy link
Member Author

CasperWA commented Mar 3, 2021

Added a test for the get_attribute_fields() method. The test could also inspire a different way of determining the attribute fields in general, if one can get a hold of the relevant resource attribute pydantic model... :)

Otherwise, it's ready for review @ml-evs.

@CasperWA
Copy link
Member Author

CasperWA commented Mar 3, 2021

It's worth mentioning @rartino and possibly @merkys as well for this PR, as we're slightly changing the OpenAPI schema (which will later be passed onto the central OPTIMADE repository).

What: The change is for schema fields that use enumerations only, adding information about the default value and field descriptions - something that is already present for other non-enumeration schema fields. It also uses the allOf OpenAPI key to reference the enumeration model. This is a discussed topic in the the OpenAPI community, but is definitely a valid representation (relevant documentation).

Why: This is the updated way of handling Python Enum classes when used as the field types in pydantic models (pydantic being the Python package we use to define the schema as Python data classes). So if we want to keep up with the latest version of pydantic and not be left behind, this is more-or-less a mandatory change. In theory it can be changed with a dedicated custom serializer function, but this is a lot of effort to put in.

@ml-evs
Copy link
Member

ml-evs commented Mar 3, 2021

I'm fine with this, but for continuity I think it makes sense to release the 1.0.1 of the specification before we make this change (I have just merged the most recent schema sync PR). Will review the new code soon.

@CasperWA
Copy link
Member Author

CasperWA commented Mar 3, 2021

I'm fine with this, but for continuity I think it makes sense to release the 1.0.1 of the specification before we make this change (I have just merged the most recent schema sync PR). Will review the new code soon.

Yes and no. It shouldn't take long to update the schemas in the specification repo again immediately if we just hail the right people for 5 min. :) But to find out how long time it would take, we would first need an evaluation of this PR by those right people (hint @rartino, @merkys, ...) 😄 (specifically revieweing the OpenAPI schemas.)

Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed OpenAPI schemas and I am fine with their changes. In Python code I see only a single change in comment and addition of a test, which is almost always a good thing. Thus I tend to approve this PR.

As for updating the OpenAPI schemas for OPTIMADE, I am fine with both pre-1.0.1 and post-1.0.1 merge. Pre-1.0.1 merge is sensible due to specified default value for aggregate.

@CasperWA
Copy link
Member Author

CasperWA commented Mar 4, 2021

@ml-evs I would still need you approval here as one of the code owners. Also, if you agree I will open another PR to update the schemas (again) in the OPTIMADE repository and we can try and quickly get this update in, including a release to v0.13.3.

@CasperWA
Copy link
Member Author

CasperWA commented Mar 4, 2021

Ah! I found another "issue". The "title" OpenAPI key is not present for properties using Enum types. Indeed, it is now removed from structure_features and others in the openapi.json file. Note also that it was never present for the links attributes that use Enum types, so this is an opportunity to add them for all properties using Enums.

When a pydantic model attribute/field specifies an Enum as the type, it
doesn't auto-generate the "title" key for the OpenAPI schema.
This commit adds them in explicitly.

It also adds some doc strings where it was missing for Enums.
@CasperWA
Copy link
Member Author

CasperWA commented Mar 4, 2021

@merkys I have fixed the schemas according to the comment above. You can see the changes with respect to the ones that you accepted here and see the aggregated changes by reviewing normally. Note, that there are no longer changes in any "title" values there, only additions.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CasperWA, looks good.

@ml-evs ml-evs changed the title Update pydantic to >=1.7 (~=1.8) Update pydantic to ~=1.8 Mar 4, 2021
@CasperWA
Copy link
Member Author

CasperWA commented Mar 4, 2021

Thanks @CasperWA, looks good.

I'll merge this in and open a PR in the OPTIMADE repository, discussing the extra changes since @merkys reviewed and accepted this one previously there.

CasperWA added a commit to Materials-Consortia/OPTIMADE that referenced this pull request Mar 16, 2021
The related OPTIMADE Python tools PR:
Materials-Consortia/optimade-python-tools#731.
The changes are related to enumerations (Python Enum
sub-classes), extending information about them and defining
the default value for the aggregate field attribute for a links
resource.
ml-evs pushed a commit to Materials-Consortia/OPTIMADE that referenced this pull request Apr 23, 2021
The related OPTIMADE Python tools PR:
Materials-Consortia/optimade-python-tools#731.
The changes are related to enumerations (Python Enum
sub-classes), extending information about them and defining
the default value for the aggregate field attribute for a links
resource.
ml-evs pushed a commit to Materials-Consortia/OPTIMADE that referenced this pull request May 30, 2021
The related OPTIMADE Python tools PR:
Materials-Consortia/optimade-python-tools#731.
The changes are related to enumerations (Python Enum
sub-classes), extending information about them and defining
the default value for the aggregate field attribute for a links
resource.
rartino pushed a commit to Materials-Consortia/OPTIMADE that referenced this pull request Jul 7, 2021
The related OPTIMADE Python tools PR:
Materials-Consortia/optimade-python-tools#731.
The changes are related to enumerations (Python Enum
sub-classes), extending information about them and defining
the default value for the aggregate field attribute for a links
resource.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support anyOf, allOf, etc. standard OpenAPI fields
3 participants