Refactor dash_vtk.util to import vtkmodules instead of vtk #21

xhluca · 2021-02-04T23:40:24Z

import vtkmodules avoid importing the libGL library in vtk v9.0.1, as opposed to import vtk which always loads (thus requires) libgl

About

Description of changes

Pre-Merge checklist

The project was correctly built with npm run build.
If there was any conflict, it was solved correctly.
All changes were documented in CHANGELOG.md.
All tests on CircleCI have passed.
All Percy visual changes have been approved.
Two people have 💃'd the pull request. You can be one of these people if you are a Dash core contributor.

Reference Issues

Closes #[issue number]

Other comments

jourdain · 2021-02-04T23:43:29Z

dash_vtk/utils/vtk.py

@@ -1,6 +1,6 @@
 import dash_vtk

-from vtk.util.numpy_support import vtk_to_numpy
+from vtkmodules.util.numpy_support import vtk_to_numpy
 from vtk.vtkFiltersGeometry import vtkGeometryFilter


do the same here

from vtkmodules.vtkFiltersGeometry import vtkGeometryFilter

xhluca · 2021-02-05T19:01:12Z

Seems like the py-3.6 tests are failing:

 tests/test_render.py::test_volume_rendering ⨯                    62% ██████▍   

―――――――――――――――――――――――――――――― test_usage_vtk_cfd ――――――――――――――――――――――――――――――

dash_duo = <dash.testing.composite.DashComposite object at 0x7f48047cfc18>

    def test_fn(dash_duo):
>       app = import_app(name)

tests/test_render.py:33: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
venv/lib/python3.6/site-packages/dash/testing/application_runners.py:43: in import_app
    app_module = runpy.run_module(app_file)
/usr/local/lib/python3.6/runpy.py:208: in run_module
    return _run_code(code, {}, init_globals, run_name, mod_spec)
/usr/local/lib/python3.6/runpy.py:85: in _run_code
    exec(code, run_globals)
demos/usage-vtk-cfd/app.py:13: in <module>
    from dash_vtk.utils import to_mesh_state, preset_as_options
dash_vtk/utils/__init__.py:1: in <module>
    from .vtk import to_mesh_state, to_volume_state, presets, preset_as_options
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

    import dash_vtk
    
>   from vtkmodules.util.numpy_support import vtk_to_numpy
E   ModuleNotFoundError: No module named 'vtkmodules'

dash_vtk/utils/vtk.py:3: ModuleNotFoundError

@jourdain does vtkmodules work in python 3.6?

jourdain · 2021-02-05T19:04:00Z

You are still using vtk@5.8? I would think so...

xhluca · 2021-02-05T19:57:04Z

Let me upgrade the vtk dependencies

xhluca · 2021-02-05T22:45:21Z

All the tests failed:

 tests/test_render.py::test_usage_vtk_cfd ⨯                       75% ███████▌  
 tests/test_render.py::test_usage_algorithm ✓                     88% ████████▊ 
 tests/test_render.py::test_synthetic_volume_rendering ✓         100% ██████████
=============================== warnings summary ===============================
tests/test_render.py::test_pyvista_terrain_following_mesh
  /home/circleci/project/venv/lib/python3.7/site-packages/transforms3d/quaternions.py:26: DeprecationWarning: `np.float` is a deprecated alias for the builtin `float`. To silence this warning, use `float` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.float64` here.
  Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
    _MAX_FLOAT = np.maximum_sctype(np.float)

tests/test_render.py::test_pyvista_terrain_following_mesh
  /home/circleci/project/venv/lib/python3.7/site-packages/transforms3d/quaternions.py:27: DeprecationWarning: `np.float` is a deprecated alias for the builtin `float`. To silence this warning, use `float` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.float64` here.
  Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
    _FLOAT_EPS = np.finfo(np.float).eps

