Repository navigation
gromacs engine broken #942
Description
The gromacs engine in the version on the current master is broken. Tests pass, however running the example notebooks does not work. The generated trial trajectories are continously overwritten. The reason is that using the default FileNameSetter (the one which uses numbers) is reset to the current number every time the engine.set_filenames() method is called. See this line.
Using the RandomStringFilenames instead of the standard FileNameSetter it is possible to work around the issue and run the example notebooks.
I can (and will) submit a PR with a fix, however I need your input first as I am not sure what the desired behaviour is. If I am not mistaken there are 3 possible solutions:
- change the reset to use given number +1 (results in this test failing)
- change the FileNameSetter to return the incremented value (instead of incrementing and returning the pre-increment value); essentially the same as above
- Remove the reset completely (this way engines could start with a high trajectory number and setting the filename would have no effect for future names, but tests pass)
I tend to solution 1, changing the test slightly to pass (set the filname to -1 instead of 0).
As a side note: I think it would be good to include the -nt 1 flag in the mdrun_args for the tests, i.e. forcing it to run with one thread (which is also the number of domains the system will be split into). This prevents gromacs crashing (and the test in turn never finishing) on machines with many cores, since gromacs is naturaly not able to find a domain decomposition that splits the small test system into enough parts. I will add this to the PR fixing this issue in case you agree.
Activity
dwhswenson commentedon Nov 17, 2020
I remember that test coming up when I was working on #933. I thought I'd found a way around it, but apparently not.
(Okay, a bunch of typing deleted, because after a lot of thought I found a solution. This one was complicated.)
Basic issue: changing the tests is ONLY an option if you can identify a bug in the test. Tests are the API, so we can't break that contract in a minor release. And most of
TestGromacsEngine.test_generatedates back 4 years, so it is an old test.Part of that test is that it wants to set the internal state of the file counter, and ensure, based on that internal state, that we create a file with the correct filename. That's where you're having trouble.
Fortunately, part of this can be identified as a bug. The test uses
set_filenamesto set the internal state. While that worked before #933, now the internal state has moved. It should setself.engine.filename_setter.count = 0. Thenengine.generatecallsengine.start, which callsengine.filename_setter, incrementing the number before passing it along toengine.set_filenames.At that point, I think the
reset()call inset_filenamescan be removed. My bet is that I added it there to deal with this unit test. (That bit of code was written in a bit of a hurry with a deadline looming, so I probably didn't think it through all the way.)With some luck, that'll get everything working again.
Merge pull request #946 from hejung/fix_gromacs_engine