Entos: Added entos engine #82

sjrl · 2019-06-03T18:02:57Z

I've added entos as an engine. It includes all functions to be run by qcng.compute() and the ability to run entos.build_input(), entos.execute(), and entos.parse_output(). I've begun adding support for using Jinja templates so we can cover the different ways might want to provide custom template files.

(A) ordinary build_input (need to define a base template)
(B) user wants to add stuff after normal template (A)
(C) user knows their domain language (doesn't use any QCSchema quantities)

Right now it only supports option (C). Also right now the output parser does text parsing, but this will be removed eventually since entos plans to output all of their results to a xml or json format.

…ding gradients.

sjrl · 2019-06-03T18:12:15Z

Hmm probably should have realized jinja2 isn't part of the required dependencies for QCEngine. Does it make sense to add the dependency or should we wait on that for now?

loriab · 2019-06-03T19:34:20Z

jinja2 is pretty light (dep list below), so I think it's fine to have it as an optional dependency. Can model it on networkx in qcel. https://github.com/MolSSI/QCElemental/search?q=networkx&unscoped_q=networkx

  ca-certificates    pkgs/main/linux-64::ca-certificates-2019.1.23-0
  certifi            pkgs/main/linux-64::certifi-2019.3.9-py37_0
  jinja2             pkgs/main/linux-64::jinja2-2.10.1-py37_0
  libedit            pkgs/main/linux-64::libedit-3.1.20181209-hc058e9b_0
  libffi             pkgs/main/linux-64::libffi-3.2.1-hd88cf55_4
  libgcc-ng          pkgs/main/linux-64::libgcc-ng-8.2.0-hdf63c60_1
  libstdcxx-ng       pkgs/main/linux-64::libstdcxx-ng-8.2.0-hdf63c60_1
  markupsafe         pkgs/main/linux-64::markupsafe-1.1.1-py37h7b6447c_0
  ncurses            pkgs/main/linux-64::ncurses-6.1-he6710b0_1
  openssl            pkgs/main/linux-64::openssl-1.1.1c-h7b6447c_1
  pip                pkgs/main/linux-64::pip-19.1.1-py37_0
  python             pkgs/main/linux-64::python-3.7.3-h0371630_0
  readline           pkgs/main/linux-64::readline-7.0-h7b6447c_5
  setuptools         pkgs/main/linux-64::setuptools-41.0.1-py37_0
  sqlite             pkgs/main/linux-64::sqlite-3.28.0-h7b6447c_0
  tk                 pkgs/main/linux-64::tk-8.6.8-hbc83047_0
  wheel              pkgs/main/linux-64::wheel-0.33.4-py37_0
  xz                 pkgs/main/linux-64::xz-5.2.4-h14c3975_4
  zlib               pkgs/main/linux-64::zlib-1.2.11-h7b6447c_3

sjrl · 2019-06-03T21:11:36Z

Okay so I've begun adding jinja2 as an optional dependency. If you could check my last commit (e472591) to see if the logic I'm using makes sense I'd really appreciate it. Also it looks like should add something to the files setup.cfg, setup.py, and one of the yaml files in devtools/conda-envs/, but I'm not exactly sure how to follow the networkx example in this case.

loriab · 2019-06-03T21:22:21Z

You'll want to add jinja to whichever one of the devools/conda-envs/ you'd like to "test" the templating for. The setup* files can wait, as they don't actually get run for dependencies in the usual workflow.

sjrl · 2019-06-03T21:30:59Z

I'm not sure which one of the devtools/conda-envs to add to? Should I just add it to the psi4.yaml?

Lnaden · 2019-06-03T21:33:50Z

You could make a new one and add a new test in the .travis.yml

sjrl · 2019-06-03T21:36:12Z

I'd be happy to do that too. I know we potentially plan to expand the use of jinja2 to some of the other engines. Is it worth creating a new conda-env for that? Otherwise it seems like it would be pretty easy to sneak into the psi4.yaml.

…ck to test_entos.py)

codecov · 2019-06-03T21:47:46Z

Codecov Report

Merging #82 into master will decrease coverage by 2.61%.
The diff coverage is 72.72%.

dgasmith

Cool! Thanks for the start on this.

dgasmith · 2019-06-04T15:48:07Z

qcengine/programs/entos.py

+            # }
+
+            # Perform substitution to create input file
+            str_template = jinja2.Template(template)


Something I was curious about is if we truly need jinja here, it seems our template are simple enough that they can be handled via more canonical means like finding {{molecule}} fields. I think my original suggestion may be been slightly optimistic towards the range of capabilities we could code in.

I like the idea of having a template and then being able to do a dictionary substitute. I know the string.Template module offers some support of this, but it is very limited. I think we can provide the range of capabilities we discussed with jinja2, but it'll take some tinkering to be sure.

Absolutely, jinja will totally cover our use case. But the question is do we need it? string.Template provides fairly reaching functionality.

Or even writing our own using {{method}} with double bracket replacement.

I suppose what I didn't like about string.Template is that I couldn't find a way for it to tell me what fields are present which would then need to be filled. I'd like that functionality so we can be flexible in what information is required from the user in the input.json. It's possible that we could write our own but is it more trouble than it's worth? Is this mostly so we wouldn't need to have QCEngine be dependent on jinja2 for something that is probably fairly simple?