tests/test_render.py::test_pyvista_point_cloud
tests/test_render.py::test_pyvista_point_cloud
  /home/circleci/project/venv/lib/python3.7/site-packages/vtk/util/numpy_support.py:74: DeprecationWarning: `np.bool` is a deprecated alias for the builtin `bool`. To silence this warning, use `bool` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.bool_` here.
  Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
    _vtk_np = {vtk.VTK_BIT:numpy.bool,

-- Docs: https://docs.pytest.org/en/stable/warnings.html

Results (67.61s):
       4 passed
       4 failed
         - tests/test_render.py:32 test_pyvista_terrain_following_mesh
         - tests/test_render.py:32 test_slice_rendering
         - tests/test_render.py:32 test_volume_rendering
         - tests/test_render.py:32 test_usage_vtk_cfd

Exited with code exit status 1
CircleCI received exit code 1

xhluca · 2021-02-05T23:08:08Z

So it seems like vtkmodules was added in 8.2.0 (see release 8.2.0 vs 8.1.2 where it didn't exist). I'll change the requirements

xhluca · 2021-02-05T23:43:34Z

Wow, seems like circleci for python-3.6 can't even find vtk==9.0.1 on pypi 🤦

  Could not find a version that satisfies the requirement vtk>=8.2.* (from -r requirements.txt (line 4)) (from versions: 8.0.0.dev20170717, 8.1.0, 8.1.1, 8.1.2)
No matching distribution found for vtk>=8.2.* (from -r requirements.txt (line 4))
You are using pip version 18.1, however version 21.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.

xhluca · 2021-02-06T21:01:40Z

I feel we should go with a try/except when it comes to importing the library, this way if vtkmodules are not available it falls back to vtk. For dash_vtk/utils/vtk.py, it'd be something like that:

try:
    # v9 and above
    from vtkmodules.util.numpy_support import vtk_to_numpy
    from vtkmodules.vtkFiltersGeometry import vtkGeometryFilter
except:
    # v8.1.2 and below
    from vtk.util.numpy_support import vtk_to_numpy
    from vtk.vtkFiltersGeometry import vtkGeometryFilter

Thoughts? @jourdain

jourdain · 2021-02-06T22:42:09Z

Sounds good

…

On Sat, Feb 6, 2021 at 14:01 Xing Han Lu ***@***.***> wrote: I feel we should go with a try/except when it comes to importing the library, this way if vtkmodules are not available it falls back to vtk. For dash_vtk/utils/vtk.py, it'd be something like that: try: from vtkmodules.util.numpy_support import vtk_to_numpy from vtkmodules.vtkFiltersGeometry import vtkGeometryFilterexcept: from vtk.util.numpy_support import vtk_to_numpy from vtk.vtkFiltersGeometry import vtkGeometryFilter Thoughts? @jourdain <https://github.com/jourdain> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACH45UXLS3ZSYM6FY4QLFTS5WU4HANCNFSM4XDYDOHA> .

xhluca · 2021-02-08T18:38:05Z

@jourdain I don't think vtkmodules resolve the problem.

See the logs from a test heroku app that imports vtkmodules instead of `vtk:

2021-02-08T18:36:15.429362+00:00 app[web.1]:     return _bootstrap._gcd_import(name[level:], package, level)
2021-02-08T18:36:15.429362+00:00 app[web.1]:   File "/app/.heroku/python/lib/python3.6/site-packages/vtkmodules/all.py", line 29, in <module>
2021-02-08T18:36:15.429363+00:00 app[web.1]:     from .vtkRenderingOpenGL2 import *
2021-02-08T18:36:15.429430+00:00 app[web.1]: ImportError: libGL.so.1: cannot open shared object file: No such file or directory
2021-02-08T18:36:15.430248+00:00 app[web.1]: [2021-02-08 18:36:15 +0000] [10] [INFO] Worker exiting (pid: 10)
2021-02-08T18:36:15.593373+00:00 heroku[router]: at=error code=H13 desc="Connection closed without response" method=GET path="/" host=test-vtk-deploy.herokuapp.com request_id=68847d42-ab72-4b70-a9ff-94be92a42c1e fwd="160.32.220.130" dyno=web.1 connect=1ms service=582ms status=503 bytes=0 protocol=https
2021-02-08T18:36:15.696799+00:00 app[web.1]: [2021-02-08 18:36:15 +0000] [4] [INFO] Shutting down: Master
2021-02-08T18:36:15.697103+00:00 app[web.1]: [2021-02-08 18:36:15 +0000] [4] [INFO] Reason: Worker failed to boot.

jourdain · 2021-02-08T19:03:43Z

Arg... I guess you may have to do the patching if you can like Jcr was talking about...

jourdain · 2021-02-08T19:16:06Z

Do you have the full stack? I'm not sure why we get vtkmodules.all

xhluca · 2021-02-08T19:20:54Z

Unfortunately heroku truncates the full stack :/

xhluca · 2021-02-08T19:21:43Z

I'm not sure how to patch a library inside dash-vtk's setup.py, which is what we would need if we are to use it inside utils; right?

jourdain · 2021-02-08T19:52:01Z

I don't know if there is a post-setup or something that can be executed once vtk get installed to edit/patch it.

xhluca · 2021-02-08T20:26:09Z

Seems like the deployment works: https://dash-docs-pr-1091.herokuapp.com/

@jourdain

jcfr · 2021-02-08T20:55:19Z

Arg... I guess you may have to do the patching if you can like Jcr was talking about...

If still needed, this could be done at the end of dash_vtk/setup.py

import sysconfig
site_packages_dir = sysconfig.get_paths()['purelib']

vtk_init_py = site_packages_dir + "/vtk/__init__.py"
print("Patching %s" % vtk_init_py)

has_openglkit_import = False
updated_lines = []
with open(vtk_init_py, "r") as myfile:
    for line in myfile.readlines():
        if line != "from .vtkOpenGLKit import *\n":
            updated_lines.append(line)
        else:
            has_openglkit_import = True

if has_openglkit_import:
    with open(vtk_init_py, "w") as myfile:
        myfile.writelines(updated_lines)

print("Patching %s  [%s]" % (vtk_init_py, "done" if has_openglkit_import else "skipped"))

jcfr · 2021-02-08T20:56:47Z

Logic would have to be tweaked based to be more robust and gracefully skip in the case of vtk 9 ...

xhluca · 2021-02-08T22:18:13Z

This should work for 9.0.1 but I feel this approach might be very brittle and would require a lot of error catching since there's a lot of variables (people uninstalling/reinstalling vtk, someone installing dash-vtk alongside vtk but uses vtk for other purposes and finds out they can't import libgl anymore, etc.). In the end it could create more friction.

I'd like to backtrack on the reasons why this PR was started and my various attempts at bypassing libgl. I feel it might be better to write instructions in the Readme.md and the docs to point people towards the right approach to install libgl (aptfile on heroku, apt-packages file on dash enteprise, apt-get instructions for local, etc.). Although those capabilities are not needed for dash-vtk, it sounds safer than modifying the source code of a dependency just to use dash-vtk. Sorry for all the trouble.

EDIT: *In the end we agreed to keep the try/except so we can use vtkmodules whenever it is possible, and fall back to vtk when its not. instructions about aptfile/apt-packages were added to the readme.

xhluca · 2021-02-08T23:55:39Z

@jourdain I think this is ready to be merged

jourdain · 2021-02-08T23:57:16Z

Yes LGTM

jourdain

LGTM

Update vtk.py

6e99185

jourdain reviewed Feb 4, 2021

View reviewed changes

vtk -> vtkmodules import

322bc72

Fix vtk to 8.* in demos requirements.txt

a621757

xhluca changed the title ~~Update vtk.py~~ Refactor usage of vtk to avoid libgl errors Feb 5, 2021

xhlulu added 2 commits February 5, 2021 18:10

vtk==8.* -> vtk>=8.2.*

9211f02

Add vtk to root requirements.txt

55af292

xhlulu added 5 commits February 8, 2021 13:15

Add try-except for vtkmodules importing

2d73669

Relax requirements

c75761a

Relax base requirement

3a1858a

Change circleci caching behavior

5fb6cdb

Update requirements

a7ccec8

xhlulu added 4 commits February 8, 2021 18:00

dash[dev] >= 1.15.0

610b25b

oops

f4d971c

Add message about using vtkmodules

1bb7545

Update pip version

5df7817

xhluca marked this pull request as ready for review February 8, 2021 23:55

xhluca requested review from alexcjohnson and Marc-Andre-Rivet as code owners February 8, 2021 23:55

xhluca changed the title ~~Refactor usage of vtk to avoid libgl errors~~ Refactor dash_vtk.util to import vtkmodules instead of vtk Feb 8, 2021

jourdain self-requested a review February 8, 2021 23:57

jourdain approved these changes Feb 8, 2021

View reviewed changes

xhluca merged commit 5dd3238 into master Feb 8, 2021

xhluca deleted the utils-vtkmodules branch February 9, 2021 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dash_vtk.util to import vtkmodules instead of vtk #21

Refactor dash_vtk.util to import vtkmodules instead of vtk #21

xhluca commented Feb 4, 2021 •

edited

jourdain Feb 4, 2021

xhluca commented Feb 5, 2021

jourdain commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 6, 2021 •

edited

jourdain commented Feb 6, 2021 via email

xhluca commented Feb 8, 2021

jourdain commented Feb 8, 2021

jourdain commented Feb 8, 2021

xhluca commented Feb 8, 2021

xhluca commented Feb 8, 2021

jourdain commented Feb 8, 2021

xhluca commented Feb 8, 2021

jcfr commented Feb 8, 2021

jcfr commented Feb 8, 2021

xhluca commented Feb 8, 2021 •

edited

xhluca commented Feb 8, 2021

jourdain commented Feb 8, 2021

jourdain left a comment

Refactor dash_vtk.util to import vtkmodules instead of vtk #21

Refactor dash_vtk.util to import vtkmodules instead of vtk #21

Conversation

xhluca commented Feb 4, 2021 • edited

About

Description of changes

Pre-Merge checklist

Reference Issues

Other comments

jourdain Feb 4, 2021

Choose a reason for hiding this comment

xhluca commented Feb 5, 2021

jourdain commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 5, 2021

xhluca commented Feb 6, 2021 • edited

jourdain commented Feb 6, 2021 via email

xhluca commented Feb 8, 2021

jourdain commented Feb 8, 2021

jourdain commented Feb 8, 2021

xhluca commented Feb 8, 2021

xhluca commented Feb 8, 2021

jourdain commented Feb 8, 2021

xhluca commented Feb 8, 2021

jcfr commented Feb 8, 2021

jcfr commented Feb 8, 2021

xhluca commented Feb 8, 2021 • edited

xhluca commented Feb 8, 2021

jourdain commented Feb 8, 2021

jourdain left a comment

Choose a reason for hiding this comment

xhluca commented Feb 4, 2021 •

edited

xhluca commented Feb 6, 2021 •

edited

xhluca commented Feb 8, 2021 •

edited