#28012 closed task (fixed)
py3: import certain things from collections.abc
Reported by:  Samuel Lelièvre  Owned by:  

Priority:  major  Milestone:  sage9.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: 
Description (last modified by )
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 fromcollections
or fromcollections.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
Branch:  → u/slelievre/collections.abc 

comment:2 Changed 3 years ago by
Commit:  → e04fa482bab365d19cbc5946800728b3fc382345 

Keywords:  py3 added 
comment:3 Changed 3 years ago by
Cc:  Frédéric Chapoton John Palmieri added 

Status:  new → needs_review 
comment:4 Changed 3 years ago by
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
Status:  needs_review → needs_work 

More changes are needed, as revealed by
$ git grep "collections\."
I'll push a commit.
comment:6 Changed 3 years ago by
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
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
Description:  modified (diff) 

comment:9 Changed 3 years ago by
Description:  modified (diff) 

comment:10 Changed 3 years ago by
Milestone:  sage8.8 → sage8.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
Milestone:  sage8.9 → sage9.1 

Ticket retargeted after milestone closed
comment:12 Changed 2 years ago by
Milestone:  sage9.1 → sage9.2 

Moving tickets to milestone sage9.2 based on a review of last modification date, branch status, and severity.
comment:13 Changed 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:14 Changed 2 years ago by
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
Branch:  u/slelievre/collections.abc → public/ticket/28012collections.abc 

Commit:  e04fa482bab365d19cbc5946800728b3fc382345 → 2187c31c9dac5a4d53cfcf5a8e271ed46d03fac3 
Keywords:  collections.abc added 
Status:  needs_work → needs_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:
2187c31  t28012: Collections Abstract Base Classes

comment:16 Changed 2 years ago by
Commit:  2187c31c9dac5a4d53cfcf5a8e271ed46d03fac3 → 29faddf94c784f0a08e0ad7a106c9f3e3a2c990b 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
29faddf  t28012: Collections Abstract Base Classes

comment:18 Changed 2 years ago by
Description:  modified (diff) 

comment:19 Changed 2 years ago by
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
Cc:  Travis Scrimshaw added 

comment:21 Changed 2 years ago by
Commit:  29faddf94c784f0a08e0ad7a106c9f3e3a2c990b → 489eccff3e15a229539b6086f5b5276dee366888 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
489eccf  t28012: Collections Abstract Base Classes

comment:22 Changed 2 years ago by
comment:23 Changed 2 years ago by
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:: 35 35 36 36 from ipywidgets.widgets import SelectionSlider, ValueWidget, ToggleButtons 37 37 from ipywidgets.widgets.interaction import interactive, signature 38 from collections import Iterable, Iterator, OrderedDict 38 from collections.abc import Iterable, Iterator 39 from collections import OrderedDict 39 40 from .widgets import EvalText, SageColorPicker 40 41 from .widgets_sagenb import input_grid 41 42 from 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/sage9.2.beta11/local/lib/python3.7/sitepackages/babel/localedata.py", line 17, in <module> from collections import MutableMapping
another with
File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/TESTING/sage9.2.beta11/local/lib/python3.7/sitepackages/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
Status:  needs_review → needs_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
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
Commit:  489eccff3e15a229539b6086f5b5276dee366888 → 1ee0124092f2759a2958905fb19402bca9bcffec 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1ee0124  t28012: Collections Abstract Base Classes

comment:27 Changed 2 years ago by
Commit:  1ee0124092f2759a2958905fb19402bca9bcffec → 0ce9e4043a959f5f2a21ad59f7179a2485c47575 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0ce9e40  t28012: Collections Abstract Base Classes

comment:28 Changed 2 years ago by
Status:  needs_work → needs_review 

Fixed and rebased on 9.2.beta12. Sorry for one more forcepush.
comment:29 Changed 2 years ago by
Status:  needs_review → needs_work 

Still needs work. Almost there. Will push shortly.
comment:30 Changed 2 years ago by
Commit:  0ce9e4043a959f5f2a21ad59f7179a2485c47575 → 4f8013611b5c27658e3ba49aea0bf1b4066b4adb 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4f80136  t28012: collections.abc

comment:32 Changed 2 years ago by
Reviewers:  → John Palmieri 

Status:  needs_review → positive_review 
Looks good to me.
comment:33 Changed 2 years ago by
Branch:  public/ticket/28012collections.abc → 4f8013611b5c27658e3ba49aea0bf1b4066b4adb 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:34 Changed 2 years ago by
Milestone:  sage9.3 → sage9.2 

comment:35 Changed 2 years ago by
Commit:  4f8013611b5c27658e3ba49aea0bf1b4066b4adb 

Followup at #30768 for unsilencing the deprecation warning.
comment:36 Changed 2 years ago by
Description:  modified (diff) 

New commits:
#28012 import from collections.abc