Skip to content

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Feb 2, 2021

This PR:

  1. continues to escape % in azure pipelines:

##[warning]%25 detected in ##vso command. In March 2021, the agent command parser will be updated to unescape this to %. To opt out of this behavior, set a job level variable DECODE_PERCENTS to false. Setting to true will force this behavior immediately. More information can be found at https://github.com/microsoft/azure-pipelines-agent/blob/master/docs/design/percentEncoding.md

  1. investigates failling viz test on MacOS and Linux

@GuillaumeFavelier GuillaumeFavelier self-assigned this Feb 2, 2021
@GuillaumeFavelier
Copy link
Contributor Author

I suspect test_brain_screenshot for now.

@GuillaumeFavelier
Copy link
Contributor Author

Indeed paintEvent fails often.

This one is from macos / conda:

Details
mne/viz/_brain/tests/test_brain.py::test_layered_mesh[pyvista] PASSED    [  2%]
mne/viz/_brain/tests/test_brain.py::test_single_hemi[pyvista-lh] SKIPPED [  2%]
mne/viz/_brain/tests/test_brain.py::test_single_hemi[pyvista-rh] SKIPPED [  3%]
mne/viz/_brain/tests/test_brain.py::test_brain_screenshot[pyvista] PASSED [  3%]
mne/viz/_brain/tests/test_brain.py::test_brain_time_viewer[pyvista] PASSED [  4%]
Fatal Python error: Segmentation fault

Current thread 0x000000010b236dc0 (most recent call first):
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/vtkmodules/qt/QVTKRenderWindowInteractor.py", line 381 in paintEvent
  File "/Users/runner/work/mne-python/mne-python/mne/viz/backends/_pyvista.py", line 1069 in _process_events
  File "/Users/runner/work/mne-python/mne-python/mne/viz/backends/_pyvista.py", line 249 in ensure_minimum_sizes
  File "/usr/local/miniconda/envs/mne/lib/python3.8/contextlib.py", line 120 in __exit__
  File "/Users/runner/work/mne-python/mne-python/mne/viz/backends/_pyvista.py", line 635 in show
  File "/Users/runner/work/mne-python/mne-python/mne/viz/_brain/_brain.py", line 2643 in show
  File "/Users/runner/work/mne-python/mne-python/mne/viz/_brain/_brain.py", line 683 in setup_time_viewer
  File "/Users/runner/work/mne-python/mne-python/mne/viz/_3d.py", line 2002 in _plot_stc
  File "/Users/runner/work/mne-python/mne-python/mne/viz/_3d.py", line 1826 in plot_source_estimates
  File "<decorator-gen-143>", line 24 in plot_source_estimates
  File "/Users/runner/work/mne-python/mne-python/mne/source_estimate.py", line 653 in plot
  File "/Users/runner/work/mne-python/mne-python/mne/viz/_brain/tests/test_brain.py", line 912 in _create_testing_brain
  File "/Users/runner/work/mne-python/mne-python/mne/viz/_brain/tests/test_brain.py", line 536 in test_brain_traces
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/python.py", line 183 in pytest_pyfunc_call
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 84 in <lambda>
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/python.py", line 1641 in runtest
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 162 in pytest_runtest_call
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 84 in <lambda>
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 255 in <lambda>
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 311 in from_call
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 254 in call_runtest_hook
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 215 in call_and_report
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 126 in runtestprotocol
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/runner.py", line 109 in pytest_runtest_protocol
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 84 in <lambda>
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/main.py", line 348 in pytest_runtestloop
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 84 in <lambda>
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/main.py", line 323 in _main
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/main.py", line 269 in wrap_session
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/main.py", line 316 in pytest_cmdline_main
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/callers.py", line 187 in _multicall
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 84 in <lambda>
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/manager.py", line 93 in _hookexec
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/pluggy/hooks.py", line 286 in __call__
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/config/__init__.py", line 162 in main
  File "/usr/local/miniconda/envs/mne/lib/python3.8/site-packages/_pytest/config/__init__.py", line 185 in console_main
  File "/usr/local/miniconda/envs/mne/bin/pytest", line 8 in <module>
