Opened 16 months ago

Last modified 10 months ago

#25668 closed defect

Run relocate-once.py with Sage's Python2 and with error checking — at Version 36

Reported by: jhpalmieri Owned by:
Priority: minor Milestone: sage-8.5
Component: distribution Keywords: Anaconda, RecursionError
Cc: chapoton, embray, jhpalmieri, saraedum, slelievre, vbraun Merged in:
Authors: John Palmieri Reviewers:
Report Upstream: Reported upstream. No feedback yet. Work issues:
Branch: u/jhpalmieri/python2-relocate-once (Commits) Commit: 030c1bc8519f9eb3be9ffc8cc37951e6dfc43ca5
Dependencies: Stopgaps:

Description (last modified by slelievre)

Running Sage for the first time after installing from binary under Linux or macOS leads to running relocate-once.py. When it is run by Python3 (eg the one installed by Anaconda), it leads to the following error.

$ ./sage
RecursionError: maximum recursion depth exceeded during compilation
************************************************************************
It seems that you are attempting to run Sage from an unpacked source
tarball, but you have not compiled it yet (or maybe the build has not
finished). You should run `make` in the Sage root directory first.
If you did not intend to build Sage from source, you should download
a binary tarball instead. Read README.txt for more information.
************************************************************************

which seems to come from Python3 choking on a very long line in relocate-once.py, because of the Python3 bug described as Python issue 32758.

See reports by Sage users at

Reported in "upstream" binary-pkg at https://github.com/sagemath/binary-pkg/issues/16

To fix the issue, this ticket

  • makes relocate-once.py be run with Sage's Python2, which should prevent the RecursionError,
  • adds error-checking after running relocate-once.py, with a meaningful error in case of failure.

Change History (36)

comment:1 Changed 16 months ago by jhpalmieri

According to "j.c." on ask.sagemath.org, the solution to installing a binary with Ananconda installed is:

On the off-chance you're using Anaconda, try commenting out the export 
PATH="/anaconda/bin:$PATH" line in the .bash_profile file (and restarting 
your terminal) before you run Sage for the first time.

You can uncomment it again after sage successfully runs (until you have 
to rebuild or upgrade Sage).
Last edited 16 months ago by jhpalmieri (previous) (diff)

comment:2 Changed 15 months ago by chapoton

  • Priority changed from major to critical

this probably deserves the "critical" status, given the number of people trapped by the error message..

comment:3 Changed 15 months ago by slelievre

  • Cc chapoton jhpalmieri slelievre added
  • Keywords Anaconda RecursionError added

comment:4 Changed 15 months ago by slelievre

  • Branch set to u/slelievre/binary_releases__give_better_error_message_if_anaconda_is_present

comment:5 Changed 15 months ago by slelievre

  • Authors set to Samuel Lelièvre
  • Commit set to c374e9a35563ef5593f3e955281cecb646359a94
  • Status changed from new to needs_review

New commits:

c374e9aImprove error message when Sage fails to start with RecursionError

comment:6 Changed 15 months ago by jhpalmieri

This is catching all errors, not just a RecursionError, I think. Is it worth trying to catch just that one?

comment:7 follow-up: Changed 15 months ago by jhpalmieri

And why does the if block if [ ! -x "$SAGE_LOCAL/bin/sage" ]; then ... run at all? This is bash, not Python, so how is anaconda interfering? What is causing the recursion error?

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

Replying to jhpalmieri:

And why does the if block if [ ! -x "$SAGE_LOCAL/bin/sage" ]; then ... run at all? This is bash, not Python, so how is anaconda interfering? What is causing the recursion error?

Exactly my thought too! The only reasonable explanation is that anaconda itself sets some $SAGE_XXX variables.

comment:9 Changed 15 months ago by jhpalmieri

First, I think we should catch an error if relocate-once.py does not run successfully, and in particular that's when the message about Anaconda should be printed.

Second, there are several problems in relocate-once.py:

  • Related to anaconda and/or Python 3: the RecursionError comes from the line starting
    p('local/lib/libflint-13.5.2.dylib').patch(1552, 1684)...
    
    That line is 61957 characters long (!), and I wonder if its length is causing problems. Certainly if I delete just that line, I no longer get the RecursionError.
  • related to Python 3, if I delete the aforementioned line, I get
    Rewriting paths for your new installation directory
    ===================================================
    
    This might take a few minutes but only has to be done once.
    
    Traceback (most recent call last):
      File "/Users/jpalmier/Downloads/SageMath/relocate-once.py", line 145, in <module>
        p('build/make/Makefile-auto').substitute().save()
      File "/Users/jpalmier/Downloads/SageMath/relocate-once.py", line 133, in __call__
        filename = os.path.join(self.root_path, filename)
      File "/anaconda3/lib/python3.6/posixpath.py", line 94, in join
        genericpath._check_arg_types('join', a, *p)
      File "/anaconda3/lib/python3.6/genericpath.py", line 151, in _check_arg_types
        raise TypeError("Can't mix strings and bytes in path components") from None
    TypeError: Can't mix strings and bytes in path components
    

comment:10 Changed 15 months ago by jdemeyer

We really need a test machine where this error occurs to see what the value of $SAGE_LOCAL is at that point.

comment:11 follow-up: Changed 15 months ago by jdemeyer

OK, so the problem is with relocate-once.py. That wasn't obvious at all so I guess the error handler there should be improved...

comment:12 in reply to: ↑ 11 ; follow-ups: Changed 15 months ago by jhpalmieri

Replying to jdemeyer:

OK, so the problem is with relocate-once.py. That wasn't obvious at all so I guess the error handler there should be improved...

You mean, there should be an error handler there?

comment:13 Changed 15 months ago by jhpalmieri

By the way, I have now installed Anaconda on OS X, and it's easy enough to add or remove the bad parts in my PATH to test things. Others should feel free to do the same.

comment:14 in reply to: ↑ 12 Changed 15 months ago by jdemeyer

Is the problem just that the script is run with Python 3 instead of Python 2?

comment:15 in reply to: ↑ 12 ; follow-up: Changed 15 months ago by jdemeyer

Replying to jhpalmieri:

You mean, there should be an error handler there?

I'm just wondering who is displaying the message RecursionError: maximum recursion depth exceeded during compilation? That's not how Python displays errors by default.

comment:16 follow-up: Changed 15 months ago by jhpalmieri

If I run relocate-once.py using Sage's Python 3.6.6, I do not get the recursion error. (I get the string/bytes error, of course.) So maybe Anaconda's Python 3.6.5 is badly configured?

comment:17 Changed 15 months ago by jhpalmieri

Further debugging: if I put some echo statements in the sage script before and after running relocate-once.py, they both execute. If I put a print statement in relocate-once.py, it doesn't print. I am really wondering if the long line makes anaconda's Python 3 barf before it even loads the file.

comment:18 Changed 15 months ago by jhpalmieri

It looks something like the issue discussed in https://bugs.python.org/issue32758.

comment:19 in reply to: ↑ 15 ; follow-up: Changed 15 months ago by slelievre

Replying to jdemeyer:

I'm just wondering who is displaying the message

RecursionError: maximum recursion depth exceeded during compilation

That's not how Python displays errors by default.

Searching the web for [ "maximum recursion depth exceeded during compilation" ] yields many results, including the following one not related to Sage:

It might indicate that the error has something to do with a call to eval(?).

This other result shows that the error can occur even just running sage --version:

comment:20 in reply to: ↑ 19 Changed 15 months ago by jdemeyer

Replying to slelievre:

It might indicate that the error has something to do with a call to eval(?).

Or more generally, just the Python parser. All the evidence points to Python crashing while parsing the .py file, before it even executes anything. So it crashes with just the error message without a traceback.

comment:21 follow-up: Changed 15 months ago by jdemeyer

One obvious fix in Sage is to add error checking in the top-level sage script:

# If this is a freshly-unpacked binary tarball then run the installer
# Note: relocate-once.py deletes itself upon successful completion
if [ -x "$SAGE_ROOT/relocate-once.py" ]; then
    "$SAGE_ROOT/relocate-once.py"
fi

comment:22 Changed 15 months ago by jhpalmieri

It's not the length of the line, it's the number of methods on the line. If I change patch(1552, 1684) (etc.) to patch(1,1) throughout, the line gets much shorter, but it still crashes. 1500 methods seems to be the limit.

comment:23 in reply to: ↑ 21 Changed 15 months ago by jhpalmieri

Replying to jdemeyer:

One obvious fix in Sage is to add error checking in the top-level sage script:

# If this is a freshly-unpacked binary tarball then run the installer
# Note: relocate-once.py deletes itself upon successful completion
if [ -x "$SAGE_ROOT/relocate-once.py" ]; then
    "$SAGE_ROOT/relocate-once.py"
fi

Yes, exactly.

comment:24 in reply to: ↑ 16 Changed 15 months ago by jhpalmieri

Replying to jhpalmieri:

If I run relocate-once.py using Sage's Python 3.6.6, I do not get the recursion error. (I get the string/bytes error, of course.) So maybe Anaconda's Python 3.6.5 is badly configured?

Maybe I was wrong about this: Sage's Python 3 is now giving this error on a similar file: [1,2,3] with .sort() appended 1500 times – not good Python, but enough methods to give Python 3 problems. Not Python 2, though.

comment:25 Changed 15 months ago by jhpalmieri

In any case, down the line, we should also fix the Python 3 incompatibilities in relocate-once.py.

comment:26 Changed 15 months ago by jdemeyer

  • Authors Samuel Lelièvre deleted
  • Branch u/slelievre/binary_releases__give_better_error_message_if_anaconda_is_present deleted
  • Commit c374e9a35563ef5593f3e955281cecb646359a94 deleted
  • Status changed from needs_review to needs_work

comment:27 follow-up: Changed 15 months ago by jhpalmieri

Should we check whether we're running Python 3, whether executing relocate-once.py exits successfully, or both?

comment:28 in reply to: ↑ 27 Changed 15 months ago by jdemeyer

Don't check whether you are doing the wrong thing, just do the right thing: python2 relocate-once.py.

And yes, the exit status must be checked.

comment:29 follow-up: Changed 15 months ago by jhpalmieri

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

On my OS X machine:

$ which python2
$ ls /usr/bin/pyth*
/usr/bin/python           /usr/bin/python2.7        /usr/bin/pythonw
/usr/bin/python-config    /usr/bin/python2.7-config /usr/bin/pythonw2.7

So do we hard-code python2.7? Seems too fragile. Do we do python2 relocate-once.py || python2.6 relocate-once.py || python2.7 relocate-once.py || ...? Clunky. command -v python2 && python2 relocate-once.py, etc.? What do you suggest?

It seems like right now, the best we can do is check the exit status and print a message. Maybe try to detect if Python 3 is the problem. I've also opened an issue for Python 3 compatibility at the sagemath/binary-pkg github page.

comment:30 Changed 15 months ago by jhpalmieri

Here is one suggestion:

  • sage

    diff --git a/sage b/sage
    index 64629eee30..81ab7e908e 100755
    a b export SAGE_ROOT 
    125125# Note: relocate-once.py deletes itself upon successful completion
    126126if [ -x "$SAGE_ROOT/relocate-once.py" ]; then
    127127    "$SAGE_ROOT/relocate-once.py"
     128    if [ $? -ne 0 ]; then
     129        PYVER=`python -c 'import sys; print(sys.version_info[0])'`
     130        if [ $PYVER = '3' ]; then
     131           echo >&2
     132           echo >&2 "Error: Sage must run the script 'relocate-once.py' to complete"
     133           echo >&2 "this installation. The script is not compatible with Python 3,"
     134           echo >&2 "which is your system's version of Python. Please ensure that"
     135           echo >&2 "the command 'python' runs Python 2. This only needs to be done"
     136           echo >&2 "the first you run Sage."
     137           echo >&2
     138           echo >&2 "If you have Anaconda installed, for example, it may have modified"
     139           echo >&2 "your PATH. Check your PATH by running the terminal command"
     140           echo >&2 "    echo \$PATH"
     141           echo >&2 "If it starts with something containing '/anaconda3/bin', you need"
     142           echo >&2 "to remove this from your PATH the first time you run Sage. After"
     143           echo >&2 "Sage has run once, you may restore the original value of PATH."
     144           exit 1
     145        else
     146           echo >&2 "Error running the script 'relocate-once.py'."
     147           exit 1
     148        fi
     149    fi
    128150fi
    129151
    130152# Run the actual Sage script

comment:31 in reply to: ↑ 29 ; follow-up: Changed 15 months ago by jdemeyer

Replying to jhpalmieri:

Do we do python2 relocate-once.py || python2.6 relocate-once.py || python2.7 relocate-once.py || ...? Clunky.

Clunky but I think it's the best solution. I would probably skip the python2.6 check though.

Do we already know why things go wrong with Anaconda?

comment:32 Changed 15 months ago by slelievre

Couldn't relocate-once.py be run using the Python2 shipped by Sage?

comment:33 Changed 15 months ago by jhpalmieri

  • Branch set to u/jhpalmieri/python2-relocate-once

comment:34 in reply to: ↑ 31 Changed 15 months ago by jhpalmieri

  • Authors set to John Palmieri
  • Commit set to 478b430f1a304b2ff6cd5ed97d45692e140574c8
  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Replying to jhpalmieri:

Do we do python2 relocate-once.py || python2.6 relocate-once.py || python2.7 relocate-once.py || ...? Clunky.

Clunky but I think it's the best solution. I would probably skip the python2.6 check though.

Do we already know why things go wrong with Anaconda?

It seems to me that either Python 3 in general, or Anaconda's Python 3, doesn't like lines with too many attributes/methods. Even without that line, the script is not compatible with Python 3.

Replying to slelievre:

Couldn't relocate-once.py be run using the Python2 shipped by Sage?

That's a good idea, let's do that. This branch works for me.


New commits:

478b430trac 25668: run relocate-once.py with Sage's python2.

comment:35 Changed 15 months ago by git

  • Commit changed from 478b430f1a304b2ff6cd5ed97d45692e140574c8 to 030c1bc8519f9eb3be9ffc8cc37951e6dfc43ca5

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

030c1bctrac 25668: run relocate-once.py with Sage's python2.

comment:36 Changed 15 months ago by slelievre

  • Description modified (diff)
  • Summary changed from Binary releases: give better error message if anaconda is present to Run relocate-once.py with Sage's Python2 and with error checking

Making ticket summary and description reflect the diagnosis and the proposed fix.

Note: See TracTickets for help on using tickets.