From 94146e7f3144041f213dfa9e6782e0d729573924 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 00:43:26 -0700 Subject: [PATCH 01/12] [ENH] Centralize virtual/physical $DISPLAYs This PR addresses several (related) problems: - Makes `$DISPLAY` and the config `display_variable` optional (close #2055) - Should fix #1403 since xvfb-run is not used anymore. - Relates to #1400: - Will reduce its impact because now Xvfb is called only once and only if it is absolutely necessary - May make it worse in some cases when Xvfb fails to create a listener (root needed?). This PR adds a `config.get_display()` which identifies what display should be used, and creates a virtual one if necessary. Also adds a few unit tests to the config object to make sure precedence is fulfilled. --- nipype/interfaces/base.py | 56 ++++--------------------------- nipype/utils/config.py | 52 ++++++++++++++++++++++++++-- nipype/utils/tests/test_config.py | 41 ++++++++++++++++++++++ 3 files changed, 98 insertions(+), 51 deletions(-) create mode 100644 nipype/utils/tests/test_config.py diff --git a/nipype/interfaces/base.py b/nipype/interfaces/base.py index 19cf9ccaa6..844f97d4d6 100644 --- a/nipype/interfaces/base.py +++ b/nipype/interfaces/base.py @@ -1010,32 +1010,6 @@ def _check_version_requirements(self, trait_object, raise_exception=True): version, max_ver)) return unavailable_traits - def _run_wrapper(self, runtime): - sysdisplay = os.getenv('DISPLAY') - 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 """ @@ -1071,6 +1045,9 @@ def run(self, **inputs): # initialize provenance tracking env = deepcopy(dict(os.environ)) + if self._redirect_x: + env['DISPLAY'] = config.get_display() + runtime = Bunch(cwd=os.getcwd(), returncode=None, duration=None, @@ -1080,8 +1057,9 @@ def run(self, **inputs): platform=platform.platform(), hostname=platform.node(), version=self.version) + try: - runtime = self._run_wrapper(runtime) + runtime = self._run_interface(runtime) outputs = self.aggregate_outputs(runtime) runtime.endTime = dt.isoformat(dt.utcnow()) timediff = parseutc(runtime.endTime) - parseutc(runtime.startTime) @@ -1446,7 +1424,7 @@ def get_max_resources_used(pid, mem_mb, num_threads, pyfunc=False): return mem_mb, num_threads -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 @@ -1458,13 +1436,6 @@ def run_command(runtime, output=None, timeout=0.01, redirect_x=False): # Init variables PIPE = subprocess.PIPE 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] @@ -1727,14 +1698,6 @@ 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 @@ -1754,10 +1717,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 @@ -1785,8 +1744,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/utils/config.py b/nipype/utils/config.py index ebea9e5816..5817940ab3 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -44,7 +44,6 @@ [execution] create_report = true crashdump_dir = %s -display_variable = :1 hash_method = timestamp job_finished_timeout = 5 keep_inputs = false @@ -82,7 +81,8 @@ def mkdir_p(path): class NipypeConfig(object): - """Base nipype config class + """ + Base nipype config class """ def __init__(self, *args, **kwargs): @@ -91,6 +91,7 @@ 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 if os.path.exists(config_dir): self._config.read([config_file, 'nipype.cfg']) @@ -172,3 +173,50 @@ def update_matplotlib(self): def enable_provenance(self): self._config.set('execution', 'write_provenance', 'true') self._config.set('execution', 'hash_method', 'content') + + 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: + 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, 'new_display'): + setattr(self._display, 'vdisplay_num', + self._display.new_display) + + return ':%d' % self._display.vdisplay_num diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py new file mode 100644 index 0000000000..9883721e29 --- /dev/null +++ b/nipype/utils/tests/test_config.py @@ -0,0 +1,41 @@ +# -*- 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 pytest +from nipype import config + +@pytest.mark.parametrize('dispnum', range(5)) +def test_display_config(monkeypatch, dispnum): + """Check that the display_variable option is used""" + 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', dispstr) + assert config.get_display() == dispstr + +def test_display_noconfig_nosystem(monkeypatch): + """Check that when no display 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.delitem(os.environ, 'DISPLAY', raising=False) + assert int(config.get_display().split(':')[-1]) > 80 \ No newline at end of file From e8e18f039ce6ab7c7bd19c045e083bf02275c7c0 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 01:03:58 -0700 Subject: [PATCH 02/12] update documentation --- doc/users/config_file.rst | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/doc/users/config_file.rst b/doc/users/config_file.rst index 7d55cc522d..d6d83ae762 100644 --- a/doc/users/config_file.rst +++ b/doc/users/config_file.rst @@ -69,12 +69,17 @@ 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 - `_ + What ``$DISPLAY`` environment variable should utilize those interfaces + that require an X server. These interfaces should have the attribute + ``_redirect_x = True``. This option is very useful when the system has + an X server listening in the default port 6000, but ``$DISPLAY`` is + not defined. In that case, set ``display_variable = :0``. Similarly, + it can be used to point X-based interfaces to other servers, like VNC, + `xnest `_ or `Xvfb `_ and you would like to redirect all spawned windows to - it. (possible values: any X server address; default value: not + it. If not 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* From 486cef3823321678a98a0abe53e13ed134383a13 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 08:33:40 -0700 Subject: [PATCH 03/12] fix failing tests --- nipype/interfaces/base.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/base.py b/nipype/interfaces/base.py index 844f97d4d6..7f7642261d 100644 --- a/nipype/interfaces/base.py +++ b/nipype/interfaces/base.py @@ -1699,8 +1699,9 @@ def help(cls, returnhelp=False): def _get_environ(self): if isdefined(self.inputs.environ): - out_environ.update(self.inputs.environ) - return out_environ + return self.inputs.environ + else: + return {} def version_from_command(self, flag='-v'): cmdname = self.cmd.split()[0] From 44b4880b042d109b35c61db918f46a8577eae5f5 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 10:31:15 -0700 Subject: [PATCH 04/12] fix (and cover with test) the case --- nipype/utils/config.py | 6 ++++-- nipype/utils/tests/test_config.py | 10 +++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index 5817940ab3..97831190a6 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -202,14 +202,16 @@ def _mock(): Xvfb = namedtuple('Xvfb', ['vdisplay_num', 'stop']) self._display = Xvfb(ndisp, _mock) return sysdisplay - else: + # 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.') + 'and Xvfb could not be imported.') self._display = Xvfb(nolisten='tcp') self._display.start() diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 9883721e29..017bf95af2 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -38,4 +38,12 @@ def test_display_noconfig_nosystem(monkeypatch): 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]) > 80 \ No newline at end of file + assert int(config.get_display().split(':')[-1]) > 80 + +def test_display_empty(monkeypatch): + """Check that when no display 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', '') + assert int(config.get_display().split(':')[-1]) > 80 From 8a16650764dddfbaef7cbe0f03fa796135d1fd90 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 11:10:00 -0700 Subject: [PATCH 05/12] fix afni.SkullStrip with AFNI >= 16.2.07 --- nipype/interfaces/afni/preprocess.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nipype/interfaces/afni/preprocess.py b/nipype/interfaces/afni/preprocess.py index 97455ec69f..02413121f1 100644 --- a/nipype/interfaces/afni/preprocess.py +++ b/nipype/interfaces/afni/preprocess.py @@ -2146,11 +2146,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 From 0a718c13108da20318251ad5c7e5ba445d44306a Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 16:29:08 -0700 Subject: [PATCH 06/12] tests on new config --- nipype/utils/config.py | 8 ++-- nipype/utils/tests/test_config.py | 69 +++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 8 deletions(-) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index 97831190a6..a0fae4353e 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -217,8 +217,8 @@ def _mock(): self._display.start() # Older versions of Xvfb used vdisplay_num - if hasattr(self._display, 'new_display'): - setattr(self._display, 'vdisplay_num', - self._display.new_display) + if hasattr(self._display, 'vdisplay_num'): + return ':%d' % self._display.vdisplay_num - return ':%d' % self._display.vdisplay_num + if hasattr(self._display, 'new_display'): + return ':%d' % self._display.new_display diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 017bf95af2..86ad23a81d 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -3,8 +3,21 @@ # 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): @@ -15,6 +28,7 @@ def test_display_config(monkeypatch, dispnum): 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""" @@ -24,6 +38,7 @@ def test_display_system(monkeypatch, 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 @@ -32,18 +47,64 @@ def test_display_config_and_system(monkeypatch): monkeypatch.setitem(os.environ, 'DISPLAY', dispstr) assert config.get_display() == dispstr -def test_display_noconfig_nosystem(monkeypatch): + +def test_display_noconfig_nosystem_patched(monkeypatch): """Check that when no display 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.delitem(os.environ, 'DISPLAY', raising=False) - assert int(config.get_display().split(':')[-1]) > 80 + monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch) + assert config.get_display() == ":2010" + + +def test_display_empty_patched(monkeypatch): + """Check that when no display 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 when no display 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.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 when no display 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', 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 when no display 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.delitem(os.environ, 'DISPLAY', raising=False) + assert int(config.get_display().split(':')[-1]) > 1000 + -def test_display_empty(monkeypatch): +@pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') +def test_display_empty_installed(monkeypatch): """Check that when no display 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', '') - assert int(config.get_display().split(':')[-1]) > 80 + assert int(config.get_display().split(':')[-1]) > 1000 From 2f83d08e29239d962e51491c97dc9cec5ff9e600 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 16:38:36 -0700 Subject: [PATCH 07/12] hookup a callback to close the display (if virtual) when nipype exits --- nipype/utils/config.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index a0fae4353e..700a50240a 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -11,21 +11,19 @@ ''' from __future__ import print_function, division, unicode_literals, absolute_import import os -import shutil import errno -from warnings import warn +import atexit from io import StringIO from distutils.version import LooseVersion from simplejson import load, dump import numpy as np from builtins import str, object, open -from future import standard_library -standard_library.install_aliases() - -import configparser from ..external import portalocker +import configparser +from future import standard_library +standard_library.install_aliases() NUMPY_MMAP = LooseVersion(np.__version__) >= LooseVersion('1.12.0') @@ -194,6 +192,7 @@ def get_display(self): sysdisplay = sysdisplay or os.getenv('DISPLAY') if sysdisplay: from collections import namedtuple + def _mock(): pass @@ -222,3 +221,16 @@ def _mock(): 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(): + from nipype import config + from nipype import logging + config.stop_display() + logging.getLogger('interface').info('Closing display (if virtual)') From 7644acf555c687b4fe2761271a38718f816bb366 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 19:09:34 -0700 Subject: [PATCH 08/12] fix tests test_Commandline_environ, test_CommandLine_output --- nipype/interfaces/tests/test_base.py | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) 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() From f634c0b541423dcb19e1c480f1ad31aa93a82342 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 22:13:25 -0700 Subject: [PATCH 09/12] fix conflicts on config file --- nipype/utils/config.py | 116 ++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 60 deletions(-) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index 4ea4d611ed..39bdda4fef 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -13,25 +13,18 @@ import os import errno import atexit +from warnings import warn from io import StringIO from distutils.version import LooseVersion import configparser import numpy as np -<<<<<<< HEAD -from builtins import str, object, open -from ..external import portalocker -import configparser - -from future import standard_library -standard_library.install_aliases() -======= 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() @@ -40,7 +33,6 @@ 'profile_runtime': ('resource_monitor', '1.0'), 'filemanip_level': ('utils_level', '1.0'), } ->>>>>>> upstream/master NUMPY_MMAP = LooseVersion(np.__version__) >= LooseVersion('1.12.0') @@ -97,9 +89,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() @@ -107,11 +97,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)) -<<<<<<< HEAD self._display = None -======= self._resource_monitor = None ->>>>>>> upstream/master + if os.path.exists(config_dir): self._config.read([config_file, 'nipype.cfg']) @@ -127,8 +115,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') @@ -144,6 +131,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], @@ -156,6 +144,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) @@ -169,9 +158,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 @@ -179,6 +170,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: @@ -189,6 +181,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: @@ -204,6 +197,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()): @@ -211,14 +205,55 @@ 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') -<<<<<<< HEAD + @property + def resource_monitor(self): + """Check if resource_monitor is available""" + if self._resource_monitor is not None: + return self._resource_monitor + + # Cache config from nipype config + self.resource_monitor = self._config.get( + 'execution', 'resource_monitor') or False + return self._resource_monitor + + @resource_monitor.setter + def resource_monitor(self, value): + # Accept string true/false values + if isinstance(value, (str, bytes)): + value = str2bool(value.lower()) + + if value is False: + self._resource_monitor = False + elif value is True: + if not self._resource_monitor: + # Before setting self._resource_monitor check psutil availability + self._resource_monitor = False + try: + import psutil + self._resource_monitor = LooseVersion( + psutil.__version__) >= LooseVersion('5.0') + except ImportError: + pass + finally: + if not self._resource_monitor: + warn('Could not enable the resource monitor: psutil>=5.0' + ' could not be imported.') + self._config.set('execution', 'resource_monitor', + ('%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""" @@ -277,47 +312,8 @@ def stop_display(self): @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)') -======= - @property - def resource_monitor(self): - """Check if resource_monitor is available""" - if self._resource_monitor is not None: - return self._resource_monitor - - # Cache config from nipype config - self.resource_monitor = self._config.get( - 'execution', 'resource_monitor') or False - return self._resource_monitor - - @resource_monitor.setter - def resource_monitor(self, value): - # Accept string true/false values - if isinstance(value, (str, bytes)): - value = str2bool(value.lower()) - - if value is False: - self._resource_monitor = False - elif value is True: - if not self._resource_monitor: - # Before setting self._resource_monitor check psutil availability - self._resource_monitor = False - try: - import psutil - self._resource_monitor = LooseVersion( - psutil.__version__) >= LooseVersion('5.0') - except ImportError: - pass - finally: - if not self._resource_monitor: - warn('Could not enable the resource monitor: psutil>=5.0' - ' could not be imported.') - self._config.set('execution', 'resource_monitor', - ('%s' % self._resource_monitor).lower()) - - def enable_resource_monitor(self): - self.resource_monitor = True ->>>>>>> upstream/master From 5151bf60b1aca0424362ba72b101b8f2c31082f8 Mon Sep 17 00:00:00 2001 From: oesteban Date: Tue, 3 Oct 2017 22:22:16 -0700 Subject: [PATCH 10/12] update CHANGES --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) 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) ===================== From fc75e0f970d1567aea02ea25f66dde97becf2e22 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 4 Oct 2017 11:40:29 -0700 Subject: [PATCH 11/12] raise error when trying to run Xvfb on Mac. close #1400 --- nipype/utils/config.py | 9 +++++++++ nipype/utils/tests/test_config.py | 12 ++++++++++++ 2 files changed, 21 insertions(+) diff --git a/nipype/utils/config.py b/nipype/utils/config.py index 39bdda4fef..fe1524c6d6 100644 --- a/nipype/utils/config.py +++ b/nipype/utils/config.py @@ -11,6 +11,7 @@ ''' from __future__ import print_function, division, unicode_literals, absolute_import import os +import sys import errno import atexit from warnings import warn @@ -284,6 +285,14 @@ def _mock(): 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'] diff --git a/nipype/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 86ad23a81d..032212dba1 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -108,3 +108,15 @@ def test_display_empty_installed(monkeypatch): 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 when no display 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.delitem(os.environ, 'DISPLAY', '') + + monkeypatch.setattr(sys, 'platform', 'darwin') + with pytest.raises(RuntimeError): + config.get_display() From 82a2c6a04c1d06a97bd41ab6fa1d893aa93789f4 Mon Sep 17 00:00:00 2001 From: oesteban Date: Wed, 4 Oct 2017 13:50:07 -0700 Subject: [PATCH 12/12] address @effigies' comments --- doc/users/config_file.rst | 22 +++++++++--------- nipype/utils/tests/test_config.py | 37 +++++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/doc/users/config_file.rst b/doc/users/config_file.rst index 5303fe1fb5..b196047e97 100644 --- a/doc/users/config_file.rst +++ b/doc/users/config_file.rst @@ -73,18 +73,16 @@ Execution ``false``; default value: ``true``) *display_variable* - What ``$DISPLAY`` environment variable should utilize those interfaces - that require an X server. These interfaces should have the attribute - ``_redirect_x = True``. This option is very useful when the system has - an X server listening in the default port 6000, but ``$DISPLAY`` is - not defined. In that case, set ``display_variable = :0``. Similarly, - it can be used to point X-based interfaces to other servers, like VNC, - `xnest `_ - or `Xvfb `_ - and you would like to redirect all spawned windows to - it. If not set, nipype will try to configure a new virtual server using - Xvfb. (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/utils/tests/test_config.py b/nipype/utils/tests/test_config.py index 032212dba1..4cb7bcd350 100644 --- a/nipype/utils/tests/test_config.py +++ b/nipype/utils/tests/test_config.py @@ -21,7 +21,7 @@ @pytest.mark.parametrize('dispnum', range(5)) def test_display_config(monkeypatch, dispnum): - """Check that the display_variable option is used""" + """Check that the display_variable option is used ($DISPLAY not set)""" config._display = None dispstr = ':%d' % dispnum config.set('execution', 'display_variable', dispstr) @@ -44,12 +44,12 @@ def test_display_config_and_system(monkeypatch): config._display = None dispstr = ':10' config.set('execution', 'display_variable', dispstr) - monkeypatch.setitem(os.environ, 'DISPLAY', dispstr) + monkeypatch.setitem(os.environ, 'DISPLAY', ':0') assert config.get_display() == dispstr def test_display_noconfig_nosystem_patched(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """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') @@ -59,7 +59,10 @@ def test_display_noconfig_nosystem_patched(monkeypatch): def test_display_empty_patched(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """ + 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') @@ -69,7 +72,10 @@ def test_display_empty_patched(monkeypatch): def test_display_noconfig_nosystem_notinstalled(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """ + 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') @@ -80,7 +86,10 @@ def test_display_noconfig_nosystem_notinstalled(monkeypatch): def test_display_empty_notinstalled(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """ + 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') @@ -92,7 +101,10 @@ def test_display_empty_notinstalled(monkeypatch): @pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') def test_display_noconfig_nosystem_installed(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """ + 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') @@ -102,7 +114,10 @@ def test_display_noconfig_nosystem_installed(monkeypatch): @pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed') def test_display_empty_installed(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """ + 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') @@ -111,7 +126,11 @@ def test_display_empty_installed(monkeypatch): def test_display_empty_macosx(monkeypatch): - """Check that when no display is specified, a virtual Xvfb is used""" + """ + 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')