Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#28012 closed task (fixed)

py3: import certain things from collections.abc

Reported by: Samuel Lelièvre Owned by:
Priority: major Milestone: sage-9.2
Component: python3 Keywords: days101, py3, collections.abc
Cc: Frédéric Chapoton, John Palmieri, Samuel Lelièvre, Vincent Delecroix, Travis Scrimshaw Merged in:
Authors: Samuel Lelièvre Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 4f80136 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Samuel Lelièvre)

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

  • 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 #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.

Change History (36)

comment:1 Changed 3 years ago by Samuel Lelièvre

Branch: u/slelievre/collections.abc

comment:2 Changed 3 years ago by Samuel Lelièvre

Commit: e04fa482bab365d19cbc5946800728b3fc382345
Keywords: py3 added

New commits:

e04fa48#28012 import from collections.abc

comment:3 Changed 3 years ago by Samuel Lelièvre

Cc: Frédéric Chapoton John Palmieri added
Status: newneeds_review

comment:4 Changed 3 years ago by Samuel Lelièvre

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

$ git grep "from collections import"

comment:5 Changed 3 years ago by Samuel Lelièvre

Status: needs_reviewneeds_work

More changes are needed, as revealed by

$ git grep "collections\."

I'll push a commit.

comment:6 Changed 3 years ago by Samuel Lelièvre

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

comment:7 Changed 3 years ago by John Palmieri

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.

comment:8 Changed 3 years ago by Jeroen Demeyer

Description: modified (diff)

comment:9 Changed 3 years ago by Jeroen Demeyer

Description: modified (diff)

comment:10 Changed 3 years ago by Erik Bray

Milestone: sage-8.8sage-8.9

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).

comment:11 Changed 3 years ago by Erik Bray

Milestone: sage-8.9sage-9.1

Ticket retargeted after milestone closed

comment:12 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.2

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

comment:13 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:14 Changed 2 years ago by Matthias Köppe

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

comment:15 Changed 2 years ago by Samuel Lelièvre

Branch: u/slelievre/collections.abcpublic/ticket/28012-collections.abc
Commit: e04fa482bab365d19cbc5946800728b3fc3823452187c31c9dac5a4d53cfcf5a8e271ed46d03fac3
Keywords: collections.abc added
Status: needs_workneeds_review

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

comment:16 Changed 2 years ago by git

Commit: 2187c31c9dac5a4d53cfcf5a8e271ed46d03fac329faddf94c784f0a08e0ad7a106c9f3e3a2c990b

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

29faddft-28012: Collections Abstract Base Classes

comment:17 Changed 2 years ago by Samuel Lelièvre

I had left one file out, I forced push.

comment:18 Changed 2 years ago by Samuel Lelièvre

Description: modified (diff)

comment:19 Changed 2 years ago by Samuel Lelièvre

Cc: Vincent Delecroix added

Vincent, I now see you already did the work at #28315 a year ago.

Sorry for delaying this. Would you like to review?

comment:20 Changed 2 years ago by Matthias Köppe

Cc: Travis Scrimshaw added

comment:21 Changed 2 years ago by git

Commit: 29faddf94c784f0a08e0ad7a106c9f3e3a2c990b489eccff3e15a229539b6086f5b5276dee366888

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

489eccft-28012: Collections Abstract Base Classes

comment:22 Changed 2 years ago by Samuel Lelièvre

Rebased on 9.2.beta11


New commits:

489eccft-28012: Collections Abstract Base Classes

comment:23 Changed 2 years ago by John Palmieri

I'm getting some deprecation warnings. Some can be fixed by this:

  • src/sage/repl/ipython_kernel/interact.py

    diff --git a/src/sage/repl/ipython_kernel/interact.py b/src/sage/repl/ipython_kernel/interact.py
    index 5fb318a6e4..ac0376ca63 100644
    a b EXAMPLES:: 
    3535
    3636from ipywidgets.widgets import SelectionSlider, ValueWidget, ToggleButtons
    3737from ipywidgets.widgets.interaction import interactive, signature
    38 from collections import Iterable, Iterator, OrderedDict
     38from collections.abc import Iterable, Iterator
     39from collections import OrderedDict
    3940from .widgets import EvalText, SageColorPicker
    4041from .widgets_sagenb import input_grid
    4142from sage.structure.element import parent

Some are coming from other Python packages, though. (This is with a Sage build using a system Python 3.7.) For example, one doctest fails because of

      File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage-9.2.beta11/local/lib/python3.7/site-packages/babel/localedata.py", line 17, in <module>
        from collections import MutableMapping

another with

      File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage-9.2.beta11/local/lib/python3.7/site-packages/requests/packages/urllib3/util/selectors.py", line 11, in <module>
        from collections import namedtuple, Mapping

Maybe we need to delay the suppression of the silencing of the deprecation warning. ;)

comment:24 Changed 2 years ago by Samuel Lelièvre

Status: needs_reviewneeds_work

I agree:

  • src/repl_ipython_kernel/interact.py does need the change you say
  • suppressing the silencing of the warnings does need to be delayed

I will take care of that. Any advice about my question in 15?

comment:25 Changed 2 years ago by John Palmieri

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.

comment:26 Changed 2 years ago by git

Commit: 489eccff3e15a229539b6086f5b5276dee3668881ee0124092f2759a2958905fb19402bca9bcffec

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

1ee0124t-28012: Collections Abstract Base Classes

comment:27 Changed 2 years ago by git

Commit: 1ee0124092f2759a2958905fb19402bca9bcffec0ce9e4043a959f5f2a21ad59f7179a2485c47575

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

0ce9e40t-28012: Collections Abstract Base Classes

comment:28 Changed 2 years ago by Samuel Lelièvre

Status: needs_workneeds_review

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

comment:29 Changed 2 years ago by Samuel Lelièvre

Status: needs_reviewneeds_work

Still needs work. Almost there. Will push shortly.

comment:30 Changed 2 years ago by git

Commit: 0ce9e4043a959f5f2a21ad59f7179a2485c475754f8013611b5c27658e3ba49aea0bf1b4066b4adb

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

4f80136t-28012: collections.abc

comment:31 Changed 2 years ago by Samuel Lelièvre

Status: needs_workneeds_review

Please review.

comment:32 Changed 2 years ago by John Palmieri

Reviewers: John Palmieri
Status: needs_reviewpositive_review

Looks good to me.

comment:33 Changed 2 years ago by Volker Braun

Branch: public/ticket/28012-collections.abc4f8013611b5c27658e3ba49aea0bf1b4066b4adb
Resolution: fixed
Status: positive_reviewclosed

comment:34 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.3sage-9.2

comment:35 Changed 2 years ago by Samuel Lelièvre

Commit: 4f8013611b5c27658e3ba49aea0bf1b4066b4adb

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

comment:36 Changed 2 years ago by Samuel Lelièvre

Description: modified (diff)
Note: See TracTickets for help on using tickets.