-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Remove fixes/compat for Python 2 #11991
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
Comments
I also can't wait for this to happen, but I think we might want to wait for the final 0.20.0 release or even 0.20.1 before removing Py2 code to make backports easier. |
From experience with IPython/Jupyter and backport: do not rush to remove code, it will make backport way harder, and trip git bisect/git blame. We still have some py2 compat code here and there close to 18 month after dropping 2.7 and only remove it when it bother us. Also feel free to use our backport bot for trivial backport. |
Should we add some tests to make sure at least the new PRs don't include things such as |
If master is Py3only, the change to get python2-ism is slim. "I'm going to start implementing this feature by making sure it works with Python 2 even if the whole library is Python3 only" said no-one ever. There are few construct that will likely survive the transition (not making use of general unpacking... etc), but nothing easy to detect. One thing you might be able to do is turn more deprecation warnings into fatal errors. |
Thanks for sharing your experience on this @Carreau ! |
There are a few further references to Py2:
|
I am working on sklearn/datasets/base.py |
The function sklearn.datasets.base._pkl_filepath is a compatibility function for Python 2 support. The function expects path components as *args and constructs and returns the path to a pickel from it. It modifies the last path component as follows: - For Python 3, it adds a suffix ('_py3' by default, or the value of the kwarg 'py3_suffix' if specified). - For Python 2, it does nothing . It then concatenates its arguments using os.path.join and returns the result. If Python 2 support is to be deprecated, we can - remove the function _pkl_filepath and - replace all calls to it by direct calls to os.path.join.
Working on sklearn/ensemble/tests/test_forest.py |
Issue 'Remove fixes/compat for Python 2' An extra test with a list raised an exception in Python2. Since Python2 is deprecated we do not need this extra test any more.
The function sklearn.datasets.base._pkl_filepath is a compatibility function for Python 2 and Python 3 support. The function expects path components as *args and constructs and returns the path to a pickel from it. It modifies the last path component as follows: For Python 3, it adds a suffix ('_py3' by default, or the value of the kwarg 'py3_suffix' if specified). For Python 2, it does nothing . It then concatenates its arguments using os.path.join and returns the result. If Python 2 support is to be deprecated, we can simplify the code: Just always add the suffix. We cannot get rid of the function entirely since some users still might have Python 2 pickles on their filesystem. We can also shorten the docstring.
Is there any pending task in this issue? |
@albertvillanova you could check and see if everything in #11991 (comment) is done, or at least a PR is open for them. |
@adrinjalali that is the reason why I was asking if there are any additional tasks, because the ones mentioned in this issue seem to have been already addressed. |
Then I think it's safe to close this one. We can always have PRs which do further cleanups, but seems like this is mostly done. Feel free to re-open @jnothman if you see obvious leftovers. |
I think all of the points were indeed addressed here. @albertvillanova thanks! If you are looking for for another issue to contribute, you can also have a look at stalled PRs starting with those that are short and not too controversial as they are more likely to get merged faster. |
@rth thanks for the advice. However it's easier said than done. It seems that most of the PR are waiting for reviews, aren't they? |
@albertvillanova I've put some effort in checking stalled PR and labelling them as "help wanted": perhaps you will find your way in this pool. |
Thank you @cmarmo. That is very useful! |
This can be broken up into multiple pull requests.
Remove:
from __future__
importssklearn.externals.six
super(cls, self)
is nowsuper()
Admittedly I'm not sure it is wise to rush this change, given that we might still be backporting fixes to 0.20.X for a little while...
The text was updated successfully, but these errors were encountered: