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

py3: import certain things from collections.abc #28012

Closed
slel opened this issue Jun 19, 2019 · 43 comments
Closed

py3: import certain things from collections.abc #28012

slel opened this issue Jun 19, 2019 · 43 comments

Comments

@slel
Copy link
Member

slel commented Jun 19, 2019

Background (read
Python 3 documentation for the collections module
for more detail):

  • Collections Abstract Base Classes
    ("CABCs") moved to the collections.abc module in Python 3.3.

  • These "Collections Abstract Base Classes" are: AsyncGenerator,
    AsyncIterable, AsyncIterator, Awaitable, Bytestring,
    Callable, Collection, Container, Coroutine, Generator,
    Hashable, ItemsView, Iterable, Iterator, KeysView,
    Mapping, MappingView, MutableMapping, MutableSequence,
    MutableSet, Reversible, Sequence, Set, Sized, ValuesView.

  • For backwards compatibility, CABCs continue to be visible
    in the collections module through Python 3.8; and can be
    imported either from collections or from collections.abc.

  • Starting with Python 3.9, CABCs must be imported from collections.abc.

  • In Python 2 they could only be imported from 'collections',
    not from 'collections.abc'. So while Sage was keeping
    Py2 and Py3 compatible it made sense to import from collections.

  • In Python 3.7 and Python 3.8, importing them from collections
    gives a deprecation warning. This warning was silenced in Sage,
    see py3: last test in tests/cmdline.py #28002.

In this ticket we import the classes from collections.abc.

We do not suppress the silencing of the deprecation warning yet.
See comment:23 and #30768.

CC: @fchapoton @jhpalmieri @slel @videlec @tscrim

Component: python3

Keywords: days101, collections.abc

Author: Samuel Lelièvre

Branch: 4f80136

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/28012

@slel slel added this to the sage-8.8 milestone Jun 19, 2019
@slel
Copy link
Member Author

slel commented Jun 19, 2019

Branch: u/slelievre/collections.abc

@slel
Copy link
Member Author

slel commented Jun 19, 2019

Commit: e04fa48

@slel
Copy link
Member Author

slel commented Jun 19, 2019

New commits:

e04fa48#28012 import from collections.abc

@slel
Copy link
Member Author

slel commented Jun 19, 2019

comment:4

Note: to check where such changes were needed I followed advice in a comment at #28002 and did

$ git grep "from collections import"

@slel
Copy link
Member Author

slel commented Jun 19, 2019

comment:5

More changes are needed, as revealed by

$ git grep "collections\."

I'll push a commit.

@slel
Copy link
Member Author

slel commented Jun 19, 2019

comment:6

The relevant lines in the output of git grep "collections\." seem to be:

src/sage/arith/misc.py:    if isinstance(ks[0], collections.Iterable):
src/sage/combinat/finite_state_machine.py:                         collections.Iterator):
src/sage/combinat/ranker.py:    from collections.abc import Iterable, Sequence
src/sage/combinat/ranker.py:    tuple (or more generally a :class:`collections.Sequence`), an
src/sage/combinat/ranker.py:    :class:`collections.Iterable`). See :trac:`15919`.
src/sage/combinat/ranker.py:    Lists, tuples, and other :class:`sequences <collections.Sequence>`::
src/sage/combinat/shuffle.py:        assert(isinstance(l1, collections.Iterable) and
src/sage/combinat/shuffle.py:               isinstance(l2, collections.Iterable)
src/sage/combinat/shuffle.py:        assert(all(isinstance(elem, collections.Iterable) for elem in l1))
src/sage/combinat/shuffle.py:        assert(all(isinstance(elem, collections.Iterable) for elem in l2))
src/sage/combinat/shuffle.py:        assert(isinstance(l1, collections.Iterable) and
src/sage/combinat/shuffle.py:               isinstance(l2, collections.Iterable)
src/sage/databases/conway.py:class DictInMapping(collections.Mapping):
src/sage/databases/conway.py:class ConwayPolynomials(collections.Mapping):
src/sage/game_theory/normal_form_game.py:    from collections.abc import MutableMapping
src/sage/geometry/cone.py:                            collections.Hashable,
src/sage/geometry/cone.py:                            collections.Iterable):
src/sage/geometry/cone.py:                                   collections.Container):
src/sage/geometry/fan.py:                            collections.Callable,
src/sage/geometry/fan.py:                            collections.Container):
src/sage/geometry/lattice_polytope.py:class LatticePolytopeClass(SageObject, collections.Hashable):
src/sage/geometry/lattice_polytope.py:                   collections.Hashable):
src/sage/matrix/matrix_integer_sparse.pyx:    from collections.abc import Iterator, Sequence
src/sage/matrix/matrix_modn_sparse.pyx:    from collections.abc import Iterator, Sequence
src/sage/misc/converting_dict.py:            if isinstance(arg, collections.Mapping):
src/sage/modules/with_basis/morphism.py:        if not isinstance(diagonal, collections.Callable):
src/sage/plot/colors.py:class Colormaps(collections.MutableMapping):
src/sage/plot/point.py:    if isinstance(points, collections.Iterator):
src/sage/repl/ipython_kernel/interact.py:    from collections.abc import Iterable, Iterator
src/sage/repl/ipython_kernel/widgets_sagenb.py:    from collections.abc import Iterable, Sequence

@jhpalmieri
Copy link
Member

comment:7

Two comments: there may be instances of this in other packages, like matplotlib. Also, it's not just warnings silenced in #28002, but an older silencing in sage/all.py.

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Jul 3, 2019

comment:10

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@embray
Copy link
Contributor

embray commented Dec 30, 2019

comment:11

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-8.9, sage-9.1 Dec 30, 2019
@mkoeppe
Copy link
Member

mkoeppe commented May 1, 2020

comment:12

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 1, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 20, 2020
@mkoeppe
Copy link
Member

mkoeppe commented Aug 22, 2020

comment:14

Now that we don't support py2 any more, the try/except approach is not needed any more.

@slel
Copy link
Member Author

slel commented Aug 22, 2020

Changed keywords from days101 to days101, collections.abc

@slel
Copy link
Member Author

slel commented Aug 22, 2020

comment:15

Sorry for letting over a year pass by. Here is a new branch. Please review.

The file finite_state_machine.py has import collections
and then collections.defaultdict, collections.OrderedDict,
collections.deque, collections.namedtuple, collections.abc.Iterator.

Would there be an advantage, or a disadvantage, to doing the following instead:

from collections import defaultdict, deque, namedtuple, OrderedDict
from collections.abc import Iterator

and then using defaultdict, OrderedDict, deque, namedtuple, Iterator?

I changed to that style in some other files.


New commits:

2187c31t-28012: Collections Abstract Base Classes

@slel
Copy link
Member Author

slel commented Aug 22, 2020

Changed commit from e04fa48 to 2187c31

@jhpalmieri
Copy link
Member

comment:25

The typical (but not universal) Sage style seems to be "from blah import foo" and then "foo(...)" rather than "import blah" and then "blah.foo(...)". So that's what I would choose, as you suggested.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from 489eccf to 1ee0124

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1ee0124t-28012: Collections Abstract Base Classes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Changed commit from 1ee0124 to 0ce9e40

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 7, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0ce9e40t-28012: Collections Abstract Base Classes

@slel
Copy link
Member Author

slel commented Sep 7, 2020

comment:28

Fixed and rebased on 9.2.beta12. Sorry for one more force-push.

@slel
Copy link
Member Author

slel commented Sep 8, 2020

comment:29

Still needs work. Almost there. Will push shortly.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2020

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4f80136t-28012: collections.abc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 10, 2020

Changed commit from 0ce9e40 to 4f80136

@slel
Copy link
Member Author

slel commented Sep 10, 2020

comment:31

Please review.

@jhpalmieri
Copy link
Member

comment:32

Looks good to me.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@vbraun
Copy link
Member

vbraun commented Sep 15, 2020

Changed branch from public/ticket/28012-collections.abc to 4f80136

@vbraun vbraun closed this as completed in 395be93 Sep 15, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.2 Sep 15, 2020
@slel
Copy link
Member Author

slel commented Oct 13, 2020

comment:35

Follow-up at #30768 for unsilencing the deprecation warning.

@slel
Copy link
Member Author

slel commented Oct 13, 2020

Changed commit from 4f80136 to none

@slel

This comment has been minimized.

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

No branches or pull requests

6 participants