Skip to content

ENH: Centralize virtual/physical $DISPLAYs #2203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Oct 5, 2017
Merged

ENH: Centralize virtual/physical $DISPLAYs #2203

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
94146e7
[ENH] Centralize virtual/physical $DISPLAYs
oesteban Oct 3, 2017
e8e18f0
update documentation
oesteban Oct 3, 2017
486cef3
fix failing tests
oesteban Oct 3, 2017
44b4880
fix (and cover with test) the case
oesteban Oct 3, 2017
8a16650
fix afni.SkullStrip with AFNI >= 16.2.07
oesteban Oct 3, 2017
0a718c1
tests on new config
oesteban Oct 3, 2017
2f83d08
hookup a callback to close the display (if virtual) when nipype exits
oesteban Oct 3, 2017
7644acf
fix tests test_Commandline_environ, test_CommandLine_output
oesteban Oct 4, 2017
2c89c58
Merge remote-tracking branch 'upstream/master' into fix/VirtualDisplay
oesteban Oct 4, 2017
f634c0b
fix conflicts on config file
oesteban Oct 4, 2017
5151bf6
update CHANGES
oesteban Oct 4, 2017
46dff1c
Merge remote-tracking branch 'upstream/master' into fix/VirtualDisplay
oesteban Oct 4, 2017
fc75e0f
raise error when trying to run Xvfb on Mac. close #1400
oesteban Oct 4, 2017
82a2c6a
address @effigies' comments
oesteban Oct 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions doc/users/config_file.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
<http://www.x.org/archive/X11R7.5/doc/man/man1/Xnest.1.html>`_
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 <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
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little unclear. If I understand it correctly, we use the input $DISPLAY variable unless this is set? Under that assumption, here's a proposed rewrite:

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: ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. display_variable should override $DISPLAY. I'll change the code and use your proposed text for the documentation.


*remove_unnecessary_outputs*
Expand Down
5 changes: 3 additions & 2 deletions nipype/interfaces/afni/preprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah - is this really true - crazy!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, tell me about it. I'll report this to AFNI, to make sure this is not a mistake linking.

self._redirect_x = False


Expand Down
61 changes: 10 additions & 51 deletions nipype/interfaces/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
"""
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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]
Expand Down Expand Up @@ -1727,17 +1698,10 @@ 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 self.inputs.environ
else:
return {}

def version_from_command(self, flag='-v'):
cmdname = self.cmd.split()[0]
Expand All @@ -1754,10 +1718,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

Expand Down Expand Up @@ -1785,8 +1745,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)
Expand Down
28 changes: 23 additions & 5 deletions nipype/interfaces/tests/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
78 changes: 70 additions & 8 deletions nipype/utils/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand All @@ -44,7 +42,6 @@
[execution]
create_report = true
crashdump_dir = %s
display_variable = :1
hash_method = timestamp
job_finished_timeout = 5
keep_inputs = false
Expand Down Expand Up @@ -82,7 +79,8 @@ def mkdir_p(path):


class NipypeConfig(object):
"""Base nipype config class
"""
Base nipype config class
"""

def __init__(self, *args, **kwargs):
Expand All @@ -91,6 +89,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'])

Expand Down Expand Up @@ -172,3 +171,66 @@ 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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand it, if this has not been called before workflow runtime, then each node will get a copy of config and thus start up its own Xvfb object.

And I don't see anywhere where it's stopped.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first time you call it, one display will be locked. As a matter of fact, when I first wrote the tests (https://github.com/oesteban/nipype/blob/fix/VirtualDisplay/nipype/utils/tests/test_config.py#L9-L49) I realized I had to manally reset the config object all the times because once it is called it is fixed to that display. Therefore, all those config._display = None and those re-setting/removing the nipype config setting.

Does this reply to your question?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I meant to put at the end of the first sentence: "Is this the desired behavior?" Sounds like it is.

Similarly, the fact that self._display.stop() is no longer called anywhere is intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was actually the problem: nipype would spawn an endless number of displays.

Well, it is probably nice to add a self._display.stop() at the end when running a workflow. I'm not sure though about how to do that when running bare interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added a callback when python exists the session :D.

"""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 $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():
from nipype import config
from nipype import logging
config.stop_display()
logging.getLogger('interface').info('Closing display (if virtual)')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nipype is going to spawn many Xvfbs, shouldn't the stop be associated with the interface, rather than just closing one when Python quits?

I think I must be missing something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the opposite. You want nipype to run only one Xvfb if possible. Let's say you start an ipython session, run an interface with _redirect_x = True and then another interface than again requires X. With the current implementation only one Xvfb is started. Only when you exit ipython the Xvfb service is killed.

The rationale is precisely stop spawning many Xvfbs which was the case before

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. And you're testing to make sure you're not producing multiple by running multiple interfaces? Because my first comment noted that it looks like each Node makes a copy of config before spawning an xvfb, so it's not clear they would actually share it.

Again, this is from a very cursory look, so it's possible I'm missing things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@effigies - config makes copies of sections, not the entire config object. so as long as the config.get_display is called, nipype maintains a global config/state.

Loading