MAINT: Add scraper test

[larsoner]

Hopefully will show a failure with latest pyvista observed in MNE-Python

[larsoner]

@banesullivan based on a local git bisect I've isolated a regression in sphinx-gallery scraping with PyVistaQt due to pyvista/pyvista#3889. On MNE-Python we see it in CIs when trying to build this example, and I've now added a test here that shows it, too:

https://github.com/pyvista/pyvistaqt/actions/runs/4105641264/jobs/7083928451#step:9:93

=================================== FAILURES ===================================
_________________________ test_sphinx_gallery_scraping _________________________
tests/test_plotting.py:9[83](https://github.com/pyvista/pyvistaqt/actions/runs/4105641264/jobs/7083928451#step:9:84): in test_sphinx_gallery_scraping
    scraper(block, block_vars, gallery_conf)
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/pyvista/utilities/sphinx_gallery.py:60: in __call__
    plotter.screenshot(fname)
/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/pyvista/plotting/plotting.py:5604: in screenshot
    return self._save_image(self.image, filename, return_img)
pyvistaqt/rwi.py:426: in __getattr__
    raise AttributeError(self.__class__.__name__ +
E   AttributeError: BackgroundPlotter has no attribute named image

It's not really clear to me why that PR broke stuff here. Do you have time to look?

[banesullivan]

Ah, this makes sense. pyvistaqt.BackgroundPlotter inherits from BasePlotter, not Plotter. The image attribute is mostly handled in Plotter.show() (not available to pyvistaqt.BackgroundPlotter)

[banesullivan]

I'll look into this. My first reaction is that BackgroundPlotter needs to handle how it wants to store the image and thus this will require changes here rather than upstream in PyVista.

[banesullivan]

E AttributeError: BackgroundPlotter has no attribute named image

Though this error makes no sense. BasePlotter.image definitely exists... https://github.com/pyvista/pyvista/blob/860c3824a6c9d5589b9de9ee89cc5940e9220fef/pyvista/plotting/plotting.py#L1705

[banesullivan]

I can't install pyvistaqt locally to debug...

RuntimeError: No Qt binding was found, got: No Qt bindings could be found

after conda install -c conda-forge qtpy 😕

[larsoner]

I can't install pyvistaqt locally to debug...

You also need conda install -c conda-forge pyqt. qtpy is just a wrapper to different Qt implementations. pyqt is the one that is available on conda-forge. Or if you want to pip install, you can pip install pyqt6 or pip install pyside6 etc.

Though this error makes no sense. BasePlotter.image definitely exists... https://github.com/pyvista/pyvista/blob/860c3824a6c9d5589b9de9ee89cc5940e9220fef/pyvista/plotting/plotting.py#L1705

I suspect what is happening is that the code you changed is raising an error. This error causes it to move on from the __getattr__ of BasePlotter and move on to the __getattr__ of the render window interactor, which then emits the final AttributeError. But this is speculation, I can't recall/don't know how attribute resolution in Python proceeds...

[banesullivan]

Thanks for the install tip... thought it must be something simple!

I think you are spot on with __getattr__ and indeed the subclass here overrides it:

pyvistaqt/pyvistaqt/rwi.py

Lines 419 to 427 in 71605d3

	def __getattr__(self, attr):
	"""Makes the object behave like a vtkGenericRenderWindowInteractor"""
	if attr == '__vtk__':
	return lambda t=self._Iren: t
	elif hasattr(self._Iren, attr):
	return getattr(self._Iren, attr)
	else:
	raise AttributeError(self.__class__.__name__ +
	" has no attribute named " + attr)

I think that needs to be updated to also check BasePlotter

[larsoner]

I think that needs to be updated to also check BasePlotter

But why would it need to do that now when it didn't earlier? Just to test this idea I changed rwi.py to explicitly use BasePlotter (which I think class attribute lookup was already silently/implicitly doing):

        else:
            if attr == 'image':
                from pyvista import BasePlotter
                return BasePlotter.image.fget(self)
            raise AttributeError(self.__class__.__name__ +
                  " has no attribute named " + attr)

now I get to see the actual error:

_________________________________________________________________ test_sphinx_gallery_scraping _________________________________________________________________
tests/test_plotting.py:983: in test_sphinx_gallery_scraping
    scraper(block, block_vars, gallery_conf)
../pyvista/pyvista/utilities/sphinx_gallery.py:60: in __call__
    plotter.screenshot(fname)
../pyvista/pyvista/plotting/plotting.py:5604: in screenshot
    return self._save_image(self.image, filename, return_img)
pyvistaqt/rwi.py:428: in __getattr__
    return BasePlotter.image.fget(self)
../pyvista/pyvista/plotting/plotting.py:1710: in image
    self._check_rendered()
../pyvista/pyvista/plotting/plotting.py:1692: in _check_rendered
    raise AttributeError(
E   AttributeError: 
E   This plotter has not yet been set up and rendered with ``show()``.
E   Consider setting ``off_screen=True`` for off screen rendering.

So I suspect this might be the new error that is occurring, even though it shows up as an AttributeError in rwi.py. In other words, both subclasses RWI and BasePlotter now raise AttributeError as of pyvista/pyvista#3889 -- whereas they didn't before, I think -- and the earliest class in the MRO gets their error raised, making it hard to see the true underlying error.

So I suspect that PR changed somethng with the screenshot / image behavior that is problematic...

[codecov]

Codecov Report

Merging #306 (240f92a) into main (71605d3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #306   +/-   ##
=======================================
  Coverage   97.31%   97.31%           
=======================================
  Files           8        8           
  Lines         670      671    +1     
  Branches       82       82           
=======================================
+ Hits          652      653    +1     
  Partials       18       18           

[banesullivan]

Ooo, this is bigger than just pyvista/pyvista#3889

We need to set self._rendered = True in pyvistaqt. This should be a quick fix.

diff --git a/pyvistaqt/plotting.py b/pyvistaqt/plotting.py
index c022348..f7a58f0 100644
--- a/pyvistaqt/plotting.py
+++ b/pyvistaqt/plotting.py
@@ -268,6 +268,7 @@ class QtInteractor(QVTKRenderWindowInteractor, BasePlotter):
                     renderer.enable_depth_peeling()

         self._first_time = False  # Crucial!
+        self._rendered = True
         LOG.debug("QtInteractor init stop")

     def _setup_interactor(self, off_screen: bool) -> None:

Not that we do the same thing for _first_time here too

[banesullivan]

We need to do a better job of expose _rendered and _first_time as those are subclass-specific private attributes used in BasePlotter

[larsoner]

We need to set self._rendered = True in pyvistaqt. This should be a quick fix.

Can you push here to get yourself the credit? :)

[banesullivan]

Note: we may want self._rendered = True somewhere else...

[banesullivan]

Will do. I'll poke around and see if there is a better place to put it

[banesullivan]

Actually. The better way to go about this may be to override _check_rendered() here. I'm going to push my quick fix and let you see what you think, @larsoner.

[larsoner]

Thanks for the quick fix @banesullivan !

We should probably cut a pyvistaqt bugfix release

[banesullivan]

Can you handle the patch release, or do you want me to?

[larsoner]

I've never done it for pyvistaqt, but if it's just 1) tag with appropriate name, 2) push tag, 3) cut release on GitHub, and 4) update version here, then I can do it

[larsoner]

... of course I mean after step 0) merge #307 :)