Opened 4 months ago

Last modified 9 days ago

#27826 needs_review enhancement

Deprecate 'long(...)'

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-8.9
Component: python3 Keywords:
Cc: embray Merged in:
Authors: John Palmieri Reviewers:
Report Upstream: N/A Work issues: Rebase
Branch: u/jhpalmieri/deprecate-long (Commits) Commit: ce2eb939124f015e888a1df6a2823318341e1b7e
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

This is part of #27696. long(...) is not defined in Python 3, and so long should be deprecated in Sage built with Python 2.

Change History (34)

comment:1 Changed 4 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/deprecate-long

comment:2 Changed 4 months ago by jhpalmieri

  • Commit set to d5fcb32c0b0100383e09754d319eb28ba18c5862
  • Status changed from new to needs_review

New commits:

d5fcb32trac 27826: deprecate __long__ methods for many Sage classes

comment:3 in reply to: ↑ description ; follow-up: Changed 4 months ago by jdemeyer

  • Status changed from needs_review to needs_info

Replying to jhpalmieri:

Notable classes which are not included: integers and rational numbers. This is mainly because those classes are used in so many places in the Sage library that deprecation warnings for them would get triggered many times during doctesting.

Are there really that many places where an integer/rational is explicitly converted to a long? That worries me. I really want to understand better why you're not applying this everywhere (needs_info for that reason).

About the patch itself:

  1. in src/sage/libs/mpmath/ext_main.pyx you messed up the spacing of """
  1. I don't understand the comment This is deprecated, but the ``TypeError`` takes precedence over the deprecation warning when this is run. How does the error "take precedence" over the warning, what are you trying to say?

comment:4 follow-up: Changed 4 months ago by jhpalmieri

Regarding integer/rational: if I add deprecation warnings to those, I get over 800 files with failures. Many involve set_random_seed or other uses of randomization:

sage -t --long src/sage/parallel/reference.py
**********************************************************************
File "src/sage/parallel/reference.py", line 34, in sage.parallel.reference.parallel_iter
Failed example:
    set_random_seed(0)
Expected nothing
Got:
    doctest:warning
      File "/Users/jpalmier/Desktop/Sage/git/sage/src/bin/sage-runtests", line 179, in <module>
        err = DC.run()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 1232, in run
        self.run_doctests()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 933, in run_doctests
        self.dispatcher.dispatch()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2000, in dispatch
        self.parallel_dispatch()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1897, in parallel_dispatch
        w.start()  # This might take some time
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2183, in start
        super(DocTestWorker, self).start()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/multiprocessing/process.py", line 130, in start
        self._popen = Popen(self)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/multiprocessing/forking.py", line 126, in __init__
        code = process_obj._bootstrap()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/multiprocessing/process.py", line 267, in _bootstrap
        self.run()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2139, in run
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2477, in __call__
        doctests, extras = self._run(runner, options, results)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2526, in _run
        result = runner.run(test)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 869, in run
        return self._run(test, compileflags, out)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.parallel.reference.parallel_iter[2]>", line 1, in <module>
        set_random_seed(Integer(0))
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/superseded.py", line 102, in deprecation
        warning(trac_number, message, DeprecationWarning, stacklevel)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/superseded.py", line 148, in warning
        warn(message, warning_class, stacklevel)
    :
    DeprecationWarning: use of long is deprecated, since long() will no longer be supported in Python 3
    See https://trac.sagemath.org/27826 for details.
**********************************************************************

An example without randomness:

sage -t --long src/sage/quadratic_forms/extras.py
**********************************************************************
File "src/sage/quadratic_forms/extras.py", line 50, in sage.quadratic_forms.extras.is_triangular_number
Failed example:
    F1 = [n for n in range(1,100*(100+1)/2)
          if is_triangular_number(n)]
Expected nothing
Got:
    doctest:warning
      File "/Users/jpalmier/Desktop/Sage/git/sage/src/bin/sage-runtests", line 179, in <module>
        err = DC.run()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 1232, in run
        self.run_doctests()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 933, in run_doctests
        self.dispatcher.dispatch()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2000, in dispatch
        self.parallel_dispatch()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1897, in parallel_dispatch
        w.start()  # This might take some time
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2183, in start
        super(DocTestWorker, self).start()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/multiprocessing/process.py", line 130, in start
        self._popen = Popen(self)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/multiprocessing/forking.py", line 126, in __init__
        code = process_obj._bootstrap()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/multiprocessing/process.py", line 267, in _bootstrap
        self.run()
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2139, in run
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2477, in __call__
        doctests, extras = self._run(runner, options, results)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2526, in _run
        result = runner.run(test)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 869, in run
        return self._run(test, compileflags, out)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1095, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.quadratic_forms.extras.is_triangular_number[7]>", line 1, in <module>
        F1 = [n for n in range(Integer(1),Integer(100)*(Integer(100)+Integer(1))/Integer(2))
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/superseded.py", line 102, in deprecation
        warning(trac_number, message, DeprecationWarning, stacklevel)
      File "/Users/jpalmier/Desktop/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/superseded.py", line 148, in warning
        warn(message, warning_class, stacklevel)
    :
    DeprecationWarning: use of long is deprecated, since long() will no longer be supported in Python 3
    See https://trac.sagemath.org/27826 for details.

comment:5 in reply to: ↑ 3 ; follow-up: Changed 4 months ago by jhpalmieri

Replying to jdemeyer:

About the patch itself:

  1. in src/sage/libs/mpmath/ext_main.pyx you messed up the spacing of """

Easy to fix.

  1. I don't understand the comment This is deprecated, but the ``TypeError`` takes precedence over the deprecation warning when this is run. How does the error "take precedence" over the warning, what are you trying to say?

By "take precedence", I mean that no deprecation warning is printed, just the TypeError. I'm trying to explain why there is no doctest with a deprecation warning. That could be clearer.

Last edited 4 months ago by jhpalmieri (previous) (diff)

comment:6 in reply to: ↑ 4 ; follow-up: Changed 4 months ago by jdemeyer

Replying to jhpalmieri:

Regarding integer/rational: if I add deprecation warnings to those, I get over 800 files with failures. Many involve set_random_seed or other uses of randomization

The question is why do you get so many failures?

comment:7 in reply to: ↑ 5 Changed 4 months ago by jdemeyer

Replying to jhpalmieri:

By "take precedence", I mean that no deprecation warning is printed, just the TypeError. I'm trying to explain why there is no doctest with a deprecation warning. That could be clearer.

I would just remove that note, it's fine.

comment:8 Changed 4 months ago by git

  • Commit changed from d5fcb32c0b0100383e09754d319eb28ba18c5862 to 2a1b5e40057252c426a7716408e89d1e097540fa

Branch pushed to git repo; I updated commit sha1. New commits:

2a1b5e4trac 27826: fix typo

comment:9 in reply to: ↑ 6 ; follow-up: Changed 4 months ago by jhpalmieri

Replying to jdemeyer:

Replying to jhpalmieri:

Regarding integer/rational: if I add deprecation warnings to those, I get over 800 files with failures. Many involve set_random_seed or other uses of randomization

The question is why do you get so many failures?

