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

Kickstart support drop for Python < 3.7 #499

Merged
merged 16 commits into from
Jul 1, 2022

Conversation

leogama
Copy link
Contributor

@leogama leogama commented Jun 4, 2022

@mmckerns and @anivegesana, plus anyone interested: With the prospect of dill 0.3.6 release —it would be fun if it matched the lowest Python version supported, 3.7— in the near future, it is an opportunity to remove some cruft from the code base after the official support drop for Python 2 and Python < 3.7. Important: this is not a refactoring.

I propose to do so in discrete steps to facilitate review and minimize the chances of introducing bugs. The first steps should only remove code that Python/PyPy >= 3.7 will never run, with minimal code rewrite beyond necessary indentation changes and substitution of variables left as unconditional constants, after the other changes, by its literal values.

I plan to do each step in a separated commit, so that you guys can review the individual diffs and any unit test failure is easy to catch:

  • 1. Remove unreachable code and unused Python version constants (e.g. IS_PYPY2), and convert the new unconditional constants left to literals.
  • 2. Convert conditional code written as strings passed exec() due to syntax limitations to common if-else code. Remove conditional imports. Remove from __future__ imports.
  1. Clean-up now unused types definitions if they are not in _reverse_typemap, to still allow unpickling of objects created with older combinations of python+dill versions. Unalias some types for internal usage, like BytesIO that is renamed to StringIO —it was a bit confusing the first time I met it. → moved to Support drop for Python < 3.7: step 3 — remove unused types and save_*() functions #504
  2. Rewrite some snippets of code to take advantage of new Python 3 syntax if it improves code quality or speed, or if the used functions are deprecated in 3.7 → moved to Support drop for Python < 3.7: step 4 — update code to use Python 3.7 features #505

Neither all steps are strictly necessary, nor they need to be completed before the next release. If you have the time, it would be important to thoroughly review every change made (the more eyeballs, the better). Please feel completely free to make changes by committing directly here and to add or remove steps to the ones I'm suggesting.

Step 1 of code clean-up: mechanically remove if-else branches that would never
run in Python/PyPy >= 3.7.  There's still some unused or redundant code, left
for the next step to facilitate review.

Note: all the remaining `sys.hexversion` tests were standardized to use
numerical comparison and hexadecimal integer literals (e.g. `0x30b00a7` for
version 3.11.0a7) as most of them already were.
@anivegesana
Copy link
Contributor

There are a couple more that are no longer needed. save_slice is one of them, but it will require a little bit of reasoning and trying them all out in pickle to see which ones are in CPython now.

@leogama
Copy link
Contributor Author

leogama commented Jun 5, 2022

There are a couple more that are no longer needed. save_slice is one of them, but it will require a little bit of reasoning and trying them all out in pickle to see which ones are in CPython now.

Yes, the now obsolete save_<type>() functions can be safely removed in my proposed step 3, while keeping they correspondent _create_<type>() functions for backward compatibility. And you are right in that every component removed and modified in this critical step must be evaluated carefully and tested.

@leogama leogama marked this pull request as ready for review June 8, 2022 17:16
@mmckerns
Copy link
Member

mmckerns commented Jun 8, 2022

I'd split your steps into two or three different PRs -- keep this one as (1 & 2), and create a new PR for (4). I'm not sure the best place for (3). I think we can get (1 & 2) into dill-0.3.6.

Official end of support for 3.7 from python.org is 2023-06-27. So that means dill will end support for 3.7 in 2023 or 2024. It's still a good way off.

@leogama
Copy link
Contributor Author

leogama commented Jun 9, 2022

I'd split your steps into two or three different PRs -- keep this one as (1 & 2), and create a new PR for (4). I'm not sure the best place for (3).

Agreed. Is there a way to create an empty draft PR? Or do I nead at least a stub change?

@leogama
Copy link
Contributor Author

leogama commented Jun 9, 2022

@mmckerns I managed to open the PRs with empty commits. You can add the milestone to this one now.

@mmckerns mmckerns added this to the dill-0.3.6 milestone Jun 9, 2022
@leogama
Copy link
Contributor Author

leogama commented Jun 21, 2022

@mmckerns and @anivegesana: this is ready for review.

@mmckerns mmckerns self-requested a review June 21, 2022 19:54
dill/__diff.py Outdated Show resolved Hide resolved
tests/__main__.py Outdated Show resolved Hide resolved
tests/__main__.py Outdated Show resolved Hide resolved
dill/detect.py Outdated Show resolved Hide resolved
dill/detect.py Outdated Show resolved Hide resolved
dill/detect.py Outdated Show resolved Hide resolved
@mmckerns
Copy link
Member

I've reviewed all files except _dill.py and _objects.py, and all looks good thus far. I've commented where there are/were issues. Will finish this tomorrow.

dill/_dill.py Outdated Show resolved Hide resolved
dill/_dill.py Outdated Show resolved Hide resolved
dill/_dill.py Outdated Show resolved Hide resolved
dill/_dill.py Outdated Show resolved Hide resolved
Copy link
Member

@mmckerns mmckerns left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Thanks for all the hard (and tedious) work. Please address all of my comments and change requests. One thing to also watch out for in dill._dill is cases where we want to always want to trap any exception, regardless if it looks like there's only a particular type that is possible (i.e. the exception is used more like a if/else). I noted a few cases that I couldn't remember fully what scope the exception block was intended to have.

@leogama leogama requested a review from mmckerns June 26, 2022 19:25
@leogama
Copy link
Contributor Author

leogama commented Jun 26, 2022

Applied the requested corrections and then merged with master HEAD. All tests passing 100%. I'm feeling this is ready to go!

@leogama
Copy link
Contributor Author

leogama commented Jun 26, 2022

Generally LGTM. Thanks for all the hard (and tedious) work.

You're welcome! 🙂 Applying one step at a time to all files sped up the process. Vim search and substitutions also helped a lot. But I'm pretty sure that reviewing for you was as much as tedious as editing for me...

One thing to also watch out for in dill._dill is cases where we want to always want to trap any exception, regardless if it looks like there's only a particular type that is possible (i.e. the exception is used more like a if/else). I noted a few cases that I couldn't remember fully what scope the exception block was intended to have.

In the end, except for the ImportErrors, I left most cases as catch-all except Exception clauses. Either because I spotted they were a general error trap or because I had no idea of the kind of error expected. However, even if it stays as it is, it would be great if you could annotate the ones you still remember the scope, no matter how vague. Sometimes it is possible to infer what kind of things can go wrong based on the context, but some generic exception blocks seem like real mysteries.

@mmckerns
Copy link
Member

it would be great if you could annotate the ones you still remember the scope, no matter how vague.

Some of this code is 15 years old. I feel like I should get into a time machine and tell younger me to document the exception blocks more.

@mmckerns
Copy link
Member

Applying one step at a time to all files sped up the process.

Side note: can you email me a list of the keywords you searched for in your removal of dead code? I'm making similar changes on the other UQF codes.

@leogama
Copy link
Contributor Author

leogama commented Jun 28, 2022

Applying one step at a time to all files sped up the process.

Side note: can you email me a list of the keywords you searched for in your removal of dead code? I'm making similar changes on the other UQF codes.

Not many, really. What I remember is, for the conditional code:

  • PY3
  • IS_PYPY2
  • OLD3?
  • sys.hexversion

I think you'll find most Python 2 stuff by looking for these (if the other packages use the same conventions). Perhaps look for case insensitive py3, py2 and python 2 too. But after that you can look for Py2 specific idioms:

  • __builtin__, __future__
  • try/except [ImportError] blocks for conditional imports
  • u".*" or u'.*' strings (just delete the u)
  • super(cls, self) can become super()
  • class .*(object): can delete (object)
  • exec .* statements
  • other things listed in https://python-future.org/compatible_idioms.html

I didn't check for everything in dill though. Also, maybe take a look at https://docs.python.org/3/howto/pyporting.html

@mmckerns
Copy link
Member

mmckerns commented Jun 28, 2022

Thanks, that's similar to my list. I've also included attr( and version and 0x2/0x3. I often used getattr for 2/3 compatibility.

@leogama
Copy link
Contributor Author

leogama commented Jul 1, 2022

@mmckerns: just pushed the merge commit with the trace PR.

@mmckerns mmckerns merged commit 4220929 into uqfoundation:master Jul 1, 2022
@leogama leogama deleted the drop-2.7-3.6 branch July 1, 2022 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants