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

fix vtk version support #2961

Merged
merged 5 commits into from Jul 14, 2022
Merged

fix vtk version support #2961

merged 5 commits into from Jul 14, 2022

Conversation

akaszynski
Copy link
Member

Resolves #2867 by implementing matrix testing for VTK and Python.

  • Python 3.7 --> vtk==8.1.2
  • Python 3.8 --> vtk==9.0.3
  • Python 3.9 --> vtk==9.1
  • Python 3.10 --> vtk==9.2.0

Additionally, because we aren't testing Python 3.7 with 8.1.2, we've had some VTK version support regressions only discovered when locally testing 3.7. This PR fixes those regressions.


Also, I propose that we support the oldest VTK version with a viable wheel available for actively supported versions of Python available on PyPI. For example, the lowest supported version of Python as of this PR is Python 3.7, and the lowest supported version of VTK for 3.7 is vtk==8.1.2.

This is reflected in the install_requires in our setup.py.

As soon as support for Python 3.7 is dropped, we can then drop support for both Python 3.7 and VTK 8.1.2 in both our python_requires and install_requires.

As much as I would prefer to follow NumPy's version support NEP 29 deprecation policy, I know of many who are stuck using older versions of Python and need support for older versions of VTK and cannot upgrade agressively. With this approach we can still depricate support for older versions of VTK and Python, but in a manner that works for all that use this library with any currently supported version of Python.

@github-actions github-actions bot added maintenance Low-impact maintenance activity bug Uh-oh! Something isn't working as expected. labels Jul 9, 2022
@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #2961 (f7d1df5) into main (c75d631) will decrease coverage by 4.27%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2961      +/-   ##
==========================================
- Coverage   94.30%   90.02%   -4.28%     
==========================================
  Files          76       76              
  Lines       16565    16569       +4     
==========================================
- Hits        15622    14917     -705     
- Misses        943     1652     +709     

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

This, in general, looks good to me. Maybe for a future PR: we mix skipping tests completely and checking that we raise with VTKVersionError in the code base. In my opinion it would be better to check that it raises to make sure that we handle these cases with useful errors.

setup.py Outdated
@@ -18,7 +18,7 @@
'pillow',
'appdirs',
'scooby>=0.5.1',
'vtk',
'vtk>=8.1.2,<9.3',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does setup.py support <=9.2.0.*?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was having trouble permitting a release candidates with any version range unless you specifically permit it, as in:

pyvista/setup.py

Lines 21 to 22 in 88a8940

'vtk>=8.1.2, <=9.2.0; python_version < "3.10"',
'vtk>=8.1.2, <=9.2.0, ==9.2.0rc1; python_version == "3.10"',

What I did was explicitly permit the release candidate wheel, but only for Python 3.10. Personally, I don't feel that release candidates should be permitted by default, but only in special circumstances like general unavailability otherwise as in the case of Python 3.10

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me. Ideally the rc part could be removed before the release in pyvista. If it can't, should we use some form of ==9.2.0.*, so that later release candidates can be used? I'm not sure. Maybe best to pin it and manually keep updating it until the release? This way there are no surprises.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it can't, should we use some form of ==9.2.0.*, so that later release candidates can be used?

If someone knows how to get release candidates working with a version range, please let me know. Otherwise, I'll permit ==9.2.0rc for PyVista for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

See my other suggestion, if that doesn't work, I'm in favor of ==9.2.0rc1 and manually bumping it as needed, and hopefully removing it before next release (vtk had a 1 month release cycle last time).

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed then. Let's keep to this for now and hopefully 9.2.0 is out before the next release of PyVista.

@akaszynski
Copy link
Member Author

This, in general, looks good to me. Maybe for a future PR: we mix skipping tests completely and checking that we raise with VTKVersionError in the code base. In my opinion it would be better to check that it raises to make sure that we handle these cases with useful errors.

Agreed, that seems like a better practice.

setup.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

Should we also pin python_requires to be <= 3.10?

@akaszynski
Copy link
Member Author

Should we also pin python_requires to be <= 3.10?

I'm not sure. I don't see an upper limit for numpy, and I think that we should permit those who want to use PyVista with an unsupported version of Python.

Co-authored-by: MatthewFlamm <39341281+MatthewFlamm@users.noreply.github.com>
@akaszynski
Copy link
Member Author

This is ready for review/approval. Checking showing up as yellow just means we need to update our branch protection rules once this PR is merged.

Copy link
Contributor

@MatthewFlamm MatthewFlamm left a comment

Choose a reason for hiding this comment

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

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected. maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supported vtk versions
2 participants