➕ Extra import fix of example

[tkoyama010]

Overview

Extra import fix of example. I found this in the sphinx compiling in local. We could not find the error by pytest --doctest-modules . Is there any good way to find this error in CI?

Details

• Same with ➕ Fix import of example #1424

[codecov]

Codecov Report

Merging #1425 (2acbe25) into main (8d0fe22) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1425   +/-   ##
=======================================
  Coverage   90.05%   90.05%           
=======================================
  Files          65       65           
  Lines       12437    12437           
=======================================
  Hits        11200    11200           
  Misses       1237     1237           

[akaszynski]

Is there any good way to find this error in CI?

Issue here is the namespace is shared between docstrings. I don't think there's a way getting around this, but I think @adeak might have looked into it.

Approving as CI appears to have run correctly.

[akaszynski]

Merging too since admin can override checks passing.

[tkoyama010]

@akaszynski Thanks

[adeak]

It seems the issue is deeper than I'd thought: it's that module globals are visible from doctests!

I've opened a discussion at pytest-dev/pytest#8791 after not being able to find a solution with an hour's digging.

[tkoyama010]

@adeak Thanks. Fantastic job!

[adeak]

So according to the pytest maintainer avoiding this is not supported, and not even really possible. Bummer.

I'm wondering how hard it would be to use stdlib doctest as a parser to do this on our own:

import doctest
import pyvista as pv

# find and parse all docstrings in core.filters.poly_data
doctests = doctest.DocTestFinder().find(pv.core.filters.poly_data, globs={})
# 37 docstrings, out of which 25 have doctests (non-empty .example attr)

# find the first non-empty doctest
doct = next(dt for dt in doctests if dt.examples)
# <DocTest pyvista.core.filters.poly_data.PolyDataFilters.clean from /home/adeak/pyvista/pyvista/core/filters/poly_data.py:1131 (7 examples)>

# print each doctest row with expected output
for example in doct.examples:
    print(example.source.rstrip())
    if example.want:
        print(example.want)

This prints the following:

import pyvista as pv
import numpy as np
points = np.array([[0, 0, 0], [0, 1, 0], [1, 0, 0]])
faces = np.array([3, 0, 1, 2, 3, 0, 3, 3])
mesh = pv.PolyData(points, faces)
mout = mesh.clean()
print(mout.faces)  # doctest:+SKIP
[3 0 1 2]

We'd "only" have to

1. walk a tree of pyvista's structure, passing every interesting submodule into DocTestFinder
2. parse each doctest example by example, using an initially empty globals() dict
3. execute each line and comparing outputs, watching out for doctest: +SKIPs.

In other words, we'd only have to reimplement a skeleton of pytest 😐 Perhaps we can just stick to checking missing imports, which would mean we can just exec() each example one after the other, and this would then raise. Whether the actual results are correct would be taken care of the doctesting of pytest or sphinx...

[adeak]

OK, here's another proof of concept because apparently that's my thing now:

In [23]: import doctest
    ...: from functools import partial
    ...: from io import StringIO
    ...: import pyvista as pv
    ...: pv.OFF_SCREEN = True
    ...: 
    ...: # find and parse all docstrings in a submodule that I found to have a broken doctest
    ...: doctests = doctest.DocTestFinder().find(pv.core.pointset, globs={})
    ...: 
    ...: for dt in doctests:
    ...:     if not dt.examples:
    ...:         continue
    ...:     globs = {'print': partial(print, file=StringIO())}
    ...:     for example in dt.examples:
    ...:         try:
    ...:             exec(example.source, globs)
    ...:         except Exception as e:
    ...:             print(f'FAILED: {dt}! ({e})')
    ...:             break
    ...:     else:
    ...:         print(f'PASSED: {dt}...')
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid from /home/adeak/pyvista/pyvista/core/pointset.py:1122 (12 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.cast_to_unstructured_grid from /home/adeak/pyvista/pyvista/core/pointset.py:1236 (9 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.cell_coords from /home/adeak/pyvista/pyvista/core/pointset.py:1521 (4 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.cell_id from /home/adeak/pyvista/pyvista/core/pointset.py:1471 (5 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.compute_connections from /home/adeak/pyvista/pyvista/core/pointset.py:1735 (4 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.compute_connectivity from /home/adeak/pyvista/pyvista/core/pointset.py:1688 (4 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.dimensions from /home/adeak/pyvista/pyvista/core/pointset.py:None (3 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.hide_cells from /home/adeak/pyvista/pyvista/core/pointset.py:1325 (4 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.neighbors from /home/adeak/pyvista/pyvista/core/pointset.py:1564 (11 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.save from /home/adeak/pyvista/pyvista/core/pointset.py:1291 (9 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.show_cells from /home/adeak/pyvista/pyvista/core/pointset.py:1365 (6 examples)>...
PASSED: <DocTest pyvista.core.pointset.ExplicitStructuredGrid.visible_bounds from /home/adeak/pyvista/pyvista/core/pointset.py:None (5 examples)>...
PASSED: <DocTest pyvista.core.pointset.PointSet.remove_cells from /home/adeak/pyvista/pyvista/core/pointset.py:62 (3 examples)>...
PASSED: <DocTest pyvista.core.pointset.PolyData from /home/adeak/pyvista/pyvista/core/pointset.py:100 (14 examples)>...
PASSED: <DocTest pyvista.core.pointset.StructuredGrid from /home/adeak/pyvista/pyvista/core/pointset.py:908 (11 examples)>...
PASSED: <DocTest pyvista.core.pointset.StructuredGrid.hide_cells from /home/adeak/pyvista/pyvista/core/pointset.py:1071 (8 examples)>...
PASSED: <DocTest pyvista.core.pointset.UnstructuredGrid from /home/adeak/pyvista/pyvista/core/pointset.py:514 (7 examples)>...
FAILED: <DocTest pyvista.core.pointset.UnstructuredGrid._from_arrays from /home/adeak/pyvista/pyvista/core/pointset.py:638 (10 examples)>! (name 'np' is not defined)
PASSED: <DocTest pyvista.core.pointset.UnstructuredGrid.cast_to_explicit_structured_grid from /home/adeak/pyvista/pyvista/core/pointset.py:856 (9 examples)>...

(Note that penultimate failure.) And sure enough:

pyvista/pyvista/core/pointset.py

Lines 658 to 665 in ec5aa34

	Examples
	--------
	>>> import numpy
	>>> import vtk
	>>> import pyvista
	>>> offset = np.array([0, 9])
	>>> cells = np.array([8, 0, 1, 2, 3, 4, 5, 6, 7, 8, 8, 9, 10, 11, 12, 13, 14, 15])
	>>> cell_type = np.array([vtk.VTK_HEXAHEDRON, vtk.VTK_HEXAHEDRON], np.int8)

(note the np vs numpy discrepancy).

I think we might even be able to get this to work. Several things to take care of still:

1. discovering submodules and classes/functions automatically
2. skipping doctest SKIP lines (easy)
3. not skipping doctest SKIP lines at least for some local runs outside CI to weed out glaring errors hiding from CI?
4. reporting the results in some sane way. Fixing line number reporting is mostly a wish (in the genie sense).

---

There has to be a smarter way to explore PyVista automatically, but for now I'm leaving this here mostly as a note for myself:

from types import ModuleType
import pyvista as pv

pv_root = pv.__file__.removesuffix('__init__.py')  # python 3.9
for attr_name in dir(pv):
    attr = getattr(pv, attr_name)
    if not (callable(attr) or isinstance(attr, ModuleType)):
        continue

    keep = False
    if hasattr(attr, '__file__'):
        if attr.__file__.startswith(pv_root):
            keep = True
    elif hasattr(attr, '__module__'):
        parent_name = attr.__module__
        if parent_name.startswith('pyvista'):
            keep = True
    print('keep' if keep else 'nokeep', attr)

Now we just need to recurse. Hopefully there are no cycles so we don't even need something like BFS.

[akaszynski]

One consideration would be to simply add this script in our testing directory and call it separately in our CI. Simply adding it in a PR is enough to have this part of our codebase and depending on the pytest developers, we may have to implement it as part of our regular checks.

I think running this from time to time outside of our normal CI might be fine too as a fun project if you want to keep doing that @adeak.

[adeak]

@akaszynski I've opened #1439 to start implementing this. Although it's a draft, I'd appreciate if you could take a look when your time permits, just to adjust the design if necessary. There's no rush at all, this is just a heads-up. I'm going to fix the few broken doctests it's found anyway.