Fix some validator-specific crashes #395

ml-evs · 2020-07-10T12:31:23Z

This PR aims to fix some crashes of the validator, caused either by validator-specific code (cf50cf3) or by crashes internal to the pydantic validator (2b4c3bd). It also adds a few niceties to the validator, including the separation of "internal errors" from expected validation errors, as listed below. Importantly, this PR does not update any of the models that are currently causing validation errors, these should be addressed in another PR.

The commits can all be reviewed separately, 8e92308 dominates the diff but is mostly just piping code without any "business logic".

Changes:

cf50cf3 Fixed another occurrence of an optional field without a default value (my fault). I can't spot any more in the codebase (using the slightly convoluted grep below) (closes Validator treats top-level 'included' array as mandatory #393):
```
$ grep "Optional.*Field(" -r . -A 1 -h | grep -v "\-\-" | grep -v Field | grep -v None | grep -v default | grep -v "ok"
```
2b4c3bd Fixed issue where passing a dict to pydantic when it was expecting a list was causing a crash with a huge traceback (closes Validation of 'structures' type crashes #397)
442a407 Improved help string for --verbosity (closes Validator verbosity levels need more detailed description #396).
8e92308 Added internal error handling to prevent all future crashes. These get reported separately to validation errors.
3a917aa Added a couple of self-explanatory quality of life flags, --fail_fast and --skip_optional.
ced32ba Allow for links objects to be passed to validation in pagination test (rather than just strings) and enforce a maximum pagination recursion depth when validating responses (5)
84ab0b1 Pending improvements to the queries we try as validation, I've reduced the set of queries tested from the spec examples to only those that appear mandatory. What remains is still very useful for validating the responses format.
Several changes to validator CLI flags, and renamed script as optimade-validator.

ml-evs · 2020-07-10T12:33:10Z

Once the checks pass, I'll shamelessly wield some executive power and merge this so that we can close #393 and @merkys can get on with his work!

merkys

I confirm I no longer experience #393 on this branch. Many thanks!

ml-evs · 2020-07-10T12:48:51Z

I confirm I no longer experience #393 on this branch. Many thanks!

Great, if you're happy staying on this branch, I'll bunch any other fixes together in this PR.

We're getting a weird build failure for Python 3.7 (I saw it in @CasperWA's pytest PR too) so its perhaps not advisable to merge and release just yet.

merkys · 2020-07-10T12:58:05Z

Great, if you're happy staying on this branch, I'll bunch any other fixes together in this PR.

Sure. I am as well comfortable with switching between branches.

We're getting a weird build failure for Python 3.7 (I saw it in @CasperWA's pytest PR too) so its perhaps not advisable to merge and release just yet.

Take your time!

CasperWA · 2020-07-10T13:43:04Z

We're getting a weird build failure for Python 3.7 (I saw it in @CasperWA's pytest PR too) so its perhaps not advisable to merge and release just yet.

Yeah, this is due to GitHub Actions update of the minor 3.7 version they use. We're seeing the same issues for AiiDA. I am unclear how to solve this.

ml-evs · 2020-07-10T14:30:03Z

Yeah, this is due to GitHub Actions update of the minor 3.7 version they use. We're seeing the same issues for AiiDA. I am unclear how to solve this.

Ugh, tried pinning to 3.7.7 inside actions and it won't let you. Pretty bad of GH to force you to switch to 3.7.8 (released 8 days ago...), presumably some package (seems to be PyYAML?) is pinned to 3.7.7 or below so it is failing to install. 3.7.8 isn't even out on conda yet!

ml-evs · 2020-07-10T14:33:42Z

Just found the relevant issues (raised by AiiDA peeps!) here, actions/setup-python#107

If move to the setup-python@v2 action we can pin the version to 3.7.7 for now until PyYAML is happy again.

CasperWA · 2020-07-10T14:34:05Z

Yeah, this is due to GitHub Actions update of the minor 3.7 version they use. We're seeing the same issues for AiiDA. I am unclear how to solve this.

Ugh, tried pinning to 3.7.7 inside actions and it won't let you. Pretty bad of GH to force you to switch to 3.7.8 (released 8 days ago...), presumably some package (seems to be PyYAML?) is pinned to 3.7.7 or below so it is failing to install. 3.7.8 isn't even out on conda yet!

I believe we can work around it temporarily by not testing adapters (i.e., AiiDA and more) for v3.7 for now perhaps?
This would mean simply adding some if logic to a couple of steps, having it check the python version.

CasperWA · 2020-07-10T14:34:59Z

Just found the relevant issues (raised by AiiDA peeps!) here, actions/setup-python#107

