search

MolSSI / QCEngine

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

Psi4 error reporting pre-empted by KeyError #266

Closed dotsdl closed 4 years ago

dotsdl commented 4 years ago

Description

Currently, useful error reporting with psi4 under the Psi4Harness can be lost due to a missing "success" key on output_data. This PR makes attempted access of this key return False if not present.

Credit to @dgasmith for pointing this out.

Changelog description

Error messages from Psi4Harness no longer swallowed by KeyError.

Status

codecov[bot] commented 4 years ago

Codecov Report

Merging #266 into master will decrease coverage by 0.02%. The diff coverage is 64.70%.

dotsdl commented 4 years ago

Looks like we're also being hit by the latest black update here.

loriab commented 4 years ago

The CI failure is for the new code, right?

On the black changes, I think let's try to merge some PRs, then do a separate one for formatting.

dotsdl commented 4 years ago

@loriab ah yes, you're right. It looks like my changes to the error message pathways mangled what is expected in those tests. I'll dive into these to find what's going awry.

dotsdl commented 4 years ago

Found the issue that was causing tests to fail; was an inconsistency in the error handling logic. Fixed and moved to its own function so we don't have the same delicate decision tree in two places.

Should be good to go if all tests pass on Travis!

dotsdl commented 4 years ago

@loriab this one is ready to go! Should improve our error reporting from psi4 failures substantially!

loriab commented 4 years ago

great, thanks, this was the PR I was keen to get in!