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 filter examples and validate optional cases #227

Merged
merged 8 commits into from Mar 19, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Mar 13, 2020

This PR completes the automation of scraping the spec for examples, since they are now all parseable as of Materials-Consortia/OPTIMADE#263. It adds the optional filters as tests in the validator that are printed with less emphasis, and do not cause the validator to return a non-zero exit code.

@ml-evs ml-evs requested a review from CasperWA March 13, 2020 14:25
@codecov
Copy link

codecov bot commented Mar 13, 2020

Codecov Report

Merging #227 into master will decrease coverage by 0.23%.
The diff coverage is 69.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
- Coverage   87.57%   87.33%   -0.24%     
==========================================
  Files          43       43              
  Lines        1916     1943      +27     
==========================================
+ Hits         1678     1697      +19     
- Misses        238      246       +8     
Flag Coverage Δ
#unittests 87.33% <69.81%> (-0.24%) ⬇️
Impacted Files Coverage Δ
optimade/validator/validator.py 67.81% <60.97%> (+0.43%) ⬆️
optimade/validator/data/__init__.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 16917b2...6715bfc. Read the comment docs.

@ml-evs ml-evs force-pushed the ml-evs/optional_filter_tasks branch from 304241e to 7598a8b Compare March 13, 2020 19:44
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.

Good work, thanks @ml-evs !

I have some minor suggested changes.
It is generally a very practical code, which is fine by me. But the amount of if-else statements is increasing rapidly, which tells me there may be a better design for the whole thing. However, this is definitely for a later point, when we're all bored and want to do something "fun" 😅

optimade/validator/data/__init__.py Show resolved Hide resolved
optimade/validator/data/__init__.py Outdated Show resolved Hide resolved
optimade/validator/data/__init__.py Outdated Show resolved Hide resolved
optimade/validator/validator.py Show resolved Hide resolved
optimade/validator/validator.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
tests/server/test_server_validation.py Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/optional_filter_tasks branch from 7598a8b to ad5c62d Compare March 17, 2020 12:57
ml-evs and others added 7 commits March 17, 2020 13:05
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
@ml-evs ml-evs force-pushed the ml-evs/optional_filter_tasks branch from bb41c65 to 6715bfc Compare March 17, 2020 13:06
@ml-evs
Copy link
Member Author

ml-evs commented Mar 17, 2020

Good work, thanks @ml-evs !

I have some minor suggested changes.
It is generally a very practical code, which is fine by me. But the amount of if-else statements is increasing rapidly, which tells me there may be a better design for the whole thing. However, this is definitely for a later point, when we're all bored and want to do something "fun" sweat_smile

Yes, the important thing now to consider is validating the results properly (beyond checking that the response is in the right format)... this has been useful to catch some problems with the filterparser and transformer, but it's pretty meaningless if all we're doing his querying non-existent fields!

I'm not entirely sure how we should go about doing this. We could potentially just find one structure in the API and use that to construct a load of queries that should return at least that structure, and kind of trawl and validate that way. Hopefully this can be guided more by other implementations!

@ml-evs ml-evs requested a review from CasperWA March 17, 2020 13:10
@CasperWA
Copy link
Member

Yes, the important thing now to consider is validating the results properly (beyond checking that the response is in the right format)... this has been useful to catch some problems with the filterparser and transformer, but it's pretty meaningless if all we're doing his querying non-existent fields!

True. We need more real-world implementations as testbeds.
I think we can and should use providers.optimade.org as a testbed for the index meta-database, and then I am drawing a blank for the regular server. The best approach is probably still our own setup? Either running a server locally or querying heroku.

I'm not entirely sure how we should go about doing this. We could potentially just find one structure in the API and use that to construct a load of queries that should return at least that structure, and kind of trawl and validate that way. Hopefully this can be guided more by other implementations!

I don't think scraping the spec further will end in a fruitful result. Rather, we should either re-use some queries/model structures from the other OPTIMADE repository tests and/or use real-world implementations and/or have the consortium come up with a list of queries/structures that can be used. The latter would also solidify this validator as the "official" OPTIMADE implementation validator to be used.

I don't know if I misunderstood your comment in answering here, but I think my comments are still valid in their own right 😅

@ml-evs
Copy link
Member Author

ml-evs commented Mar 17, 2020

I think we can and should use providers.optimade.org as a testbed for the index meta-database, and then I am drawing a blank for the regular server. The best approach is probably still our own setup? Either running a server locally or querying heroku.

100% agree

I don't think scraping the spec further will end in a fruitful result. Rather, we should either re-use some queries/model structures from the other OPTIMADE repository tests and/or use real-world implementations and/or have the consortium come up with a list of queries/structures that can be used. The latter would also solidify this validator as the "official" OPTIMADE implementation validator to be used.

I don't know if I misunderstood your comment in answering here, but I think my comments are still valid in their own right sweat_smile

It's not the point I was making, but I do agree! In order to avoid getting people to put random data into their databases so that it can be queried, my suggestion was more to find any structure in the database (i.e. /structures?page_limit=1), then perform queries that should definitely return that structure, i.e. query for its ID, query for formula, query on its relationships etc. in order to check that the underlying mechanisms are working.

The spec scraping stuff is only really useful to check what the database says it has implemented, and what causes backend errors.

@ml-evs
Copy link
Member Author

ml-evs commented Mar 17, 2020

We should leave this to an issue discussing the future of the validator though :)

@CasperWA
Copy link
Member

(...) In order to avoid getting people to put random data into their databases so that it can be queried, my suggestion was more to find any structure in the database (i.e. /structures?page_limit=1), then perform queries that should definitely return that structure, i.e. query for its ID, query for formula, query on its relationships etc. in order to check that the underlying mechanisms are working.

Ah. It would be a good check, I guess, of the filter working. Especially if using NOT to check a structure is not returned. E.g., if an implementation just disregards the filter and returns all regardless of input.
However, it also seems incomplete and not completely rigorous in its testing method. I think we'll have to come up with a robust algorithm/logic for testing this.

I would even go so far as to say that it's the implementation's own responsibility to ensure their system works properly... However, I definitely see the benefit of having a central "official" tester to make sure it works generally as expected.

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.

Very good.
All of the txt files are already added to the MANIFEST in some way, right?

@ml-evs
Copy link
Member Author

ml-evs commented Mar 19, 2020

All of the txt files are already added to the MANIFEST in some way, right?

Yep!

@ml-evs ml-evs merged commit 098aa32 into master Mar 19, 2020
@ml-evs ml-evs deleted the ml-evs/optional_filter_tasks branch March 19, 2020 01:52
@ml-evs ml-evs added this to In progress in Road to optimade-python-tools 1.0 via automation Mar 31, 2020
@ml-evs ml-evs moved this from In progress to Done in Road to optimade-python-tools 1.0 Mar 31, 2020
@CasperWA CasperWA mentioned this pull request Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants