-
Notifications
You must be signed in to change notification settings - Fork 124
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
Declare Python 3.7+ support #212
Conversation
I would think 3.6+ would have to be fine at this point. |
Up to you. It can either be 1) the minimum Python version that provides all the language features you use, or 2) the minimum Python version that you actually test, or 3) somewhere in-between. Same for libspatialindex. 3.6 is also EOL now, and many libraries like numpy only support 3.8+. |
My preferred minimum is also py36, which is under some support by (e.g.) Ubuntu 18.04 for at least another year. |
Let's point the docs at https://numpy.org/neps/nep-0029-deprecation_policy.html for our Python deprecation policy. Rtree releases are about at most once-per year. |
I actually personally disagree with numpy's Python deprecation policy (3.8+). My own project sticks to PyTorch's Python support timeline (3.6+) but if I didn't have to worry about dependencies I would either support all versions of Python that I can test in CI or stick to the Python EOL timeline (3.7+). I think numpy deprecates too early even though they don't need new Python language features. So my personal recommendation would be 3+ if you aren't using any new language features, 3.5+ if you're planning on adding type hints, or 3.7+ if you don't want to support EOL Python. |
Let's go 3.5+ then. If you are willing to make a PR that adds type hints, people would find them very useful. |
I am very interested in adding type hints but it might be on a case by case basis. For functions with good docstrings that document parameter and return type it will be really easy. For undocumented functions whose purpose I don't understand I won't be of much help. Not sure when I'll have time to get to this but hopefully in the near future. |
Okay, I set the minimum supported Python version to 3.5. I also increased the number of Python versions that are tested in CI and that wheels are generated for. Let me know if you want me to undo that last change. |
It looks like the Windows tests are failing for older versions of Python. Is this a known issue or can we solve it or should I skip these failing tests? |
I don't think it is worth trying to keep 3.5 around. I'm not sure what the specific issue is, but presumably it is in how ctypes resolves DLLs. |
It looks like the issue also hits Python 3.6. Should we bump to 3.7+ for all platforms or only for Windows? |
This would be preferable for me. In our next release in a year, no one is going to care about 3.5 or 3.6 if they even care right now. |
Windows tests are happy again. Not sure why docs tests are failing. I'm explicitly installing Python 3.10 but later steps are still using Python 3.6. |
In TorchGeo, we use ReadTheDocs CI instead of GitHub Actions to ensure that the docs build without any warnings/errors. This also has the benefit that you can directly view the docs that would result from a PR being merged. That's also an option here, although I would love to get to the bottom of why these tests are failing. |
The issue with the docs is the docker container we use |
See OSGeo/PROJ#3043 to get Python 3.8 in the container |
@mwtoews so once that PR is merged, the container will use Python 3.8? I'll still probably get started on switching the CI to RtD while waiting for that, it's a good idea anyway. |
Some manual steps are required for the docker image to be updated. I don't have any direct control in this process. Nevertheless, RTD is a great option! |
|
Looks like the tests are passing now, thanks @mwtoews! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Since #137, rtree no longer supports Python 2. However, this isn't documented anywhere. This PR explicitly documents that Python 3 is required.
Note: if we add type hints as proposed in #170 the minimum version will likely increase to Python 3.5+.