Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update plots before scraping #5283

Merged
merged 31 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d5ba83e
try update call
alexrockhill Dec 5, 2023
f93ad14
figured it out
alexrockhill Dec 6, 2023
1ebe8cb
fix failure
alexrockhill Dec 6, 2023
444c87f
add test
alexrockhill Dec 6, 2023
2de61ec
isort
alexrockhill Dec 6, 2023
dfad6ed
Merge branch 'main' into update
alexrockhill Dec 6, 2023
2ee4c09
test changed not exact checksum
alexrockhill Dec 6, 2023
ceb9116
hit new lines
alexrockhill Dec 6, 2023
e876082
isort
alexrockhill Dec 6, 2023
65208f7
add test dep
alexrockhill Dec 6, 2023
84faff6
req in wrong place
alexrockhill Dec 6, 2023
c2a6efe
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 6, 2023
63cb55c
add bindings
alexrockhill Dec 6, 2023
e161c7a
Merge branch 'update' of https://github.com/alexrockhill/pyvista into…
alexrockhill Dec 6, 2023
a514b1a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Dec 6, 2023
9070a0e
Apply suggestions from code review
tkoyama010 Dec 6, 2023
9111266
configure dependencies
alexrockhill Dec 6, 2023
f4a003d
oops bad formatting
alexrockhill Dec 6, 2023
c088baf
Apply suggestions from code review
tkoyama010 Dec 6, 2023
507bc0c
Update tests/test_tinypages.py
tkoyama010 Dec 6, 2023
f6b6109
Update tests/tinypages/plot_cone.py
tkoyama010 Dec 6, 2023
97679cf
Update tests/test_tinypages.py
tkoyama010 Dec 6, 2023
b15ef1b
fix numbering
alexrockhill Dec 6, 2023
a829fee
add cover
alexrockhill Dec 6, 2023
659f3c1
shoot forgot precommit
alexrockhill Dec 6, 2023
e0a550e
still no cover on dynamic scraper, refactor
alexrockhill Dec 6, 2023
5cb7ae4
new function
alexrockhill Dec 7, 2023
7959a11
forgot no cover
alexrockhill Dec 7, 2023
789d628
Apply suggestions from code review
tkoyama010 Dec 7, 2023
8110f17
Merge branch 'main' of https://github.com/pyvista/pyvista into update
alexrockhill Dec 7, 2023
7b74324
fix coverage
alexrockhill Dec 7, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/workflows/testing-and-deployment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,11 @@ jobs:
restore-keys: |
Examples-1-

- name: Setup xvfb dependencies
run: |
git clone --depth=1 https://github.com/mne-tools/mne-python.git --branch main --single-branch
./mne-python/tools/setup_xvfb.sh

Comment on lines +169 to +173
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah... why was this added here? This needs to be removed ASAP as this section of the tests is strictly non-graphics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further I want to discourage the use of custom scripts from external projects in our core CI routines. We should keep any configuration and set up limited to well-established GitHub Actions or scripts managed by PyVista.

Adding and executing arbitrary shell scripts is a major red flag and security risk albeit this is from a well-trusted downstream project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable. They were added for PyQtx.QWidgets.QApplication.process_events testing. Not sure there is a way to fake that for non-graphics tests, I just don't know there might be though. Otherwise since it was tested it could be no covered.

- name: Build wheel and install pyvista
run: |
pip install build
Expand Down
20 changes: 17 additions & 3 deletions pyvista/plotting/utilities/sphinx_gallery.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ def html_rst(
return images_rst


def _process_events_before_scraping(plotter):
"""Process events such as changing the camera or object before scraping."""
if plotter.iren is not None and plotter.iren.initialized:
plotter.update()
if hasattr(plotter, "app") and plotter.app is not None:
plotter.app.processEvents()


class Scraper:
"""
Save ``pyvista.Plotter`` objects.
Expand Down Expand Up @@ -84,7 +92,8 @@ def __call__(self, block, block_vars, gallery_conf):
image_names = list()
image_path_iterator = block_vars["image_path_iterator"]
figures = pyvista.plotting.plotter._ALL_PLOTTERS
for _, plotter in figures.items():
for plotter in figures.values():
_process_events_before_scraping(plotter)
fname = next(image_path_iterator)
if hasattr(plotter, "_gif_filename"):
# move gif to fname
Expand All @@ -97,7 +106,7 @@ def __call__(self, block, block_vars, gallery_conf):
return figure_rst(image_names, gallery_conf["src_dir"])


class DynamicScraper:
class DynamicScraper: # pragma: no cover
"""
Save ``pyvista.Plotter`` objects dynamically.

Expand All @@ -110,6 +119,10 @@ class DynamicScraper:

"""

def __repr__(self):
"""Return a stable representation of the class instance."""
return f"<{type(self).__name__} object>"

Comment on lines +122 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexrockhill, why was this needed? We do not intend for this class to be used or displayed to users as far as I understand?

Asking because I'm going to see if I can re-land this PR with less impact

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added when I was trying to add coverage of the dynamic scraper but then not removed when adding that coverage exposed a bug where the dynamic scraper failed to close plots and so that was punted to a future PR.

def __call__(self, block, block_vars, gallery_conf):
"""Save the figures generated after running example code.

Expand All @@ -122,7 +135,8 @@ def __call__(self, block, block_vars, gallery_conf):
image_names = list()
image_path_iterator = block_vars["image_path_iterator"]
figures = pyvista.plotting.plotter._ALL_PLOTTERS
for _, plotter in figures.items():
for plotter in figures.values():
_process_events_before_scraping(plotter)
fname = next(image_path_iterator)
# if hasattr(plotter, '_gif_filename'):
# raise RuntimeError('GIFs are not supported with DynamicScraper.')
Expand Down
3 changes: 3 additions & 0 deletions requirements_test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@ meshio<5.4.0
nest_asyncio<1.5.9
numpydoc==1.6.0
param<2.1.0
PyQt6!=6.6.1,<6.7.0
PyQt6-Qt6!=6.6.1,<6.7.0
pytest<7.5.0
pytest-cov<4.2.0
pytest-memprof<0.3.0
pytest-xdist<3.4.0
pytest_pyvista==0.1.8
qtpy<2.5.0
Comment on lines +15 to +22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not test Qt dependencies in PyVista. This was a part of the entire motivation for making PyVistaQt a separate repository

Sphinx<7.2.0
sphinx-gallery<0.15.0
sympy<1.13.0
Expand Down
50 changes: 50 additions & 0 deletions tests/plotting/test_scraper.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import os
import os.path as op

from matplotlib.pyplot import imread
import pytest
from qtpy.QtWidgets import QApplication

import pyvista as pv
from pyvista.plotting.utilities.sphinx_gallery import Scraper
Expand All @@ -10,6 +12,54 @@
pytestmark = pytest.mark.skip_plotting


def test_scraper_with_app(tmpdir, monkeypatch, n_win=2):
pytest.importorskip('sphinx_gallery')
monkeypatch.setattr(pv, 'BUILDING_GALLERY', True)
pv.close_all()

scraper = Scraper()

plotters = [pv.Plotter(off_screen=True) for _ in range(n_win)]

# add cone, change view to test that it takes effect
plotters[0].iren.initialize()
plotters[0].app = QApplication([])
plotters[0].add_mesh(pv.Cone())
plotters[0].camera_position = 'xy'

plotters[1].add_mesh(pv.Cone())

src_dir = str(tmpdir)
out_dir = op.join(str(tmpdir), '_build', 'html')
img_fnames = [
op.join(src_dir, 'auto_examples', 'images', f'sg_img_{n}.png') for n in range(n_win)
]

gallery_conf = {"src_dir": src_dir, "builder_name": "html"}
target_file = op.join(src_dir, 'auto_examples', 'sg.py')
block = None
block_vars = dict(
image_path_iterator=iter(img_fnames),
example_globals=dict(a=1),
target_file=target_file,
)

os.makedirs(op.dirname(img_fnames[0]))
for img_fname in img_fnames:
assert not os.path.isfile(img_fname)

os.makedirs(out_dir)
scraper(block, block_vars, gallery_conf)
for img_fname in img_fnames:
assert os.path.isfile(img_fname)

# test that the plot has the camera position updated with a checksum when the Plotter has an app instance
assert imread(img_fnames[0]).sum() != imread(img_fnames[1]).sum()

for plotter in plotters:
plotter.close()


@pytest.mark.parametrize('n_win', [1, 2])
def test_scraper(tmpdir, monkeypatch, n_win):
pytest.importorskip('sphinx_gallery')
Expand Down