/Users/runner/work/_temp/848dc3ed-f378-442c-bf2d-a133e80b43e9.sh: line 2:  1851 Segmentation fault: 11  pytest -vv mne/viz
mne/viz/_brain/tests/test_brain.py::test_brain_traces[pyvista-surface-lh] 

@GuillaumeFavelier
Copy link
Contributor Author

paintEvent on master:

    def paintEvent(self, ev):
        self._Iren.Render()

Could it be possible that processEvents() triggers paintEvent() before the render window is setup correctly? Considering that ensure_minimum_size() is called before show(), maybe there is an implicit order to follow here.

@GuillaumeFavelier
Copy link
Contributor Author

I'll refactor ensure_minimum_size() to verify

@larsoner
Copy link
Member

larsoner commented Feb 3, 2021

Sometimes it's in paintEvent of PySurfer, too:

https://github.com/mne-tools/mne-python/pull/8831/checks?check_run_id=1823678469#step:12:3075

Ideally this would be solved by VTK Render() being smart enough to be a no-op if it actually can't render, but in the meantime maybe we need to patch paintEvent in the respective vtkRenderWindowInteractor(s) at the right times to force it to be a no-op. This would mean somewhere in Brain and somewhere in PySurfer we could use a similar monkey-patching @contextmanager that makes paintEvent a no-op. WDYT?

This would not fix the failures we get with set_icon sometimes but it might get us part of the way there

@GuillaumeFavelier
Copy link
Contributor Author

I agree with you, patching paintEvent could help but technically how do you know when show() is done so that it can be restored?

I think we might need to patch showEvent as well and implement proper synchronization of those.

@larsoner
Copy link
Member

larsoner commented Feb 3, 2021

I agree with you, patching paintEvent could help but technically how do you know when show() is done so that it can be restored?

I would just patch it in a contextmanager during ensure_sizes to fix PyVista and when closing to fix PySurfer for now

@GuillaumeFavelier
Copy link
Contributor Author

This is easier to implement, I'll try your idea first.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 3, 2021

@larsoner I tried d5b1ba1, let me know if you had something similar different in mind.

I don't remember pysurfer architecture, I have to dig to find which one needs the fix.

@larsoner
Copy link
Member

larsoner commented Feb 3, 2021

Following the traceback downward:

  File "/usr/share/miniconda/envs/mne/lib/python3.8/site-packages/tvtk/pyface/ui/qt4/QVTKRenderWindowInteractor.py", line 370 in paintEvent
  File "/usr/share/miniconda/envs/mne/lib/python3.8/site-packages/tvtk/pyface/ui/qt4/scene.py", line 69 in paintEvent
  File "/usr/share/miniconda/envs/mne/lib/python3.8/site-packages/pyface/ui/qt4/gui.py", line 84 in process_events
  File "/usr/share/miniconda/envs/mne/lib/python3.8/site-packages/surfer/viz.py", line 183 in _force_render
  File "/usr/share/miniconda/envs/mne/lib/python3.8/site-packages/surfer/viz.py", line 2313 in _close
  File "/usr/share/miniconda/envs/mne/lib/python3.8/site-packages/surfer/viz.py", line 2300 in close

I would add the context manager to surfer.Brain.close or surfer.Brain._close. I don't think _force_render would be the right place.

@GuillaumeFavelier
Copy link
Contributor Author

I ran into this locally:

  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/tvtk/pyface/ui/qt4/QVTKRenderWindowInteractor.py", line 370 in paintEvent
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/tvtk/pyface/ui/qt4/scene.py", line 69 in paintEvent
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/pyface/ui/qt4/gui.py", line 84 in process_events
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/surfer/viz.py", line 183 in _force_render
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/surfer/viz.py", line 468 in __init__

so even init needs to be patched apparently.

@GuillaumeFavelier
Copy link
Contributor Author

Or better to patch _force_render directly actually since it seems related to both segfaults.

@GuillaumeFavelier
Copy link
Contributor Author

