[WIP] Remove star imports, pin down public namespace

[adeak]

Overview

In a third-party PR thread it was pointed out that mypy makes some odd, opinionated choices when it handles the --strict flag. In a nutshell: when pyvista is installed as a dependency downstream and someone runs mypy --strict foo.py on a script that uses pyvista, mypy pretends that it doesn't know that pyvista has any attributes. For a bit more context see this mypy issue and this flag in the mypy docs.

So

1. since we use star imports in our inits, mypy would refuse to find attributes anyway,
2. but even if we switch to explicit named imports, we either have to do from subpackage import name as name (over my dead body) or do from subpackage import name, other_name, ... but additionally define __all__ (a list normally used to fine-tune star import behaviour; something our users shouldn't be doing anyway).

I don't like the necessary workaround with __all__ one bit, but getting rid of star imports is a reasonably noble task, so we might as well add some dirt to make mypy happy, since this would evidently benefit downstream libraries and maybe even some end users.

Details

What this is

This PR (expected to be WIP for a while) is meant as a step to remove the star imports, in order to facilitate discussions concerning concrete implementations. Since there are quite a few heavy design decisions to be made, I figured it's probably easier to talk about this if we have specifics to look at.

So as of the last commit when this PR was opened (5ade1f0) this is a first-guess attempt at removing star imports from the pyvista.utilities subpackage tree. I plan to progress with other subpackages in time in this PR, also as a function of any discussion here. For now the implementation intends to keep the API surface the same (especially for the main pyvista namespace), but I would recommend taking the opportunity to scrap whatever we can from the public API to get rid of some bloat.

Remarks

In no particular order of importance:

1. I've kept almost everything from the current pyvista namespace in pyvista.__all__, with the exception of names coming from the standard library or third-party modules (e.g. pyvista.np, pyvista.pi). I have, however, commented out a bunch of private-looking or unnecessary-looking things from the list, with the plan to delete these (and hopefully a bunch of others) before this PR is done.
2. I've added a new test module with the sole purpose of checking that any __all__s contained in __init__.pys only contain names that actually exist in the given namespace. I think this is useful, but I don't insist on this having its own test module.
3. For now I've kept duplicate items between pyvista.__all__ and pyvista.utilities.__all__. I think we should ideally only end up with any name being in at most one of these lists: if pyvista.utilities.misc.PyVistaDeprecationWarning is exposed as pyvista.PyVistaDeprecationWarning then we should not add this item to pyvista.utilities.__all__. (See also some of the questions below.)

(See also remarks below that I'll post as comments in the diff.)

Open questions

0. Do we actually want to go ahead with this work, to replace star imports with explicit imports? If yes, do we really want to add __all__s to make mypy happy?
1. Do we want to add module-level __all__s, e.g. pyvista.utilities.misc.__all__? I think not, because we're not actually trying to facilitate star imports, and mypy can surely figure out what names are available in actual "worker" modules.
2. As noted in the previous section: should we group each name being exposed into exactly one __all__? This could hint to users to only access a given attribute at the highest level where it's exposed. And this could help reduce the public API surface by explicitly pushing back less-important public attributes to lower-level __all__s.
3. Alternatively to the previous point: in the higher-level __init__.pys (specifically, pyvista.__init__), do we want to do things like from pyvista.utilities.misc import ... (as it is in this PR when it's opened), or do we want to build up subpackage namespaces level by level, and do from pyvista.utilities import ... (with one large wall of imports per top-level subpackage)? (If we snowball namespaces like this then this is probably a contradictory choice to the previous question. This snowballing would be mostly what we're doing now.)
4. Bikeshed level, but eventually important none the less: how should we order these huge import walls we'll end up with (meaning the order of names being imported in a given from ... import statement)? Do what isort would do? Group semantically? Group alphabetically, uppercase before/intermixed with lowercase names?
5. How important is it to preserve subpackage namespaces, e.g. pyvista.utilities.*? Do you think end users might rely on these? I took a glance at pyvistaqt (1.5th party library) and mne-python (3rd party direct dependent), and they don't seem to be doing any weird things, and any subpackages they access are in pyvista.plotting (which we'll probably want to mostly preserve anyway...).
6. How open are we to paring down the main public namespace at this time? If we want to be cautious we can preserve the status quo (within reason, see e.g. removing stdlib names) for now, and remove names once this PR is done (or later). But we could also take one deep breath and chop while we're here. As always it will be hard to tell in advance how many people might be affected by removing things we never intended to be fully public.

Example

Here's a simple script demonstrating the typing issues:

import pyvista

mesh = pyvista.PolyData
noise1 = pyvista.perlin_noise
noise2 = pyvista.utilities.perlin_noise
noise3 = pyvista.utilities.common.perlin_noise

When I run this function in an env where pyvista is a regular installed package, it runs fine, meaning that all of the above attributes exist in the respective namespaces. Running mypy --strict on the above script on main does

tmp_mypy.py:3: error: Module has no attribute "PolyData"  [attr-defined]
tmp_mypy.py:4: error: Module has no attribute "perlin_noise"  [attr-defined]
tmp_mypy.py:5: error: "Module pyvista.utilities" does not explicitly export attribute "perlin_noise"  [attr-defined]
Found 3 errors in 1 file (checked 1 source file)

and on this PR's branch (as of opening this PR) it does

tmp_mypy.py:3: error: Module has no attribute "PolyData"  [attr-defined]
Found 1 error in 1 file (checked 1 source file)

The reason for this remaining error is that even though we have 'PolyData' in pyvista.__all__, the remaining star import in pyvista.__init__ that pulls in the name from pyvista.core implies that mypy is blind to it. The other errors from earlier were resolved because in each __init__.py there are explicit imports and explicit mentions of 'perlin_noise' in __all__.

---

@pyvista/developers please chime in if you have the time and the opinions to decide how to tackle the fine details (and the broad ones) here. Even (especially) if you think we should take a very different road from what I've shown here. And please raise any points I missed.

[adeak]

I've only kept subpackages here that broke our own CI. Note that if we delete things from here it should only affect

import pyvista
pyvista.misc.something

whereas pyvista.utilities.misc.something could still work, especially if one does from pyvista.utilities.misc import something.

[adeak]

I've added this block because it was missing. This makes the API surface here larger, but it seemed more consistent with the status quo, and we can still remove what's unnecessary here later in the life of this PR. This also shows how names can be redundant: since we're using from pyvista.utilities.misc import ... in pyvista.__init__, we don't really have to have these here.

Depending on how we decide to structure the higher-level inits, we might end up with very lean subpackage inits.

[adeak]

Subpackages commented out to match imports in the main pyvista.__init__...

[adeak]

I figured we should skip subpackages which don't have an __all__ (which is most of our subpackages). If we go forward with this PR then we'll want to end up with __all__s anywhere, so it's a bit of an incentive to see SKIPPED instead of PASSED for these subpackages.

[codecov]

Codecov Report

Merging #3764 (15b7046) into main (c23df22) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3764   +/-   ##
=======================================
  Coverage   94.30%   94.30%           
=======================================
  Files          94       94           
  Lines       20321    20331   +10     
=======================================
+ Hits        19163    19173   +10     
  Misses       1158     1158           

[larsoner]

0. Do we actually want to go ahead with this work, to replace star imports with explicit imports? If yes, do we really want to add __all__s to make mypy happy?

I haven't thought about all this stuff, but just my general thought to guide how I try to think about things like this: I typically try to follow whatever the greater scientific Python stack does. For example I often look toward NumPy, since they have to deal with a ton of downstream users, a lot of code to maintain, finite bandwidth, and they think pretty hard about this stuff (and as an aside, I tend to appreciate / agree with their choices!). And by following their example, it's likely that things will work properly, and expertise can be shared, etc.

In this particular case, it looks like they do set __all__ in various ways:

https://github.com/numpy/numpy/blob/68d7aadd30cb7a4f6f32e76715b38b644df18602/numpy/__init__.py#L194

It seems like a high one-time cost to add this stuff, but after that it's not too bad. In SciPy we have __all__ defined, and any time I've needed to add some new function, it's easy enough to do:

from _newprivatething import my_fun

__all__ = [..., 'my_fun']

2. As noted in the previous section: should we group each name being exposed into exactly one all?

... so if we follow NumPy, they don't appear to adhere to this, because of the __all__.extend(othermod.__all__) in the link above. I'm not sure we should, either. pyvista.plotting.Plotter should exist, probably be in pyvista.plotting.__init__.__all__, yet pyvista.Plotter is the public API endpoint for this so it should definitely land in that __all__ for example. But I could be wrong about this particular point.

[adeak]

Thanks for the points @larsoner. Although some of the plumbing under numpy is terrifying, these are great data points :)

