#24559 closed enhancement (fixed)

py3: better backwards-compatibility for Python longs

Reported by: embray Owned by:
Priority: major Milestone: sage-8.3
Component: python3 Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: e3b8148 (Commits) Commit: e3b8148c764377ac86fa532432676ca357e3f56b
Dependencies: Stopgaps:

Description

This does three little things to better support long-related functionality, especially in the doctests. This allows a lot of tests to pass on Python 3 that wouldn't otherwise, or would need to be skipped and/or have Python 3 specific copies:

  • The Sage preparser supports Python 2 style long literals, like 42L. These are just treated as normal ints on Python 3 (we could also consider adding a deprecation warning for this usage). I actually wish Python itself had done this (just as they restored backwards-compat for u'' literals--that they didn't I think is just that explicit longs is relatively uncommon.
  • The doctest parser has a fixup for recognizing long literals in the expected output of a test, and treating them as normal ints on Python 3. This is probably the weakest part of the patch in terms of implementation--it's very naïve and I can imagine the possibility of false positives, though it is still pretty unlikely. Open to better ideas on this.
  • In sage.all I added, on Python 3 only, a new global called long which is just a function that wraps int() and (except in the tests, for now) raises a DeprecationWarning for using long(). This still allows a lot of tests to work. I deliberately made it a simple function, and not a class, as the latter would tempt too many errors with code like isinstance(x, long).

Change History (30)

comment:1 Changed 20 months ago by embray

  • Dependencies set to #24258
  • Status changed from new to needs_review

comment:2 Changed 20 months ago by git

  • Commit changed from 55f7a642ae60f371eed22c4aa4550cc8a30ecb78 to e3c14f32ac26b184520ee530b3e4de754971a004

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

e3c14f3Properly handle cases like '1rL'

comment:3 follow-up: Changed 20 months ago by jdemeyer

I don't really with the condition deprecation depending on DOCTEST_MODE. Doctests should be as close as possible to a normal interactive session. I'm fine with the long = int alias, but then I would remove the deprecation.

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

Replying to jdemeyer:

I don't really with the condition deprecation depending on DOCTEST_MODE. Doctests should be as close as possible to a normal interactive session. I'm fine with the long = int alias, but then I would remove the deprecation.

This was added very deliberately in part to help with porting the doctests, so as to allow the great many doctests that use long() to continue working for now without interruption (while at the same time giving a useful warning for user code).

My intention, though not explicitly stated, was to later (once we want to switch Sage to Python 3) remove the condition so that it's easy to find and fix/remove such tests later.

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

Replying to embray:

My intention, though not explicitly stated, was to later (once we want to switch Sage to Python 3) remove the condition so that it's easy to find and fix/remove such tests later.

I think it's better to just enable the alias now without any deprecation and then later add an unconditional deprecation. Since Sage doesn't really support Python 3 for the moment, the deprecation won't have much effect anyway.

comment:6 Changed 20 months ago by embray

Why do you think that's better? Why is this wrong?

comment:7 follow-up: Changed 20 months ago by embray

There's a difference between "supporting Python 3" and "having Python 3 as the default Sage". I think in some version--hopefully before long (I'm running pretty low on obvious errors to fix, and most of the problems I'm seeing are minor issues in doctests) there will be a Sage that supports Python 3.

For backwards compatibility we'll want to keep Python 2 as the default for some time, but I would definitely encourage users to move to Python 3 sooner rather than later. To that end, I envision that soon Sage will have Python 2 and Python 3 binary releases, with some emphasis on getting people to use the latter. To that end it will be good to help as much as possible with transitioning.

That said, there's also a question of when to switch the default in the docs over. If the docs are Python 3 then I think there should be no long() without a deprecation warning.

In other words, I'd be willing to change it, but it depends a lot on bigger plans regarding how Sage will transition to Python 3 that aren't particularly settled. But I feel right now that this currently with an eye toward being as helpful as possible for transitioning.

comment:8 in reply to: ↑ 7 Changed 20 months ago by jdemeyer

Replying to embray:

If the docs are Python 3 then I think there should be no long() without a deprecation warning.

Then it should be the same in doctests too. I'm fine with a deprecation or no deprecation either way, but I am protesting the condition on DOCTEST_MODE.

comment:9 Changed 20 months ago by embray

I don't know why. It seems to me the whole point of having DOCTEST_MODE flag is to control behavior in the context of doctests. Obviously this should be used sparingly, but one can always find special cases.

That said, I took a step back from this and rethought it a bit. I thought of adding a long() in sage.all as sort of a helper for transition to Python 3. That said, the lack of long() in Python 3 is hardly the biggest difference, and users aren't often in the habit of using the long built-in very much anyways (especially not in Sage where it's almost entirely unnecessary).

