-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix some validator-specific crashes #395
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Sure. I am as well comfortable with switching between branches.
Take your time! |
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! |
Just found the relevant issues (raised by AiiDA peeps!) here, actions/setup-python#107 If move to the |
I believe we can work around it temporarily by not testing adapters (i.e., AiiDA and more) for v3.7 for now perhaps? |
Ah cheers :) I was too lazy to find it myself 😅 Even though I'm technically part of the AiiDA developers team ;) |
a2be123
to
95f5811
Compare
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. |
95f5811
to
f33ef2e
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
87d3c63
to
442a407
Compare
1f90287
to
84ab0b1
Compare
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here @ml-evs !
I have a few suggested changes :)
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
0cde9df
to
04ac0ad
Compare
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
04ac0ad
to
56708a1
Compare
We have another validator-action chicken/egg problem, as it needs to be updated with the new way of handling |
Making the PR for this too |
The error message from the validator (in this repo) might also be a bit better, see this line. optimade_validator: error: unrecognized arguments: http://gh_actions_host:3213/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Use -t for --as-type (instead of -a). Co-authored-by: Andrius Merkys <andrius.merkys@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! :)
I missed this train, but I will open new issues should any further problems occur. |
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:
--verbosity
(closes Validator verbosity levels need more detailed description #396).--fail_fast
and--skip_optional
.optimade-validator
.