Waterdynamics.HydrogenBondLifetimes nprocs > 1 appears to be broken

[IAlibay]

Expected behavior

Passing nprocs > 1, should call the multiprocessing portion of HydrogenBondLifetimes and return normally.

Actual behavior

The code fails with an import error for sys:

ts= 1
ts= 2
/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/base.py:116: DeprecationWarning: Setting the following kwargs should be done in the run() method: start, stop
  DeprecationWarning)
/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/base.py:116: DeprecationWarning: Setting the following kwargs should be done in the run() method: start, stop
  DeprecationWarning)
/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/hbonds/hbond_analysis.py:650: SelectionWarning: No acceptors found in selection 2. You might have to specify a custom 'acceptors' keyword. Selection will update so continuing with fingers crossed.
  warnings.warn(errmsg, category=SelectionWarning)
/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/hbonds/hbond_analysis.py:650: SelectionWarning: No donors found in selection 2. You might have to specify a custom 'donors' keyword. Selection will update so continuing with fingers crossed.
  warnings.warn(errmsg, category=SelectionWarning)
error
trying again
Process Process-1:
/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/hbonds/hbond_analysis.py:650: SelectionWarning: No acceptors found in selection 2. You might have to specify a custom 'acceptors' keyword. Selection will update so continuing with fingers crossed.
  warnings.warn(errmsg, category=SelectionWarning)
/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/hbonds/hbond_analysis.py:650: SelectionWarning: No donors found in selection 2. You might have to specify a custom 'donors' keyword. Selection will update so continuing with fingers crossed.
  warnings.warn(errmsg, category=SelectionWarning)
Process Process-2:
Traceback (most recent call last):
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/waterdynamics.py", line 659, in _HBA
    sys.stdout.flush()
NameError: name 'sys' is not defined
Traceback (most recent call last):
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/waterdynamics.py", line 653, in _HBA
    h.run(verbose=verbose)
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/hbonds/hbond_analysis.py", line 936, in run
    verbose=kwargs.get('verbose', False))
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/lib/log.py", line 350, in __init__
    assert numsteps > 0, "numsteps step must be >0"
AssertionError: numsteps step must be >0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "/biggin/b131/bioc1523/software/anaconda/python3.6/2019.7/envs/all/lib/python3.7/site-packages/MDAnalysis/analysis/waterdynamics.py", line 658, in _HBA
    sys.stdout.flush()
NameError: name 'sys' is not defined

Fixing the code to import sys leads to an infinite loop due to a repeated attempt to re-excute the failing code block:

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Lines 651 to 661 in 0dede25

	while True:
	try:
	h.run(verbose=verbose)
	break
	except:
	print("error")
	print("trying again")
	sys.stdout.flush()
	sys.stdout.flush()
	conn.send(h.timeseries[0])
	conn.close()

Some extra comments

So I went back through the git history, and the removal of sys occured in the very first PR #300.

It is therefore not clear to me that this code has ever worked. That being said, it is very much possible that I'm not calling this in the right manner.

Code to reproduce the behavior

import MDAnalysis as mda
from MDAnalysis.tests.datafiles import waterPSF, waterDCD
from MDAnalysis.analysis import waterdynamics

SELECTION1 = "byres name OH2"
SELECTION2 = "byres name P1" 

universe = mda.Universe(waterPSF, waterDCD)

# Calling this without nproc works and is covered by tests
hbl = waterdynamics.HydrogenBondLifetimes(universe, SELECTION1, SELECTION2, 0, 10, 2, nproc=2)

hbl.run()

Currently version of MDAnalysis

• Which version are you using? (run python -c "import MDAnalysis as mda; print(mda.__version__)") 0.20.1 (tested on latest develop too)
• Which version of Python (python -V)? 3.7.3
• Which operating system? Ubuntu 18.04.3 LTS

[IAlibay]

So it looks like the root cause of the error is related to the values passed to start and stop in:

mdanalysis/package/MDAnalysis/analysis/waterdynamics.py

Lines 644 to 650 in 0dede25

	h = MDAnalysis.analysis.hbonds.HydrogenBondAnalysis(universe,
	finalGetResidue1,
	finalGetResidue2,
	distance=3.5,
	angle=120.0,
	start=frame - 1,
	stop=frame)

Since frame is being set to ts.frame, on the first call start is being set to -1, and start to 1, which seems to be causing this failure:

Traceback (most recent call last):
  File "/home/mcria/software/python/anaconda3/envs/mda-dev/lib/python3.7/multiprocessing/process.py", line 297, in _bootstrap
    self.run()
  File "/home/mcria/software/python/anaconda3/envs/mda-dev/lib/python3.7/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "/home/mcria/Github/mdanalysis/package/MDAnalysis/analysis/waterdynamics.py", line 653, in _HBA
    h.run()
  File "/home/mcria/Github/mdanalysis/package/MDAnalysis/analysis/hbonds/hbond_analysis.py", line 957, in run
    verbose=kwargs.get('verbose', False))
  File "/home/mcria/Github/mdanalysis/package/MDAnalysis/lib/log.py", line 350, in __init__
    assert numsteps > 0, "numsteps step must be >0"
AssertionError: numsteps step must be >0

Setting start to frame and stop to frame + 1 (which I think makes sense since stop should be non-exclusive) seems to allow the function call to complete. That being said, whilst it can now be used to replicate the following tests from test_waterdynamics.py:

mdanalysis/testsuite/MDAnalysisTests/analysis/test_waterdynamics.py

Lines 45 to 49 in 0dede25

	def test_HydrogenBondLifetimes(universe):
	hbl = waterdynamics.HydrogenBondLifetimes(
	universe, SELECTION1, SELECTION1, 0, 5, 3)
	hbl.run()
	assert_almost_equal(hbl.timeseries[2][1], 0.75, 5)

I am not sure if this code is working as intended (this type of multiprocessing call with the use of while True to continually re-submit the inner function call isn't something I'm versed in).

[orbeckst]

I recently saw this while True loop and really was thinking "infinite loops should not be part of the code base...".

Maybe one of the original authors ( @alejob ?) can comment here? Otherwise we can also decide to remove the multiprocessing functionality. Are there any tests for it?

[orbeckst]

Let me be a bit more explicit so that we can move forward: unless @alejob has a quick fix + tests that also work with the desired convention of start=0 inclusive and stop exclusive in the next two days, please remove the multiprocessing nproc > 1 functionality.