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

Add a six.moves for collections and collections.abc in preparation fo… #241

Closed
wants to merge 2 commits into from
Closed

Add a six.moves for collections and collections.abc in preparation fo… #241

wants to merge 2 commits into from

Conversation

WildCard65
Copy link

…r Python 3.7+ (Closes #155)

@webknjaz
Copy link
Contributor

@benjaminp could you please take a look at this PR?

@rth
Copy link

rth commented Jun 9, 2018

This would be very helpful to use six for improving compatibility with the upcoming Python 3.7

six.py Outdated
@@ -300,6 +300,7 @@ class _MovedItems(_LazyModule):
MovedModule("urllib_robotparser", "robotparser", "urllib.robotparser"),
MovedModule("xmlrpc_client", "xmlrpclib", "xmlrpc.client"),
MovedModule("xmlrpc_server", "SimpleXMLRPCServer", "xmlrpc.server"),
MovedModule("collections_abc", "collections", "collections.abc", sys.version_info[0:2] >= (3, 3)),

Choose a reason for hiding this comment

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

Why not just sys.version_info >= (3, 3) instead of sys.version_info[0:2] >= (3, 3)?

Copy link
Author

Choose a reason for hiding this comment

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

Consistency with the PYX variables at file's beginning

six.py Outdated
@@ -102,9 +102,9 @@ def __get__(self, obj, tp):

class MovedModule(_LazyDescr):

def __init__(self, name, old, new=None):
def __init__(self, name, old, new=None, use_new=True):

Choose a reason for hiding this comment

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

How does this help?

Choose a reason for hiding this comment

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

Sorry, I see now. Perhaps you should pass use_new in by name to make the code below clear.

Choose a reason for hiding this comment

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

Would it be better to parametrise by "version_changed" and do the comparison here?
Or leave it all on the call side, but use

MovedModule("collections_abc", "collections", "collections.abc" if sys.version_info[0:2] >= (3, 3) else "collections")

Copy link
Author

Choose a reason for hiding this comment

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

Let's wait to see what @benjaminp says before we change anything, I only use that parameter because any Python 3 version below 3.3 doesn't include the "abc" submodule.

Copy link

@timhoffm timhoffm Nov 1, 2018

Choose a reason for hiding this comment

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

Alternatively to a boolean use_new one could add an argument that specifies from which version on the new behavior should be used, e.g. since='3.3' or new_since='3.3'.

@jaraco
Copy link
Contributor

jaraco commented Jul 3, 2018

Since six deals with the Python 2/3 boundary, this request seems misplaced, unless the project seeks to expand its scope for general cross-version compatibility.

@WildCard65
Copy link
Author

WildCard65 commented Jul 3, 2018 via email

@jnothman
Copy link

jnothman commented Jul 3, 2018

@jaraco: if not making this change forces libraries to either use hacks or drop Python 2 support (when otherwise they support Py>=3.4), then surely it's within six's scope.

amueller pushed a commit to scikit-learn/scikit-learn that referenced this pull request Jul 14, 2018
…1431)

Closes #11121

This PR removes the deprecation warning about ABC being moved from `collections` to `collections.abc` when importing scikit-learn in Python 3.7.

In the end, I put `collections.abc.{Sequence, Iterable, Mapping, Sized}` in the namespace of `sklearn.utils.fixes`. This was the simplest way I could find, and while it has the drawback of obfuscating the real module name, other approached appeared more problematic and a similar approach is currently used e.g. for `utils.fixes.signature` which is an alias for `inspect.signature`.

We can't just patch six with benjaminp/six#241, because sklearn uses six from 5 years ago, which would  need updating and I'm not sure if it could have side effects (e.g. for pickling backward compatibility etc).

**Edit**: This adds a test checking that generally no warnings are raised when importing scikit-learn top-level modules.
wdevazelhes pushed a commit to wdevazelhes/scikit-learn that referenced this pull request Jul 15, 2018
…ikit-learn#11431)

Closes scikit-learn#11121

This PR removes the deprecation warning about ABC being moved from `collections` to `collections.abc` when importing scikit-learn in Python 3.7.

In the end, I put `collections.abc.{Sequence, Iterable, Mapping, Sized}` in the namespace of `sklearn.utils.fixes`. This was the simplest way I could find, and while it has the drawback of obfuscating the real module name, other approached appeared more problematic and a similar approach is currently used e.g. for `utils.fixes.signature` which is an alias for `inspect.signature`.

We can't just patch six with benjaminp/six#241, because sklearn uses six from 5 years ago, which would  need updating and I'm not sure if it could have side effects (e.g. for pickling backward compatibility etc).

**Edit**: This adds a test checking that generally no warnings are raised when importing scikit-learn top-level modules.
@WildCard65
Copy link
Author