I would prefer to address that on another ticket: deal with the easy cases here (and also #27829) and then think about ZZ and QQ.

I haven't investigated at all, just saw that it was a mess and so I wanted to put it off. Does Python's range try to convert to long in Python 2? Or do some other Python built-ins do that?

comment:10 Changed 4 months ago by git

  • Commit changed from 2a1b5e40057252c426a7716408e89d1e097540fa to c70dce2a8effb5723cf1d6ab4cd5f798986f3fb4

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

c70dce2trac 27826: fix typo, remove some unneeded NOTEs.

comment:11 in reply to: ↑ 9 Changed 4 months ago by jdemeyer

  • Status changed from needs_info to needs_work

Replying to jhpalmieri:

I would prefer to address that on another ticket: deal with the easy cases here (and also #27829) and then think about ZZ and QQ.

-1

We clearly have no clue what we're doing or what the consequences are of that deprecation.

comment:12 Changed 4 months ago by jhpalmieri

I have no idea what your objection is or what work you think needs doing on this ticket. Can you explain?

comment:13 Changed 4 months ago by jdemeyer

In some cases (for integers and rationals), deprecating __long__ causes a large number of warnings. We don't know why there are so many warnings. So deprecating __long__ seems to have unforeseen consequences. This is a sign that we should be prudent when deprecating __long__.

I would much prefer to deal with all cases of __long__ together (this does not necessarily mean on one ticket, but it does mean understanding the consequences of deprecating all __long__ methods).

We might also make a difference between plain Python classes and Cython cdef classes.

comment:14 follow-up: Changed 4 months ago by jdemeyer

What's your plan for external packages calling long(...)? For example, fpylll does this and I don't think it's wrong for them to do that.

This is especially true from the point of view of Cython and the Python/C API, where the long class in Python 2 is the same as the int class in Python 3 (you might think that Python 3 removed the long class but what really happened is they removed int and then renamed long -> int).

So I'm now leaning towards closing this ticket as "wontfix".

comment:15 Changed 4 months ago by jdemeyer

Also numpy uses long internally (in their C code at least).

comment:16 in reply to: ↑ 14 ; follow-up: Changed 4 months ago by jhpalmieri

As far as I can tell, __long__ methods are not used in Python 3 (despite what you say about the renaming long -> int, int(x) does not call x.__long__()). Let me add a new goal to this, or rather make explicit a previously implicit goal: we want users to stop using the long() function, in order to ease the transition to Python 3. Deprecating __long__ is a way to do this. Is there a better way to accomplish this?

comment:17 follow-up: Changed 4 months ago by jhpalmieri

I would point out that there are many calls to long in sage.misc.randstate, so fixing those will help with some of these issues.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 4 months ago by jdemeyer

Could we replace long() in the REPL by a custom long() which shows a deprecation warning and then calls the real long()?

comment:19 in reply to: ↑ 17 Changed 4 months ago by jdemeyer

Replying to jhpalmieri:

I would point out that there are many calls to long in sage.misc.randstate, so fixing those will help with some of these issues.

But I don't agree that those calls are problematic. We really need a long there (or int in Python 3 terminology), so calling long() (which Cython understands as int on Python 3) seems the right thing to do.

comment:20 Changed 4 months ago by git

  • Commit changed from c70dce2a8effb5723cf1d6ab4cd5f798986f3fb4 to 9eafb320af9eabcbea251dc6ed1f4e042e8f3717

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

9eafb32trac 27826: in Python 2, deprecate the "long" function.

comment:21 in reply to: ↑ 18 Changed 4 months ago by jhpalmieri

Replying to jdemeyer:

Could we replace long() in the REPL by a custom long() which shows a deprecation warning and then calls the real long()?

Here's a version which attempts this.

comment:22 Changed 4 months ago by jhpalmieri

  • Status changed from needs_work to needs_review

comment:23 Changed 4 months ago by jhpalmieri

  • Description modified (diff)
  • Summary changed from Deprecate __long__ for many classes to Deprecate 'long(...)'

comment:24 Changed 4 months ago by jhpalmieri

  • Description modified (diff)

comment:25 Changed 4 months ago by jdemeyer

  • Cc embray added

Erik, what's your opinion on this?

comment:26 Changed 4 months ago by embray

Has this been tested on Python 3? I fear it might have problems on Python 3, ironically, due to #24755 but I'm not sure.

comment:27 Changed 4 months ago by jhpalmieri

Most of the new deprecation warnings are doctested with tests tagged # py2, so this shouldn't cause any issues with #24755. I believe as I was developing this, I was testing in parallel with both Python 2 and 3. I'm double-checking now...

comment:28 Changed 4 months ago by jhpalmieri

I've just run doctests with Python 3, and I don't see any new failures coming from this ticket. make ptest-python3 passes, and running tests on the other files changed here doesn't reveal any new problems.

Last edited 4 months ago by jhpalmieri (previous) (diff)

comment:29 Changed 3 months ago by jhpalmieri

Ping

comment:30 Changed 3 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:31 Changed 2 months ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Rebase

The branch apparently doesn't merge.

comment:32 Changed 2 months ago by git

  • Commit changed from 9eafb320af9eabcbea251dc6ed1f4e042e8f3717 to ce2eb939124f015e888a1df6a2823318341e1b7e

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

ce2eb93trac 27826: in Python 2, deprecate the "long" function.

comment:33 Changed 2 months ago by jhpalmieri

  • Status changed from needs_work to needs_review

Rebased.

comment:34 Changed 9 days ago by jhpalmieri

Any comments from anyone? I just tested with 8.9.rc0 on OS X (after doing git rebase): no failures with Python 2 or Python 3.

Note: See TracTickets for help on using tickets.