#25668 closed defect (fixed)
Run relocate-once.py with Sage's Python2 and with error checking
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: | Jeroen Demeyer, Volker Braun |
Report Upstream: | Reported upstream. No feedback yet. | Work issues: | |
Branch: | abb2a89 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
Running Sage for the first time after installing from binary under
Linux or macOS leads to running relocate-once.py
.
Before this ticket, when run by Python 3 (eg as installed by Anaconda), it would lead 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
- https://ask.sagemath.org/question/42736/linux-binary-seems-to-be-source/
- https://ask.sagemath.org/question/42562/recursionerror-when-installing-sagemath-82/
- https://ask.sagemath.org/question/35132/how-do-i-solve-this-installation-problem/
Reported in "upstream" binary-pkg at binary-pkg issue 16 -- see the fix there.
After that fix, relocate-once.py can run with Python 3, and this ticket adds error-checking after running relocate-once.py, with a meaningful error in case of failure.
The tentative solution at one point was to make relocate-once.py
be run
with Sage's Python2, whence the now-inaccurate ticket summary.
Change History (59)
comment:1 Changed 4 years ago by
comment:2 Changed 4 years ago by
- 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 4 years ago by
- Cc chapoton jhpalmieri slelievre added
- Keywords Anaconda RecursionError added
comment:4 Changed 4 years ago by
- Branch set to u/slelievre/binary_releases__give_better_error_message_if_anaconda_is_present
comment:5 Changed 4 years ago by
- Commit set to c374e9a35563ef5593f3e955281cecb646359a94
- Status changed from new to needs_review
New commits:
c374e9a | Improve error message when Sage fails to start with RecursionError
|
comment:6 Changed 4 years ago by
This is catching all errors, not just a RecursionError
, I think. Is it worth trying to catch just that one?
comment:7 follow-up: ↓ 8 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 startingp('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 theRecursionError
. - 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 4 years ago by
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: ↓ 12 Changed 4 years ago by
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: ↓ 14 ↓ 15 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
Is the problem just that the script is run with Python 3 instead of Python 2?
comment:15 in reply to: ↑ 12 ; follow-up: ↓ 19 Changed 4 years ago by
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: ↓ 24 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
It looks something like the issue discussed in https://bugs.python.org/issue32758.
comment:19 in reply to: ↑ 15 ; follow-up: ↓ 20 Changed 4 years ago by
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 4 years ago by
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: ↓ 23 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
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 4 years ago by
In any case, down the line, we should also fix the Python 3 incompatibilities in relocate-once.py
.
comment:26 Changed 4 years ago by
- 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: ↓ 28 Changed 4 years ago by
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 4 years ago by
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: ↓ 31 Changed 4 years ago by
- 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 4 years ago by
Here is one suggestion:
-
sage
diff --git a/sage b/sage index 64629eee30..81ab7e908e 100755
a b export SAGE_ROOT 125 125 # Note: relocate-once.py deletes itself upon successful completion 126 126 if [ -x "$SAGE_ROOT/relocate-once.py" ]; then 127 127 "$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 128 150 fi 129 151 130 152 # Run the actual Sage script
comment:31 in reply to: ↑ 29 ; follow-up: ↓ 34 Changed 4 years ago by
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 4 years ago by
Couldn't relocate-once.py
be run using the Python2 shipped by Sage?
comment:33 Changed 4 years ago by
- Branch set to u/jhpalmieri/python2-relocate-once
comment:34 in reply to: ↑ 31 ; follow-up: ↓ 37 Changed 4 years ago by
- 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:
478b430 | trac 25668: run relocate-once.py with Sage's python2.
|
comment:35 Changed 4 years ago by
- Commit changed from 478b430f1a304b2ff6cd5ed97d45692e140574c8 to 030c1bc8519f9eb3be9ffc8cc37951e6dfc43ca5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
030c1bc | trac 25668: run relocate-once.py with Sage's python2.
|
comment:36 Changed 4 years ago by
- 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.
comment:37 in reply to: ↑ 34 Changed 4 years ago by
comment:38 Changed 4 years ago by
If this really works, then positive review. But you have to convince me that you tested it properly.
comment:39 Changed 4 years ago by
First, I'm relying on this assumption: relocate-once.py
will only be present in binary releases (and therefore $SAGE_ROOT/local/bin/python2
exists). I tested by downloading a binary release. I also downloaded Anaconda and let it modify my path (when you install, at least on OS X, it writes to $HOME/.profile
). So I had this:
$ echo $PATH /Users/palmieri/anaconda3/bin:.... $ which python /Users/palmieri/anaconda3/bin/python $ python --version Python 3.6.5 :: Anaconda, Inc.
Then running ./sage
from the binary distribution works:
$ ./sage Rewriting paths for your new installation directory =================================================== This might take a few minutes but only has to be done once. patching ... ... ┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 8.2, Release Date: 2018-05-05 │ │ Type "notebook()" for the browser-based notebook interface. │ │ Type "help()" for help. │ └────────────────────────────────────────────────────────────────────┘ sage: 3+5 8 sage: Sq(2)**2 Sq(1,1)
I also took a binary distribution and broke relocate-once.py
. The result of running ./sage
:
$ ./sage File "/Users/palmieri/Desktop/Sage_stuff/sage_builds/BDIST/SageMath/relocate-once.py", line 9 print('hello") ^ SyntaxError: EOL while scanning string literal Error running the script 'relocate-once.py'.
Note that I didn't actually build a binary distribution, I just patched the file SAGE_ROOT/sage
by hand.
The error message if I run ./sage
from a freshly unpacked source tarball of course remains the same (since relocate-once.py
does not exist, the if block is not executed).
What else would you like me to check?
comment:40 Changed 4 years ago by
On which system did you check?
comment:41 Changed 4 years ago by
OS X – it is the only system I have easy access to.
comment:42 Changed 4 years ago by
shall we put this into "positive review" now ?
comment:43 Changed 4 years ago by
- Milestone changed from sage-8.3 to sage-8.4
update milestone 8.3 -> 8.4
comment:44 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
comment:45 Changed 4 years ago by
Any progress on this? I would very much like it to get merged in 8.4, since it has been a common issue, at least on ask.sagemath.org.
comment:46 Changed 4 years ago by
- Cc embray saraedum added
comment:47 Changed 4 years ago by
Ping?
comment:48 Changed 4 years ago by
- Cc vbraun added
- Milestone changed from sage-8.4 to sage-8.5
Volker, would you review this?
comment:49 Changed 4 years ago by
Maybe it works, but it seems a bit sketchy to use the Python in Sage to run relocate-once.py. I admit, I don't quite understand exactly what it is relocate-once.py
does, but if the Python in Sage and the libraries it depends on haven't been "relocated" yet, how can you guarantee they'll work properly?
I think it would make more sense to modify relocate-once.py
to just work better on more Pythons. In particular, I don't think it's necessary for it be implemented in such a way that it does not contain extremely long expressions that tax the parser/peephole optimizers in Python.
comment:50 Changed 4 years ago by
This is a rather fragile workaround, run Sage's python2 while the rpaths in the binary point to the wrong place and pray that none of those shared libraries end up being used by relocate-once.
The thing that needs to be done eventually is making relocate-once python3 compatible, including not emitting very long chains of method calls.
comment:51 Changed 4 years ago by
A compromise would be to use the system Python 2 if we can find it (via something like python2 relocate-once.py || python2.7 relocate-once.py || ...
) and otherwise call Sage's Python 2 and hope that it works. Is it better to have something that you know will break if the system python
runs Python 3, or to have something that might work in that situation, even if it's fragile?
Otherwise, this puts the ball in the court of the people who maintain the Sage binary distributions. I seem to have opened up https://github.com/sagemath/binary-pkg/issues/16 6 months ago, but no progress yet. I don't have the time to figure out how relocate-once.py
is generated and then to redo it to avoid the long chains of methods.
comment:52 follow-up: ↓ 55 Changed 4 years ago by
I'm fixing this at https://github.com/sagemath/binary-pkg/issues/16
comment:53 Changed 4 years ago by
I don't know if this has ever happened specifically with the relocate-once.py script, but I've also had problems in the past with Python 2.7 bytecode compiler crashing on extremely long expressions, particularly arithmetic expressions like a = 1; a + a + a + a + ...
repeating a few thousand times (incidentally only if a
is a variable; if it's a 1
literal I think those constant expressions get optimized before reaching the compiler).
In fact it was only a couple years ago I think that they fixed this to at least raise a RuntimeError
instead of segfaulting.
comment:54 Changed 4 years ago by
- Commit changed from 030c1bc8519f9eb3be9ffc8cc37951e6dfc43ca5 to abb2a89e1d211acc7d7d25c233707fe299f04f15
comment:55 in reply to: ↑ 52 Changed 4 years ago by
Replying to vbraun:
I'm fixing this at https://github.com/sagemath/binary-pkg/issues/16
Thanks!
For this ticket, the only thing left to do is error-checking. Ready for review.
New commits:
3b0bacb | trac 25668: run relocate-once.py with Sage's python2.
|
abb2a89 | trac 25668: add error-checking after running relocate-once.py.
|
comment:56 Changed 4 years ago by
- Priority changed from critical to minor
comment:57 Changed 4 years ago by
- Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
- Status changed from needs_review to positive_review
comment:58 Changed 4 years ago by
- Branch changed from u/jhpalmieri/python2-relocate-once to abb2a89e1d211acc7d7d25c233707fe299f04f15
- Resolution set to fixed
- Status changed from positive_review to closed
comment:59 Changed 4 years ago by
- Commit abb2a89e1d211acc7d7d25c233707fe299f04f15 deleted
- Description modified (diff)
(Editing ticket description to reflect that the fix is not the one suggested by the ticket summary.)
According to "j.c." on ask.sagemath.org, the solution to installing a binary with Ananconda installed is: