API Deprecate externals.six #12916

qinhanmin2014 · 2019-01-03T14:37:21Z

Remove six.py and some redundant imports.

rth

Sounds good but I wonder how to deal with the fact that our sklearn/externals are not really private and as a result, someone could have relied on sklearn/externals/six.py.

Maybe we could remove the six imports, and add a deprecation warning on top of sklearn.externals.six and keep it for at least one extra version? Though, that doesn't sound optimal either and I can be convinced against it.

qinhanmin2014 · 2019-01-03T14:57:31Z

Maybe we could remove the six imports, and add a deprecation warning on top of sklearn.externals.six and keep it for at least one extra version? Though, that doesn't sound optimal either and I can be convinced against it.

Not sure, I tend to remove it. It seems strange to keep six.py after removing python 2.X, and I think users should rely on official version of six (which is still under active development), not six in scikit-learn. Maybe we can inform it in what's new.

rth

Yes, let's at least document this in what's new, and wait for a second opinion.

Otherwise LGTM.

adrinjalali · 2019-01-03T17:23:55Z

I think it makes sense to deprecate externals in 0.21 and make it private as a whole (also discussed in #11182). Then once it's private, then remove six. If I understand it correctly, our convention is that we don't break any public API without a deprecation cycle, and unfortunately externals is still public.

So I'd suggest:

move externals to _externals
make a wrapper around _externals as externals and raise a deprecation warning whenever it's used.
Remove six (and possible joblib by then, I guess), once externals is removed.

qinhanmin2014 · 2019-01-04T01:37:12Z

I think it makes sense to deprecate externals in 0.21 and make it private as a whole (also discussed in #11182). Then once it's private, then remove six.

Maybe it makes sense to treat six specially?

adrinjalali · 2019-01-04T16:18:30Z

I personally would really like to see it gone of course. But I still think the responsible thing to do is to keep it and go through the deprecation cycle. Also, this is the kinda thing about which, I'd like to know the opinion of the rest of the TC.

To be clear, I'm just raising the issue and I'm personally not voting against removing it. I just would rather most of us be on the same page before removing it.

qinhanmin2014 · 2019-01-05T04:25:12Z

To be clear, I'm just raising the issue and I'm personally not voting against removing it. I just would rather most of us be on the same page before removing it.

Yes, this might be controversial. Let's wait for more opinions.
My point here is to drop useless code ASAP. I think users should rely on official version of six (which is still under active development), not six in scikit-learn. This is why I add a note in what's new and remove the file directly.

adrinjalali · 2019-01-05T17:31:10Z

I just realized I removed externals.funcsigs.py in #12746 myself, and we just did it. Following that, at least I should say we should simply remove six here as well :D

qinhanmin2014 · 2019-01-06T11:27:10Z

we just did it

I don't think we did it is a good reason for you to approve this PR @adrinjalali :)
We should think about the problem itself (maybe some reviewers missed this point in that huge PR).
Overall, my point is that we can remove things which are introduced for python < 3.5 directly without deprecation cycle, because this can be considered as part of our effort to drop python < 3.5.

jnothman · 2019-01-07T22:37:34Z

i I'm happy with deprecating six, and externals if we unvendor joblib for next release. I don't see any substantial benefit in "just removing".

adrinjalali · 2019-01-07T22:51:56Z

i I'm happy with deprecating six, and externals if we unvendor joblib for next release. I don't see any substantial benefit in "just removing".

Does that imply we should do the same for externals/funcsigs.py (i.e. add it back and deprecate it, which was removed in #12746)?

jnothman · 2019-01-07T23:09:16Z

I think its (funcsigs) removal will unlikely cause problems, but it certainly doesn't hurt much to put it back with deprecation

amueller · 2019-01-09T22:36:51Z

+1 for deprecating stuff instead of removing. I purposefully left six in.

This reverts commit 4d1eba8.

This reverts commit 750eb06.

qinhanmin2014 · 2019-01-26T07:28:27Z

I guess there's enough consensus to deprecate.

doc/whats_new/v0.21.rst

This reverts commit 76cbf0c.

"sklearn.externals.six" was removed from scikit-learn [v0.21](https://scikit-learn.org/dev/whats_new/v0.21.html#sklearn-externals) and the [PR](scikit-learn/scikit-learn#12916) said users should rely on the offical version of six instead of the one in scikit-learn.

* Add import for BaseRequest in dtool/_grave_depcache.py Missing import reported when running doctests: ``` Traceback (most recent call last): File "/virtualenv/env3/lib/python3.6/site-packages/xdoctest/utils/util_import.py", line 187, in _custom_import_modpath module = import_module_from_name(modname) File "/virtualenv/env3/lib/python3.6/site-packages/xdoctest/utils/util_import.py", line 345, in import_module_from_name module = importlib.import_module(modname) File "/virtualenv/env3/lib/python3.6/importlib/__init__.py", line 126, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 994, in _gcd_import File "<frozen importlib._bootstrap>", line 971, in _find_and_load File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 665, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 678, in exec_module File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed File "/wbia/wildbook-ia/wbia/dtool/_grave_depcache.py", line 138, in <module> class AlgoRequest(BaseRequest, ut.NiceRepr): NameError: name 'BaseRequest' is not defined ``` * Add "import utool" to dtool/_grave_depcache.py Missing import reported when running doctests: ``` Traceback (most recent call last): File "/virtualenv/env3/lib/python3.6/site-packages/xdoctest/utils/util_import.py", line 187, in _custom_import_modpath module = import_module_from_name(modname) File "/virtualenv/env3/lib/python3.6/site-packages/xdoctest/utils/util_import.py", line 345, in import_module_from_name module = importlib.import_module(modname) File "/virtualenv/env3/lib/python3.6/importlib/__init__.py", line 126, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 994, in _gcd_import File "<frozen importlib._bootstrap>", line 971, in _find_and_load File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 665, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 678, in exec_module File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed File "/wbia/wildbook-ia/wbia/dtool/_grave_depcache.py", line 141, in <module> class AlgoRequest(BaseRequest, ut.NiceRepr): NameError: name 'ut' is not defined ``` * Import "six" directly instead of "sklearn.externals.six" "sklearn.externals.six" was removed from scikit-learn [v0.21](https://scikit-learn.org/dev/whats_new/v0.21.html#sklearn-externals) and the [PR](scikit-learn/scikit-learn#12916) said users should rely on the offical version of six instead of the one in scikit-learn. * Fix guitool import in init/sysres.py When running doctests: ``` DOCTEST TRACEBACK Traceback (most recent call last): File "/virtualenv/env3/lib/python3.6/site-packages/xdoctest/doctest_example.py", line 553, in run exec(code, test_globals) File "<doctest:/wbia/wildbook-ia/wbia/gui/id_review_api.py::make_ensure_match_img_nosql_func:0>", line rel: 5, abs: 669, in <module> >>> cm, qreq_ = wbia.testdata_cm() File "/wbia/wildbook-ia/wbia/init/main_helpers.py", line 524, in testdata_cm a=a, File "/wbia/wildbook-ia/wbia/init/main_helpers.py", line 494, in testdata_cmlist a=a, File "/wbia/wildbook-ia/wbia/init/main_helpers.py", line 451, in testdata_qreq_ **kwargs, File "/wbia/wildbook-ia/wbia/init/main_helpers.py", line 342, in testdata_expanded_aids ibs = wbia.opendb(defaultdb=defaultdb) File "/wbia/wildbook-ia/wbia/main_module.py", line 594, in opendb defaultdb=defaultdb, allow_newdir=allow_newdir, db=db, dbdir=dbdir File "/wbia/wildbook-ia/wbia/init/sysres.py", line 346, in get_args_dbdir return db_to_dbdir(defaultdb, allow_newdir=allow_newdir) File "/wbia/wildbook-ia/wbia/init/sysres.py", line 219, in db_to_dbdir work_dir = get_workdir() File "/wbia/wildbook-ia/wbia/init/sysres.py", line 92, in get_workdir work_dir = set_workdir() File "/wbia/wildbook-ia/wbia/init/sysres.py", line 121, in set_workdir work_dir = guiselect_workdir() File "/wbia/wildbook-ia/wbia/init/sysres.py", line 156, in guiselect_workdir guitool.ensure_qtapp() NameError: name 'guitool' is not defined ``` This is because we imported guitool like this `import wbia.guitool`, instead I changed it to `from wbia import guitool`.

MNT Remove six.py

4d1eba8

rth reviewed Jan 3, 2019

View reviewed changes

what's new

750eb06

rth approved these changes Jan 3, 2019

View reviewed changes

This was referenced Jan 5, 2019

Make externals private? #11182

Closed

RFC Definition of public API #12927

Closed

adrinjalali approved these changes Jan 5, 2019

View reviewed changes

qinhanmin2014 added 3 commits January 26, 2019 15:12

Revert "MNT Remove six.py"

b6c0c4a

This reverts commit 4d1eba8.

Revert "what's new"

d0fed76

This reverts commit 750eb06.

deprecate

7e1f65e

qinhanmin2014 changed the title ~~MNT Remove six.py~~ API Deprecate externals.six Jan 26, 2019

rth approved these changes Jan 26, 2019

View reviewed changes

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved

rth's review

11e6381

jnothman approved these changes Jan 27, 2019

View reviewed changes

jnothman merged commit 60be46c into scikit-learn:master Jan 27, 2019

qinhanmin2014 deleted the remove-six branch January 27, 2019 13:56

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jan 30, 2019

API Deprecate externals.six (scikit-learn#12916)

4078baa

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 6, 2019

API Deprecate externals.six (scikit-learn#12916)

59e94d0

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 7, 2019

API Deprecate externals.six (scikit-learn#12916)

e335067

jrbourbeau mentioned this pull request Feb 28, 2019

[WIP] Replace externals.six.integer_types with int #13342

Merged

trevorstephens mentioned this pull request Apr 22, 2019

Remove dependency on scikit-learn's six trevorstephens/gplearn#151

Closed

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

API Deprecate externals.six (scikit-learn#12916)

76cbf0c

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "API Deprecate externals.six (scikit-learn#12916)"

5e812f3

This reverts commit 76cbf0c.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "API Deprecate externals.six (scikit-learn#12916)"

82c5805

This reverts commit 76cbf0c.

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

API Deprecate externals.six (scikit-learn#12916)

f3c7812

qinhanmin2014 mentioned this pull request Oct 21, 2019

MNT Remove six imports #15251

Merged

karenc mentioned this pull request Jun 10, 2020

Fix import errors reported by doctests WildMeOrg/wildbook-ia#30

Merged

nikosmichas mentioned this pull request Nov 4, 2020

Replace deprecated sklearn.externals.six MSc-in-Data-Science/class_material#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Deprecate externals.six #12916

API Deprecate externals.six #12916

qinhanmin2014 commented Jan 3, 2019

rth left a comment

qinhanmin2014 commented Jan 3, 2019

rth left a comment

adrinjalali commented Jan 3, 2019

qinhanmin2014 commented Jan 4, 2019

adrinjalali commented Jan 4, 2019

qinhanmin2014 commented Jan 5, 2019

adrinjalali commented Jan 5, 2019

qinhanmin2014 commented Jan 6, 2019

jnothman commented Jan 7, 2019 via email

adrinjalali commented Jan 7, 2019

jnothman commented Jan 7, 2019 via email

amueller commented Jan 9, 2019

qinhanmin2014 commented Jan 26, 2019

API Deprecate externals.six #12916

API Deprecate externals.six #12916

Conversation

qinhanmin2014 commented Jan 3, 2019

rth left a comment

Choose a reason for hiding this comment

qinhanmin2014 commented Jan 3, 2019

rth left a comment

Choose a reason for hiding this comment

adrinjalali commented Jan 3, 2019

qinhanmin2014 commented Jan 4, 2019

adrinjalali commented Jan 4, 2019

qinhanmin2014 commented Jan 5, 2019

adrinjalali commented Jan 5, 2019

qinhanmin2014 commented Jan 6, 2019

jnothman commented Jan 7, 2019 via email

adrinjalali commented Jan 7, 2019

jnothman commented Jan 7, 2019 via email

amueller commented Jan 9, 2019

qinhanmin2014 commented Jan 26, 2019