SHOULD/MUST/OPTIONAL fields in models #453

ml-evs · 2020-08-05T14:43:13Z

Following the discussion in #399, this PR is a place to try out ways of incorporating the "SHOULD" level of support for some fields in the specification. This PR does the following:

adds a couple of wrappers around pydantic's Field (namely OptimadeField and StrictField).
- StrictField disallows keys outside of the pydantic Field signature, plus a couple of extras that we use (unit, pattern and uniqueItems), unfortunately. It emits a warning if a description is not supplied. Any tips on how to do this in a more pydantic way would be appreciated...
- OptimadeField calls StrictField but also forces all fields to supply a queryable and a support attribute for all fields
~~switch away from pattern=... to regex=... inside Field(...) where possible so that regexes are applied automatically on validation.~~
fix a couple of fields that @CasperWA spotted elsewhere that have a typo decsription that means the schema never saw their descriptions... this is basically what StrictField will prevent in the future.

codecov · 2020-08-05T14:47:35Z

Codecov Report

Merging #453 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #453      +/-   ##
==========================================
+ Coverage   91.48%   91.58%   +0.10%     
==========================================
  Files          61       61              
  Lines        3065     3103      +38     
==========================================
+ Hits         2804     2842      +38     
  Misses        261      261

Flag	Coverage Δ
#project	`91.58% <100.00%> (+0.10%)`	⬆️
#validator	`63.61% <98.11%> (+0.32%)`	⬆️

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

Impacted Files	Coverage Δ
optimade/models/baseinfo.py	`95.23% <100.00%> (ø)`
optimade/models/entries.py	`97.50% <100.00%> (+0.06%)`	⬆️
optimade/models/index_metadb.py	`100.00% <100.00%> (ø)`
optimade/models/jsonapi.py	`92.56% <100.00%> (+0.06%)`	⬆️
optimade/models/links.py	`100.00% <100.00%> (ø)`
optimade/models/optimade_json.py	`95.74% <100.00%> (ø)`
optimade/models/references.py	`97.72% <100.00%> (+0.05%)`	⬆️
optimade/models/responses.py	`97.61% <100.00%> (+0.05%)`	⬆️
optimade/models/structures.py	`95.08% <100.00%> (ø)`
optimade/models/utils.py	`90.41% <100.00%> (+7.48%)`	⬆️

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 06dd2be...1ec5005. Read the comment docs.

optimade/models/structures.py

ml-evs · 2020-09-09T12:19:16Z

optimade/models/utils.py