[adeak]

The doc build breakage is very confusing. It has one error for each of the includes in doc/api/core/helpers.rst, looking like this:

WARNING: autodoc: failed to import method 'helpers.wrap' from module 'pyvista'; the following exception was raised:
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sphinx/util/inspect.py", line 376, in safe_getattr
    return getattr(obj, name, *defargs)
AttributeError: module 'pyvista.plotting.helpers' has no attribute 'wrap'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sphinx/ext/autodoc/importer.py", line 98, in import_object
    obj = attrgetter(obj, mangled_name)
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sphinx/ext/autodoc/__init__.py", line 306, in get_attr
    return autodoc_attrgetter(self.env.app, obj, name, *defargs)
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sphinx/ext/autodoc/__init__.py", line 2804, in autodoc_attrgetter
    return safe_getattr(obj, name, *defargs)
  File "/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/sphinx/util/inspect.py", line 392, in safe_getattr
    raise AttributeError(name) from exc
AttributeError: wrap

In the .rst we refer to pyvista.helpers.wrap etc. which are aliases for pyvista.utilities.helpers.wrap

pyvista/doc/api/core/helpers.rst

Line 10 in 52b81ae

.. automethod:: pyvista.helpers.wrap

The catch is that the alias was already present, even though in pyvista/__init__.py I commented out its explicit import from pyvista.utilities (probably pulled in from side channels).

What's confusing is that the error sounds like it's trying to look up the names in pyvista.plotting.helpers rather than pyvista.utilities.helpers. I have no idea why, and I don't understand why this is happening. I've pushed a change to explicitly from pyvista.utilities import helpers, but I don't expect this to change anything because the alias was already there.

[adeak]

Nope, I just botched my local check. In the breaking commit I had

$ python -c 'import pyvista; print(pyvista.helpers)'
<module 'pyvista.plotting.helpers' from '/home/adeak/pyvista/pyvista/plotting/helpers.py'>

which explains everything. Restoring the right helpers fixed the issue.

Goes to show how we're on the right path by removing star imports :D

[adeak]

I added back pyvista.cells because examples/00-load/linear-cells.py references pyvista.cells.Wedge.

This must have caused broken intersphinx references in previous versions of this PR, so I'll have to test locally if these emit warnings, or can be told to do so. I wouldn't want to end up with a bunch of silently broken doc links due to this PR.

[adeak]

This is here because we have a pyvista.fileio.get_ext(...) in pyvista/plotting/plotting.py, but we also have pyvista.get_ext() so we should probably remove either.

[adeak]

This is here due to doc/api/core/helpers.rst so it's probably here for good, pyvista.helpers is likely to be used downstream.

[adeak]

This is here for a pyvista.reader.MultiBlockPlot3DReader and a pyvista.reader.Plot3DFunctionEnum, both available at the top-level namespace. But perhaps downstream expects this too...

[adeak]

This is here for pyvista.transformations.apply_transformation_to_points (in docs) and pyvista.transformations.axis_angle_rotation (in a handful of tests), and neither names are available at the top-level namespace.

[adeak]

I changed this and the other test in this module because these were the only internal uses of pyvista.pointset, and considering its contents it's very likely that anyone would/should be using the top-level names, as it was changed here.

