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

0.10.0 grammer, elasticsearch transformer, setuptools extra #63

Merged
merged 12 commits into from Oct 25, 2019

Conversation

markus1978
Copy link
Contributor

Sorry for doing multiple things at the same time. But it is all kinda connected.

This PR contains:

  • 0.10.0 grammer file
  • elasticsearch transformer (only for 0.10.0)
  • an 'extra' server (as in pip install optimade[server]) that omits all server/transformer related requirements

Unfortunately, I had to break compatibility with older grammars. The old grammars put AND and OR on the same precedence in contradiction to the spec. The necessary new grammar rules will break transformers on the old grammars.

The tests do not really run the queries on elasticsearch, they only parse the example queries and transform them. I don't have the time to get into the necessary travis ci/elasticsearch setup. Unfortunately, there is no easy mongomock kinda thing for elasticsearch.

@codecov
Copy link

codecov bot commented Sep 27, 2019

Codecov Report

Merging #63 into master will increase coverage by 1.88%.
The diff coverage is 87.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   78.17%   80.06%   +1.88%     
==========================================
  Files          21       23       +2     
  Lines         724      908     +184     
==========================================
+ Hits          566      727     +161     
- Misses        158      181      +23
Impacted Files Coverage Δ
...imade/filtertransformers/tests/test_transformer.py 100% <100%> (ø) ⬆️
...ade/filtertransformers/tests/test_elasticsearch.py 84.61% <84.61%> (ø)
optimade/filtertransformers/elasticsearch.py 87.97% <87.97%> (ø)

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 5e5daf0...a6045a9. Read the comment docs.

@markus1978
Copy link
Contributor Author

I missed a dependency on the first try.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thanks for this @markus1978! Similar to my review of the Django transformer, I've never used ElasticSearch so you're much better equipped to assess your own work, but again this looks really useful. Summary of my comments:

  1. Could you contribute the grammar to the v0.10 PR [WIP] Adding grammar for v0.9.8 #52 please?
  2. Could we divide the requirements up further into each backend?
  3. Do we really need to depend on ASE?

requirements.txt Outdated Show resolved Hide resolved
optimade/grammar/v0.10.0.g Outdated Show resolved Hide resolved
@markus1978
Copy link
Contributor Author

@ml-evs let me respond to your 3 summary points:

for 3. I am just using the ase.data.chemical_symbols list. I could simply copy it, I guess it is a pretty stable piece of information.

for 2. Can do

for 1. This is the big one. I started with the newest grammar in #52 (0.9.8), but it was not working for me. I guess it is mixing various lark versions? Anyhow, I could not resolve the issues in it. But, the bigger problem is that Transformers are tied to the grammar rules. In a way, you can't really design them independently. I happy to commit my grammar to other places, but I don't really want to adapt the Transformer to another grammar. I think, this will also become an issue for the other transformers, especially when the grammar becomes more and more complex. For example, I tried to make the grammar compatible with the older one, but simply could not find a way to do it. Maybe this even gets to the point, where you have multiple grammars, each per Transformer?

@ml-evs
Copy link
Member

ml-evs commented Oct 23, 2019

Hi @markus1978, thanks for your comments and sorry for the delay getting back to you. I've just made a PR #64 for point 2 that splits requirements up per backend (please feel free to review...). Once that's merged I can rebase your PR and switch the requirements around a bit.

Regarding 3. you're clearly correct that we need to have backend/transformer-dependent grammars. I'm happy to accept this PR if you make it clear that the grammar you've written is intended for the elasticsearch transformer, then hopefully we can figure out a more general solution at a later date.

@markus1978
Copy link
Contributor Author

@ml-evs , I changed the grammar file name to v.0.10.0.elastic.g. This way, the grammar is still available via version=(0, 10, 0). Let me know, if you had something different in mind.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes @markus1978! Happy to accept.

@ml-evs ml-evs merged commit 18f79cf into Materials-Consortia:master Oct 25, 2019
@markus1978 markus1978 deleted the master branch June 11, 2020 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants