Introduce grammar v0.10.1 #112

CasperWA · 2019-12-12T16:52:44Z

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

I guess this needs some more additions as the OPTiMaDe spec is updated to v0.10.1.

IMPORTANT: This PR will up the package version to 0.3.0 and the OPTiMaDe version to 0.10.1.

codecov · 2019-12-12T16:54:23Z

Codecov Report

Merging #112 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #112   +/-   ##
=======================================
  Coverage   84.86%   84.86%           
=======================================
  Files          39       39           
  Lines        1817     1817           
=======================================
  Hits         1542     1542           
  Misses        275      275

Flag	Coverage Δ
#unittests	`84.86% <ø> (ø)`	⬆️

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 8303424...b377b7b. Read the comment docs.

ml-evs

This also looks good to me, are we expecting any further grammar changes before 0.10.1, or can we get this merged?

CasperWA · 2019-12-22T17:14:39Z

This also looks good to me, are we expecting any further grammar changes before 0.10.1, or can we get this merged?

Not at the moment - but since v0.10.1 is not officially out, there definitely may come some. But most likely not for the grammar - so this should be good to go @ml-evs once approved.

CasperWA · 2020-01-07T09:45:03Z

Newest commit should also address the fact that constant <op> constant is not allowed if both are strings (Materials-Consortia/OPTIMADE#229).

ml-evs · 2020-01-08T12:46:33Z

This PR (nearly) closes #86, it would be good to have this working in the interim and then we can target #98 in a future PR.

ml-evs

This LGTM, though I'm trusting the grammar tests to spot any issues in the grammar file itself!

optimade/filtertransformers/mongo.py

CasperWA · 2020-01-08T13:53:05Z

This PR (nearly) closes #86, it would be good to have this working in the interim and then we can target #98 in a future PR.

This PR doesn't really address the usage of the grammar, only the grammar itself.
So nothing should have really changed with respect to implementing LENGTH query (#86).

With respect to "the interrim" here, do you mean after or before merging this PR?

ml-evs · 2020-01-08T21:14:29Z

This PR doesn't really address the usage of the grammar, only the grammar itself.
So nothing should have really changed with respect to implementing LENGTH query (#86).

Misunderstanding: I thought length queries didn't work at all before this.

With respect to "the interrim" here, do you mean after or before merging this PR?

After!

CasperWA · 2020-01-08T22:21:55Z

This LGTM, though I'm trusting the grammar tests to spot any issues in the grammar file itself!

Right - I can see from the diff here that it's basically impossible to see the changes from v0.10.0.
So here's a short list:

Introduce non_string_value.
Use OPERATOR non_string_value instead of value_op_rhs for the value in constant_first_comparison.
Change predicate_comparison to length_op_rhs and set the value to the new LENGTH [ OPERATOR ] value.
Add length_op_rhs to property_first_comparison.
Remove predicate_comparison from expression_phrase.

CasperWA · 2020-01-08T22:26:19Z

The only issue I'm having, is how to clearly make a 501 Not Implemented response when constant <op> constant is made, where constant are strings?

We could probably do it in the transformer, but that's backend-specific, so one would have to always implement it for all transformers, which seems a bit weird. So I'd rather implement this in the grammar itself, in some way.

Also, is it fine if one of the constants is a string? This is the case now, as the first constant can be a string.

CasperWA · 2020-01-09T09:15:08Z

Since this PR updates the package version, I think I'm going to push and publish a tag to master immediately after merging this.

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

Co-Authored-By: Matthew Evans <7916000+ml-evs@users.noreply.github.com>

ml-evs

This is looking very close now @CasperWA!

As this constant <op> constant stuff is turning out to be quite fiddly, would you mind adding a couple of negative tests that show it raising the error? If it's something that has to be handled in the transformers, it would be good if the test is there and skipped even if its not working correctly as of this PR.

tests/filtertransformers/test_mongo.py

CasperWA · 2020-01-10T13:28:12Z

As this constant <op> constant stuff is turning out to be quite fiddly, would you mind adding a couple of negative tests that show it raising the error? If it's something that has to be handled in the transformers, it would be good if the test is there and skipped even if its not working correctly as of this PR.

Working on it now.

Could you possibly just push the changes you had in mind concerning using $where or something like that? Just don't touch the tests, since I am in the process of updating them.

EDIT: Reading the stackoverflow page @fekad has linked to in the transformer, it seems the intent was more to not use $where, but instead add information to an extra?

ml-evs · 2020-01-10T13:36:58Z

EDIT: Reading the stackoverflow page @fekad has linked to in the transformer, it seems the intent was more to not use $where, but instead add information to an extra?

Yeah it probably needs a bit more discussion on that issue (so I won't add anything to this PR), I'll write my thoughts there when I get a sec

There is no need to have the old MongoTransformer. The current MongoTransformer should work for the newest grammar always. Either that or one should create a new Transformer per grammar version - this may be desired. Start to convert previously skipped tests to run, using `assertRaises` for not yet implemented parts.

ml-evs · 2020-01-10T13:53:16Z

Also does this currently close #82?

CasperWA · 2020-01-10T14:01:31Z

Also does this currently close #82?

Technically, yes. But as stated in the issue, the Transformer should be further tested, so that we now it truly works. But it should... At least for constant OPERATOR non-string-value, e.g., "5 < nelements" will be turned into {"nelements": {"$gt": 5}}.

However, does not work (as far as I know) for property OPERATOR property, so there is no flip, it will always be from left to right as written, but that should still make sense, I guess?

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

optimade-python-tools/tests/filtertransformers/test_mongo.py

Line 7 in 5b4e813

version = (0, 10, 0)

Does this also need to be updated? Currently the mongo tests run against the old grammar.

A special grammar expression `not_implemented_string` is used to detect if strings are compared, while still making sure the Transformer can handle the error. This should be removed when comparisons of strings are properly documented in the specification.

This is for all "valid" filter statement that have not yet been implemented in the Transformer.

CasperWA · 2020-01-10T15:27:56Z

optimade-python-tools/tests/filtertransformers/test_mongo.py

Line 7 in 5b4e813

version = (0, 10, 0)

Does this also need to be updated? Currently the mongo tests run against the old grammar.

Yup, it does. And it has. Feel free to re-review :)

CasperWA · 2020-01-10T16:02:24Z

Also does this currently close #82?

Technically, yes. But as stated in the issue, the Transformer should be further tested, so that we now it truly works. But it should... At least for constant OPERATOR non-string-value, e.g., "5 < nelements" will be turned into {"nelements": {"$gt": 5}}.

However, does not work (as far as I know) for property OPERATOR property, so there is no flip, it will always be from left to right as written, but that should still make sense, I guess?

Thinking a bit more about this, I am not so sure this is true. There are several comparisons done in property_first_comparison that are not taken into account for constant_first_comparison that essentially "handles" RHS comparisons.

EDIT: Hmm... but then, reading the spec, it seems the RHS comparisons are only valid for logical operators, which is exactly what constant_first_comparison supports ... So it may work? But should be properly tested.

ml-evs · 2020-01-13T13:37:18Z

Thinking a bit more about this, I am not so sure this is true. There are several comparisons done in property_first_comparison that are not taken into account for constant_first_comparison that essentially "handles" RHS comparisons.

EDIT: Hmm... but then, reading the spec, it seems the RHS comparisons are only valid for logical operators, which is exactly what constant_first_comparison supports ... So it may work? But should be properly tested.

I think either way we should try to get this merged so we can have a chance to break it. There's still things to discuss in #86 and #98 for us to be completely compliant with 0.10.1 but this PR is both very large and very useful :)

I'm happy to accept this as is, and you can decide whether you want to add some further tests for RHS comparisons (and whether to close #82) before merging!

optimade/filterparser/lark_parser.py

CasperWA · 2020-01-14T11:08:27Z

Is it all right to merge this, publish it as version 0.3.0 and then perhaps start using a develop branch for future PRs?

ml-evs · 2020-01-14T15:19:41Z

Is it all right to merge this, publish it as version 0.3.0 and then perhaps start using a develop branch for future PRs?

Makes sense to me.

CasperWA added the grammar Concerns the Lark grammar files label Dec 12, 2019

CasperWA requested review from fekad and ml-evs December 12, 2019 16:52

ml-evs reviewed Dec 22, 2019

View reviewed changes

CasperWA requested a review from ml-evs December 23, 2019 21:42

ltalirz mentioned this pull request Dec 24, 2019

Checklist for OPTiMaDe v0.10.1 #29

Closed

16 tasks

CasperWA mentioned this pull request Jan 6, 2020

Minor changes to specification v0.10.1-develop #115

Closed

9 tasks

CasperWA force-pushed the update_grammar_v0.10.1 branch 2 times, most recently from d70e3e2 to ed7c985 Compare January 8, 2020 09:53

ml-evs reviewed Jan 8, 2020

View reviewed changes

optimade/filtertransformers/mongo.py Show resolved Hide resolved

CasperWA force-pushed the update_grammar_v0.10.1 branch from 50c16b9 to bdef029 Compare January 8, 2020 13:51

CasperWA force-pushed the update_grammar_v0.10.1 branch from 0c01508 to 9cd2929 Compare January 8, 2020 22:10

CasperWA force-pushed the update_grammar_v0.10.1 branch from 2099a19 to 4e7c0ed Compare January 9, 2020 10:50

CasperWA and others added 6 commits January 10, 2020 10:53

Introduce grammar v0.10.1

2407fc4

Update from v0.10.0: Implement Materials-Consortia/OPTIMADE#221

Update to v0.10.1 in config

a58cd95

Avoid string <op> string comparisons

7121b81

Up package version to 0.3.0 and OPTiMaDe to 0.10.1

2217e39

Add helpful exception message for length_op_rhs

22ff5c1

Co-Authored-By: Matthew Evans <7916000+ml-evs@users.noreply.github.com>

Adding a bit of tests to transformer and server

3cd55cc

CasperWA force-pushed the update_grammar_v0.10.1 branch from 4e0f3f0 to 3cd55cc Compare January 10, 2020 09:56

Merge branch 'master' into update_grammar_v0.10.1

9ef5b68

CasperWA requested review from ml-evs and ltalirz January 10, 2020 10:03

ml-evs requested changes Jan 10, 2020

View reviewed changes

ml-evs reviewed Jan 10, 2020

View reviewed changes

tests/filtertransformers/test_mongo.py Outdated Show resolved Hide resolved

Merge branch 'master' into update_grammar_v0.10.1

5b4e813

CasperWA added 3 commits January 10, 2020 15:44

Handle VisitError exceptions in servers

a563588

Make sure a 501 response occurs

71ab66c

This is for all "valid" filter statement that have not yet been implemented in the Transformer.

CasperWA requested a review from ml-evs January 10, 2020 15:57

Merge branch 'master' into update_grammar_v0.10.1

66e85c3

ml-evs reviewed Jan 13, 2020

View reviewed changes

optimade/filterparser/lark_parser.py Outdated Show resolved Hide resolved

Removed defunct comment

8303424

ml-evs approved these changes Jan 13, 2020

View reviewed changes

Merge branch 'master' into update_grammar_v0.10.1

b377b7b

CasperWA merged commit af87544 into master Jan 14, 2020

CasperWA deleted the update_grammar_v0.10.1 branch January 14, 2020 19:09

CasperWA mentioned this pull request Feb 5, 2020

Add more tests #104

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce grammar v0.10.1 #112

Introduce grammar v0.10.1 #112

CasperWA commented Dec 12, 2019 •

edited

codecov bot commented Dec 12, 2019 •

edited

ml-evs left a comment

CasperWA commented Dec 22, 2019 •

edited

CasperWA commented Jan 7, 2020

ml-evs commented Jan 8, 2020 •

edited

ml-evs left a comment

CasperWA commented Jan 8, 2020

ml-evs commented Jan 8, 2020

CasperWA commented Jan 8, 2020

CasperWA commented Jan 8, 2020

CasperWA commented Jan 9, 2020

ml-evs left a comment

CasperWA commented Jan 10, 2020 •

edited

ml-evs commented Jan 10, 2020

ml-evs commented Jan 10, 2020

CasperWA commented Jan 10, 2020 •

edited

ml-evs commented Jan 10, 2020

CasperWA commented Jan 10, 2020

CasperWA commented Jan 10, 2020 •

edited

ml-evs commented Jan 13, 2020 •

edited

CasperWA commented Jan 14, 2020

ml-evs commented Jan 14, 2020

Introduce grammar v0.10.1 #112

Introduce grammar v0.10.1 #112

Conversation

CasperWA commented Dec 12, 2019 • edited

codecov bot commented Dec 12, 2019 • edited

Codecov Report

ml-evs left a comment

Choose a reason for hiding this comment

CasperWA commented Dec 22, 2019 • edited

CasperWA commented Jan 7, 2020

ml-evs commented Jan 8, 2020 • edited

ml-evs left a comment

Choose a reason for hiding this comment

CasperWA commented Jan 8, 2020

ml-evs commented Jan 8, 2020

CasperWA commented Jan 8, 2020

CasperWA commented Jan 8, 2020

CasperWA commented Jan 9, 2020

ml-evs left a comment

Choose a reason for hiding this comment

CasperWA commented Jan 10, 2020 • edited

ml-evs commented Jan 10, 2020

ml-evs commented Jan 10, 2020

CasperWA commented Jan 10, 2020 • edited

ml-evs commented Jan 10, 2020

CasperWA commented Jan 10, 2020

CasperWA commented Jan 10, 2020 • edited

ml-evs commented Jan 13, 2020 • edited

CasperWA commented Jan 14, 2020

ml-evs commented Jan 14, 2020

CasperWA commented Dec 12, 2019 •

edited

codecov bot commented Dec 12, 2019 •

edited

CasperWA commented Dec 22, 2019 •

edited

ml-evs commented Jan 8, 2020 •

edited

CasperWA commented Jan 10, 2020 •

edited

CasperWA commented Jan 10, 2020 •

edited

CasperWA commented Jan 10, 2020 •

edited

ml-evs commented Jan 13, 2020 •

edited