[adeak]

I've merged the imports from pyvista.utilities.* into one big import. It seems more maintainable to let subpackages be responsible for their internal organization. If we spell out from pyvista.utilities.misc import ... here, future refactors moving things around in subpackages would make us have to change multiple imports. With the new logistics here we let the subdivision of pyvista.utilities.* be the private business of pyvista/utilities/__init__.py.

[adeak]

I've since restored the pyvista.colors name, but it might be cleaner to keep the "proper" import in the test here, not sure.

[adeak]

This is a major thorn in my side, but if I add pyvista_ndarray to the preceding block of imports (where it belongs) mypy thinks that what gets imported is the pyvista.core.pyvista_ndarray module rather than the pyvista.core.pyvista_ndarray.pyvista_ndarray class. Despite the fact that in pyvista/core/__init__.py it correctly finds that from .pyvista_ndarray import pyvista_ndarray refers to the class, not the module. So this seems like a mypy issue. I have no other idea to work around this than to use this ugly fully qualified import in pyvista/__init__.py.

[larsoner]

I doubt anyone in the wild has done from pyvista.core.pyvista_ndarray import pyvista_ndarray (right?) in which case you could git mv pyvista/core/pyvista_ndarray.py pyvista/core/_pyvista_ndarray.py and then adjust the imports. Seems cleaner anyway since pyvista.core.pyvista_ndarray is not meant to be a public module I assume...

[adeak]

Yeah, we're currently offering a very broad API surface (mostly unintentionally I think). These are partly the kinds of "public API paring down" steps I have in mind, but need to discuss in detail to make sure we're all on board and don't break downstream (or at least are ready to do that break and urge downstream to use proper import paths).

[banesullivan]

I'd like for us to put full attention on this and help finish this up for a 0.38.2 patch release if possible. PyVista's public namespace is having a handful of issues that I think this will resolve

[adeak]

Branch partly updated. Will have to manually add all the names that were added to main in the meantime... Using

git diff --no-color branch_root upstream/main |grep '^[+-]\(class\|def\) \|^+++ b'

and after some cleanup of the output I have

