search

MolSSI / QCEngine

Quantum chemistry program executor and IO standardizer (QCSchema).
https://molssi.github.io/QCEngine/
BSD 3-Clause "New" or "Revised" License
163 stars 79 forks source link

Added basic harness for TeraChem Protocol Buffer Server mode. #289

Closed coltonbh closed 3 years ago

coltonbh commented 3 years ago

Description

Added a basic ProgramHarness for TeraChem's "server mode".

Changelog description

Added a basic ProgramHarness for TeraChem's "server mode".

Status

Given this is my first PR please let me know what else your team would like to see! I'm trying to get an MVP version up and working so the code you see is rather minimal. The underlying library for communicating with the server--tcpb--needs a little love. I'll be patching it up to have better error handling over the coming weeks and intend to expand this ProgramHarness to reflect these changes once made.

Let me know what else your team would need! I'm happy to leave this PR open until a more robust harness is built (a couple of weeks, most likely), but I wanted to get eyes on as soon as possible. Thanks!

coltonbh commented 3 years ago

Closes #284

Thanks, @loriab for your comments on making this PR. Please let me know what else is best practice for QCEngine. Thanks!

codecov[bot] commented 3 years ago

Codecov Report

Merging #289 (5bddf80) into master (43639f2) will increase coverage by 0.15%. The diff coverage is 50.81%.

coltonbh commented 3 years ago

@loriab thanks for the review. I'll get to this a bit later this week (have a busy week!). Good to know what basic features you want tested and functional.

coltonbh commented 3 years ago

Added (simple) hf test!

coltonbh commented 3 years ago

Hi @loriab where do we stand on this? I think I've made all changes requested :) Just checking in so this doesn't get back-burnered indefinitely. Thanks for helping to shepherd this along!

coltonbh commented 3 years ago

@loriab fixed! I believe I was following the "???" specification used in https://github.com/MolSSI/QCEngine/blob/7ca5ed0011f356d05a0951370fb9acb407f4558b/qcengine/cli.py#L89 and https://github.com/MolSSI/QCEngine/blob/7ca5ed0011f356d05a0951370fb9acb407f4558b/qcengine/cli.py#L109. Looks like just returning "None" is the better option and I can let the cli fill in the "???" when None is returned.

I had tried the higher level methods that you suggest from the dftd4.py's get_version method. However, they failed a few edge cases since it's possible tcpb exists but the version is too old to have the attribute etc... Can't remember exactly what all the edge cases were 😬 but code as written correctly produced the correct behavior under all cases. I was finding it quite hard to mock out modules with/without attributes for tests so I just punted on putting those in tests since most other harnesses seemed to lack tests for their get_version method and the juice didn't seem worth the squeeze.

That said, the code as written works under all cases and I recall running into issues with the which_program and safe_version higher-level methods. I'd prefer to leave it (with the "???" removed), but can go over it again with the higher level methods to see what breaks again if you'd like me to dig deeper on this.

Let me know! Or if all looks good you can take it from here :)

Tests pass locally!

coltonbh commented 3 years ago

WOOT WOOT!!! 🥳

coltonbh commented 3 years ago

closes #284