search

MolSSI / QCEngine

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

Some Turbomole fixes #405

Closed cvsik closed 11 months ago

cvsik commented 1 year ago

Description

This PR incorporates some fixes/additions for the Turbomole runner:

@loriab I'm actually @maxscheurer but from my corporate GH account 😄

Questions

Changelog description

Status

codecov[bot] commented 1 year ago

Codecov Report

Merging #405 (a08832d) into master (bededd8) will decrease coverage by 0.04%. The diff coverage is 40.00%.

Additional details and impacted files
loriab commented 11 months ago

Is the new test okay using xfail?

sure, that's fine

How should one allow a user-specified scratch directory? Is TaskConfig -> execute the correct route?

Yes, that's it. User-side here: https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/tests/test_canonical_config.py#L160-L161 . Code side here (first and last lines): https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/cfour/runner.py#L85-L154 . The former is a something of a standard test you could add turbomole to sometime.

I changed the (unusual?) jobs_per_node default from 2 to 1, is that okay, or was there a reason for it being set to 2 specifically?

I'm leery of that change. Was it causing trouble as 2? I don't know why it's not 1 (admittedly more usual) -- maybe 2 avoided only serial testing?

make format reformats almost all files...

I've squashed that now :-)

Anyways, all lgtm. After 414 is in and tests can pass, I can adapt this PR. Main question is jobs_per_node, I guess. Thanks!

cvsik commented 11 months ago

I'm leery of that change. Was it causing trouble as 2? I don't know why it's not 1 (admittedly more usual) -- maybe 2 avoided only serial testing?

IIRC if you don't specify anything, then this divides the number of cores for the job by 2, which I think is a bit unexpected (because initially you don't know why your program only uses half of the available cores) 😄

loriab commented 11 months ago

The jobs_per_node traces back to here https://github.com/dgasmith/QCEngine/commit/41e7da039102195a5f17e5526a485f5cdefc6bff . I still don't see why it's 2, unless just to have something multiprocess to test against.

Changing to 1 causes a gamess test to fail, but not for a good reason. Still investigating ...

EDIT: the OpenMM error has been a fluke before, but it seems a little stickier today.

loriab commented 11 months ago

Ok, see #415. I think the jobs_per_node should be reset to 1, but not in isolation. Do you agree? If so, you or I can remove it, then we'll get this one in, and maybe openmm will cooperate in CI.