+++ b/pyvista/errors.py
+class PyVistaPipelineError(RuntimeError):
+++ b/pyvista/examples/downloads.py
+def download_head_2(load=True):  # pragma: no cover
+++ b/pyvista/plotting/actor_properties.py
+class ActorProperties:
+++ b/pyvista/plotting/colors.py
+def get_default_cycler():
+def get_hexcolors_cycler():
+def get_matplotlib_theme_cycler():
+def color_scheme_to_cycler(scheme):
+def get_cycler(color_cycler):
+++ b/pyvista/plotting/mapper.py
+class UnstructuredGridVolumeRayCastMapper(
+++ b/pyvista/plotting/opts.py
+class AnnotatedIntEnum(int, Enum):
+class InterpolationType(AnnotatedIntEnum):
+class RepresentationType(AnnotatedIntEnum):
+++ b/pyvista/plotting/prop3d.py
+class Prop3D(_vtk.vtkProp3D):
+++ b/pyvista/plotting/volume.py
+class Volume(Prop3D, _vtk.vtkVolume):
+++ b/pyvista/plotting/volume_property.py
+class VolumeProperty(_vtk.vtkVolumeProperty):
+++ b/pyvista/plotting/widgets.py
+def _parse_interaction_event(interaction_event):
+++ b/pyvista/themes.py
+class _LightingConfig(_ThemeConfig):
+class _TrameConfig(_ThemeConfig):
+class DocumentProTheme(DocumentTheme):
-class _ALLOWED_THEMES(Enum):
+class _NATIVE_THEMES(Enum):
+++ b/pyvista/trame/jupyter.py
+class TrameServerDownError(RuntimeError):
+class TrameJupyterServerDownError(RuntimeError):
+class Widget(HTML):
+def launch_server(server=None):
+def build_url(
+def initialize(
+def show_trame(
+def elegantly_launch(server):
+++ b/pyvista/trame/ui.py
+def button(click, icon, tooltip):
+def checkbox(model, icons, tooltip):
+class Viewer:
+def get_or_create_viewer(plotter, suppress_rendering=False):
+def plotter_ui(
+++ b/pyvista/trame/views.py
+class _BasePyVistaView:
+class PyVistaRemoteView(VtkRemoteView, _BasePyVistaView):
+class PyVistaLocalView(VtkLocalView, _BasePyVistaView):
+class PyVistaRemoteLocalView(VtkRemoteLocalView, _BasePyVistaView):
+++ b/pyvista/utilities/algorithms.py
+def algorithm_to_mesh_handler(mesh_or_algo, port=0):
+def set_algorithm_input(alg, inp, port=0):
+class PreserveTypeAlgorithmBase(_vtk.VTKPythonAlgorithmBase):
+class ActiveScalarsAlgorithm(PreserveTypeAlgorithmBase):
+class PointSetToPolyDataAlgorithm(_vtk.VTKPythonAlgorithmBase):
+class AddIDsAlgorithm(PreserveTypeAlgorithmBase):
+class CrinkleAlgorithm(_vtk.VTKPythonAlgorithmBase):
+def outline_algorithm(inp, generate_faces=False):
+def extract_surface_algorithm(inp, pass_pointid=False, pass_cellid=False, nonlinear_subdivision=1):
+def active_scalars_algorithm(inp, name, preference='point'):
+def pointset_to_polydata_algorithm(inp):
+def add_ids_algorithm(inp, point_ids=True, cell_ids=True):
+def crinkle_algorithm(clip, source):
+def cell_data_to_point_data_algorithm(inp, pass_cell_data=False):
+def point_data_to_cell_data_algorithm(inp, pass_point_data=False):
+def triangulate_algorithm(inp):
+def decimation_algorithm(inp, target_reduction):
+++ b/pyvista/utilities/reader.py
+class BaseVTKReader(ABC):
+class _PVDReader(BaseVTKReader):
+++ b/pyvista/utilities/regression.py
+def run_image_filter(imfilter: _vtk.vtkWindowToImageFilter):

Almost all these names will have to be imported and added to the respective __all__s...

I'd like for us to put full attention on this and help finish this up for a 0.38.2 patch release if possible.

@banesullivan are you sure it's a good idea to add potentially breaking namespace changes to a patch release?

[banesullivan]

are you sure it's a good idea to add potentially breaking namespace changes to a patch release?

ah. If things are breaking, no. I need to dive into this PR and see how much this is breaking and see what we can preserve

[adeak]

are you sure it's a good idea to add potentially breaking namespace changes to a patch release?

ah. If things are breaking, no. I need to dive into this PR and see how much this is breaking and see what we can preserve

There's two parts of this PR:

1. Remove star imports, preserving the namespaces as closely as possible (only removing obvious-looking cruft)
2. Reduce public namespace.

The work already done is for point 1 and almost all done (pending work in previous comment). The discussion I was hoping for here is for point 2.

But you said

PyVista's public namespace is having a handful of issues that I think this will resolve

Any such change can come from point 2 (if I do point 1 right). Solving "namespace issues" is inherently a breaking change because it will involve changing names in the API.

[adeak]

I've added everything from the above list with the exception of

#skip:
+++ b/pyvista/errors.py
+class PyVistaPipelineError(RuntimeError):
+++ b/pyvista/plotting/prop3d.py
+class Prop3D(_vtk.vtkProp3D):

# skip although technically visible through pyvista.widgets:
+++ b/pyvista/plotting/widgets.py
+def _parse_interaction_event(interaction_event):

The first two names seemed missing from the public API entirely, and while the last one is visible via pyvista.widgets, I removed pyvista.widgets entirely in this PR because I'm pretty sure it's not meant to be directly accessible like that.

TODO still:

1. keep merging main, checking if the public API changes
2. build docs locally to see any missing stuff, including intersphinx links
3. devise a rigorous check to compare the namespaces on main vs this branch to make sure only fluff was removed.