Add CI workflow to test VTK master branch #3704

banesullivan · 2022-12-09T18:47:26Z

This uses VTK's GitLab package index to download and install a wheel for the latest VTK (master branch). To install, run:

pip install vtk --pre --no-cache --extra-index-url https://wheels.vtk.org

Specifically, this branch uses the OSMesa wheel to prevent the need to xvfb

This runs as a cron job on main, has a workflow_dispatch trigger to run on request, and has a branch naming pattern for 'maint/vtk-master*' to be able to open branches/PRs that will verify things are working against VTK master.

codecov · 2022-12-09T19:33:30Z

Codecov Report

Merging #3704 (9ccfbde) into main (965618e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3704   +/-   ##
=======================================
  Coverage   95.77%   95.77%           
=======================================
  Files          97       97           
  Lines       20755    20755           
=======================================
  Hits        19879    19879           
  Misses        876      876

banesullivan · 2022-12-20T06:18:34Z

vtkExplicitStructuredGrid DeepCopy is broken on master. Tracking here: https://gitlab.kitware.com/vtk/vtk/-/issues/18758

banesullivan · 2022-12-20T06:26:05Z

Threshold API changes

AttributeError: 'vtkmodules.vtkFiltersCore.vtkThreshold' object has no attribute 'ThresholdBetween'

There is a MR to re-add this method: https://gitlab.kitware.com/vtk/vtk/-/merge_requests/9709

There is also a discussion about these changes here: https://discourse.vtk.org/t/unnecessary-vtk-api-change/9929

Other errors

Two errors we may need to look into on our side:

FAILED tests/plotting/test_plotting.py::test_left_button_down - Failed: DID NOT RAISE <class 'ValueError'>
FAILED tests/plotting/test_plotting.py::test_plot_composite_poly_complex - Failed: DID NOT WARN. No warnings of type (<class 'numpy.ComplexWarning'>,) were emitted.

banesullivan · 2022-12-20T06:43:23Z

@akaszynski, do you recall why you added this check in #732:

pyvista/pyvista/plotting/plotting.py

Lines 1774 to 1777 in 4bc2064

    
           if hasattr(self.ren_win, 'GetOffScreenFramebuffer'): 
        
               if not self.ren_win.GetOffScreenFramebuffer().GetFBOIndex(): 
        
                   # must raise a runtime error as this causes a segfault on VTK9 
        
                   raise ValueError('Invoking helper with no framebuffer')

This is no longer failing for VTK master branch and thus the left_button_down test will be updated here.

pyvista/core/filters/data_set.py

akaszynski · 2023-01-04T19:49:57Z

@akaszynski, do you recall why you added this check in #732:

pyvista/pyvista/plotting/plotting.py

Lines 1774 to 1777 in 4bc2064

if hasattr(self.ren_win, 'GetOffScreenFramebuffer'):

if not self.ren_win.GetOffScreenFramebuffer().GetFBOIndex():

# must raise a runtime error as this causes a segfault on VTK9

raise ValueError('Invoking helper with no framebuffer')

This is no longer failing for VTK master branch and thus the left_button_down test will be updated here.

It's been a while, but I do recall genuine segfaults and having to work around them. I agree with your workaround.

.github/workflows/vtk-pre-test.yml

banesullivan · 2023-02-02T18:07:08Z

Okay! CI is finally running with the vtk_osmesa package and tests are not segfaulting!

Unfortunately we have tests failures with the main branch of VTK

banesullivan · 2023-02-02T18:26:38Z

@pyvista/developers, this is ready for review. IMO, we should merge this without addressing the testing failures, as this PR is to set up a CI routine that will make it easier to debug PyVista+VTK master issues.

After merging, we should follow up with individual PRs that address the testing failures here so that we can have targeted development for specific issues.

banesullivan · 2023-02-02T18:32:57Z

.github/workflows/vtk-pre-test.yml

+      - name: Set up VTK with OSMesa
+        run: |
+          pip uninstall vtk -y
+          pip install vtk_osmesa --pre --no-cache --extra-index-url https://gitlab.kitware.com/api/v4/projects/13/packages/pypi/simple


@akaszynski, @MatthewFlamm, @AlejandroFernandezLuces: this is the issue I am referring to in pyvista/pytest-pyvista#41

From CI:

$ pip uninstall vtk -y Found existing installation: vtk 9.2.5 Uninstalling vtk-9.2.5: Successfully uninstalled vtk-9.2.5 Looking in indexes: https://pypi.org/simple, https://gitlab.kitware.com/api/v4/projects/13/packages/pypi/simple Collecting vtk_osmesa ...

I think this is a separate issue from the issue you linked, but the impact is sometimes encountered in installing that package. We have a dependency for vtk in pyvista, but apparently vtk_osmesa can be used without problem. But, this means that pyvista still looks for vtk. The problem is in the dependencies for pyvista itself, not in pytest-pyvista IMO. I dont know if there is a solution for an 'either/or' dependency resolution in pip specification.

Indeed the issue is pyvista having vtk in its hard requirements. This is something I think we will have a lot more friction changing. And so a simpler fix is to just remove pyvista from pytest-pyvista's hard requirements to work around this.

banesullivan · 2023-04-17T17:16:09Z

@pyvista/developers, I recommend merging this PR as is to help track testing failures against VTK master. The workflow here is manually triggered, so will not cause failures on PRs.

larsoner

The workflow here is manually triggered, so will not cause failures on PRs.

Works for me as long as someone sets up to be notified on failures and actually fixes them. In MNE we have had a run for a while now where we install all sorts of stuff either from main/master including PyVista(Qt) and other stuff from daily/weekly CDNs (NumPy, SciPy, pandas, as of a couple of days ago VTK, etc.), but we do run it on all PRs rather than on a schedule. It means that once a month or so some CI breaks in someone's PR for an unrelated reason, but these are easy to ignore during review ("merge anyway ignoring requirements", or just don't add to "Required" list!) but also annoying enough that we fix them. It also catches stuff like using a deprecated APIs which would be missed until after merge by a scheduled run. You might consider trying this approach and seeing how onerous it is, and resorting to a scheduled one only if necessary.

But a scheduled run is better than no run at all so +1 from me either way! Hopefully a follow-up PR soon will make this green 🙂

larsoner · 2023-04-17T20:29:41Z

.github/workflows/vtk-pre-test.yml

+      - name: Set up VTK with OSMesa
+        run: |
+          pip uninstall vtk -y
+          pip install vtk_osmesa --pre --no-cache --extra-index-url https://wheels.vtk.org


It would be good to have one run that uses VTK-osmesa and another that uses Xvfb-and-standard-VTK. The failure modes could be different and the latter is a (much more?) widely used test case.

I think I will put that off for another PR -- due to current time constraints and because I want to use the new OSMesa wheel throughout our testing suite.

akaszynski · 2023-04-18T05:13:47Z

Works for me as long as someone sets up to be notified on failures and actually fixes them

I'm of the opinion that it's better to be notified of failures early on and have this workflow (at least for one version of python) be always on.

However, I'm reminded of alert fatigue If this fails every so often, it's acceptable. If it's all the time and we can't correct it, we should have the option of disabling it.

Since it's failing right now, let's keep this manual for now and consider enabling it by default later.

banesullivan · 2023-04-18T13:41:20Z

... You might consider trying this approach and seeing how onerous it is, and resorting to a scheduled one only if necessary.

Thanks for the excellent example. I'd prefer not to have flaky CI jobs on PRs -- it signals that some things are okay to fail and then we as a community are quick to ignore that failure and soon other failures because we're used to merging with a red X. "alert fatigue" as @akaszynski pointed out. I've seen this be a big problem on some projects that now I'm very averse to it.

Since it's failing right now, let's keep this manual for now and consider enabling it by default later.

Indeed - this is failing right now, and I think we need to raise an issue or two upstream in VTK before these things can be addressed. I suspect many failures moving forward are going to be things are broken upstream and thus we don't have a lot of control here in PyVista -- a further reason I'd prefer this to be a manual job we intentionally run to report issues upstream at once.

One such (serious) issue was discovered in this PR (see https://gitlab.kitware.com/vtk/vtk/-/issues/18758). I need to dig into the logs here and report any current failures

Add CI workflow to test VTK master branch

81faf80

github-actions bot added the maintenance Low-impact maintenance activity label Dec 9, 2022

banesullivan added 4 commits December 9, 2022 11:50

[debug] Trigger workflow in PR

1c026b6

Fix VTK install

a636874

--yes

615550e

Verbose testing

5d5e4cb

Merge branch 'main' into maint/vtk-master-testing

85ee6ce

Fix ThresholdBetween usage

afe459e

Hackily fix test_left_button_down test

62627ba

banesullivan commented Dec 20, 2022

View reviewed changes

pyvista/core/filters/data_set.py Outdated Show resolved Hide resolved

Merge branch 'main' into maint/vtk-master-testing

b71d573

banesullivan commented Jan 10, 2023

View reviewed changes

.github/workflows/vtk-pre-test.yml Outdated Show resolved Hide resolved

banesullivan commented Jan 10, 2023

View reviewed changes

.github/workflows/vtk-pre-test.yml Outdated Show resolved Hide resolved

banesullivan commented Jan 10, 2023

View reviewed changes

.github/workflows/vtk-pre-test.yml Outdated Show resolved Hide resolved

banesullivan commented Jan 10, 2023

View reviewed changes

.github/workflows/vtk-pre-test.yml Outdated Show resolved Hide resolved

banesullivan and others added 9 commits January 17, 2023 09:40

Merge branch 'main' into maint/vtk-master-testing

c062850

Merge branch 'main' into maint/vtk-master-testing

a890ad0

Use new OSMesa wheels

ea18b91

Add Python 3.11

1df508c

Fix pytest command

1f62bb1

Uninstall vtk

020b364

Local install of PyVista

0568fdf

Fix check

e4d8897

Fix path

c4d7ada

Imporve CI condition

340a18a

banesullivan marked this pull request as ready for review February 2, 2023 18:25

Use 'maint/vtk-master*'

d4ac0a4

banesullivan commented Feb 2, 2023

View reviewed changes

banesullivan mentioned this pull request Feb 2, 2023

Remove PyVista from dependencies pyvista/pytest-pyvista#41

Merged

banesullivan added 6 commits February 6, 2023 13:32

Merge branch 'main' into maint/vtk-master-testing

119914d

Merge branch 'main' into maint/vtk-master-testing

23bae38

Drop 3.7

240c848

Merge branch 'main' into maint/vtk-master-testing

748cfb5

Use wheels.vtk.org

eedec89

Fix VTK9 usage

9ccfbde

larsoner reviewed Apr 17, 2023

View reviewed changes

akaszynski approved these changes Apr 18, 2023

View reviewed changes

akaszynski merged commit 5968867 into main Apr 19, 2023
24 of 28 checks passed

akaszynski deleted the maint/vtk-master-testing branch April 19, 2023 18:59

akaszynski mentioned this pull request Apr 30, 2023

Release 0.39.0 #4359

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CI workflow to test VTK master branch #3704

Add CI workflow to test VTK master branch #3704

banesullivan commented Dec 9, 2022 •

edited

codecov bot commented Dec 9, 2022 •

edited

banesullivan commented Dec 20, 2022

banesullivan commented Dec 20, 2022

banesullivan commented Dec 20, 2022

akaszynski commented Jan 4, 2023

banesullivan commented Feb 2, 2023

banesullivan commented Feb 2, 2023 •

edited

banesullivan Feb 2, 2023

banesullivan Feb 2, 2023

MatthewFlamm Feb 2, 2023

banesullivan Feb 2, 2023 •

edited

banesullivan commented Apr 17, 2023

larsoner left a comment

larsoner Apr 17, 2023

banesullivan Apr 18, 2023

akaszynski commented Apr 18, 2023

banesullivan commented Apr 18, 2023

Add CI workflow to test VTK master branch #3704

Add CI workflow to test VTK master branch #3704

Conversation

banesullivan commented Dec 9, 2022 • edited

codecov bot commented Dec 9, 2022 • edited

Codecov Report

banesullivan commented Dec 20, 2022

banesullivan commented Dec 20, 2022

Threshold API changes

Other errors

banesullivan commented Dec 20, 2022

akaszynski commented Jan 4, 2023

banesullivan commented Feb 2, 2023

banesullivan commented Feb 2, 2023 • edited

banesullivan Feb 2, 2023

Choose a reason for hiding this comment

banesullivan Feb 2, 2023

Choose a reason for hiding this comment

MatthewFlamm Feb 2, 2023

Choose a reason for hiding this comment

banesullivan Feb 2, 2023 • edited

Choose a reason for hiding this comment

banesullivan commented Apr 17, 2023

larsoner left a comment

Choose a reason for hiding this comment

larsoner Apr 17, 2023

Choose a reason for hiding this comment

banesullivan Apr 18, 2023

Choose a reason for hiding this comment

akaszynski commented Apr 18, 2023

banesullivan commented Apr 18, 2023

banesullivan commented Dec 9, 2022 •

edited

codecov bot commented Dec 9, 2022 •

edited

banesullivan commented Feb 2, 2023 •

edited

banesullivan Feb 2, 2023 •

edited