+    for key in kwargs:
+        if key not in set(
+            list(_PYDANTIC_FIELD_KWARGS)
+            + ["unit", "pattern", "uniqueItems", "support", "queryable", "sortable"]


I feel like there must be a better way of doing this, but customising pydantic Fields doesn't seem well documented. This "works" and should catch any potential typos we could make when creating fields. It's a bit of a leaky abstraction...

ml-evs · 2020-09-09T13:03:21Z

tests/models/test_utils.py

+    detail = re.escape(
+        "Not creating StrictField ((Ellipsis,), {'random_key': 'disallowed'}) with forbidden keywords ['random_key']."
+    )
+    detail = "forbidden keywords"


Here (and below) the full warning/error detail wouldn't match, even when using re.escape. Not sure why...

CasperWA · 2020-09-09T14:29:05Z

* switch away from `pattern=...` to `regex=...` inside `Field(...)` where possible so that regexes are applied automatically on validation.

Some of these were done on purpose, since we were either already testing this, or I found that the additional validation was time-consuming and irrelevant. The latter may be the case for the fields with constant values...

CasperWA

Some comments.

I am not sure why you're getting the issues concerning the tests and `re' matching. But I've provided a suggested change for you to try out :)

I wonder if we can do this in Config of a parent model instead, but perhaps not. Perhaps this is the best way. It's at least quite elegant in the end, I think. At least for what it is.
Otherwise we should create a custom JSON encoder that can do all this and be used when creating the OpenAPI specification?

optimade/models/baseinfo.py

optimade/models/references.py

optimade/models/utils.py

tests/models/test_utils.py

ml-evs · 2020-09-09T19:05:45Z

I am not sure why you're getting the issues concerning the tests and `re' matching. But I've provided a suggested change for you to try out :)

+1

I wonder if we can do this in Config of a parent model instead, but perhaps not. Perhaps this is the best way. It's at least quite elegant in the end, I think. At least for what it is.

Yeah, I think the only other option is to define OptimadeModel(BaseModel) and StrictModel(BaseModel), but I think that would involve a fair bit more hackery.

Otherwise we should create a custom JSON encoder that can do all this and be used when creating the OpenAPI specification?

Yep, I think we have no other choice if we want this stuff in the schema.

ml-evs · 2020-09-09T19:07:38Z

* switch away from `pattern=...` to `regex=...` inside `Field(...)` where possible so that regexes are applied automatically on validation.
Some of these were done on purpose, since we were either already testing this, or I found that the additional validation was time-consuming and irrelevant. The latter may be the case for the fields with constant values...

I only changed them because I didn't like that I had to allow pattern through StrictField as a special case, but then I couldn't get rid of it for the URL regex anyway. I guess you're right, it is redundant for the const case (I doubt it even runs the regex?) Happy to revert this one if you want.

CasperWA

Like for the other PR, very minor nit-picky things, mainly doc strings.

And just like the other PR, a great acknowledgement of your work here @ml-evs, thanks!

optimade/models/references.py

optimade/models/utils.py

CasperWA · 2020-09-10T10:14:05Z

* switch away from `pattern=...` to `regex=...` inside `Field(...)` where possible so that regexes are applied automatically on validation.
Some of these were done on purpose, since we were either already testing this, or I found that the additional validation was time-consuming and irrelevant. The latter may be the case for the fields with constant values...
I only changed them because I didn't like that I had to allow pattern through StrictField as a special case, but then I couldn't get rid of it for the URL regex anyway. I guess you're right, it is redundant for the const case (I doubt it even runs the regex?) Happy to revert this one if you want.

I'm pretty sure the const value is simply set at some point and the validation is then run. So for these values I'd rather just revert to not uneccesarily clutter the list of validators. It's a form of an OpenAPI schema hack already as it is ;)
And indeed, pattern is added as a result of the regex, but regex also adds a validator.

ml-evs · 2020-09-10T11:19:17Z

I'm pretty sure the const value is simply set at some point and the validation is then run. So for these values I'd rather just revert to not uneccesarily clutter the list of validators. It's a form of an OpenAPI schema hack already as it is ;)

Yep, fair, reverted.

ml-evs · 2020-09-10T18:07:49Z

Have squashed this down to 3 commits, so ready when you are happy with it @CasperWA.

ml-evs · 2020-09-10T18:08:57Z

Annoyingly GH was hiding some remaining unresolved comments, so feel free to disagree with me on those still...

optimade/models/references.py

optimade/models/structures.py

optimade/models/entries.py

Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>

- Added tests.models.test_utils - Use StrictField and OptimadeField wherever possible

- Fixed fields with missing support/queryable - Fixed some docstring formatting - Made Person->name a MUST - Add support levels for subfields - Promote subfields to full OptimadeFields

CasperWA

Great! This should be about ready to get in 👍 cheers @ml-evs

ml-evs force-pushed the ml-evs/relax_models branch 2 times, most recently from 2d1f9b4 to be897e5 Compare August 5, 2020 14:49

ml-evs commented Aug 5, 2020

View reviewed changes

optimade/models/structures.py Outdated Show resolved Hide resolved

ml-evs mentioned this pull request Sep 1, 2020

Review the required properties of StructureResourceAttributes in openapi.json #198

Closed

CasperWA mentioned this pull request Sep 1, 2020

Fix field optonality inconsistency in schema #482

Merged

ml-evs force-pushed the ml-evs/relax_models branch 4 times, most recently from c535c76 to 932ba34 Compare September 9, 2020 01:31

ml-evs mentioned this pull request Sep 9, 2020

Missing field descriptions in schema for Species->name and Person->name #492

Closed

ml-evs force-pushed the ml-evs/relax_models branch from 932ba34 to ecdb1e1 Compare September 9, 2020 01:33

ml-evs commented Sep 9, 2020

View reviewed changes

ml-evs force-pushed the ml-evs/relax_models branch from 481309c to 4feee5f Compare September 9, 2020 13:04

CasperWA reviewed Sep 9, 2020

View reviewed changes

ml-evs force-pushed the ml-evs/relax_models branch 3 times, most recently from fa6b682 to 784e6de Compare September 9, 2020 22:08

CasperWA requested changes Sep 10, 2020

View reviewed changes

optimade/models/references.py Show resolved Hide resolved

optimade/models/utils.py Show resolved Hide resolved

optimade/models/utils.py Show resolved Hide resolved

ml-evs force-pushed the ml-evs/relax_models branch 2 times, most recently from 8452de5 to 64c9a62 Compare September 10, 2020 11:19

ml-evs force-pushed the ml-evs/relax_models branch 2 times, most recently from 2293a25 to df40d6b Compare September 10, 2020 18:07

ml-evs requested a review from CasperWA September 10, 2020 18:09

ml-evs force-pushed the ml-evs/relax_models branch from df40d6b to 022be53 Compare September 11, 2020 13:07

CasperWA requested changes Sep 11, 2020

View reviewed changes

optimade/models/references.py Outdated Show resolved Hide resolved

optimade/models/structures.py Outdated Show resolved Hide resolved

optimade/models/structures.py Show resolved Hide resolved

optimade/models/entries.py Show resolved Hide resolved

ml-evs force-pushed the ml-evs/relax_models branch 2 times, most recently from a6c66fc to cb3979e Compare September 11, 2020 19:48

ml-evs requested a review from CasperWA September 11, 2020 19:48

ml-evs force-pushed the ml-evs/relax_models branch from cb3979e to f40f894 Compare September 11, 2020 19:49

ml-evs and others added 3 commits September 11, 2020 20:51

Fix typos: decsription->description (closes #492)

4dad104

Co-authored-by: Casper Welzel Andersen <casper.andersen@epfl.ch>

Introduce OptimadeField and StrictField wrappers

f7ab2a0

- Added tests.models.test_utils - Use StrictField and OptimadeField wherever possible

Apply suggestions from code review

1ec5005

- Fixed fields with missing support/queryable - Fixed some docstring formatting - Made Person->name a MUST - Add support levels for subfields - Promote subfields to full OptimadeFields

ml-evs force-pushed the ml-evs/relax_models branch from f40f894 to 1ec5005 Compare September 11, 2020 19:51

CasperWA approved these changes Sep 11, 2020

View reviewed changes

ml-evs merged commit d04d3fe into master Sep 11, 2020

CasperWA deleted the ml-evs/relax_models branch September 15, 2020 10:06

ml-evs mentioned this pull request Dec 14, 2020

Our models and validator are too strict #399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SHOULD/MUST/OPTIONAL fields in models #453

SHOULD/MUST/OPTIONAL fields in models #453

ml-evs commented Aug 5, 2020 •

edited

codecov bot commented Aug 5, 2020 •

edited

ml-evs Sep 9, 2020

ml-evs Sep 9, 2020

CasperWA commented Sep 9, 2020 •

edited

CasperWA left a comment

ml-evs commented Sep 9, 2020

ml-evs commented Sep 9, 2020

CasperWA left a comment

CasperWA commented Sep 10, 2020

ml-evs commented Sep 10, 2020

ml-evs commented Sep 10, 2020

ml-evs commented Sep 10, 2020

CasperWA left a comment

SHOULD/MUST/OPTIONAL fields in models #453

SHOULD/MUST/OPTIONAL fields in models #453

Conversation

ml-evs commented Aug 5, 2020 • edited

codecov bot commented Aug 5, 2020 • edited

Codecov Report

ml-evs Sep 9, 2020

Choose a reason for hiding this comment

ml-evs Sep 9, 2020

Choose a reason for hiding this comment

CasperWA commented Sep 9, 2020 • edited

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Sep 9, 2020

ml-evs commented Sep 9, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA commented Sep 10, 2020

ml-evs commented Sep 10, 2020

ml-evs commented Sep 10, 2020

ml-evs commented Sep 10, 2020

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Aug 5, 2020 •

edited

codecov bot commented Aug 5, 2020 •

edited

CasperWA commented Sep 9, 2020 •

edited