So maybe adding this helper isn't that useful anyways.

So what if we only injected it into the globals when running the doctests, just temporarily, no deprecation warning or anything. Later, when we want to start moving the tests to be more Python 3-compatible, we just remove it and get NameErrors? everywhere the tests try to use it.

comment:10 Changed 20 months ago by jdemeyer

OK, I'll stop fighting here. I don't really agree, but if you want to add long on Python 3 if DOCTEST_MODE without deprecation, please go ahead.

comment:11 follow-up: Changed 20 months ago by embray

Thanks--though what do you think about my other proposal of just injecting long for the purpose of the doctests only? I'm personally leaning toward it as a preference.

I understand your feeling that the doctests should reflect what a real user sees. The problem with Sage is that the only tests (with a few exceptions) are the doctests. And for the purposes of porting to Python 3 I don't think that philosophy really applies. It's more important just that the tests are passing so that we can narrow down on real bugs (until we make Python 3 the default, in which case yes we don't want to special case anything in the doctests if possible).

comment:12 in reply to: ↑ 11 Changed 20 months ago by jdemeyer

Replying to embray:

Thanks--though what do you think about my other proposal of just injecting long for the purpose of the doctests only? I'm personally leaning toward it as a preference.

Go for it.

comment:13 Changed 20 months ago by chapoton

  • Dependencies #24258 deleted
  • Status changed from needs_review to needs_work

comment:14 Changed 19 months ago by embray

  • Status changed from needs_work to needs_review

Why needs work?

Th recent patchbot failures on this ticket are all a joke--they're broken patchbots. This one passed with the current branch on this ticket, and isn't a broken patchbot: https://patchbot.sagemath.org/log/24559/Ubuntu/16.04/x86_64/3.13.0-123-generic/sagemath-patchbot-docker/2018-02-04%2014:11:23

comment:15 Changed 19 months ago by embray

  • Status changed from needs_review to needs_work

Ignore me--I did do the proposed work, but it seems I haven't pushed it to the upstream branch yet...

comment:16 Changed 19 months ago by git

  • Commit changed from e3c14f32ac26b184520ee530b3e4de754971a004 to 036a788b50a1b538c8fbd5bdb5a4ca7d07ba4a2e

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

12ff3beFix various minor encoding issues
bf6cf97Correct some encoding issues on Python 2.
4b92b47A few other miscellaneous fixes to the doctests tests
ed24910Various fixes intended to ensure that opened files are closed explictly
47e464dUse the restore_atexit context manager for this code to work on Python 3,
8995cceOn Python 3, allow Python 2-style long literals (they are just treated as normal int literals)
3a6d2fcDoctest normalization for Python 2 long literals.
22c3544On Python 3 add a 'long' built-in to Sage that acts as a replacement for the Python 2 'long' built-in at least for type conversions.
45450f3Properly handle cases like '1rL'
036a788py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only

comment:17 Changed 19 months ago by embray

  • Dependencies set to #24343
  • Status changed from needs_work to needs_review

This depends on #24343, but the new approach to handling long(...) can be see in the latest commit.

Now, instead of providing a user-visible replacement for long (which as I've already remarked is probably not that useful anyways, or at least a slippery-slope to providing other replacements for old Python 2 builtins), it just injects it into the globals for doctests. This should be seen purely as a transitional measure.


036a788py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only
Last edited 19 months ago by embray (previous) (diff)

comment:18 Changed 19 months ago by jdemeyer

  • Dependencies changed from #24343 to #24343, #24922

comment:19 Changed 19 months ago by jdemeyer

  • Branch changed from u/embray/python3/long-literals to u/jdemeyer/python3/long-literals

comment:20 Changed 19 months ago by jdemeyer

  • Commit changed from 036a788b50a1b538c8fbd5bdb5a4ca7d07ba4a2e to 875149800aaa3d0660cf0d7c3789da0542748611

Rebased


New commits:

af86e5cVarious fixes intended to ensure that opened files are closed explictly where
436ed26A few other miscellaneous fixes to the doctests tests
e566901Use the restore_atexit context manager for this code to work on Python 3,
6fd2912Add run option to restore_atexit context
46017b6Use restore_atexit(run=True) in doctest framework
13f433cmake this exception message more flexible to the exact point in the Python interpreter where it occurred
4a60b3dMerge commit '436ed266f954ecf03f894b9c6539cca353a68ad6'; commit '13f433cbfd91cd09d2415305b3c54859ef1e983c' into HEAD
9b24fb6On Python 3, allow Python 2-style long literals (they are just treated as normal int literals)
9178517Doctest normalization for Python 2 long literals.
8751498py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only

comment:21 Changed 18 months ago by embray

Jeroen, I see you rebased this, but does that constitute a "review"?

IIRC, the only open question on this ticket had to do with my plan for aliasing long in Python 3. Originally I was adding an alias in the global namespace for all users (that would raise a deprecation warning). But now I've gone back on that, and am only injecting long as an alias for int into the globals for doctests.

My experience with testing this, so far, has been that it allows a lot of tests to pass on Python 3 that wouldn't otherwise, with few exceptions, and that it's a decent transitional approach.

The extra_globals dict I added could prove useful for a few other things too.

comment:22 Changed 18 months ago by git

  • Commit changed from 875149800aaa3d0660cf0d7c3789da0542748611 to e3b8148c764377ac86fa532432676ca357e3f56b

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

8cbe1b3On Python 3, allow Python 2-style long literals (they are just treated as normal int literals)
5f01333Doctest normalization for Python 2 long literals.
e3b8148py3: instead of providing a user-visible 'long' alias, just inject it into the globals for doctests, just as a transitional measure for the doctests only

comment:23 Changed 18 months ago by jdemeyer

  • Dependencies #24343, #24922 deleted

comment:24 Changed 18 months ago by jdemeyer

I don't like this syntax:

    """
    Extra objects to place in the global namespace in which tests are run.
    Normally this should be empty but there are special cases where it may
    be useful.

    In particular, on Python 3 add ``long`` as an alias for ``int`` so that
    tests that use the ``long`` built-in (of which there are many) still pass.
    We do this so that the test suite can run on Python 3 while Python 2 is
    still the default.
    """

It looks too much like a docstring but it isn't. Use a # comment instead.

comment:25 follow-up: Changed 18 months ago by embray

That's standard syntax in Sphinx for documenting class attributes.

comment:26 in reply to: ↑ 25 Changed 18 months ago by jdemeyer

Replying to embray:

That's standard syntax in Sphinx for documenting class attributes.

According to the Sphinx documentation, Sphinx autodoc also supports the special comment syntax #: for this. And in fact, Sphinx itself uses that syntax.

You may think that there is no harm in using strings-which-are-not-docstrings like that, but the problem is that it teaches a bad habit. In particular the following does not contain a doctest despite looking like it does:

class X(object):
    attr = "something"
    """
    EXAMPLES::
        
        sage: 1 + 1
        3
    """

comment:27 Changed 18 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:28 Changed 18 months ago by embray

I don't think it's a bad habit. It's perfectly acceptable syntax and I think preferable from the Sphinx perspective over the special comment syntax. If there were a notion in the Python language of attaching a docstring to a particular class attribute that's probably how I'd write it (unfortunately the concept doesn't quite fit in with how the language works, though personally I can think of ways Python might use it).

I think your concern is more a peculiarity of Sage, which overemphasizes doctests in the first place (or, perhaps if this really a problem, the doctest parser should be modified to look in triple-quoted strings anywhere in a class or module body--I think that might make sense anyways).

comment:29 Changed 17 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:30 Changed 17 months ago by vbraun

  • Branch changed from u/jdemeyer/python3/long-literals to e3b8148c764377ac86fa532432676ca357e3f56b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.