Repository navigation
Fix: OPS hangs when external engine errors/exits unexpectedly - #913
Conversation
Problem: I was using GROMACS and OPS couldn't tell when GROMACS had exited due to errors. Reason: External engine get's checked if it's still running using subprocess.Popen.process.is_running() call. This doesn't actually communicate with the process to ask if it's still running, it returns the value corresponding to last time it was checked. In this case, when the process was set up, so case of not running never gets called. Solution: Altered to use subprocess.Popen.process.poll() - return None is process still running, and the return code if it isn't. I've also altered the stop signal to copy with the process already having exited of it's own accord
dwhswenson
left a comment
dwhswenson
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
I had seen this error before (when bad Gromacs input was given) but I hadn't tracked down the problem. Based on your solution, it looks like the issue is that when external engines crash due to an error, they remain in zombie status. From the docs for psutil, is_running will return True if the process is a zombie.
I expect I thought that the advantage of using is_running over poll was that is_running will give the correct value even if the process id has been reassigned to a new process. However, if the pid is held by a zombie, I don't think this will be a concern (if in the future we find that this is a problem, the if criteria could use both conditions).
The one thing that is missing here is an addition to unit tests. This would basically be a test using the slow test engine where you start the engine, manually send it a killsig, and expect that getting the next frame will raise the RuntimeError. @oliverdutton : let me know whether you want to add that or would prefer me to. (I don't know how much experience you have writing tests with pytest.) The relevant test suite is in openpathsampling/tests/test_external_engine.py.
(Also, I'll change the name of the PR -- that's because the PR title shows up in our release notes, so I'll change it to express what I think users need to know.)
dwhswenson
changed the title
|
Ah, that explains it. I wasn't entirely sure what .is_running was doing. Zombies explain it. I don't have lots of experience with unittests so I'll leave it to you if that's okay. |
…e not aligned. int added when choosing frame as half the len of trajectory current snapshot only set if trajectory has length (not death on creation)
…g from mdp may follow)
… be put on master
|
Sorry for the delay on this; I meant to come back and add tests myself. Life got busy. Then it looks like a few accidental commits got added to this branch, and I was going to try to figure out how to deal with them. Life got busier. Regarding those extra commits:
|
|
Wonderful. |
You mean the mdp parsing I guess? This should be easy to rebase/merge, I didn't touch the write_frame function :)
|
Yep; glad it doesn't cause any headaches!
Good point; completely reasonable to move that into the
@oliverdutton : Do you recall the reason for this? I had assumed it might be something weird with |
Problem: I was using GROMACS and OPS couldn't tell when GROMACS had exited due to errors.
Reason: External engine get's checked if it's still running using subprocess.Popen.process.is_running() call. This doesn't actually communicate with the process to ask if it's still running, it returns the value corresponding to last time it was checked. In this case, when the process was set up, so case of not running never gets called.
Solution: Altered to use subprocess.Popen.process.poll() - return None is process still running, and the return code if it isn't.
I've also altered the stop signal to copy with the process already having exited of it's own accord