Skip to content

MRG Drop legacy python / remove six dependencies#12639

Merged
rth merged 32 commits intoscikit-learn:masterfrom
amueller:drop_legacy_python
Jan 3, 2019
Merged

MRG Drop legacy python / remove six dependencies#12639
rth merged 32 commits intoscikit-learn:masterfrom
amueller:drop_legacy_python

Conversation

@amueller
Copy link
Copy Markdown
Member

@amueller amueller commented Nov 21, 2018

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

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

jnothman commented Nov 21, 2018

There is an issue open for this...

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Nov 21, 2018

Maybe not a PR.

@amueller
Copy link
Copy Markdown
Member Author

amueller commented Nov 21, 2018

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
Copy Markdown
Member

rth commented Nov 21, 2018

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

@jorisvandenbossche
Copy link
Copy Markdown
Member

jorisvandenbossche commented Nov 23, 2018

@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
Copy Markdown
Member

qinhanmin2014 commented Dec 14, 2018

I guess we can close, see #12746

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

adrinjalali commented Dec 14, 2018

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
Copy Markdown
Member Author

amueller commented Dec 14, 2018

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

@amueller
Copy link
Copy Markdown
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
Copy Markdown
Contributor

JarnoRFB commented Dec 30, 2018

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
Copy Markdown
Member Author

amueller commented Dec 30, 2018

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

@adrinjalali
Copy link
Copy Markdown
Member

adrinjalali commented Dec 31, 2018

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

+1 more reviewer

@rth rth rth approved these changes

Reviewers whose approvals may not affect merge requirements

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