`verify_image_cache` not called when setting `auto_close=False`

[user27182]

The test_ssao_pass test:

https://github.com/pyvista/pyvista/blob/5f8fea51169091ce25f83db99e29387f816b4e29/tests/plotting/test_plotting.py#L3118-L3129

def test_ssao_pass():
    ugrid = pv.ImageData(dimensions=(2, 2, 2)).to_tetrahedra(5).explode()
    pl = pv.Plotter()
    pl.add_mesh(ugrid)

    pl.enable_ssao()
    pl.show(auto_close=False)

    # ensure this fails when ssao disabled
    pl.disable_ssao()
    with pytest.raises(RuntimeError):
        pl.show()

apparently currently passes CI, since no errors are currently raised by pytest on main. But, if the test is passing, this also means that with #171, the ssao_pass image should not be copied into the failed_image_dir. But, this is being copied into the failed images directory. I tested this locally and this line definitely executes:

pytest-pyvista/pytest_pyvista/pytest_pyvista.py

Line 227 in 1095211

raise RegressionError(msg)

But pytest isn't failing the test for some reason. When I make this change though:

-    pl.show(auto_close=False)
+    pl.show()

Then pytest properly raises an error and fails the test. So the auto_close keyword is somehow silencing this error. Maybe there is a bare Except somewhere catching the RegressionError and preventing it from being raised?

[user27182]

It would be good to identify a minimal example to reproduce this.

This #198 is seeing issues with --disallow_unused_cache, similar to #196 (comment). Tests that close the plotter manually seem to be failing with unused images:
https://github.com/pyvista/pytest-pyvista/actions/runs/17301909825/job/49115030241?pr=198

Details

============================= pytest-pyvista ERROR =============================
Unused cached image file(s) detected (17). The following images are
cached, but were not generated or skipped by any of the tests:
['enable_custom_trackball_style_1.png', 'enable_custom_trackball_style_2.png', 'enable_custom_trackball_style_3.png', 'enable_custom_trackball_style_4.png', 'enable_custom_trackball_style_5.png', 'enable_custom_trackball_style_6.png', 'enable_custom_trackball_style_7.png', 'enable_custom_trackball_style_8.png', 'enable_custom_trackball_style_9.png', 'fly_to_mouse_position.png', 'fly_to_right_click.png', 'fly_to_right_click_multi_render.png', 'image_properties_1.png', 'image_properties_2.png', 'interactive_update_1.png', 'plot_update.png', 'screenshot_notebook.png']

These images should either be removed from the cache, or the corresponding
tests should be modified to ensure an image is generated for comparison.
=========================== short test summary info ============================
SKIPPED [1] tests/plotting/jupyter/test_trame.py:283: #5262
SKIPPED [1] tests/conftest.py:400: title offset is a tuple of floats from vtk >= 9.3
SKIPPED [1] tests/plotting/test_plotting.py:677: Does not display correctly within OSMesa
SKIPPED [1] tests/plotting/test_plotting.py:3142: VTK and Matplotlib version incompatibility. For VTK<=9.2.2, MathText requires matplotlib<3.6
SKIPPED [1] tests/plotting/test_plotting.py:3205: Does not display correctly within OSMesa
SKIPPED [1] tests/plotting/test_plotting.py:3227: Does not display correctly within OSMesa
SKIPPED [1] tests/conftest.py:400: This is broken on VTK 9.3
SKIPPED [1] tests/plotting/test_plotting.py:5188: Does not display correctly within OSMesa
XFAIL tests/plotting/test_plotting.py::test_direction_objects[Capsule_legacy-pos] - Test capsule separately for different vtk versions. Expected to fail if testing with wrong version.
XFAIL tests/plotting/test_plotting.py::test_direction_objects[Capsule_legacy-neg] - Test capsule separately for different vtk versions. Expected to fail if testing with wrong version.
FAILED tests/plotting/test_conftest.py::test_skip_plotting_mark[True] - ValueError: Pytest terminal summary report not found
FAILED tests/plotting/test_conftest.py::test_skip_plotting_mark[False] - ValueError: Pytest terminal summary report not found
Error: Process completed with exit code 1.

[user27182]

I think this error partly stems from #189, or at least it seems that this PR exacerbates this issue. If #189 is reverted and --disallow_unused_cache is enabled, I suspect CI will pass. This is tested in #199

[user27182]

This test passed before #189

import pyvista as pv

def test_callback(verify_image_cache):
    verify_image_cache.skip = True
    pl = pv.Plotter(off_screen=True)
    pl.show(auto_close=False)
    assert pl._before_close_callback is not None
    pl.close()

But the assertion fails with #189. Hence, there is no callback, and verify_image_cache.__call__ isn't called, and hence the test doesn't get added to VISITED_CACHED_IMAGE_NAMES set.

[edabor]

It seems that the weakref is messing things up. When removing it, the above test works.
@akaszynski what was the motivation of using a weak ref here ? I'm not an expert on the topic.

[user27182]

Not sure about the weak ref, but maybe we can also monkeypatch Plotter.close (in addition to Plotter.show) to ensure it’s not None and can still call verify_image_cache().

[user27182]

It's also possible that using the weak ref is a bug with PyVista if it means that a callback that was previously set is not kept, since it then isn't used (when using auto_close=False).

Would need to test this more to see if this is an issue outside of the context of using verify image cache.

[user27182]

Not sure about the weak ref, but maybe we can also monkeypatch Plotter.close (in addition to Plotter.show) to ensure it’s not None and can still call verify_image_cache().

Instead of wrapping either close or show, we could consider wrapping the plotter callback directly. #189 removed the use of the global theme callback, and replaced it with a wrapper around show, so this would keep the old use of the theme callback but keep the wrapper fix from #189

[akaszynski]

@akaszynski what was the motivation of using a weak ref here ?

It's been a while, but I think we were trying to avoid the potential for a self reference, which would block garbage collection.

[edabor]

Here's what I tried so far:

import pyvista as pv


class foo:
    def __call__(self, pl: pv.Plotter): ...


callback = foo()


# OK
pl = pv.Plotter(off_screen=True)
pl.show(auto_close=False, before_close_callback=callback)
assert pl._before_close_callback is not None
pl.close()

# Not working
pl = pv.Plotter(off_screen=True)
pl.show(auto_close=False, before_close_callback=foo())
assert pl._before_close_callback is not None
pl.close()

# Not working
pl = pv.Plotter(off_screen=True)
pl.show(auto_close=False, before_close_callback=lambda a: ...)
assert pl._before_close_callback is not None
pl.close()


# OK
def _callback(pl): ...


pl = pv.Plotter(off_screen=True)
pl.show(auto_close=False, before_close_callback=_callback)
assert pl._before_close_callback is not None
pl.close()

It seems that the weak ref turns dead when the callback is an object defined during the method call.
Knowing that and inspired by @user27182 comment, I managed to get something working such that:

I'll propose a PR with this modification.