Opened 23 months ago

Closed 8 months ago

Last modified 7 months ago

#28012 closed task (fixed)

py3: import certain things from collections.abc

Reported by: slelievre Owned by:
Priority: major Milestone: sage-9.2
Component: python3 Keywords: days101, py3, collections.abc
Cc: chapoton, jhpalmieri, slelievre, vdelecroix, tscrim 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 slelievre)

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 23 months ago by slelievre

  • Branch set to u/slelievre/collections.abc

comment:2 Changed 23 months ago by slelievre

  • Commit set to e04fa482bab365d19cbc5946800728b3fc382345
  • Keywords py3 added

New commits:

e04fa48#28012 import from collections.abc

comment:3 Changed 23 months ago by slelievre

  • Cc chapoton jhpalmieri added
  • Status changed from new to needs_review

comment:4 Changed 23 months ago by slelievre

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 23 months ago by slelievre

  • Status changed from needs_review to needs_work

More changes are needed, as revealed by

$ git grep "collections\."

I'll push a commit.

comment:6 Changed 23 months ago by slelievre

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 23 months ago by jhpalmieri

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 23 months ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 23 months ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 22 months ago by embray

  • Milestone changed from sage-8.8 to sage-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 16 months ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:12 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

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

comment:13 Changed 9 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:14 Changed 9 months ago by mkoeppe

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

comment:15 Changed 9 months ago by slelievre

  • Branch changed from u/slelievre/collections.abc to public/ticket/28012-collections.abc
  • Commit changed from e04fa482bab365d19cbc5946800728b3fc382345 to 2187c31c9dac5a4d53cfcf5a8e271ed46d03fac3
  • Keywords collections.abc added
  • Status changed from needs_work to 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:

2187c31t-28012: Collections Abstract Base Classes

comment:16 Changed 9 months ago by git

  • Commit changed from 2187c31c9dac5a4d53cfcf5a8e271ed46d03fac3 to 29faddf94c784f0a08e0ad7a106c9f3e3a2c990b

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 9 months ago by slelievre

I had left one file out, I forced push.

comment:18 Changed 9 months ago by slelievre

  • Description modified (diff)

comment:19 Changed 9 months ago by slelievre

  • Cc vdelecroix 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 8 months ago by mkoeppe

  • Cc tscrim added

comment:21 Changed 8 months ago by git

  • Commit changed from 29faddf94c784f0a08e0ad7a106c9f3e3a2c990b to 489eccff3e15a229539b6086f5b5276dee366888

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 8 months ago by slelievre

Rebased on 9.2.beta11


New commits:

489eccft-28012: Collections Abstract Base Classes

comment:23 Changed 8 months ago by jhpalmieri

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 8 months ago by slelievre

  • Status changed from needs_review to 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 8 months ago by jhpalmieri

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 8 months ago by git

  • Commit changed from 489eccff3e15a229539b6086f5b5276dee366888 to 1ee0124092f2759a2958905fb19402bca9bcffec

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 8 months ago by git

  • Commit changed from 1ee0124092f2759a2958905fb19402bca9bcffec to 0ce9e4043a959f5f2a21ad59f7179a2485c47575

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 8 months ago by slelievre

  • Status changed from needs_work to needs_review

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

comment:29 Changed 8 months ago by slelievre

  • Status changed from needs_review to needs_work

Still needs work. Almost there. Will push shortly.

comment:30 Changed 8 months ago by git

  • Commit changed from 0ce9e4043a959f5f2a21ad59f7179a2485c47575 to 4f8013611b5c27658e3ba49aea0bf1b4066b4adb

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

4f80136t-28012: collections.abc

comment:31 Changed 8 months ago by slelievre

  • Status changed from needs_work to needs_review

Please review.

comment:32 Changed 8 months ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Looks good to me.

comment:33 Changed 8 months ago by vbraun

  • Branch changed from public/ticket/28012-collections.abc to 4f8013611b5c27658e3ba49aea0bf1b4066b4adb
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:34 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.2

comment:35 Changed 7 months ago by slelievre

  • Commit 4f8013611b5c27658e3ba49aea0bf1b4066b4adb deleted

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

comment:36 Changed 7 months ago by slelievre

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