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

Drop Python 3.7 #3938

Merged
merged 4 commits into from
Mar 9, 2023
Merged

Drop Python 3.7 #3938

merged 4 commits into from
Mar 9, 2023

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Feb 2, 2023

According to PEP 537, Python 3.7 will stop maintenance in June 2023. NumPy's NEP 29 dropped Python 3.7 in 2021. So it's time to drop it here too, and replace it with Python 3.11.

This PR does the following:

  • Remove typing-extensions dependencies
  • Adjust python versions in various CI workflows
  • Update docs and example code
  • Update versions for Trove classifiers
  • Also clean-up Python 3.6 logic in pyvista/utilities/cells.py

@github-actions github-actions bot added dependencies Pull requests that update a dependency file documentation Anything related to the documentation/website maintenance Low-impact maintenance activity labels Feb 2, 2023
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #3938 (6a5686e) into main (890bba6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3938      +/-   ##
==========================================
- Coverage   94.52%   94.52%   -0.01%     
==========================================
  Files          97       97              
  Lines       20470    20465       -5     
  Branches     3431     3431              
==========================================
- Hits        19350    19344       -6     
- Misses       1119     1120       +1     
  Partials        1        1              

@banesullivan
Copy link
Member

Dropping Python 3.7 also means dropping support for VTK<9.

I'm +1 for this overall but this needs to be treated with care to make sure the community around pyvista is ready and able to drop support for VTK8.

Further, we will need to clean up all of logic that maintains backward compatibility with VTK8 (imports, API changes, etc.). See usage of the pyvista._vtk.VTK9 bool.

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 2, 2023

should dropping VTK<9 also be included in this PR, or separately? I don't mind either way.

@banesullivan
Copy link
Member

should dropping VTK<9 also be included in this PR, or separately? I don't mind either way.

It's impossible (or difficult rather) to use VTK<9 after dropping Python 3.7 support, so yes, IMO, we should remove all VTK8 support in PyVista when dropping support for Python 3.7. This would clean up a lot of "if VTK9: do this else: do that" logic, lazy imports, and a bit of other cruft.

@mwtoews, considering that. I want to encourage you to hold off for a bit as this will be touching a good chunk of the codebase and then may not get support/approval to merge for a while -- leaving you with a stale PR that will be tough to keep up to date.

I'm +1 for doing this months ago, but I want to make sure dropping Python 3.7 and VTK8 support has buy-in.

I propose opening an issue to discuss this topic and, when a date is agreed upon, revisit this

@banesullivan
Copy link
Member

FYI, Python 3.11 is currently supported though not tested on CI. I'm going to add these CI workflows with the latest version of VTK (9.2.5) soon

@akaszynski
Copy link
Member

It's impossible (or difficult rather) to use VTK<9 after dropping Python 3.7 support, so yes, IMO, we should remove all VTK8 support in PyVista when dropping support for Python 3.7. This would clean up a lot of "if VTK9: do this else: do that" logic, lazy imports, and a bit of other cruft.

I feel that dropping support for Python3.7 just means that users will be forced to use an older version of PyVista, and that's totally ok. It's opt-in and lets us keep up-to-date. If user really, absolutely need a patch fix for an older version, we can backport it, but I'm going to heavily discourage that.

Let's let the general community weigh in, but I'm happy if we want to drop support for Python 3.7 in pyvista==0.39.0 and drop support for VTK<9.0 (in a separate PR but in the same release).

@banesullivan
Copy link
Member

drop support for VTK<9.0 (in a separate PR but in the same release).

I feel VTK<9 and Python 3.7 should be dropped simultaneously.

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 2, 2023

My personal take is to try and maintain no more than 4 concurrent Python minor versions. These minor versions are generally anticipated each year (with a few exceptions), which is why the intention of this PR was to keep the number of CI workflow jobs the same (rather than increasing the CI load further). I'll pause on this PR until a consensus has been met, but would be happy to help out further when the timing is right.

@MatthewFlamm
Copy link
Contributor

It is starting to get very close to the EOL for 3.7 in terms of # of possible releases here. I don't think it necessarily makes much sense to support 3.7 if it is only recommended to use for a few months. Users of 3.7 can still use older versions of pyvista. By the time of the next release it will easily be within 4 months from EOL, which seems like a very reasonable time frame to me. I think numpy-style removing support 1.5+ years in advance was a bit quick. I guess I'm saying that I'm +1 for this change or anytime in the next few months.

I don't think there are significant features in 3.8 that will greatly improve the code here, so there is no big maintenance reason from python to remove support. The biggest gain is the push to remove vtk8. I'm surprised that we support multiple major versions of vtk here. I doubt vtk supports changes to vtk 8 at this point, but I haven't checked. While it currently works, it makes certain parts of the code complicated and overall lowers the velocity of changes. We currently turn off huge portions of testing for vtk8 so it feels a bit like it already isn't supported in some ways.

I think it will be easier from a PR perspective to make these changes separately but in the same release. This will also help as continuing the blueprint for future Python version removals. My thought is that it will be significantly more involved to remove vtk8, so it makes sense to do this first, as it gives us the possibility to postpone it by a release in case it takes time.

@banesullivan
Copy link
Member

try and maintain no more than 4 concurrent Python minor versions

I want to share some feedback from @prisae that I agree with:

Given that Python changed after 3.7 from a ~1.5 year release-cadence to a ~1 year cadence, one might want to test one version more. Hence 3.7-3.11 now, later 3.8-3.12, etc.

@banesullivan
Copy link
Member

I've updated CI to use Python 3.11 in #3944. @mwtoews, you'll need to rebase this PR and remove any 3.11-related changes

@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 6, 2023

If it also makes sense to also remove VTK8 in this PR, I can re-title and add subsequent commits to unravel this logic.

@banesullivan
Copy link
Member

If it also makes sense to also remove VTK8 in this PR, I can re-title and add subsequent commits to unravel this logic.

IMO, the Python 3.7 drop and VTK8 drop should come as one PR and thus one commit on main but it looks like others have different views

I say charge ahead with a simultaneous Python3.7 and VTK8 removal, as that'll get my approval to merge but fair warning that others here might want to handle this differently.

I want to re-emphasize that this might not get merged for a while, and after removing VTK8 support, the diff will likely become tough to manage with other development

@akaszynski
Copy link
Member

I think it will be easier from a PR perspective to make these changes separately but in the same release. This will also help as continuing the blueprint for future Python version removals. My thought is that it will be significantly more involved to remove vtk8, so it makes sense to do this first, as it gives us the possibility to postpone it by a release in case it takes time.

I'd like to drop 3.7 support first and then VTK 8 secondly. I agree with @MatthewFlamm in regards to providing a blueprint for future PRs. It also makes it easier to follow the blame.

@banesullivan
Copy link
Member

Fair enough! I'm on board as long as the VTK8 drop comes first

@akaszynski
Copy link
Member

@mwtoews, does that sound reasonable from your end?

@mwtoews mwtoews changed the title Drop Python 3.7, support 3.11 Drop Python 3.7 Feb 7, 2023
@mwtoews
Copy link
Contributor Author

mwtoews commented Feb 7, 2023

Good plan from my perspective, I'll hold this PR to only drop Python 3.7 and earlier logic. I don't mind rebasing this after dropping VTK8. However, it seems to me that a core dev should take a lead on that PR, as I'm not as familiar how different VTK releases are integrated with pyvista. Any takers? If nobody in another week or so I could consider taking this challenge up.

@akaszynski
Copy link
Member

I'd like to drop 3.7 support first and then VTK 8 secondly. I agree with @MatthewFlamm in regards to providing a blueprint for future PRs. It also makes it easier to follow the blame.

Looks like we did this the other way around with the merge of #4018, but I'm still fine with moving forward with this.

If there are no further issues with dropping Python 3.7 in main little early before it's EOL, let's merge this. When release time comes in May/June, we'll be right on time for its EOL.

akaszynski
akaszynski previously approved these changes Mar 8, 2023
@akaszynski akaszynski dismissed their stale review March 8, 2023 04:28

bad review...

pyvista/core/dataset.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.

LGTM

@akaszynski akaszynski merged commit 2af35da into pyvista:main Mar 9, 2023
@mwtoews mwtoews deleted the drop-37-add-311 branch March 9, 2023 18:37
@akaszynski akaszynski mentioned this pull request Apr 30, 2023
6 tasks
@banesullivan banesullivan mentioned this pull request May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Anything related to the documentation/website maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants