Skip to content
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.

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

Workarounds for VTK 9.1.0 #1097

Merged
merged 9 commits into from Nov 10, 2021

Conversation

larsoner
Copy link
Contributor

@larsoner larsoner commented Oct 16, 2021

This PR is now ready to go with a few caveats. As commented by @prabhuramachandran in #1097 (comment), we are observing segfaults when we test against vtk 9.1.0 from PyPI. See GitHub Actions CI output on commits a63b08a, a053a10 or 53345b1. The segfaults with the VTK 9.1.0 are happening on all platforms.

For now, we are choosing to ignore those failures and we are only testing against vtk 9.0.3 on CI but note that we have tested these changes locally on Mac OS and Windows.

Original PR description by @larsoner

Depends/builds on #1050. Now I can at least get it to build+install on 9.1.0-rc2, but now I get:

mne/viz/tests/test_3d.py ..F
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> traceback >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
mne/viz/tests/test_3d.py:125: in test_plot_sparse_source_estimates
    brain = plot_source_estimates(
<decorator-gen-163>:24: in plot_source_estimates
    ???
mne/viz/_3d.py:1795: in plot_source_estimates
    return _plot_stc(
mne/viz/_3d.py:1877: in _plot_stc
    brain = Brain(**kwargs)
../PySurfer/surfer/viz.py:488: in __init__
    brain = _Hemisphere(subject_id, **kwargs)
../PySurfer/surfer/viz.py:3078: in __init__
    self._geo_mesh = mlab.pipeline.triangular_mesh_source(
../mayavi/mayavi/tools/sources.py:1401: in triangular_mesh_source
    data_source.reset(x=x, y=y, z=z, triangles=triangles, scalars=scalars)
../mayavi/mayavi/tools/sources.py:831: in reset
    pd.polys = None
E   traits.trait_errors.TraitError: Cannot set the undefined 'polys' attribute of a 'PolyData' object.

Two issues I had to fix on top of #1050

  1. V. was missing in the signatures, so I had to work around it
  2. Arguments had type hints, so I had to sanitize those

I'm not 100% sure I did either of these correctly, but again it at least let me build and try doing something, and even import mayavi (which indicates that tvtk.Version.vtk_version is exposed properly as a trait/property now). The fact that PolyData is broken is not a good sign, though.

This is about all the time I can devote to this, it would be good if someone else could look and fix things for 9.1.0...

@larsoner larsoner changed the title WIP: Workaround more WIP: Workarounds for 9.1.0-rc2 Oct 16, 2021
@prabhuramachandran
Copy link
Member

Thank you, I will be taking a look at this tomorrow and later this week. Now that 9.1 is out, I want to fix this ASAP and push out a release that works with the latest VTK.

@larsoner
Copy link
Contributor Author

larsoner commented Nov 8, 2021

Sounds good, feel free to take over or push commits here if you want @prabhuramachandran !

@prabhuramachandran
Copy link
Member

@larsoner -- thanks, do you mind if I force-push once I make some headway? I rebased to master.

@larsoner
Copy link
Contributor Author

larsoner commented Nov 8, 2021

Yes feel free

With this all the mayavi tests pass but a little more work is needed.
- Fixes issues due to changes to the docstrings.
- Fixes method signatures with callbacks and default arguments.
- Fixes some test errors.
@prabhuramachandran
Copy link
Member

So the tests are passing for me on macos with VTK 9.1 but there is some other issue on Linux with gh-actions, @rahulporuri can you please take a look. I hate that the tests stop when one architecture fails and do not have any more time this week.

@prabhuramachandran
Copy link
Member

Looks like there are some major segfaults with VTK-9.1.0. The tests pass well with 9.0.3 and most of the time locally with 9.1.0 for me but on CI they segfault repeatably without any error message. Locally I get a traceback like this:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x0000000165aa4c5f libvtkRenderingFreeType-9.1.dylib`vtkMathTextFreeTypeTextRenderer::SetScaleToPowerOfTwoInternal(bool) + 47
    frame #1: 0x0000000165b84c9f libvtkRenderingCore-9.1.dylib`vtkFlagpoleLabel::vtkFlagpoleLabel() + 559
    frame #2: 0x0000000165b83d04 libvtkRenderingCore-9.1.dylib`vtkFlagpoleLabel::New() + 52
    frame #3: 0x000000016c030668 libvtkRenderingVR-9.1.dylib`vtkOpenGLAvatar::vtkOpenGLAvatar() + 328
    frame #4: 0x000000016c0304dc libvtkRenderingVR-9.1.dylib`vtkOpenGLAvatar::New() + 28
    frame #5: 0x000000016499c811 libvtkWrappingPythonCore3.8-9.1.dylib`PyVTKObject_FromPointer + 609
    frame #6: 0x000000016499c599 libvtkWrappingPythonCore3.8-9.1.dylib`PyVTKObject_New + 153
    frame #7: 0x00000001000c3829 python`type_call + 41

So this does not seem to be mayavi specific although I need to investigate a little more. I think the build issues are fixed in this PR.

@rahulporuri rahulporuri changed the title WIP: Workarounds for 9.1.0-rc2 Workarounds for VTK 9.1.0-rc2 Nov 10, 2021
@rahulporuri rahulporuri changed the title Workarounds for VTK 9.1.0-rc2 Workarounds for VTK 9.1.0 Nov 10, 2021
Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

Mostly LGTM.

To summarize the contents of this PR, it looks like we're modifying the docstrings to remove the V. prefix, handling the awkward initial condition for vtkEuclideanClusterExtraction.Radius$. Beyond these, we seem to be special casing some more vtk classes when generating wrappers. Everything else feels unimportant. @prabhuramachandran is that an apt summary?

Note : I tested the MayaVi application on this branch locally and it seemed to work fine. The mayavi testsuite crashed on my machine when i used unittest discover and nosetests but the tvtk testsuite ran with some failures. The tvtk test failures are only observed with unittest discover, not nosetests.

@@ -206,7 +226,7 @@ def test_method_signature(self):
p.get_method_signature(o.GetColor))
if hasattr(vtk, 'vtkArrayCoordinates'):
self.assertEqual([([None], ('float', 'float', 'float')),
([None], (['float', 'float', 'float'],))],
([None], (['float', 'float', 'float'],))],
Copy link
Contributor

Choose a reason for hiding this comment

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

tangential : after we do the release, i would really like to run black on the codebase to address all such indentation/style issues in one go.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, although I do not like black, maybe blue?

@prabhuramachandran
Copy link
Member

@rahulporuri -- thanks, yes the summary seems accurate to me.

@prabhuramachandran
Copy link
Member

Thanks. Merging for now, will look at the segfaults a little later.

@prabhuramachandran prabhuramachandran merged commit 29d7409 into enthought:master Nov 10, 2021
@larsoner larsoner deleted the workaround-more branch November 10, 2021 14:44
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.

None yet

3 participants