Support the no-VTK OCP modules for testing cadquery_ocp_novtk

[bernhard-42]

ocp-build-system creates a no-VTK version cadquery_ocp_novtk.
To test it I use a patched build123d
Would it be possible to add the following change:

diff --git a/src/build123d/topology/shape_core.py b/src/build123d/topology/shape_core.py
index 7ebeaae..487541f 100644
--- a/src/build123d/topology/shape_core.py
+++ b/src/build123d/topology/shape_core.py
@@ -48,6 +48,7 @@ from __future__ import annotations

 import copy
 import itertools
+import os
 import warnings
 from abc import ABC, abstractmethod
 from typing import (
@@ -104,8 +105,9 @@ from OCP.GProp import GProp_GProps
 from OCP.Geom import Geom_Line
 from OCP.GeomAPI import GeomAPI_ProjectPointOnSurf
 from OCP.GeomLib import GeomLib_IsPlanarSurface
-from OCP.IVtkOCC import IVtkOCC_Shape, IVtkOCC_ShapeMesher
-from OCP.IVtkVTK import IVtkVTK_ShapeData
+if os.environ.get("NOVTK") is None:
+    from OCP.IVtkOCC import IVtkOCC_Shape, IVtkOCC_ShapeMesher
+    from OCP.IVtkVTK import IVtkVTK_ShapeData
 from OCP.Prs3d import Prs3d_IsoAspect
 from OCP.Quantity import Quantity_Color
 from OCP.ShapeAnalysis import ShapeAnalysis_Curve
@@ -152,8 +154,9 @@ from build123d.geometry import (
 from typing_extensions import Self

 from typing import Literal
-from vtkmodules.vtkCommonDataModel import vtkPolyData
-from vtkmodules.vtkFiltersCore import vtkPolyDataNormals, vtkTriangleFilter
+if os.environ.get("NOVTK") is None:
+    from vtkmodules.vtkCommonDataModel import vtkPolyData
+    from vtkmodules.vtkFiltersCore import vtkPolyDataNormals, vtkTriangleFilter


 if TYPE_CHECKING:  # pragma: no cover

There would be no change in build123d behaviour when NOVTK isn't set, which is the default on every machine.

However, I could set it in my github action for the test:

NOVTK=1 pytest --ignore tests/test_direct_api/test_jupyter.py --ignore tests/test_direct_api/test_vtk_poly_data.py

[bernhard-42]

This approach would be even better:

diff --git a/src/build123d/topology/shape_core.py b/src/build123d/topology/shape_core.py
index 7ebeaae..feda338 100644
--- a/src/build123d/topology/shape_core.py
+++ b/src/build123d/topology/shape_core.py
@@ -104,8 +104,13 @@ from OCP.GProp import GProp_GProps
 from OCP.Geom import Geom_Line
 from OCP.GeomAPI import GeomAPI_ProjectPointOnSurf
 from OCP.GeomLib import GeomLib_IsPlanarSurface
-from OCP.IVtkOCC import IVtkOCC_Shape, IVtkOCC_ShapeMesher
-from OCP.IVtkVTK import IVtkVTK_ShapeData
+try:
+    from OCP.IVtkOCC import IVtkOCC_Shape, IVtkOCC_ShapeMesher
+    from OCP.IVtkVTK import IVtkVTK_ShapeData
+    from vtkmodules.vtkCommonDataModel import vtkPolyData
+    from vtkmodules.vtkFiltersCore import vtkPolyDataNormals, vtkTriangleFilter
+except:
+    warnings.warn("No VTK support")
 from OCP.Prs3d import Prs3d_IsoAspect
 from OCP.Quantity import Quantity_Color
 from OCP.ShapeAnalysis import ShapeAnalysis_Curve
@@ -152,9 +157,6 @@ from build123d.geometry import (
 from typing_extensions import Self

 from typing import Literal
-from vtkmodules.vtkCommonDataModel import vtkPolyData
-from vtkmodules.vtkFiltersCore import vtkPolyDataNormals, vtkTriangleFilter
-

 if TYPE_CHECKING:  # pragma: no cover
     from .zero_d import Vertex  # pylint: disable=R0801

[bernhard-42]

This is not meant to be used by users of build123d and should not be documented (at the end, it is not straightforward to install build123d without VTK).
It is only for the test case of ocp-build-system, so that I do not need to patch build123d. As such, I'd say it is ok that one can still call vtk based routines and get an error. The tests will omit the two test cases

[jdegenstein]

I support this idea.

[snoyer]

I'd say it is ok that one can still call vtk based routines and get an error.

looks like the VTK stuff is only used in Shape.to_vtk_poly_data() so maybe it can even be fully accounted for by having a try/except NameError in there? Something like:

try:
    from OCP.IVtkOCC import IVtkOCC_Shape, IVtkOCC_ShapeMesher
    from OCP.IVtkVTK import IVtkVTK_ShapeData
    from vtkmodules.vtkCommonDataModel import vtkPolyData
    from vtkmodules.vtkFiltersCore import vtkPolyDataNormals, vtkTriangleFilter
except ImportError:
    pass # don't warn for now

    def to_vtk_poly_data(self, ...):
        try:
            ... # original VTK stuff
        except NameError:
            raise ImportError("optional VTK modules are required") # or something?

edit: actually it's also used in jupyter_tools.py, nervermind

[bernhard-42]

@snoyer My idea was to change the absolute minimum that allows to run the tests.
I don't want to block any development on build123d that might involve vtk in the future.

The proposed try ... except change ensures that build123d can be imported.
Combined with --ignore for the vtk tests this is all actually needed.

[snoyer]

@bernhard-42 I understand what you're trying to do and I understand that it would not be such a big deal to skip the failing import and let the VTK methods crash on call (because the import would nominally/hopefully not be missing for real users).

However that's still asking build123d to accommodate for OCP in a way that leaves their code with potentially known unintentional exceptions (however unlikely they may be). I was just trying to see if there was a minimally intrusive way to tie up these loose ends and go from "not a big deal" to "not a deal at all" (by expecting the crash in the VTK methods and warning properly from there).

As mentioned in my edit the changes would not be as minimal as I thought so it's possibly not worth doing more that what you suggest.

(the real proper way to do it would be to refactor and only import VTK when needed in Jupyter/IPython contexts instead of baking it into the shape class, but that's another discussion)

[gumyr]

I prefer to solve this by moving the vtk functionality out of shape_core entirely. Who is the consumer of Shape.to_vtk_poly_data? The only user in build123d is writer.SetInputData(shape.to_vtk_poly_data(tolerance, angular_tolerance, True))in jupyter_tools.py. Could this method be made a function instead and put into another module, like jupyter_tools.py? It seems like making it optional would then be much easier.

[bernhard-42]

I created a pull request #887 that moves all VTK code to vtk_tools.py