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

Add support levels to validator config #503

Merged
merged 12 commits into from Sep 24, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 15, 2020

This PR adds the concept of support queryability from the model schemas into the validator config.

This allows for some changes in the validator to enable running query checks on specification fields where implementations have not explicitly provided the type in their /info endpoints.

One consequence of this that we can discuss... if a field is present in the /info/<entry> of an implementation, the validator will report a failure in two cases (where it didn't before):

  1. If the field has a null value in the structure that is "chosen" to generate queries.
  2. If the field has queryable level of "MUST" (even if the field is optional) then this will now be a failure.

I think I'm fine with 2. but 1. feels like more of a grey area...

Still to-do (in this PR or elsewhere):

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #503 into master will decrease coverage by 0.04%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   91.59%   91.55%   -0.05%     
==========================================
  Files          62       62              
  Lines        3119     3127       +8     
==========================================
+ Hits         2857     2863       +6     
- Misses        262      264       +2     
Flag Coverage Δ
#project 91.55% <81.81%> (-0.05%) ⬇️
#validator 63.76% <81.81%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
optimade/validator/validator.py 82.46% <76.47%> (-0.22%) ⬇️
optimade/models/utils.py 90.41% <100.00%> (ø)
optimade/validator/config.py 100.00% <100.00%> (ø)

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 995f32d...eabb60b. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/hardcode_validator_support_levels branch from 7a6c192 to 33d7dea Compare September 15, 2020 22:57
optimade/validator/config.py Outdated Show resolved Hide resolved
@ml-evs ml-evs added the validator Related to the OPTIMADE validator label Sep 18, 2020
@ml-evs ml-evs force-pushed the ml-evs/hardcode_validator_support_levels branch from 197b3af to 3c3f3d3 Compare September 21, 2020 11:06
@ml-evs
Copy link
Member Author

ml-evs commented Sep 21, 2020

This PR now contains the changes from #511, happy to review as one PR or two.

@ml-evs ml-evs force-pushed the ml-evs/hardcode_validator_support_levels branch from 3c3f3d3 to b6267a9 Compare September 21, 2020 13:27
@ml-evs ml-evs mentioned this pull request Sep 23, 2020
5 tasks
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Great work @ml-evs

Some suggested changes inbound.

Concerning your OP null or None is considered a valid value for any property by the OPTIMADE API specification, I believe (then of course for some properties it is not allowed, but that always stated clearly). So this should definitely be allowed. But we can of course remark on it, as information, and later remove the remark if developers find it off-putting.

optimade/validator/config.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/hardcode_validator_support_levels branch from f9cc110 to 162d246 Compare September 23, 2020 14:03
@ml-evs
Copy link
Member Author

ml-evs commented Sep 23, 2020

Thanks for the review @CasperWA!

Concerning your OP null or None is considered a valid value for any property by the OPTIMADE API specification, I believe (then of course for some properties it is not allowed, but that always stated clearly). So this should definitely be allowed. But we can of course remark on it, as information, and later remove the remark if developers find it off-putting.

I've just addressed this in the last commit: this now only fails if the field is a "MUST" field, otherwise it just skips. This will still fail if the field isn't returned by the implementation by default, but I'm fixing that in #515 as its a bit more involved.

@ml-evs
Copy link
Member Author

ml-evs commented Sep 23, 2020

Concerning your OP null or None is considered a valid value for any property by the OPTIMADE API specification, I believe (then of course for some properties it is not allowed, but that always stated clearly). So this should definitely be allowed. But we can of course remark on it, as information, and later remove the remark if developers find it off-putting.

I've just addressed this in the last commit: this now only fails if the field is a "MUST" field, otherwise it just skips. This will still fail if the field isn't returned by the implementation by default, but I'm fixing that in #515 as its a bit more involved.

Actually this opens a can of worms (our server would fail it - #516) and needs #504 to be in first... I'll make a separate PR that add this commit to the validator (#517)

@ml-evs ml-evs force-pushed the ml-evs/hardcode_validator_support_levels branch 2 times, most recently from 2389642 to 162d246 Compare September 23, 2020 14:48
@ml-evs ml-evs added the blocking For issues/PRs that are blocking other PRs. label Sep 23, 2020
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Looking good, minor things that you can take or leave.

optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Show resolved Hide resolved
optimade/validator/validator.py Show resolved Hide resolved
Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/hardcode_validator_support_levels branch from e4d9e97 to eabb60b Compare September 24, 2020 11:01
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Get in

@ml-evs ml-evs merged commit 7bc4596 into master Sep 24, 2020
@CasperWA CasperWA deleted the ml-evs/hardcode_validator_support_levels branch September 24, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking For issues/PRs that are blocking other PRs. validator Related to the OPTIMADE validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants