pymavlink icon indicating copy to clipboard operation
pymavlink copied to clipboard

Drop support for Python 2

Open cclauss opened this issue 4 years ago • 25 comments

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

cclauss avatar Jun 18 '21 02:06 cclauss

@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 https://github.com/ArduPilot/pymavlink/pull/817

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

hamishwillee avatar May 24 '23 22:05 hamishwillee

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.

cclauss avatar May 25 '23 05:05 cclauss

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.

hamishwillee avatar May 25 '23 05:05 hamishwillee

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

cclauss avatar May 25 '23 05:05 cclauss

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.

hamishwillee avatar May 25 '23 06:05 hamishwillee

GitHub CI just deprecated python 2 so our CI workflow is now failing: https://github.com/actions/setup-python/issues/672

rotu avatar Jun 24 '23 21:06 rotu

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:
    • https://mavlink.io/en/guide/scripts.html
    • https://github.com/ArduPilot/pymavlink/blob/f41a355596a363b0855093268b9d6a157261fc67/README.md?plain=1#L18C1-L18C1
    • https://github.com/ArduPilot/pymavlink/blob/f41a355596a363b0855093268b9d6a157261fc67/generator/swift/README.md?plain=1#L22
  2. This looks weird and like possibly a mistake. It looks like we're running flake8 on python2 code: https://github.com/ArduPilot/pymavlink/blob/eebded18647478e7e93c78365a507d34071b9c34/.github/workflows/test.yml#L67
  3. The classifier tags are out of sync with CI versions: https://github.com/ArduPilot/pymavlink/blob/f41a355596a363b0855093268b9d6a157261fc67/setup.py#L102-L112
  4. There's python 2 compatibility code to be removed:
    • https://github.com/search?q=repo%3AArduPilot%2Fpymavlink%20print_function&type=code
    • https://github.com/ArduPilot/pymavlink/blob/f41a355596a363b0855093268b9d6a157261fc67/tools/mavfft_int.py#L22C1-L25
    • https://github.com/ArduPilot/pymavlink/blob/f41a355596a363b0855093268b9d6a157261fc67/generator/mavgen.py#L347-L351
    • https://github.com/ArduPilot/pymavlink/blob/f41a355596a363b0855093268b9d6a157261fc67/DFReader.py#L25-L28
    • https://github.com/search?q=repo%3AArduPilot%2Fpymavlink%20python2&type=code
  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.

rotu avatar Jun 25 '23 21:06 rotu

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/

cclauss avatar Jun 25 '23 21:06 cclauss

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.

https://github.com/ArduPilot/pymavlink/blob/eebded18647478e7e93c78365a507d34071b9c34/.github/workflows/test.yml#L65-L71

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.

rotu avatar Jun 26 '23 16:06 rotu

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.

cclauss avatar Jun 26 '23 17:06 cclauss

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.

rotu avatar Jun 26 '23 18:06 rotu

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.

hamishwillee avatar Jun 28 '23 07:06 hamishwillee

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?

rotu avatar Jun 28 '23 17:06 rotu

@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.

hamishwillee avatar Jun 28 '23 22:06 hamishwillee

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.

rotu avatar Jun 28 '23 23:06 rotu

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.

hamishwillee avatar Jun 29 '23 00:06 hamishwillee

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 ....

khancyr avatar Jun 29 '23 07:06 khancyr

On Mon, 26 Jun 2023, Dan Rose wrote:

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.

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

We're not asking the Python community to support their stuff, they've got their own needs and wants. But there are a lot of people out there running Bionic on their companion computers and it's not yet time to make life difficult for them. Ergo we continue to support an out-of-support Python version for now.

But Python2 is dead. Definitely, definitely dead and buried.

Here's something to think about, however. When we're investigating problems in ArduPilot it's often advantageous to be able to run very old versions of the code. However, the infrastructure on those old versions is all Python2-based. Dropping Py2 will make it more difficult to debug those old versions - will have to install older versions of pymavlink if bringing up a VM, that sort of thing. Fun times.

peterbarker avatar Jun 29 '23 09:06 peterbarker

@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.

hamishwillee avatar Jul 06 '23 00:07 hamishwillee

On Wed, 5 Jul 2023, Hamish Willee wrote:

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.

Finally! Two solid examples of where ruining people's day actually benefits someone else :-) Thanks for that. Interesting article on the history of ordred dicts in Python: https://realpython.com/python-ordereddict/

So the answer is "so that people can use Python as it is meant to be used"

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....

I'm also a touch confused here. If anything in pymavlink breaks when run under a more recent Python3 we fix it, right? So there's nothing to stop you using collections.OrderedDict in your own code, surely?

Can't speak for tridge, but I'd be entirely happy for the validator to fail on Python3 < 3.8 - so long as there's a way around that validation failure (which I think there already is).

peterbarker avatar Jul 06 '23 00:07 peterbarker

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.

cclauss avatar Jul 06 '23 04:07 cclauss

@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 avatar Jul 06 '23 04:07 hamishwillee

@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.

hamishwillee avatar Jul 06 '23 04:07 hamishwillee

On Wed, 5 Jul 2023, Christian Clauss wrote:

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 traffic blips.

@khancyr has some environment setup tests which run infrequently - it may be those.

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

I've always liked that stats page :-)

peterbarker avatar Jul 06 '23 05:07 peterbarker

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

khancyr avatar Jul 06 '23 10:07 khancyr