ENH: Centralize virtual/physical $DISPLAYs #2203

oesteban · 2017-10-03T07:56:06Z

This PR addresses several (related) problems:

Makes $DISPLAY and the config display_variable optional (close DISPLAY variable should be optional #2055)
Should fix xvfb-run not ending gracefully for CommandLine interfaces #1403 since xvfb-run is not used anymore.
Relates to xvfbwrapper error #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?).
(EDIT: added later) Fixes AFNI's SkullStrip needs a $DISPLAY again after version 16.2.07 #2206.

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. If the virtual framebuffer is created, it is created only once for the whole workflow (solves the problem of many virtual fbs on a mapnode with _redirect_x = True.

One open question is: do we want to double check that an X server is running if $DISPLAY or display_variable are :0?
https://github.com/oesteban/nipype/blob/e8e18f039ce6ab7c7bd19c045e083bf02275c7c0/nipype/utils/config.py#L180-L185

This PR addresses several (related) problems: - Makes `$DISPLAY` and the config `display_variable` optional (close nipy#2055) - Should fix nipy#1403 since xvfb-run is not used anymore. - Relates to nipy#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.

effigies · 2017-10-03T16:22:53Z

One open question is: do we want to double check that an X server is running if $DISPLAY or display_variable are :0?

This strikes me as being outside the scope of nipype. If I set DISPLAY=":0" and don't have X running, programs that need X will crash. If an interface crashes and doesn't give a "No display found on $DISPLAY" message, I think it's within scope for that interface to give a better message.

oesteban · 2017-10-03T17:38:16Z

Thanks a lot for the comment.

This strikes me as being outside the scope of nipype. If I set DISPLAY=":0" and don't have X running, programs that need X will crash. If an interface crashes and doesn't give a "No display found on $DISPLAY" message, I think it's within scope for that interface to give a better message.

Totally agree, and that's the reason why I commented that part out, just to have the code handy if needed. It was just a suggestion that could make nipype easier for the user with better error messages (instead of relying on the third party libraries for these infrastructure things).

However, there is a hidden complexity: if you set the $DISPLAY=':1' because you have a VNC server running, then the way to identify the process is different. And even more complicated if the display is remote ($DISPLAY='<ip-of-different-host>:6001').

I'll leave it there for the future (commented out), if we want to check for it only when $DISPLAY='localhost:0'.

effigies · 2017-10-03T18:54:32Z

nipype/utils/config.py

@@ -172,3 +173,52 @@ 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):


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.

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?

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?

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.

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

effigies · 2017-10-03T21:44:48Z

What about in __del__, or whatever python's cleanup method is? Or if there's a post run hook. On Oct 3, 2017 5:38 PM, "Oscar Esteban" <[email protected]> wrote: *@oesteban* commented on this pull request. ------------------------------ In nipype/utils/config.py <#2203 (comment)>:

@@ -172,3 +173,52 @@ 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): 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. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2203 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFF8jtDUnCPtK9sbiE-95Zi6cByBhIGks5soqlrgaJpZM4PruNE> .

codecov-io · 2017-10-03T23:38:54Z

Codecov Report

Merging #2203 into master will increase coverage by 0.04%.
The diff coverage is 83.21%.

@@            Coverage Diff             @@
##           master    #2203      +/-   ##
==========================================
+ Coverage   72.24%   72.28%   +0.04%     
==========================================
  Files        1177     1178       +1     
  Lines       58639    58733      +94     
  Branches     8447     8457      +10     
==========================================
+ Hits        42366    42458      +92     
+ Misses      14906    14900       -6     
- Partials     1367     1375       +8

Flag	Coverage Δ
#smoketests	`72.28% <83.21%> (+0.04%)`	⬆️
#unittests	`69.84% <83.21%> (+0.05%)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/afni/preprocess.py	`81.17% <0%> (-0.13%)`	⬇️
nipype/interfaces/tests/test_base.py	`96.79% <100%> (+0.04%)`	⬆️
nipype/interfaces/base.py	`84.82% <100%> (+1.42%)`	⬆️
nipype/utils/config.py	`67.25% <80.48%> (+3.89%)`	⬆️
nipype/utils/tests/test_config.py	`81.7% <81.7%> (ø)`
nipype/pipeline/plugins/multiproc.py	`77.94% <0%> (+1.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fde2fb...82a2c6a. Read the comment docs.

oesteban · 2017-10-04T02:23:45Z

This PR fixed the problem in MRIQC, and the whole workflow run correctly: https://circleci.com/gh/oesteban/mriqc/1368

satra · 2017-10-04T02:48:37Z

nipype/interfaces/afni/preprocess.py

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


woah - is this really true - crazy!

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

oesteban · 2017-10-04T18:42:23Z

@satra: added the error plus a unit test for it

@effigies and Satra, this is ready for a final review.

effigies · 2017-10-04T19:10:49Z

doc/users/config_file.rst

+	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)


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

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

effigies · 2017-10-04T19:14:25Z

nipype/utils/tests/test_config.py

+    config._display = None
+    dispstr = ':10'
+    config.set('execution', 'display_variable', dispstr)
+    monkeypatch.setitem(os.environ, 'DISPLAY', dispstr)


Should this not be something other than dispstr?

effigies · 2017-10-04T19:17:44Z

nipype/utils/tests/test_config.py

+
+
+def test_display_empty_patched(monkeypatch):
+    """Check that when no display is specified, a virtual Xvfb is used"""


Docstrings after this one are all the same.

effigies · 2017-10-04T19:31:42Z

nipype/utils/config.py

+    from nipype import config
+    from nipype import logging
+    config.stop_display()
+    logging.getLogger('interface').info('Closing display (if virtual)')


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.

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

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.

@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.

satra · 2017-10-04T20:29:12Z

@oesteban - a couple of follow up questions to @effigies' comments:

i'm enumerating some use cases:

one machine, one user - single multiproc process
one machine, one user - multiple processes from different scripts/slurm/etc
one machine, many users

in my head, 1 and 2 would reuse the same xvfb, however this will not work unless there is some reference counting in place and the one that shuts it down is the last one to finish. this could also result in race conditions of some kind.

3 would launch different xvfb's per user. but within each user's space considerations for 1 and 2 would apply.

oesteban · 2017-10-04T20:36:14Z

one machine, one user - single multiproc process

Will reuse same Xvfb.

one machine, one user - multiple processes from different scripts/slurm/etc

Will open one new Xvfb per process. This is not ideal, but after all we are reducing the overload of Xvfb by a lot.

one machine, many users

Will open as many Xvfbs as users x processes.

For use cases 2 and 3 we should generally recommend setting up VNC (for clusters) or a unique Xvfb, and setting display_variable in all cases.

There is a compromise here between how much we want to make it seamless to the user and basics of infrastructure. The former implementation was extremely inefficient (spawning literally hundreds of Xvfb, we've seen that in the early days of MRIQC) so this is a great advance. We also get rid of xvfb-run for CommandLine interfaces, also very advisable and not always installed in clusters.

satra · 2017-10-04T20:39:19Z

IGNORE THIS COMMENT

i think we still need to support 2. imagine submitting slurm jobs on a cluster - it gets assigned to random nodes. if every job has to start a vnc server i don't think that's a good outcome. i'd rather have every job starting an xvfb session.

satra · 2017-10-04T20:41:38Z

@oesteban - nevermind my previous comment on case 2 - i think we are still ok - case 2 will still launch a different xvfb per process.

oesteban · 2017-10-04T20:45:52Z

@satra correct :). The vnc idea is when you have one node providing that, otherwise it's better to fall back to the default (ergo Xvfb).

oesteban · 2017-10-04T20:48:07Z

An important addendum to #2203 (comment) is that Xvfb().stop() ensures that the $DISPLAY variable is set to the original value. The only side effect that I could see if that $DISPLAY == '', for which we unset $DISPLAY and it is not recovered. I thought this is not extremely relevant, though.

oesteban · 2017-10-05T13:19:59Z

@satra, @effigies, this is ready for final review

effigies

Alright. @satra addressed my one confusion point. I think this all looks reasonable.

satra · 2017-10-05T13:37:15Z

@oesteban - lgtm

Includes - nipy/nipype#2198 - nipy/nipype#2200 - nipy/nipype#2203 - nipy/nipype#2205 - nipy/nipype#2208

Includes - nipy/nipype#2198 - nipy/nipype#2200 - nipy/nipype#2203 - nipy/nipype#2205 - nipy/nipype#2208 - nipy/nipype#2211 - nipy/nipype#2212

oesteban added 3 commits October 3, 2017 00:43

update documentation

e8e18f0

fix failing tests

486cef3

fix (and cover with test) the case

44b4880

fix afni.SkullStrip with AFNI >= 16.2.07

8a16650

effigies reviewed Oct 3, 2017

View reviewed changes

oesteban added 2 commits October 3, 2017 16:29

tests on new config

0a718c1

hookup a callback to close the display (if virtual) when nipype exits

2f83d08

fix tests test_Commandline_environ, test_CommandLine_output

7644acf

satra reviewed Oct 4, 2017

View reviewed changes

oesteban added 2 commits October 3, 2017 21:53

Merge remote-tracking branch 'upstream/master' into fix/VirtualDisplay

2c89c58

fix conflicts on config file

f634c0b

oesteban changed the title ~~[ENH] Centralize virtual/physical $DISPLAYs~~ ENH: Centralize virtual/physical $DISPLAYs Oct 4, 2017

oesteban added 3 commits October 3, 2017 22:22

update CHANGES

5151bf6

Merge remote-tracking branch 'upstream/master' into fix/VirtualDisplay

46dff1c

raise error when trying to run Xvfb on Mac. close nipy#1400

fc75e0f

effigies reviewed Oct 4, 2017

View reviewed changes

address @effigies' comments

82a2c6a

effigies approved these changes Oct 5, 2017

View reviewed changes

satra merged commit 4801a4c into nipy:master Oct 5, 2017

oesteban deleted the fix/VirtualDisplay branch October 5, 2017 15:30

effigies added a commit to effigies/niworkflows that referenced this pull request Oct 5, 2017

PIN: nipy/nipype master branch

dafbfda

Includes - nipy/nipype#2198 - nipy/nipype#2200 - nipy/nipype#2203 - nipy/nipype#2205 - nipy/nipype#2208

effigies mentioned this pull request Oct 5, 2017

[ENH] Minor Xvfb and MultiProcPlugin cleanups #2211

Merged

effigies added a commit to effigies/niworkflows that referenced this pull request Oct 6, 2017

PIN: nipy/nipype master branch

335e878

Includes - nipy/nipype#2198 - nipy/nipype#2200 - nipy/nipype#2203 - nipy/nipype#2205 - nipy/nipype#2208 - nipy/nipype#2211 - nipy/nipype#2212

satra added this to the 0.14.0 milestone Oct 20, 2017



		def test_display_empty_patched(monkeypatch):
		"""Check that when no display is specified, a virtual Xvfb is used"""

ENH: Centralize virtual/physical $DISPLAYs #2203

ENH: Centralize virtual/physical $DISPLAYs #2203

Uh oh!

Conversation

oesteban commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

effigies commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

effigies commented Oct 3, 2017 via email

Uh oh!

codecov-io commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oesteban commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oesteban commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satra commented Oct 4, 2017

Uh oh!

oesteban commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satra commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

satra commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban commented Oct 4, 2017

Uh oh!

oesteban commented Oct 4, 2017

Uh oh!

oesteban commented Oct 5, 2017

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

satra commented Oct 5, 2017

Uh oh!

Uh oh!

oesteban commented Oct 3, 2017 •

edited

Loading

effigies commented Oct 3, 2017 •

edited

Loading

oesteban commented Oct 3, 2017 •

edited

Loading

codecov-io commented Oct 3, 2017 •

edited

Loading

oesteban commented Oct 4, 2017 •

edited

Loading

satra commented Oct 4, 2017 •

edited

Loading

satra commented Oct 4, 2017 •

edited

Loading