@timhoffm I decided to use your suggestion of adding a "since" version variable.

six.py Outdated
super(MovedModule, self).__init__(name)
if PY3:
if sys.version_info[0:len(since)] >= since:

Choose a reason for hiding this comment

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

The 0 can be left out.

Choose a reason for hiding this comment

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

As well as slicing at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can suggest changes via GH now :)

Suggested change
if sys.version_info[0:len(since)] >= since:
if sys.version_info >= since:

six.py Outdated
@@ -300,6 +300,7 @@ class _MovedItems(_LazyModule):
MovedModule("urllib_robotparser", "robotparser", "urllib.robotparser"),
MovedModule("xmlrpc_client", "xmlrpclib", "xmlrpc.client"),
MovedModule("xmlrpc_server", "SimpleXMLRPCServer", "xmlrpc.server"),
MovedModule("collections_abc", "collections", "collections.abc", (3, 3)),

Choose a reason for hiding this comment

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

I would use since as kwarg here to make it more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MovedModule("collections_abc", "collections", "collections.abc", (3, 3)),
MovedModule("collections_abc", "collections", "collections.abc", since=(3, 3)),

Made MovedAttribute class have similar behaviour as MovedModule (future proofing)
@WildCard65
Copy link
Author

Suggested changes have been made, I also decided to future proof MovedAttribute incase it ever becomes required.

@taleinat
Copy link

This looks ready to go, and I would appreciate avoiding the deprecation warning with Python 3.7.

@tirkarthi
Copy link

Related CPython PR : python/cpython#10596

@NeilGirdhar
Copy link

Can we push this through? This would help with other projects.

@stmcginnis
Copy link

Any way to help move this along?

@benjaminp
Copy link
Owner

My initial reaction is similar to jaraco's. The planned release date for Python 3.8 is two months before Python 2's EoL.

Note the CPython PR to actually remove collections.abc, python/cpython#10596, is stuck due to the small part of the external Python universe that is vendorized into CPython.

@NeilGirdhar
Copy link

NeilGirdhar commented Apr 25, 2019

@benjaminp Is there a compatibility library for changes between Python versions 2 and 3.8? There needs to be one for the same reason that six was needed. Why not have six do this?

(Edited: I meant from Python 2 to Python 3.8.)

@serhiy-storchaka
Copy link

For compatibility between Python 3 versions you have to just import from collections.abc. six.moves is not needed for this.

@NeilGirdhar
Copy link

NeilGirdhar commented Apr 25, 2019

@serhiy-storchaka You're right. Unfortunately, many projects need to support Python 2, which has no collections.abc. Therefore, a compatibility library is needed for this. See the linked issues in this thread.

@jaraco
Copy link
Contributor

jaraco commented Apr 25, 2019

Is there a compatibility library for changes between Python versions 3.x and 3.y?

There isn't, probably because many of the compatibility issues are selective (only between particular versions of Python), such as packages in the backports namespace or projects like configparser or importlib_resources that provide backports but following another pattern. It doesn't necessarily make sense to have one library to collect all compatibility modules and in fact such an approach has the unfortunate downside of having to drag along lots of historically compatibility that a given project may not need (and to eventually deprecate that in the project as versions become unsupported). Instead, I think it's better for each compatibility layer to have its own library. It might make sense to have a suite of compatibility libraries that cover specific compatibility concerns, such as this one which is a moved import between Python 3.3 and 3.4, might go in a "python_33_compat" package (or similar), and relied upon by projects requiring Python 3.3 or older.

@jaraco
Copy link
Contributor

jaraco commented Apr 25, 2019

Back in the Python 2 days, I used to maintain a project called jaraco.compat that would provide forward-compatibility (backports) of various language features. Perhaps that project could be revived to support small compatibilities like this one.

@jaraco
Copy link
Contributor

jaraco commented Apr 25, 2019

I'm rolling out jaraco.compat 2.2 with py33compat.collections.abc.

@ndevenish
Copy link

Is there anything blocking this? Was surprised to find that there was no option in six to avoid:

if six.PY3:
  from collections.abc import Iterable, Sequence
else:
  from collections import Iterable, Sequence

@jaraco
Copy link
Contributor

jaraco commented May 27, 2019

If you want a one-liner, use from py33compat.collections.abc import Iterable, Sequence:

~ $ python2.7 -m pip-run -q jaraco.compat>=3.1 -- -c "from py33compat.collections.abc import Iterable, Sequence" && echo done                                                  
DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.
done
~ $ python3.7 -m pip-run -q jaraco.compat>=3.1 -- -c "from py33compat.collections.abc import Iterable, Sequence" && echo done                                                  
done

@NeilGirdhar
Copy link

@jaraco The problem is that a lot of projects don't want to add another dependency. I don't see why this isn't in six.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add six.moves.collections.abc