Pretty much, we try to keep the dependancies as small as possible for our lower level libraries and code small extra bits where needed.

Okay so what is the way forward on this? @dgasmith I can switch over to using string.Template for now to keep the functionality I've added and to remove the jinja dependency, but I don't think this PR should include writing our own version of templating.

I would vote to delay the jinja integration unless absolutely necessary. Lets work through this in the slack channel to figure out what we need.

qcengine/programs/entos.py

dgasmith · 2019-06-04T15:54:30Z

qcengine/programs/entos.py

+                properties["scf_dipole_moment"] = [float(x) for x in fields[2:5]]
+            elif fields[:3] == ["SCF", "converged", "in"]:
+                properties["scf_iterations"] = int(fields[3])
+            elif fields == ["Gradient", "(hartree/bohr):"]:


Since Entos is new, I don't suppose we could convince them to drop JSON output or even CSV?

They are in the process of writing their output in XML and/or JSON. It isn't currently done, but once it is I'll update the output parser.

dgasmith

Overall this LGTM. If we can pop out the jinja stuff I think we should be ready to go.

dgasmith · 2019-06-05T17:49:38Z

qcengine/programs/entos.py

+            result = self.parse_output(proc["outfiles"], input_data)
+            return result
+
+    def execute(self, inputs, extra_outfiles=None, extra_commands=None, scratch_name=None, timeout=None):


Can you add typing info here from the base harness?

There is no typing info currently for the execute function from the base harness.

lgtm-com · 2019-06-05T19:51:29Z

This pull request introduces 1 alert when merging d9a2fea into b8fbf02 - view on LGTM.com

new alerts:

1 for Unused import

dgasmith

LGTM

dgasmith · 2019-06-06T17:33:29Z

Is this ready to go or are there other changes desired?

sjrl · 2019-06-06T17:41:55Z

This should be all good to go!

dgasmith · 2019-06-06T17:47:04Z

In we go then!

sjrl added 8 commits June 3, 2019 10:44

entos: Beginning to add in entos engine

b18a215

entos: Setting up minimal working version of the output parser for entos

94c6ab2

Entos: Starting to write input file generator

87f7be6

Entos: Finished basic input file generator for entos

9b32cd7

Entos: Updated build_input. Also updated output parser to support rea…

5b83fb8

…ding gradients.

Entos: beginning to add template functionality

cbc9942

Entos: Added execute function

d57df49

Entos: Starting to add jinja templates and updating execute function.

9de328c

sjrl force-pushed the entos branch from e472591 to 5158945 Compare June 3, 2019 21:33

sjrl force-pushed the entos branch from 5158945 to e19fa12 Compare June 3, 2019 21:40

Entos: Making jinja2 an optional dependency (also added in proper che…

5671f2b

…ck to test_entos.py)

sjrl force-pushed the entos branch from e19fa12 to 5671f2b Compare June 3, 2019 21:47

dgasmith reviewed Jun 4, 2019

View reviewed changes

Entos: PR updates

644f878

dgasmith approved these changes Jun 5, 2019

View reviewed changes

Entos: Removed jinja2 dependency

734541b

sjrl force-pushed the entos branch from d9a2fea to 734541b Compare June 5, 2019 19:52

dgasmith approved these changes Jun 6, 2019

View reviewed changes

dgasmith merged commit f76e107 into MolSSI:master Jun 6, 2019

sjrl deleted the entos branch June 6, 2019 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entos: Added entos engine #82

Entos: Added entos engine #82

sjrl commented Jun 3, 2019

sjrl commented Jun 3, 2019

loriab commented Jun 3, 2019

sjrl commented Jun 3, 2019

loriab commented Jun 3, 2019

sjrl commented Jun 3, 2019

Lnaden commented Jun 3, 2019

sjrl commented Jun 3, 2019

codecov bot commented Jun 3, 2019 •

edited

Loading

dgasmith left a comment

dgasmith Jun 4, 2019

sjrl Jun 4, 2019

dgasmith Jun 4, 2019

sjrl Jun 4, 2019

dgasmith Jun 4, 2019

sjrl Jun 4, 2019

dgasmith Jun 4, 2019

dgasmith Jun 4, 2019

sjrl Jun 4, 2019

dgasmith left a comment

dgasmith Jun 5, 2019

sjrl Jun 5, 2019

lgtm-com bot commented Jun 5, 2019

dgasmith left a comment

dgasmith commented Jun 6, 2019

sjrl commented Jun 6, 2019

dgasmith commented Jun 6, 2019

Entos: Added entos engine #82

Entos: Added entos engine #82

Conversation

sjrl commented Jun 3, 2019

sjrl commented Jun 3, 2019

loriab commented Jun 3, 2019

sjrl commented Jun 3, 2019

loriab commented Jun 3, 2019

sjrl commented Jun 3, 2019

Lnaden commented Jun 3, 2019

sjrl commented Jun 3, 2019

codecov bot commented Jun 3, 2019 • edited Loading

Codecov Report

dgasmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgasmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 5, 2019

dgasmith left a comment

Choose a reason for hiding this comment

dgasmith commented Jun 6, 2019

sjrl commented Jun 6, 2019

dgasmith commented Jun 6, 2019

codecov bot commented Jun 3, 2019 •

edited

Loading