Merge pull request #2203 from oesteban/fix/VirtualDisplay

ENH: Centralize virtual/physical $DISPLAYs

Author: satra

CHANGES

@@ -1,6 +1,9 @@
 Upcoming release
 ================
 
+* ENH: Centralize virtual/physical $DISPLAYs (https://github.com/nipy/nipype/pull/#2203)
+* ENH: New ResourceMonitor - replaces resource profiler (https://github.com/nipy/nipype/pull/#2200)
+
 
 0.13.1 (May 20, 2017)
 =====================

doc/users/config_file.rst

@@ -73,13 +73,16 @@ Execution
     ``false``; default value: ``true``)
 
 *display_variable*
-    What ``DISPLAY`` variable should all command line interfaces be
-    run with. This is useful if you are using `xnest
-    <http://www.x.org/archive/X11R7.5/doc/man/man1/Xnest.1.html>`_
-    or `Xvfb <http://www.x.org/archive/X11R6.8.1/doc/Xvfb.1.html>`_
-    and you would like to redirect all spawned windows to
-    it. (possible values: any X server address; default value: not
-    set)
+	Override the ``$DISPLAY`` environment variable for interfaces that require
+    an X server. This option is useful if there is a running X server, but 
+    ``$DISPLAY`` was not defined in nipype's environment. For example, if an X 
+    server is listening on the default port of 6000, set ``display_variable = :0``
+    to enable nipype interfaces to use it. It may also point to displays provided 
+    by VNC, `xnest <http://www.x.org/archive/X11R7.5/doc/man/man1/Xnest.1.html>`_ 
+    or `Xvfb <http://www.x.org/archive/X11R6.8.1/doc/Xvfb.1.html>`_.
+    If neither ``display_variable`` nor the ``$DISPLAY`` environment variable are
+    set, nipype will try to configure a new virtual server using Xvfb.
+    (possible values: any X server address; default value: not set)
 
 *remove_unnecessary_outputs*
     This will remove any interface outputs not needed by the workflow. If the

nipype/interfaces/afni/preprocess.py

@@ -2154,11 +2154,12 @@ class SkullStrip(AFNICommand):
 
     def __init__(self, **inputs):
         super(SkullStrip, self).__init__(**inputs)
+
         if not no_afni():
             v = Info.version()
 
-            # As of AFNI 16.0.00, redirect_x is not needed
-            if v[0] > 2015:
+            # Between AFNI 16.0.00 and 16.2.07, redirect_x is not needed
+            if v >= (2016, 0, 0) and v < (2016, 2, 7):
                 self._redirect_x = False
 
 

nipype/interfaces/base.py

@@ -54,13 +54,14 @@
 
 
 class Str(traits.Unicode):
-    pass
-
+    """Replacement for the default traits.Str based in bytes"""
 
 traits.Str = Str
 
 
 class NipypeInterfaceError(Exception):
+    """Custom error for interfaces"""
+
     def __init__(self, value):
         self.value = value
 
@@ -1014,31 +1015,6 @@ def _check_version_requirements(self, trait_object, raise_exception=True):
                                          version, max_ver))
         return unavailable_traits
 
-    def _run_wrapper(self, runtime):
-        if self._redirect_x:
-            try:
-                from xvfbwrapper import Xvfb
-            except ImportError:
-                iflogger.error('Xvfb wrapper could not be imported')
-                raise
-
-            vdisp = Xvfb(nolisten='tcp')
-            vdisp.start()
-            try:
-                vdisp_num = vdisp.new_display
-            except AttributeError:  # outdated version of xvfbwrapper
-                vdisp_num = vdisp.vdisplay_num
-
-            iflogger.info('Redirecting X to :%d' % vdisp_num)
-            runtime.environ['DISPLAY'] = ':%d' % vdisp_num
-
-        runtime = self._run_interface(runtime)
-
-        if self._redirect_x:
-            vdisp.stop()
-
-        return runtime
-
     def _run_interface(self, runtime):
         """ Core function that executes interface
         """
@@ -1080,6 +1056,9 @@ def run(self, **inputs):
         store_provenance = str2bool(config.get(
             'execution', 'write_provenance', 'false'))
         env = deepcopy(dict(os.environ))
+        if self._redirect_x:
+            env['DISPLAY'] = config.get_display()
+
         runtime = Bunch(cwd=os.getcwd(),
                         returncode=None,
                         duration=None,
@@ -1102,8 +1081,9 @@ def run(self, **inputs):
         # Grab inputs now, as they should not change during execution
         inputs = self.inputs.get_traitsfree()
         outputs = None
+
         try:
-            runtime = self._run_wrapper(runtime)
+            runtime = self._run_interface(runtime)
             outputs = self.aggregate_outputs(runtime)
         except Exception as e:
             import traceback
@@ -1313,21 +1293,14 @@ def _canonicalize_env(env):
     return out_env
 
 
-def run_command(runtime, output=None, timeout=0.01, redirect_x=False):
+def run_command(runtime, output=None, timeout=0.01):
     """Run a command, read stdout and stderr, prefix with timestamp.
 
     The returned runtime contains a merged stdout+stderr log with timestamps
     """
 
     # Init variables
     cmdline = runtime.cmdline
-
-    if redirect_x:
-        exist_xvfb, _ = _exists_in_path('xvfb-run', runtime.environ)
-        if not exist_xvfb:
-            raise RuntimeError('Xvfb was not found, X redirection aborted')
-        cmdline = 'xvfb-run -a ' + cmdline
-
     env = _canonicalize_env(runtime.environ)
 
     default_encoding = locale.getdefaultlocale()[1]
@@ -1564,17 +1537,7 @@ def help(cls, returnhelp=False):
             print(allhelp)
 
     def _get_environ(self):
-        out_environ = {}
-        if not self._redirect_x:
-            try:
-                display_var = config.get('execution', 'display_variable')
-                out_environ = {'DISPLAY': display_var}
-            except NoOptionError:
-                pass
-        iflogger.debug(out_environ)
-        if isdefined(self.inputs.environ):
-            out_environ.update(self.inputs.environ)
-        return out_environ
+        return getattr(self.inputs, 'environ', {})
 
     def version_from_command(self, flag='-v'):
         cmdname = self.cmd.split()[0]
@@ -1591,10 +1554,6 @@ def version_from_command(self, flag='-v'):
             o, e = proc.communicate()
             return o
 
-    def _run_wrapper(self, runtime):
-        runtime = self._run_interface(runtime)
-        return runtime
-
     def _run_interface(self, runtime, correct_return_codes=(0,)):
         """Execute command via subprocess
 
@@ -1622,8 +1581,7 @@ def _run_interface(self, runtime, correct_return_codes=(0,)):
         setattr(runtime, 'command_path', cmd_path)
         setattr(runtime, 'dependencies', get_dependencies(executable_name,
                                                           runtime.environ))
-        runtime = run_command(runtime, output=self.inputs.terminal_output,
-                              redirect_x=self._redirect_x)
+        runtime = run_command(runtime, output=self.inputs.terminal_output)
         if runtime.returncode is None or \
                 runtime.returncode not in correct_return_codes:
             self.raise_exception(runtime)

nipype/interfaces/tests/test_base.py

@@ -639,25 +639,43 @@ def _gen_filename(self, name):
     nib.CommandLine.input_spec = nib.CommandLineInputSpec
 
 
-def test_Commandline_environ():
+def test_Commandline_environ(monkeypatch, tmpdir):
     from nipype import config
     config.set_default_config()
+
+    tmpdir.chdir()
+    monkeypatch.setitem(os.environ, 'DISPLAY', ':1')
+    # Test environment
     ci3 = nib.CommandLine(command='echo')
     res = ci3.run()
     assert res.runtime.environ['DISPLAY'] == ':1'
+
+    # Test display_variable option
+    monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
     config.set('execution', 'display_variable', ':3')
     res = ci3.run()
     assert not 'DISPLAY' in ci3.inputs.environ
+    assert not 'DISPLAY' in res.runtime.environ
+
+    # If the interface has _redirect_x then yes, it should be set
+    ci3._redirect_x = True
+    res = ci3.run()
     assert res.runtime.environ['DISPLAY'] == ':3'
+
+    # Test overwrite
+    monkeypatch.setitem(os.environ, 'DISPLAY', ':1')
     ci3.inputs.environ = {'DISPLAY': ':2'}
     res = ci3.run()
     assert res.runtime.environ['DISPLAY'] == ':2'
 
 
-def test_CommandLine_output(setup_file):
-    tmp_infile = setup_file
-    tmpd, name = os.path.split(tmp_infile)
-    assert os.path.exists(tmp_infile)
+def test_CommandLine_output(tmpdir):
+    # Create one file
+    tmpdir.chdir()
+    file = tmpdir.join('foo.txt')
+    file.write('123456\n')
+    name = os.path.basename(file.strpath)
+
     ci = nib.CommandLine(command='ls -l')
     ci.inputs.terminal_output = 'allatonce'
     res = ci.run()