diff --git a/CHANGES b/CHANGES index 2f2ad920af..885f7875e9 100644 --- a/CHANGES +++ b/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) ===================== diff --git a/doc/users/config_file.rst b/doc/users/config_file.rst index c89406faf1..b196047e97 100644 --- a/doc/users/config_file.rst +++ b/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 - `_ - or `Xvfb `_ - 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 `_ + or `Xvfb `_. + 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 diff --git a/nipype/interfaces/afni/preprocess.py b/nipype/interfaces/afni/preprocess.py index fe8a963777..1eb29b5abe 100644 --- a/nipype/interfaces/afni/preprocess.py +++ b/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 diff --git a/nipype/interfaces/base.py b/nipype/interfaces/base.py index 93ea49eeaa..1a5561fba6 100644 --- a/nipype/interfaces/base.py +++ b/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,7 +1293,7 @@ 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 @@ -1321,13 +1301,6 @@ def run_command(runtime, output=None, timeout=0.01, redirect_x=False): # 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) diff --git a/nipype/interfaces/tests/test_base.py b/nipype/interfaces/tests/test_base.py index 34d1134e42..f042173a6d 100644 --- a/nipype/interfaces/tests/test_base.py +++ b/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() diff --git a/nipype/utils/config.py b/nipype/utils/config.py index 2442f1eb01..fe1524c6d6 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -11,7 +11,9 @@ ''' from __future__ import print_function, division, unicode_literals, absolute_import import os +import sys import errno +import atexit from warnings import warn from io import StringIO from distutils.version import LooseVersion @@ -19,11 +21,11 @@ import numpy as np from builtins import bytes, str, object, open - from simplejson import load, dump from future import standard_library -from ..external import portalocker + from .misc import str2bool +from ..external import portalocker standard_library.install_aliases() @@ -50,7 +52,6 @@ [execution] create_report = true crashdump_dir = %s -display_variable = :1 hash_method = timestamp job_finished_timeout = 5 keep_inputs = false @@ -89,8 +90,7 @@ def mkdir_p(path): class NipypeConfig(object): - """Base nipype config class - """ + """Base nipype config class""" def __init__(self, *args, **kwargs): self._config = configparser.ConfigParser() @@ -98,7 +98,9 @@ def __init__(self, *args, **kwargs): config_file = os.path.join(config_dir, 'nipype.cfg') self.data_file = os.path.join(config_dir, 'nipype.json') self._config.readfp(StringIO(default_cfg)) + self._display = None self._resource_monitor = None + if os.path.exists(config_dir): self._config.read([config_file, 'nipype.cfg']) @@ -114,8 +116,7 @@ def set_default_config(self): self._config.readfp(StringIO(default_cfg)) def enable_debug_mode(self): - """Enables debug configuration - """ + """Enables debug configuration""" self._config.set('execution', 'stop_on_first_crash', 'true') self._config.set('execution', 'remove_unnecessary_outputs', 'false') self._config.set('execution', 'keep_inputs', 'true') @@ -131,6 +132,7 @@ def set_log_dir(self, log_dir): self._config.set('logging', 'log_directory', log_dir) def get(self, section, option, default=None): + """Get an option""" if option in CONFIG_DEPRECATIONS: msg = ('Config option "%s" has been deprecated as of nipype %s. Please use ' '"%s" instead.') % (option, CONFIG_DEPRECATIONS[option][1], @@ -143,6 +145,7 @@ def get(self, section, option, default=None): return default def set(self, section, option, value): + """Set new value on option""" if isinstance(value, bool): value = str(value) @@ -156,9 +159,11 @@ def set(self, section, option, value): return self._config.set(section, option, value) def getboolean(self, section, option): + """Get a boolean option from section""" return self._config.getboolean(section, option) def has_option(self, section, option): + """Check if option exists in section""" return self._config.has_option(section, option) @property @@ -166,6 +171,7 @@ def _sections(self): return self._config._sections def get_data(self, key): + """Read options file""" if not os.path.exists(self.data_file): return None with open(self.data_file, 'rt') as file: @@ -176,6 +182,7 @@ def get_data(self, key): return None def save_data(self, key, value): + """Store config flie""" datadict = {} if os.path.exists(self.data_file): with open(self.data_file, 'rt') as file: @@ -191,6 +198,7 @@ def save_data(self, key, value): dump(datadict, file) def update_config(self, config_dict): + """Extend internal dictionary with config_dict""" for section in ['execution', 'logging', 'check']: if section in config_dict: for key, val in list(config_dict[section].items()): @@ -198,10 +206,12 @@ def update_config(self, config_dict): self._config.set(section, key, str(val)) def update_matplotlib(self): + """Set backend on matplotlib from options""" import matplotlib matplotlib.use(self.get('execution', 'matplotlib_backend')) def enable_provenance(self): + """Sets provenance storing on""" self._config.set('execution', 'write_provenance', 'true') self._config.set('execution', 'hash_method', 'content') @@ -242,4 +252,77 @@ def resource_monitor(self, value): ('%s' % self._resource_monitor).lower()) def enable_resource_monitor(self): + """Sets the resource monitor on""" self.resource_monitor = True + + def get_display(self): + """Returns the first display available""" + + # Check if an Xorg server is listening + # import subprocess as sp + # if not hasattr(sp, 'DEVNULL'): + # setattr(sp, 'DEVNULL', os.devnull) + # x_listening = bool(sp.call('ps au | grep -v grep | grep -i xorg', + # shell=True, stdout=sp.DEVNULL)) + + if self._display is not None: + return ':%d' % self._display.vdisplay_num + + sysdisplay = None + if self._config.has_option('execution', 'display_variable'): + sysdisplay = self._config.get('execution', 'display_variable') + + sysdisplay = sysdisplay or os.getenv('DISPLAY') + if sysdisplay: + from collections import namedtuple + + def _mock(): + pass + + # Store a fake Xvfb object + ndisp = int(sysdisplay.split(':')[-1]) + Xvfb = namedtuple('Xvfb', ['vdisplay_num', 'stop']) + self._display = Xvfb(ndisp, _mock) + return sysdisplay + else: + if 'darwin' in sys.platform: + raise RuntimeError( + 'Xvfb requires root permissions to run in OSX. Please ' + 'make sure that an X server is listening and set the ' + 'appropriate config on either $DISPLAY or nipype\'s ' + '"display_variable" config. Valid X servers include ' + 'VNC, XQuartz, or manually started Xvfb.') + + # If $DISPLAY is empty, it confuses Xvfb so unset + if sysdisplay == '': + del os.environ['DISPLAY'] + try: + from xvfbwrapper import Xvfb + except ImportError: + raise RuntimeError( + 'A display server was required, but $DISPLAY is not defined ' + 'and Xvfb could not be imported.') + + self._display = Xvfb(nolisten='tcp') + self._display.start() + + # Older versions of Xvfb used vdisplay_num + if hasattr(self._display, 'vdisplay_num'): + return ':%d' % self._display.vdisplay_num + + if hasattr(self._display, 'new_display'): + return ':%d' % self._display.new_display + + def stop_display(self): + """Closes the display if started""" + if self._display is not None: + self._display.stop() + + +@atexit.register +def free_display(): + """Stop virtual display (if it is up)""" + from nipype import config + from nipype import logging + config.stop_display() + logging.getLogger('interface').info('Closing display (if virtual)') diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py new file mode 100644 index 0000000000..4cb7bcd350 --- /dev/null +++ b/nipype/utils/tests/test_config.py @@ -0,0 +1,141 @@ +# -*- coding: utf-8 -*- +# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*- +# vi: set ft=python sts=4 ts=4 sw=4 et: +from __future__ import print_function, division, unicode_literals, absolute_import +import os +import sys +import pytest +from nipype import config +from mock import MagicMock +from builtins import object + +try: + import xvfbwrapper + has_Xvfb = True +except ImportError: + has_Xvfb = False + +xvfbpatch = MagicMock() +xvfbpatch.Xvfb.return_value = MagicMock(vdisplay_num=2010) + + +@pytest.mark.parametrize('dispnum', range(5)) +def test_display_config(monkeypatch, dispnum): + """Check that the display_variable option is used ($DISPLAY not set)""" + config._display = None + dispstr = ':%d' % dispnum + config.set('execution', 'display_variable', dispstr) + monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) + assert config.get_display() == config.get('execution', 'display_variable') + + +@pytest.mark.parametrize('dispnum', range(5)) +def test_display_system(monkeypatch, dispnum): + """Check that when only a $DISPLAY is defined, it is used""" + config._display = None + config._config.remove_option('execution', 'display_variable') + dispstr = ':%d' % dispnum + monkeypatch.setitem(os.environ, 'DISPLAY', dispstr) + assert config.get_display() == dispstr + + +def test_display_config_and_system(monkeypatch): + """Check that when only both config and $DISPLAY are defined, the config takes precedence""" + config._display = None + dispstr = ':10' + config.set('execution', 'display_variable', dispstr) + monkeypatch.setitem(os.environ, 'DISPLAY', ':0') + assert config.get_display() == dispstr + + +def test_display_noconfig_nosystem_patched(monkeypatch): + """Check that when no $DISPLAY nor option are specified, a virtual Xvfb is used""" + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) + monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch) + assert config.get_display() == ":2010" + + +def test_display_empty_patched(monkeypatch): + """ + Check that when $DISPLAY is empty string and no option is specified, + a virtual Xvfb is used + """ + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.setitem(os.environ, 'DISPLAY', '') + monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch) + assert config.get_display() == ':2010' + + +def test_display_noconfig_nosystem_notinstalled(monkeypatch): + """ + Check that an exception is raised if xvfbwrapper is not installed + but necessary (no config and $DISPLAY unset) + """ + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) + monkeypatch.setitem(sys.modules, 'xvfbwrapper', None) + with pytest.raises(RuntimeError): + config.get_display() + + +def test_display_empty_notinstalled(monkeypatch): + """ + Check that an exception is raised if xvfbwrapper is not installed + but necessary (no config and $DISPLAY empty) + """ + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.setitem(os.environ, 'DISPLAY', '') + monkeypatch.setitem(sys.modules, 'xvfbwrapper', None) + with pytest.raises(RuntimeError): + config.get_display() + + +@pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') +def test_display_noconfig_nosystem_installed(monkeypatch): + """ + Check that actually uses xvfbwrapper when installed (not mocked) + and necessary (no config and $DISPLAY unset) + """ + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.delitem(os.environ, 'DISPLAY', raising=False) + assert int(config.get_display().split(':')[-1]) > 1000 + + +@pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') +def test_display_empty_installed(monkeypatch): + """ + Check that actually uses xvfbwrapper when installed (not mocked) + and necessary (no config and $DISPLAY empty) + """ + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.setitem(os.environ, 'DISPLAY', '') + assert int(config.get_display().split(':')[-1]) > 1000 + + +def test_display_empty_macosx(monkeypatch): + """ + Check that an exception is raised if xvfbwrapper is necessary + (no config and $DISPLAY unset) but platform is OSX. See + https://github.com/nipy/nipype/issues/1400 + """ + config._display = None + if config.has_option('execution', 'display_variable'): + config._config.remove_option('execution', 'display_variable') + monkeypatch.delitem(os.environ, 'DISPLAY', '') + + monkeypatch.setattr(sys, 'platform', 'darwin') + with pytest.raises(RuntimeError): + config.get_display()