If move to the setup-python@v2 action we can pin the version to 3.7.7 for now until PyYAML is happy again.

Ah cheers :) I was too lazy to find it myself 😅 Even though I'm technically part of the AiiDA developers team ;)

ml-evs · 2020-07-10T14:56:01Z

For separation of concerns, I'm fixing the CI in #400 instead of here.

I've done a lot of robustifying of the validator which I will push here shortly.

codecov · 2020-07-10T15:16:03Z

Codecov Report

Merging #395 into master will decrease coverage by 0.13%.
The diff coverage is 64.56%.

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   91.09%   90.95%   -0.14%     
==========================================
  Files          55       55              
  Lines        2459     2522      +63     
==========================================
+ Hits         2240     2294      +54     
- Misses        219      228       +9

Flag	Coverage Δ
#unittests	`90.95% <64.56%> (-0.14%)`	⬇️

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

Impacted Files	Coverage Δ
optimade/validator/__init__.py	`9.37% <0.00%> (-0.63%)`	⬇️
optimade/validator/validator.py	`77.10% <65.00%> (+2.62%)`	⬆️
optimade/models/entries.py	`97.43% <66.66%> (-2.57%)`	⬇️
optimade/validator/validator_model_patches.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 b310909...001a642. Read the comment docs.

ml-evs · 2020-07-10T17:15:30Z

Hi @merkys, this PR should now fix many of the crashes/false positives you were running into on your test server. Those that remain (mostly caused by #399) will require changes to our models and must be fixed elsewhere.

If you're able to test the validator on this branch again and report any further issues before we review it, that would be great!

CasperWA

Great work here @ml-evs !

I have a few suggested changes :)

optimade/models/entries.py

optimade/validator/__init__.py

optimade/validator/validator.py

…ptional (closes #393)

These errors should be handled differently to ValidationError and ResponseError as they indicate a problem with the validator itself.

- Allow links objects to be passed in pagination - Enforced maximum recursion depth (5) for pagination tests

… improvements (#357)

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

ml-evs · 2020-07-16T22:24:45Z

We have another validator-action chicken/egg problem, as it needs to be updated with the new way of handling --verbosity (good suggestion @CasperWA :P) and the other now-hyphenated flags.

ml-evs · 2020-07-16T22:26:49Z

We have another validator-action chicken/egg problem, as it needs to be updated with the new way of handling --verbosity (good suggestion @CasperWA :P) and the other now-hyphenated flags.

Making the PR for this too

CasperWA · 2020-07-16T22:32:21Z

We have another validator-action chicken/egg problem, as it needs to be updated with the new way of handling --verbosity (good suggestion @CasperWA :P) and the other now-hyphenated flags.

The error message from the validator (in this repo) might also be a bit better, see this line.
Copied here as well for posterity:

optimade_validator: error: unrecognized arguments: http://gh_actions_host:3213/v1

CasperWA

Some very minor changes.
I think the switching of big and small in __init__.py makes sense, the other suggested changes I leave completely up to you.

But other than that, since this repository holds the source for the validator action, this PR must be merged before the validator action PR can be merged.
And as long as this test succeeds (as here), i.e., the test that has been updated to point to the latest commit of this PR (should be updated int the validator action accordingly), I see that as a "check"/success for the docker-image CI check here.

optimade/validator/__init__.py

optimade/validator/validator.py

Use -t for --as-type (instead of -a). Co-authored-by: Andrius Merkys <andrius.merkys@gmail.com>

CasperWA

Thanks for this @ml-evs !

For posterity: @ml-evs didn't like my suggestion to remove the > 0 in the latest review, due to it being more clear in its explicit form.

merkys · 2020-07-22T03:40:56Z

Hi @merkys, this PR should now fix many of the crashes/false positives you were running into on your test server. Those that remain (mostly caused by #399) will require changes to our models and must be fixed elsewhere.

Thanks a lot! :)

If you're able to test the validator on this branch again and report any further issues before we review it, that would be great!

I missed this train, but I will open new issues should any further problems occur.

ml-evs added priority/high Issue or PR with a consensus of high priority validator Related to the OPTIMADE validator labels Jul 10, 2020

merkys previously approved these changes Jul 10, 2020

View reviewed changes

ml-evs dismissed merkys’s stale review via a2be123 July 10, 2020 14:17

ml-evs force-pushed the ml-evs/hotfix_validator_models branch from a2be123 to 95f5811 Compare July 10, 2020 14:34

ml-evs mentioned this pull request Jul 10, 2020

Temporarily run AiiDA tests on Python 3.8 only #400

Merged

ml-evs force-pushed the ml-evs/hotfix_validator_models branch from 95f5811 to f33ef2e Compare July 10, 2020 15:10

ml-evs force-pushed the ml-evs/hotfix_validator_models branch 3 times, most recently from 87d3c63 to 442a407 Compare July 10, 2020 15:35

ml-evs changed the title ~~Fix "included" in patched validator models~~ Fix some validator-specific crashes Jul 10, 2020

ml-evs force-pushed the ml-evs/hotfix_validator_models branch 2 times, most recently from 1f90287 to 84ab0b1 Compare July 10, 2020 17:07

ml-evs requested review from CasperWA and merkys July 10, 2020 17:15

CasperWA requested changes Jul 16, 2020

View reviewed changes

ml-evs added 3 commits July 16, 2020 23:06

Give validator patched included a default value to make it really o…

67eebf5

…ptional (closes #393)

Validate relationships data as a list before testing type (closes #397)

a3b420a

Improve --verbosity help string for validator (closes #396)

a1211c0

ml-evs added 5 commits July 16, 2020 23:06

Add concept of InternalErrors to validator

1c4e56c

These errors should be handled differently to ValidationError and ResponseError as they indicate a problem with the validator itself.

Added validator flags --fail_fast and --skip_optional_tests

5aa2740

Improvements to pagination validation

d18e5a5

- Allow links objects to be passed in pagination - Enforced maximum recursion depth (5) for pagination tests

Reduce set of example queries from the specification, pending future…

d409c05

… improvements (#357)

Improve error message for single entry endpoint if deserialization fails

51bd483

ml-evs force-pushed the ml-evs/hotfix_validator_models branch 4 times, most recently from 0cde9df to 04ac0ad Compare July 16, 2020 22:12

Implemented suggestions from code review

56708a1

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>

ml-evs force-pushed the ml-evs/hotfix_validator_models branch from 04ac0ad to 56708a1 Compare July 16, 2020 22:13

ml-evs mentioned this pull request Jul 16, 2020

Updated validator flags Materials-Consortia/optimade-validator-action#21

Merged

Renamed entrypoint optimade_validator->optimade-validator

d71275a

CasperWA requested changes Jul 17, 2020

View reviewed changes

optimade/validator/__init__.py Outdated Show resolved Hide resolved

optimade/validator/__init__.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Show resolved Hide resolved

optimade/validator/validator.py Show resolved Hide resolved

Flip arg options the "standard" way around

001a642

Use -t for --as-type (instead of -a). Co-authored-by: Andrius Merkys <andrius.merkys@gmail.com>

ml-evs requested a review from CasperWA July 17, 2020 09:57

CasperWA approved these changes Jul 17, 2020

View reviewed changes

ml-evs merged commit 93e50b1 into master Jul 17, 2020

ml-evs deleted the ml-evs/hotfix_validator_models branch July 17, 2020 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some validator-specific crashes #395

Fix some validator-specific crashes #395

ml-evs commented Jul 10, 2020 •

edited

ml-evs commented Jul 10, 2020

merkys left a comment

ml-evs commented Jul 10, 2020

merkys commented Jul 10, 2020

CasperWA commented Jul 10, 2020

ml-evs commented Jul 10, 2020

ml-evs commented Jul 10, 2020

CasperWA commented Jul 10, 2020

CasperWA commented Jul 10, 2020

ml-evs commented Jul 10, 2020

codecov bot commented Jul 10, 2020 •

edited

ml-evs commented Jul 10, 2020

CasperWA left a comment

ml-evs commented Jul 16, 2020

ml-evs commented Jul 16, 2020

CasperWA commented Jul 16, 2020

CasperWA left a comment

CasperWA left a comment

merkys commented Jul 22, 2020

Fix some validator-specific crashes #395

Fix some validator-specific crashes #395

Conversation

ml-evs commented Jul 10, 2020 • edited

ml-evs commented Jul 10, 2020

merkys left a comment

Choose a reason for hiding this comment

ml-evs commented Jul 10, 2020

merkys commented Jul 10, 2020

CasperWA commented Jul 10, 2020

ml-evs commented Jul 10, 2020

ml-evs commented Jul 10, 2020

CasperWA commented Jul 10, 2020

CasperWA commented Jul 10, 2020

ml-evs commented Jul 10, 2020

codecov bot commented Jul 10, 2020 • edited

Codecov Report

ml-evs commented Jul 10, 2020

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Jul 16, 2020

ml-evs commented Jul 16, 2020

CasperWA commented Jul 16, 2020

CasperWA left a comment

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

merkys commented Jul 22, 2020

ml-evs commented Jul 10, 2020 •

edited

codecov bot commented Jul 10, 2020 •

edited