Skip to content

Fix CI abort: skip PyVista Plotter test on headless runners #59

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.

Sign up for GitHub

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

Merged
lmoresi merged 1 commit into from
Feb 28, 2026
Merged

Fix CI abort: skip PyVista Plotter test on headless runners #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/underworld3/materials.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ def add_callback(self, callback: Callable):
Parameters
----------
callback : callable
Function called as callback(event_type, \*args)
Function called as ``callback(event_type, *args)``
"""
self._callbacks.append(callback)

Expand Down
6 changes: 3 additions & 3 deletions src/underworld3/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ def __init__(self, name: Optional[str] = None, **kwargs):
----------
name : str, optional
Human-readable name for this model instance
\*\*kwargs : dict
**kwargs : dict
Additional arguments for Pydantic BaseModel
"""
# Handle name generation before calling super().__init__
Expand Down Expand Up @@ -540,7 +540,7 @@ def define_parameter(self, name: str, ptype=None, **kwargs):
Parameter path (e.g., 'material.viscosity', 'solver.tolerance')
ptype : ParameterType, optional
Parameter type for validation (not used yet)
\*\*kwargs : dict
**kwargs : dict
Additional arguments
"""
# TODO: Implement when parameter system is ready
Expand Down Expand Up @@ -592,7 +592,7 @@ def set_reference_quantities(self, verbose=False, nondimensional_scaling=True, *
[0-1] space while user-facing values remain in physical units.
Set to False for expert mode (dimensional units only, no scaling).
Disabling this may cause numerical conditioning issues.
\*\*quantities : dict
**quantities : dict
Named reference quantities using Pint units or UWQuantity objects,
e.g. ``domain_depth=uw.quantity(2900, "km")``.

Expand Down
18 changes: 16 additions & 2 deletions tests/test_0800_optional_modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ def check_module_available(module_name: str) -> bool:
HAS_GDAL = check_module_available("osgeo.gdal")
HAS_GEOPANDAS = check_module_available("geopandas")
HAS_PYVISTA = check_module_available("pyvista")

# Check if a display server is available for rendering.
# On headless CI (no DISPLAY, no WAYLAND), pyvista.Plotter() calls
# VTK's OpenGL probe which aborts the entire process — not catchable
# with try/except. We must skip rendering tests before they run.
import os

Copilot AI Feb 28, 2026

Copy link

Choose a reason for hiding this comment

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

The import os statement is placed mid-file at line 57, after module-level dependency flags have already been set. The existing imports at the top of the file (lines 22-25) are where os should be imported along with pytest and sys. Placing imports mid-module deviates from PEP 8 conventions and from the import pattern used throughout this file. Move import os to the top-level imports block near line 22-24, alongside import pytest and import sys. Note that os is also re-imported inside check_petsc_has_pragmatic() at line 70, which would then be unnecessary.

Copilot uses AI. Check for mistakes.
HAS_DISPLAY = bool(os.environ.get("DISPLAY") or os.environ.get("WAYLAND_DISPLAY")
or os.environ.get("PYVISTA_OFF_SCREEN"))
HAS_PYVISTA_RENDERING = HAS_PYVISTA and HAS_DISPLAY
HAS_TRAME = check_module_available("trame")

# Composite feature flags
Expand Down Expand Up @@ -98,6 +107,11 @@ def check_petsc_has_pragmatic() -> bool:
reason="Requires pyvista. Install with: pixi install -e runtime"
)

requires_pyvista_rendering = pytest.mark.skipif(
not HAS_PYVISTA_RENDERING,
reason="Requires pyvista and a display server (DISPLAY or PYVISTA_OFF_SCREEN)"

Copilot AI Feb 28, 2026

Copy link

Choose a reason for hiding this comment

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

The skip reason at line 112 mentions only DISPLAY and PYVISTA_OFF_SCREEN, but the HAS_DISPLAY check at line 58-59 also includes WAYLAND_DISPLAY. The reason message should mention all three environment variables to give users accurate guidance on how to enable the test.

Suggested change
reason="Requires pyvista and a display server (DISPLAY or PYVISTA_OFF_SCREEN)"
reason="Requires pyvista and a display server (DISPLAY, WAYLAND_DISPLAY, or PYVISTA_OFF_SCREEN)"

Copilot uses AI. Check for mistakes.
)

requires_amr = pytest.mark.skipif(
not HAS_AMR,
reason="Requires AMR-enabled PETSc. Install with: pixi install -e amr && pixi run -e amr petsc-build"
Expand Down Expand Up @@ -233,12 +247,12 @@ def test_pyvista_mesh_conversion(self):
sphere = pv.Sphere()
assert sphere is not None

@requires_pyvista
@requires_pyvista_rendering
def test_pyvista_plotter_available(self):
"""Test pyvista plotter when available."""
import pyvista as pv

# Just verify we can create a plotter (off-screen)
pv.OFF_SCREEN = True

Copilot AI Feb 28, 2026

Copy link

Choose a reason for hiding this comment

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

Setting pv.OFF_SCREEN = True as a global module-level attribute mutation inside a test method has a side effect: it permanently modifies the PyVista global state for the entire test session. Any subsequent tests that create a pv.Plotter() without off_screen=True will inherit this global flag. Since this test already passes off_screen=True directly to pv.Plotter(), the global pv.OFF_SCREEN = True assignment is redundant and should be removed to avoid unintended side effects on other tests.

Suggested change
pv.OFF_SCREEN = True

Copilot uses AI. Check for mistakes.
plotter = pv.Plotter(off_screen=True)
assert plotter is not None
plotter.close()
Expand Down