Nevermind, it can't be patched because it's not a Brain's method.

@GuillaumeFavelier
Copy link
Contributor Author

I patched close but for __init__ I ran out of ideas. It's too early to patch because the figures are not even created yet.

A good way would be to patch only _force_render and directly in PySurfer.

@larsoner
Copy link
Member

larsoner commented Feb 4, 2021

All green, one last nitpick then I think we're good!

@GuillaumeFavelier GuillaumeFavelier marked this pull request as ready for review February 4, 2021 13:54
@GuillaumeFavelier
Copy link
Contributor Author

pyvista/pyvistaqt#79 could be useful as well.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 4, 2021

Even with the patch, I still get this locally:

mne/viz/tests/test_3d.py::test_mixed_sources_plot_surface[mayavi] Fatal Python error: Segmentation fault

Current thread 0x00007f0ada6c9740 (most recent call first):
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/tvtk/pyface/ui/qt4/QVTKRenderWindowInteractor.py", line 370 in paintEvent
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/tvtk/pyface/ui/qt4/scene.py", line 69 in paintEvent
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/pyface/ui/qt4/gui.py", line 84 in process_events
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/surfer/viz.py", line 183 in _force_render
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/surfer/viz.py", line 2313 in _close
  File "/home/guillaume/source/anaconda3/envs/mne-py38/lib/python3.8/site-packages/surfer/viz.py", line 2300 in close
  File "/home/guillaume/source/mne-python/mne/viz/_3d.py", line 1922 in patched_close
  File "/home/guillaume/source/mne-python/mne/viz/tests/test_3d.py", line 782 in test_mixed_sources_plot_surface

Reproduced on linux_conda:

https://github.com/mne-tools/mne-python/runs/1831645005#step:12:3081

@larsoner
Copy link
Member

larsoner commented Feb 4, 2021

Maybe you patched the paintEvent of the wrong widget somehow?

@GuillaumeFavelier
Copy link
Contributor Author

Maybe you patched the paintEvent of the wrong widget somehow?

Maybe. When I inspected the test with pdb and it seems the _content widget is None.

@GuillaumeFavelier
Copy link
Contributor Author

e31de2e works for me locally. Waiting for CIs.

The only one I get randomly now is about set_icon.

@larsoner
Copy link
Member

larsoner commented Feb 4, 2021

Does your pyvistaqt PR fix that?

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Feb 4, 2021

I'll do pytest -vv mne/viz several times with update_app_icon=False (and pyvistaqt master) to see.

diff --git a/mne/viz/backends/_pyvista.py b/mne/viz/backends/_pyvista.py
index 769a479fb..c666d0784 100644
--- a/mne/viz/backends/_pyvista.py
+++ b/mne/viz/backends/_pyvista.py
@@ -73,6 +73,7 @@ class _Figure(object):
         self.store['off_screen'] = off_screen
         self.store['border'] = False
         self.store['auto_update'] = False
+        self.store['update_app_icon'] = False
         # multi_samples > 1 is broken on macOS + Intel Iris + volume rendering
         self.store['multi_samples'] = 1 if sys.platform == 'darwin' else 4
 
@@ -88,6 +89,7 @@ class _Figure(object):
             self.store.pop('show', None)
             self.store.pop('title', None)
             self.store.pop('auto_update', None)
+            self.store.pop('update_app_icon', None)
 
         if self.plotter is None:
             if self.plotter_class is BackgroundPlotter:

UPDATE: I ran pytest -vv mne/viz 6 times locally in those conditions and no segfault. Maybe I was lucky, I'll keep checking the CIs.

@larsoner
Copy link
Member

larsoner commented Feb 4, 2021

I'll go ahead and merge this to get things green for now, we'll need a release of pyvistaqt anyway if we're going to make that change

@GuillaumeFavelier
Copy link
Contributor Author

It's okay on my end.

@larsoner larsoner merged commit 2f7df88 into mne-tools:main Feb 4, 2021
@larsoner
Copy link
Member

larsoner commented Feb 4, 2021

Thanks @GuillaumeFavelier !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants