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 support for Python 2 #556

Open
cclauss opened this issue Jun 18, 2021 · 25 comments
Open

Drop support for Python 2 #556

cclauss opened this issue Jun 18, 2021 · 25 comments

Comments

@cclauss
Copy link
Contributor

cclauss commented Jun 18, 2021

Python 2 died on more than 500 days ago on 1/1/2020 and should be considered an insecure platform. Our installation and development processes are both more complicated because we still support legacy Python.

Current Python is v3.9.
https://devguide.python.org/#status-of-python-branches v.s.
https://devguide.python.org/devcycle/#end-of-life-branches

@hamishwillee
Copy link
Contributor

hamishwillee commented May 24, 2023

@auturgy @peterbarker Didn't you say we had dropped support for Python 2? I think we should formalise that across the project, starting by removing the Python 2.7 checks from CI.

What can I do to make this happen? It is starting to become a maintenance hassle - see #817

While we're at it, would also be good to drop Python 3.6, but that's another story.

@cclauss
Copy link
Contributor Author

cclauss commented May 25, 2023

Python 2 died 1,240 days ago on 1/1/2020. Python 3.6 died in 2021. Python 3.7 goes EOL in one month...

From my perspective, https://devguide.python.org/versions should be the metric. If the Python Core Team supports a version of Python then this repo should also support it.

If folks want to use EOL or pre-release versions of Python then maintainers of this repo should provide minimal (not recommended) support. If folks are paying for long-term support for an OS that has an EOL version of Python then their OS provider can provide them the support they are paying for and maintainers of this repo are no longer obliged to do so for free. If folks are using an OS that is beyond the standard support and are not paying for LTS, then their problems are not for maintainers of this repo to solve.

Building on secure platforms and being able to use new language optimizations and features helps both our safety and velocity. Python 3.11 is 25% faster than Python 3.10 and this optimization trend will continue in Py3.12 (currently in early beta) and then 3.13 annual releases. Supporting four versions of Python is sufficient. Let's encourage security and performance upgrades.

@hamishwillee
Copy link
Contributor

Our customers and those of the Python core team are different, and have different lifecycles. Our usage of python is not one where a million % increase in performance would make all that much difference. So I don't think we can draw the lines quite that hard.

But you're right that once the older versions start costing us to maintain, then a decision has to be made about whether those old versions are worth the cost of carrying. Definitely not for Python 2.7, but and hopefully not for Python 3.6. I've pinged people offline so I am sure this will get properly discussed at various dev calls in the coming weeks.

@cclauss
Copy link
Contributor Author

cclauss commented May 25, 2023

Your customers are doing more on their computers than just running pymavlink. If they do not care about performance then perhaps they care about safety. Running software that no longer receives security fixes can often mean that they are working on an insecure platform. https://www.cvedetails.com/vulnerability-list.php?vendor_id=10210&product_id=18230

@hamishwillee
Copy link
Contributor

hamishwillee commented May 25, 2023

They care about safety, but for the most part old Python versions do not affect that in the drone domain. The code on the flight stack isn't running Python - we just use it as part of the build toolchain. Some drones with companion computers will be running Python to send messages, but that code isn't really exposed to the same kinds of attacks that most people worry about - say in websites.

Maybe I'm wrong. But since I want this to happen I'm concentrating on the thing that I know (from experience) is most likely to affect the outcome. I'm not going to discuss this any more, because we want the same thing.

@rotu
Copy link
Contributor

rotu commented Jun 24, 2023

GitHub CI just deprecated python 2 so our CI workflow is now failing: actions/setup-python#672

@rotu
Copy link
Contributor

rotu commented Jun 25, 2023

Here's some things I noticed. Some of this might be in scope for #829.

  1. There is some documentation that needs to be updated to remove support for python 2:
  2. This looks weird and like possibly a mistake. It looks like we're running flake8 on python2 code:
    flake8 --ignore='W503,E203,E501,E741' --statistics dialects/v10/python2/*.py dialects/v20/python2/*.py
  3. The classifier tags are out of sync with CI versions:

    pymavlink/setup.py

    Lines 102 to 112 in f41a355

    classifiers=['Development Status :: 5 - Production/Stable',
    'Environment :: Console',
    'Intended Audience :: Science/Research',
    'License :: OSI Approved :: GNU Lesser General Public License v3 (LGPLv3)',
    'Operating System :: OS Independent',
    'Programming Language :: Python :: 2.7',
    'Programming Language :: Python :: 3.6',
    'Programming Language :: Python :: 3.7',
    'Programming Language :: Python :: 3.8',
    'Programming Language :: Python :: 3.9',
    'Topic :: Scientific/Engineering',
  4. There's python 2 compatibility code to be removed:
  5. It would be nice to have the version support documented. Python 3.5 and 3.5 are out-of-support and there should be a plan for until when they need to be supported.

@cclauss
Copy link
Contributor Author

cclauss commented Jun 25, 2023

Let's keep the pull requests minimal as experience tells me that review cycles can be really long on this repo.

  1. Older versions of flake8 run just fine on Python 2.

  2. Running setup.py is also deprecated.

  3. Much of the conversion and getting rid of legacy code can be automated with https://github.com/PyCQA/modernize or https://python-future.org/futurize.html plus https://beta.ruff.rs/docs/rules/#pyupgrade-up

  4. Python 3.7 is also goes end-of-life on Tuesday... https://devguide.python.org/versions/

@rotu
Copy link
Contributor

rotu commented Jun 26, 2023

Let's keep the pull requests minimal as experience tells me that review cycles can be really long on this repo.

Agreed. That's why I commented on this issue instead of on the PR. There are definitely things here out of scope

2. Older versions of `flake8` run just fine on Python 2.

It seems to me that the if statement is because mypy is incompatible with python3.5 Though it's unclear why the PATH passed to flake8 differs by python version. Or why we're calling flake8 here when it's called in a previous CI step.

if [ "${{ matrix.python-version }}" = "3.5" ]
then
flake8 --ignore='W503,E203,E501,E741' --statistics dialects/v10/python2/*.py dialects/v20/python2/*.py
else
flake8 --ignore='W503,E203,E501,E741' --statistics dialects/
mypy --config-file ./.github/workflows/mypy-generated.ini dialects/v10/*.py dialects/v20/*.py
fi

3. Running `setup.py` is also deprecated.

Running setup.py directly is deprecated. It still is part of the pip install process and populates the "classifiers" sidebar on PyPI. Those version tags are the most visible signal of which versions a Python package supports.

4. Much of the conversion and getting rid of legacy code can be automated with https://github.com/PyCQA/modernize or https://python-future.org/futurize.html plus https://beta.ruff.rs/docs/rules/#pyupgrade-up

Very cool! I didn't know about ruff. It looks like maybe futurize has already been run on the code (and that compatibility code should be removed at some point).

5. Python 3.7 is also goes end-of-life on Tuesday... https://devguide.python.org/versions/

TIL and good riddance! I wish I knew if and when pymavlink can drop support for dead versions. For all I know there's a policy for pymavlink that I just don't know about and it's just not well-documented.

@cclauss
Copy link
Contributor Author

cclauss commented Jun 26, 2023

On 2. I tried it the other way and it failed so I reverted.
On 3. trove classifiers can alternatively go in setup.cfg or more preferably pypproject.toml.
On 4. ruff --fix will remove some older syntax.
On 5. Yes. Supporting five versions of Python is enough hassle.

@rotu
Copy link
Contributor

rotu commented Jun 26, 2023

A more complete answer to 2, which is helpfully explained here.

While type hints were introduced in 3.5, variable annotations weren't introduced until 3.6. The mavgen-generated code for python3 includes variable annotations, which flake8 flags as syntactically invalid. Yet another reason to drop 3.5.

@hamishwillee
Copy link
Contributor

Just had a discussion with @peterbarker (who also chatted to @tridge). We're in principle very happy to drop support for Python 2.

What we mean by this is that:

  • we will start communicating that it is not supported
  • We won't build it in CI
  • We'll start taking PRs that strip it out of code. You're welcome to submit them.

With respect to Python 3 versions we're in principle happy to drop versions once they reach end of life, with the same caveats. I, for example, use Ubuntu 20.04 on a daily basis, which has Python 3.8 by default. If that is still a supported LTS platform for MAVLink then I would not want to necessarily drop support for Python 3.8 even if it reaches EOL.
I'll raise discussion in mav call.

@rotu
Copy link
Contributor

rotu commented Jun 28, 2023

The most important thing is to set expectations. Python 3.5, 3.6, and 3.7 are all end-of-life. @hamishwillee:

  1. For each of these out-of-support versions, when is PyMavlink dropping it as a target for development?
  2. Which, of the scripts in the tools subdirectory (many of which have no test coverage) need to be maintained and which can be removed?

@hamishwillee
Copy link
Contributor

@rotu I could not agree more re "1". Hence raising on the meeting. Currently I'm getting resistance on removing 3.6 as it is the default version installed on Ubuntu 18.04, which is in wide use. My take is "so what" - it isn't like you can't upgrade easily on that platform.

W.r.t. 2 I'd say we try run them on Python 3.6, any that fail we raise a PR to remove or fix, based on our own evaluation. It is the only way to get review because the Pymavlink reviwers are both overworked, and not all that interested in change. A PR to update that works is likely IMO to be faster to get considered than one that removes a file.

@rotu
Copy link
Contributor

rotu commented Jun 28, 2023

My take is "so what" - it isn't like you can't upgrade easily on that platform.

That's half of it. The other half is why do you feel compelled to update pymavlink if you can't be bothered to update Python?

Part of the answer is that often project dependencies leave the version unspecified, so a breaking change will "infect" downstream packages. It might be reasonable to say "we're introducing a new change. If you want compatibility, specify your version."

For new versions of the python interpreter, there's another solution: python_requires. e.g. If I add a python_requires>=3.6 constraint, then users of python 3.5 who pip install or depend on my package otherwise will get the previous version of the package.

W.r.t. 2 I'd say we try run them on Python 3.6, any that fail we raise a PR to remove or fix, based on our own evaluation.

Trouble is if a script has no tests or documentation, and I don't have a robust data set to test it on, I can't effectively run the script or know if it's working.

@hamishwillee
Copy link
Contributor

hamishwillee commented Jun 29, 2023

For new versions of the python interpreter, there's another solution: python_requires. e.g. If I add a python_requires>=3.6 constraint, then users of python 3.5 who pip install or depend on my package otherwise will get the previous version of the package.

Good point. I'm sure we'll take a PR to add Python 3.6 in the setup script. That's a good thing to do in any case, so it is not arguable.

Actually, might be good to add Python 2.7 for one pymavlink release so there is a fallback package, then remove it after?

Trouble is if a script has no tests or documentation, and I don't have a robust data set to test it on, I can't effectively run the script or know if it's working.

Then I don't see what you can do other than raise an issue for any (or the group of) scripts that have no documentation, and/or are not in CI - making it clear you'd like to validate these for Python 3. If the pymavlink team don't respond, then there is nothing else you can do.

@khancyr
Copy link
Contributor

khancyr commented Jun 29, 2023

For python 3.6, I think we will be able to move over it.

For the scripts on tools, unless we intentionnally break them with a banner, we won't be able to know who is truely using them ....

@peterbarker
Copy link
Contributor

peterbarker commented Jun 29, 2023 via email

@hamishwillee
Copy link
Contributor

hamishwillee commented Jul 6, 2023

@peterbarker

Don't break people's working setups as far as we can avoid it. That's not documented. Just trying to be nice.

In principle, of course, but exactly who's setup are we breaking if we make the change to dump Python 3.6 and 3.7.
I would say 0.001% of users who need to make other changes anyway. Is it worth maintaining old versions forever for those users, who can easily upgrade Python?

Considering the case of 18.04 companion computer users. The vast majority of them are never going to upgrade their MAVLink software to need new messages.
The very small number that need to do so (i.e. in order to get a new message) can easily be prompted that they need new Python, and installation upgrade is easy. They aren't broken, they are benefiting from improvements to the toolchain.

No one will every upgrade if we don't encourage that path.

So why do we want to upgrade?

Personally I'd like to update to a new XML validator that requires Python 3.8. I'd also like to be able to rely on ordered dicts in my code, which are also only supported from 3.8.

So the answer is "so that people can use Python as it is meant to be used", starting from the last EOL version.

@peterbarker
Copy link
Contributor

peterbarker commented Jul 6, 2023 via email

@cclauss
Copy link
Contributor Author

cclauss commented Jul 6, 2023

The PyPI download stats… https://pypistats.org/packages/pymavlink
See Python major and minor graphs. Zooming Python major into 30 day or 60 days shows a pattern where once a week there are Py2 downloads but the other days of the week it relatively flatlines. Is there a weekly CI job somewhere that is responsible for these Saturday traffic blips.

The Python minor graphics highlight that most usage is currently on Py38.

@hamishwillee
Copy link
Contributor

@cclauss Little of that traffic is likely driven by drones. Peter is also not particularly concerned about the majority. What is concerned about (rightly IMO) is cost of breaking drone consumers vs maintenance. Where we disagree is at what point the line is.
He's doing active maintenance and I am not, which "unfortunately" gives his opinion more weight.

@hamishwillee
Copy link
Contributor

@peterbarker Thank you for the article - that is interesting. The article does point out that dict is ordered "by luck" in Python 3.6, and yes you can use OrderedDict. That does make my concrete example invalid. The theory is sound though :-)

I'd suggest that that's as people want to use it. Did you see what happened when we tried to move to 3.8? It was less than a day before the screams emerged....

No I didn't. When was that? Back before Ubuntu 18.04 went EOL?

Validator ...

Yes the validator can fail gracefully (if not now, that is addable). Of course if we dropped EOL Python I wouldn't have to think about this - I could use any module supported by live Python.

Contribution

Its a pity that no features introduced after Python 3.6 are worth using, otherwise our archaic Python might be a barrier to contribution :-).

Which is the same as saying "I don't have any other pain points right now", but I suspect most people who do will just give up and stop working with us.

@peterbarker
Copy link
Contributor

peterbarker commented Jul 6, 2023 via email

@khancyr
Copy link
Contributor

khancyr commented Jul 6, 2023

yep, that spike on weekend should be ArduPilot CI testing environement with python2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants