Skip to content

MRG Drop legacy python / remove six dependencies #12639

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 32 commits into from
Jan 3, 2019
Merged

MRG Drop legacy python / remove six dependencies #12639

merged 32 commits into from
Jan 3, 2019

Conversation

amueller
Copy link
Member

Tries to drop legacy python (2.7) and remove six everywhere.

rth
rth reviewed Nov 21, 2018
View reviewed changes
@jnothman
Copy link
Member

There is an issue open for this...

@jnothman
Copy link
Member

Maybe not a PR.

@amueller
Copy link
Member Author

This is very WIP. I didn't think anyone else has started so I posted this so no-one else would. @jnothman did you mean someone else had started?

@rth
Copy link
Member

rth commented Nov 21, 2018

Noone has as far as I know. The original issue is #11991

@jorisvandenbossche
Copy link
Member

@amueller do we already want to do this now? There was a little bit discussion about that on the issue (#11991), so maybe answer there.
But just want to note: if we decide to postpone doing such a clean-up as you are doing, I am not sure it makes sense to put a lot of effort in it now, as this will be an annoying diff to rebase.

@amueller amueller mentioned this pull request Dec 10, 2018
Merged
@timbicker timbicker mentioned this pull request Dec 12, 2018
Merged
4 tasks
@qinhanmin2014
Copy link
Member

I guess we can close, see #12746

@qinhanmin2014 qinhanmin2014 mentioned this pull request Dec 14, 2018
Closed
@adrinjalali
Copy link
Member

I wouldn't close this one. This includes some much "harder" changes than the "easy" ones I've had in the other PR. I guess we thought of continuing this one after the CI is changed.

@qinhanmin2014 qinhanmin2014 reopened this Dec 14, 2018
@amueller
Copy link
Member Author

I'll try and fix it "soon" ;)

@amueller
Copy link
Member Author

amueller commented Dec 28, 2018 •
edited
Loading

This should fix all comments, I think. Should we inherit from ABC instead of using metaclass=ABCMeta ? It's in Python 3.4 https://docs.python.org/3/library/abc.html#abc.ABC

@JarnoRFB
Copy link
Contributor

Hi, just wanted to say that I have removed Python 2 style super() calls in #12812, so that we don't accidentally duplicate work.

@amueller
Copy link
Member Author

thanks @JarnoRFB.
@rth @jnothman merge this while it's mergable? ;) we can change the ABC later if we like.

@adrinjalali
Copy link
Member

This just gives me the feeling of a nice house de-cluttering right before the new year :)

rth
rth approved these changes Jan 1, 2019
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @amueller !

jnothman
jnothman approved these changes Jan 2, 2019
View reviewed changes
Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

@adrinjalali says this is like de-cluttering. Perhaps burning photos of an old lover with whom you had on-and-off trouble, but okay.

I think I'm happy with this!

sklearn/utils/murmurhash.pyx and sklearn/feature_extraction/_hashing.pyx still reference unicode, which should be removed. Otherwise LGTM

@rth
Copy link
Member

rth commented Jan 2, 2019 •
edited
Loading

sklearn/utils/murmurhash.pyx and sklearn/feature_extraction/_hashing.pyx still reference unicode, which should be removed.

Currently, we are still using language_level=2 for Cython files. #12873 aims to address that, but meanwhile not touching cython files too much is probably fine.

@rth
Copy link
Member

rth commented Jan 3, 2019

Merging because this will get conflicts if some other PRs that I'm looking into get merged, and I have addressed @jnothman 's remaining comment above.

Thanks @amueller !

@rth rth merged commit 952ef66 into scikit-learn:master Jan 3, 2019
@rth rth mentioned this pull request Jan 5, 2019
Merged
lorentzenchr pushed a commit to lorentzenchr/scikit-learn that referenced this pull request Jan 6, 2019
Remove six dependencies as in scikit-learn#12639
7648be8 
* replace six.with_metaclass(ABCMeta) by metaclass=ABCMeta

* replace six.integer_types by int

* replace six.string_types by str

* correct email address
lorentzenchr pushed a commit to lorentzenchr/scikit-learn that referenced this pull request Jan 6, 2019
Remove six dependencies as in scikit-learn#12639
14c5a26 
* replace six.with_metaclass(ABCMeta) by metaclass=ABCMeta

* replace six.integer_types by int

* replace six.string_types by str

* correct email address
adrinjalali pushed a commit to adrinjalali/scikit-learn that referenced this pull request Jan 7, 2019
@amueller @adrinjalali
MRG Drop legacy python / remove six dependencies (scikit-learn#12639)
14621e8
lorentzenchr pushed a commit to lorentzenchr/scikit-learn that referenced this pull request Jan 7, 2019
Remove six dependencies as in scikit-learn#12639
177eb4c 
* replace six.with_metaclass(ABCMeta) by metaclass=ABCMeta

* replace six.integer_types by int

* replace six.string_types by str

* rebase

* correct email address
jnothman pushed a commit that referenced this pull request Jan 8, 2019
@rth @jnothman
MAINT Remove unused utils.fixes (#12928)
7011445 
This continues the work done in #12639 on dropping the python 2 support by,
 - ~~removing unnecessary `from __future__` imports~~
 - removing unused `sklearn.utils.fixes` assuming we can agree in #12927 that `sklearn.utils.fixes` are private as was stated e.g. in #6616 (comment)
@amueller
Copy link
Member Author

amueller commented Jan 9, 2019

OMG that's such a nice thing to come home to! Yay!

(also lol at @jnothman's comment)

@asottile asottile mentioned this pull request Jan 10, 2019
Closed
@rth rth mentioned this pull request Jan 16, 2019
Merged
@jrbourbeau jrbourbeau mentioned this pull request Feb 28, 2019
Merged
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@amueller
MRG Drop legacy python / remove six dependencies (scikit-learn#12639)
6d6e628
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@rth
MAINT Remove unused utils.fixes (scikit-learn#12928)
07ce93b 
This continues the work done in scikit-learn#12639 on dropping the python 2 support by,
 - ~~removing unnecessary `from __future__` imports~~
 - removing unused `sklearn.utils.fixes` assuming we can agree in scikit-learn#12927 that `sklearn.utils.fixes` are private as was stated e.g. in scikit-learn#6616 (comment)
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "MRG Drop legacy python / remove six dependencies (scikit-lear…
7a64191 
…n#12639)"

This reverts commit 6d6e628.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
Revert "MRG Drop legacy python / remove six dependencies (scikit-lear…
9f636aa 
…n#12639)"

This reverts commit 6d6e628.
rth pushed a commit to rth/scikit-learn that referenced this pull request Jun 4, 2019
@rth
Remove six dependencies as in scikit-learn#12639
fcce3f9 
* replace six.with_metaclass(ABCMeta) by metaclass=ABCMeta

* replace six.integer_types by int

* replace six.string_types by str

* rebase

* correct email address
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@amueller @koenvandevelde
MRG Drop legacy python / remove six dependencies (scikit-learn#12639)
3102b71
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@rth @koenvandevelde
MAINT Remove unused utils.fixes (scikit-learn#12928)
fbdf4ec 
This continues the work done in scikit-learn#12639 on dropping the python 2 support by,
 - ~~removing unnecessary `from __future__` imports~~
 - removing unused `sklearn.utils.fixes` assuming we can agree in scikit-learn#12927 that `sklearn.utils.fixes` are private as was stated e.g. in scikit-learn#6616 (comment)
@rth rth mentioned this pull request Oct 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

@rth rth rth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@amueller @jnothman @rth @jorisvandenbossche @qinhanmin2014 @adrinjalali @JarnoRFB