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

Python 2 removal #3688

Merged
merged 25 commits into from
Feb 11, 2022
Merged

Python 2 removal #3688

merged 25 commits into from
Feb 11, 2022

Conversation

rwgk
Copy link
Collaborator

@rwgk rwgk commented Feb 5, 2022

Description

This PR removes Python 2 support completely, and Python 2-isms almost completely.

Please see the commit history of this PR for the cleanup steps. The cleanup is assumed to be comprehensive.

The Python 2 removal unblocks updating black, which is included in this PR (03c8bb1).

Suggested changelog entry:

Python 2 support was removed completely.

@rwgk rwgk changed the title #error BYE_BYE_GOLDEN_SNAKE Python 2 removal part 1: tests (C++ code is intentionally ~untouched) Feb 5, 2022
@rwgk rwgk marked this pull request as ready for review February 5, 2022 23:17
@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

Let's always use PY_VERSION_HEX, better to be consistent. Let's make this < 3.6, no need to drop 2.7 and < 3.6 in separate steps.

@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

#3683 prereq?

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

Let's always use PY_VERSION_HEX, better to be consistent. Let's make this < 3.6, no need to drop 2.7 and < 3.6 in separate steps.

I'll make the PY_VERSION_HEX change, but I very strongly disagree dropping 3.5 in the same PR: lumping such clearly distinct steps together is a recipe for getting into trouble. Earlier in my career I got burned many times when I did that. It's a risk, and I don't see an upside that could justify taking that risk. Additionally, if there are questions later, backtracking will be much easier if we keep the version drops separate.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

#3683 prereq?

For the C++ cleanup (part 2) for sure. I will not start on that work before #3683 is merged.

This PR, I don't know. Happy to wait, happy to merge.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

The 1 previous CI failure (https://github.com/pybind/pybind11/runs/5080406438?check_suite_focus=true) was a well-known flake.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

The force push was just for git rebase master. No other changes. (GHA still doesn't trigger reliably, unfortunately. Idk why.)

@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

Systematic removal of u prefix from u"..." and u'...' literals

Please do not do things like this by hand. Pyupgrade does this for you. Update pyupgrade in pre-commit first, then only do manual cleanup as needed.

@@ -213,6 +213,9 @@


#include <Python.h>
#if PY_VERSION_HEX < 0x03000000
#error "PYTHON 2 IS NO LONGER SUPPORTED. pybind11 v2.9.1 was the last to support Python 2."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#error "PYTHON 2 IS NO LONGER SUPPORTED. pybind11 v2.9.1 was the last to support Python 2."
#error "PYTHON < 3.6 IS NO LONGER SUPPORTED. pybind11 v2.9 was the last to support Python 2.7-3.5."

sys.stdout = original


try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is so ugly. If we had used sys.version_info < (x, y), then pyupgrade would automatically clean up this sort of stuff for us.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing is true for all our custom PY2 markers. If we used the sys.version_info idiom, then pyupgrade does it automatically. Any MyPy understands it. Etc.

@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

Nice to see all this cleanup! Mind if I use pyupgrade here?

@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

You must bump python_requires. It's the single most important thing to bump, and it must be done first, otherwise you'd break Python 2 (and 3.5, let's drop both in one shot), instead of just dropping it. I'll apply a few of these things now, you can remove them if they are not helpful or clash. :) I haven't started on 3.5 yet, to make sure you were okay with that.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

You must bump python_requires. It's the single most important thing to bump, and it must be done first, otherwise you'd break Python 2 (and 3.5, let's drop both in one shot), instead of just dropping it. I'll apply a few of these things now, you can remove them if they are not helpful or clash. :)

Feel free to take it from here. I was hoping that you could help with the things I'm not familiar with (setup tools, cmake), but maybe it's better if you take the lead on everything.

As I explained before, I strongly feel it's a mistake to handle something as major as the Python 2 removal, together with a minor version drop for the next major version, but maybe we'll get lucky and there are no accidents, and the care I wanted to give to making the git history easy to navigate is not as important as I think.

I'll review this PR when you're done. Please let me know.

@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

IMO, and from doing this drop over a dozen times, we should be doing one action: changing the minimum version supported to 3.6. Doing it in stages is loading more work on ourselves; I'm already changing things to >3.5 when I'm going too have to go back and do it again with 3.6 - If I can remember / can re-grep it.

Anyway, done for now, will revisit later.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

Anyway, done for now, will revisit later.

I looked through your 4 commits, those all look great to me.

My vote would be to merge this as soon as possible, so that we don't run into merge conflicts with other development and maintenance work. The only open question is, what do we want to do about the Centos7 block? I'll trigger the CI in another (scratch) PR to see if that's still broken.

I see Python 3.5 is still there, which I still think is best. I'm guessing the Python 3.5 version drop will only be a few lines in comparison. Unless we have a strong and compelling reason for mixing those in now, it will be more useful to have those changes visible as a separate step in the git history. To have the same obviousness for the C++ code, the disciplined & most auditable approach would be:

  • merge this PR (because it's ready already)
  • finalize and merge the clang-format PR (this could come before this PR, but I don't think it matters; it's blocking for the next one though)
  • C++ Python 2 removal PR
  • Python 3.5 removal in CI & tests PR
  • C++ Python 3.5 removal PR

I'm guessing again the C++ Python 3.5 removal will be much smaller, but even if both parts of the 3.5 removal are small, keeping them as separate PRs will be a clear assurance (especially for people looking later) that the C++ code wasn't changed when the tests were manipulated in the first step, and test changes to go with the C++ changes, if any, can be easily identified and audited, because they are separate.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 6, 2022

The force push was again required b/o git rebase master. (The existing commits are unaffected.)

@@ -29,13 +29,13 @@ repos:
- id: mixed-line-ending
- id: requirements-txt-fixer
- id: trailing-whitespace
- id: fix-encoding-pragma
exclude: ^noxfile.py$
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
exclude: ^noxfile.py$

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ This is a hook specific include that should be removed along with the offending hook.

@Skylion007
Copy link
Collaborator

I went ahead and bumped pre-commit properly.

@henryiii
Copy link
Collaborator

henryiii commented Feb 6, 2022

We should merge the clang format PR first, so we have a common point to make a branch for backports. That’s the point. If we already started dropping then we don’t have that.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 7, 2022

I went ahead and bumped pre-commit properly.

Nice! I added a mention to the PR description.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 7, 2022

We should merge the clang format PR first, so we have a common point to make a branch for backports. That’s the point. If we already started dropping then we don’t have that.

Backports for Python 2.7? — I hadn't thought of that. I firmly agree though, even a weak reason is enough to prefer merging the clang-format PR before this one, at least if we move quickly, as I hope, so that this PR doesn't suffer from bit rot symptoms.

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 7, 2022

The force push was purely for git rebase master again.

@henryiii
Copy link
Collaborator

henryiii commented Feb 7, 2022

You can see what changed in force-pushes by clicking "force pushed" above. You don't have to explain every force push as long as they are for rebasing, squashing, or adding things missing in an original comment. ;)

@henryiii
Copy link
Collaborator

We have to rewrite this anyway, since it should say "pybind11 2.9 was the last version to support Python <3.6".

@rwgk
Copy link
Collaborator Author

rwgk commented Feb 10, 2022

We have to rewrite this anyway, since it should say "pybind11 2.9 was the last version to support Python <3.6".

The Python 2 drop is much more special, I think for a couple years this will be helpful:

pybind11 2.9 was the last version to support Python 2, and <3.6

While mathematically speaking that's redundant, I believe for practical purposes that's exactly what people will want to know.

@rwgk rwgk merged commit 6493f49 into pybind:master Feb 11, 2022
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Feb 11, 2022
@rwgk rwgk changed the title Python 2 removal part 1: tests (C++ code is intentionally ~untouched) Python 2 removal Feb 11, 2022
@rwgk rwgk deleted the python2_bye_bye branch February 11, 2022 15:26
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Mar 29, 2022
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 this pull request may close these issues.

3 participants