Opened 8 years ago
Last modified 5 years ago
#14414 needs_work defect
runsnake command broken
Reported by: | nbruin | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | performance | Keywords: | |
Cc: | sage-combinat, saliola, aschilling, novoselt, ncohen | Merged in: | |
Authors: | Nils Bruin | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | public/ticket/14414 (Commits) | Commit: | 8a8a7cee9292c039034f58b43d1070a999879619 |
Dependencies: | #9386 | Stopgaps: |
Description (last modified by )
#11287 introduced a runsnake
command that tries to execute runsnake
using the system python. However, it does so without sage-native-execute
and without clearing PYTHONHOME
and PYTHONPATH
, so it has very little chance of ever working. It already comes a little closer by doing
- os.system("/usr/bin/python -E `which runsnake` %s &"%tmpfile) + os.system("unset PYTHONHOME; unset PYTHONPATH; sage-native-execute runsnake %s &"%tmpfile)
in sage/misc/dev_tools.py
(that makes the command runsnake
work on my computer).
You can't really expect the system python to work properly when the environment variables point to libraries for other python installs. See also #9386, #10286, #17735.
Attachments (1)
Change History (35)
comment:1 Changed 8 years ago by
comment:2 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
Changed 7 years ago by
comment:3 Changed 7 years ago by
Here is a patch, just as suggested.
comment:4 Changed 7 years ago by
--PING--
I just ran into this again. Can someone review this?
comment:5 follow-up: ↓ 6 Changed 7 years ago by
- Branch set to public/ticket/14414
- Commit set to f2654aea5ade94a5f43d87dc3b83906c57054545
- Status changed from new to needs_review
comment:6 in reply to: ↑ 5 Changed 7 years ago by
For me runsnake runs with and without the patch, so I am probably not the right reviewer. But I did convert the patch to a git branch in order to play with it.
comment:7 Changed 7 years ago by
comment:8 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:9 follow-up: ↓ 10 Changed 7 years ago by
I can't get runsnake to work either with or without this patch.
sage: runsnake("list(SymmetricGroup(3))") sage: Traceback (most recent call last): File "/usr/local/bin/runsnake", line 9, in <module> load_entry_point('RunSnakeRun==2.0.2b1', 'gui_scripts', 'runsnake')() File "/usr/local/lib/python2.7/dist-packages/RunSnakeRun-2.0.2b1-py2.7.egg/runsnakerun/runsnake.py", line 784, in main app = RunSnakeRunApp(0) File "/usr/lib/python2.7/dist-packages/wx-2.6-gtk2-unicode/wx/_core.py", line 7700, in __init__ self._BootstrapApp() File "/usr/lib/python2.7/dist-packages/wx-2.6-gtk2-unicode/wx/_core.py", line 7352, in _BootstrapApp return _core_.PyApp__BootstrapApp(*args, **kwargs) File "/usr/local/lib/python2.7/dist-packages/RunSnakeRun-2.0.2b1-py2.7.egg/runsnakerun/runsnake.py", line 722, in OnInit frame = MainFrame( config_parser = load_config()) File "/usr/local/lib/python2.7/dist-packages/RunSnakeRun-2.0.2b1-py2.7.egg/runsnakerun/runsnake.py", line 205, in __init__ self.CreateControls(config_parser) File "/usr/local/lib/python2.7/dist-packages/RunSnakeRun-2.0.2b1-py2.7.egg/runsnakerun/runsnake.py", line 231, in CreateControls square_style = True, File "/usr/local/lib/python2.7/dist-packages/SquareMap-1.0.1-py2.7.egg/squaremap/squaremap.py", line 135, in __init__ self.OnSize(None) File "/usr/local/lib/python2.7/dist-packages/SquareMap-1.0.1-py2.7.egg/squaremap/squaremap.py", line 226, in OnSize self.UpdateDrawing() File "/usr/local/lib/python2.7/dist-packages/SquareMap-1.0.1-py2.7.egg/squaremap/squaremap.py", line 230, in UpdateDrawing self.Draw(dc) File "/usr/local/lib/python2.7/dist-packages/SquareMap-1.0.1-py2.7.egg/squaremap/squaremap.py", line 236, in Draw brush = wx.Brush( self.BackgroundColour ) AttributeError: 'SquareMap' object has no attribute 'BackgroundColour' sage:
No window appears in reasonable time. Is this any related?
comment:10 in reply to: ↑ 9 Changed 7 years ago by
Replying to darij:
I can't get runsnake to work either with or without this patch. No window appears in reasonable time. Is this any related?
Is perhaps the version of your SquareMap mismatched with runsnake? When I installed RunSnakeRun==2.0.4
(yum install from standard Fedora 19 repositories), it complained about a version mismatch for SquareMap. At that point, the only SquareMap available from fedora's standard repositories was SquareMap-1.0.1
, but SquareMap-1.0.3
was needed. After I installed that from PyPi everything worked like a charm (with the patch here).
comment:11 follow-up: ↓ 12 Changed 7 years ago by
Thanks for the instructions!
I fear this will disappoint you, but I can now run the snake both with and without this branch...
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 15 Changed 7 years ago by
Replying to darij:
Thanks for the instructions!
I fear this will disappoint you, but I can now run the snake both with and without this branch...
:-) That means that your native python /usr/bin/python
is binary compatible with sage's python
. Good for you! However, we cannot guarantee that this is the case. In general, if we run /usr/bin/python
we have to ensure that the sage library and the sage-python library (via the PYTHON path variables) do not get in the way of standard python execution.
An alternative is to try and figure out how to install runsnake for sage's python (i.e., make an spkg out of it) but that seems like a hopelessly complicated solution relative to relying on system-provided runsnake. That said, fedora still only offers python-SquareMap?-1.0.1, which is insufficient for the runsnake they offer. But at least the beast that is wxPython is available on the system Python by default ...
comment:13 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:14 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:15 in reply to: ↑ 12 ; follow-up: ↓ 16 Changed 6 years ago by
Replying to nbruin:
An alternative is to try and figure out how to install runsnake for sage's python (i.e., make an spkg out of it) but that seems like a hopelessly complicated solution relative to relying on system-provided runsnake. That said, fedora still only offers python-SquareMap?-1.0.1, which is insufficient for the runsnake they offer. But at least the beast that is wxPython is available on the system Python by default ...
I would appreciate to have that alternative. I tried (system-wide) sudo easy_install RunSnakeRun
, but then runsnake
failed with
Traceback (most recent call last): File "/usr/bin/runsnake", line 9, in <module> load_entry_point('RunSnakeRun==2.0.4', 'gui_scripts', 'runsnake')() File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 337, in load_entry_point return get_distribution(dist).load_entry_point(group, name) File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 2333, in load_entry_point return ep.load() File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 2039, in load entry = __import__(self.module_name, globals(),globals(), ['__name__']) File "/usr/lib/python2.7/site-packages/RunSnakeRun-2.0.4-py2.7.egg/runsnakerun/runsnake.py", line 4, in <module> import wx, sys, os, logging, traceback ImportError: No module named wx
So, easy_install
apparently did not take care of dependencies. I think I met this problem before (I am not sure if I ever managed to install runsnake on my laptop). Unfortunately, runsnake seems to be unknown to the opensuse distribution. So, I can not simply install it with yast
, hoping that yast
takes care of dependencies (in contrast to easy_install).
comment:16 in reply to: ↑ 15 ; follow-up: ↓ 17 Changed 6 years ago by
Replying to SimonKing:
I would appreciate to have that alternative. I tried (system-wide)
sudo easy_install RunSnakeRun
, but thenrunsnake
failed withTraceback (most recent call last): File "/usr/bin/runsnake", line 9, in <module> load_entry_point('RunSnakeRun==2.0.4', 'gui_scripts', 'runsnake')() File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 337, in load_entry_point return get_distribution(dist).load_entry_point(group, name) File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 2333, in load_entry_point return ep.load() File "/usr/lib/python2.7/site-packages/pkg_resources.py", line 2039, in load entry = __import__(self.module_name, globals(),globals(), ['__name__']) File "/usr/lib/python2.7/site-packages/RunSnakeRun-2.0.4-py2.7.egg/runsnakerun/runsnake.py", line 4, in <module> import wx, sys, os, logging, traceback ImportError: No module named wxSo,
easy_install
apparently did not take care of dependencies.
If you google "suse runsnake" you'll get RPM hits, so at least third parties have packaged it for opensuse. I wouldn't go and install random rpms from the web, but for instance this page lists the requirements of the rpm. With a bit of luck THOSE can be satisfied by yast (the main one probably being python-wxWidgets) so that from that point your easily installed runsnake might work.
Packaging a runsnake for sage's python would involve recompiling sage's python with wx support. That sounds painful.
comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 6 years ago by
Replying to nbruin:
If you google "suse runsnake" you'll get RPM hits, so at least third parties have packaged it for opensuse. I wouldn't go and install random rpms from the web, but for instance this page lists the requirements of the rpm. With a bit of luck THOSE can be satisfied by yast (the main one probably being python-wxWidgets) so that from that point your easily installed runsnake might work.
Thank you! python-wxWidgets
was the missing bit. Previously, I have not been able to find this by searching what is offered by yast. Now, runsnake starts.
comment:18 in reply to: ↑ 17 Changed 6 years ago by
Replying to SimonKing:
Thank you!
python-wxWidgets
was the missing bit.
"Missing bit" in the sense of "runsnake can be installed with easy_install, even though it is not known to yast".
comment:19 Changed 6 years ago by
- Cc ncohen added
comment:20 Changed 6 years ago by
What's the reason anyway that the system Python is used? Why not just use Sage's Python, like every other optional package?
comment:21 Changed 6 years ago by
- Status changed from needs_review to needs_work
In any case, the commands unset PYTHONHOME; unset PYTHONPATH;
really should be moved to sage-native-execute
.
comment:22 follow-up: ↓ 23 Changed 6 years ago by
I just tried to install runsnake in my Sage Python and it went flawlessly.
For wxPython
, I followed these instructions: all I had to do was download and extract the tarball and then
cd wxPython python build-wxpython.py --prefix="$SAGE_LOCAL" --install
For runsnake, I used easy_install
:
easy_install RunSnakeRun
(all this in a Sage shell of course)
Perhaps it used to be more complicated or it is highly system-dependent, but I don't see the problem...
comment:23 in reply to: ↑ 22 Changed 6 years ago by
Replying to jdemeyer:
I just tried to install runsnake in my Sage Python and it went flawlessly.
If failed for me in the configuration process:
checking for GST... configure: WARNING: GStreamer 0.10 not available, falling back to 0.8 checking for GST... configure: WARNING: GStreamer 0.8/0.10 not available. configure: error: GStreamer not available Error running configure ERROR: failed building wxWidgets
Runsnake as packaged by Fedora works well for me (apart from the fact that SquareMap-1.0.3
was required but not packaged by Fedora. I presume this has been resolved in more modern distributions).
So, installing and building runsnake in the sage python seems a convenient solution for some people, but the building requirements are so heavy (the wxPython source tarball is 58Mb, and it requires all kinds of bells and whistles to be present on the host system that are not otherwise required for sage) that supporting a system runsnake should definitely be an option (and it's not hard to do).
The last couple of times I've just resorted to saving the profile data using "%prun -D <filename> <command" and opening it with the system runsnake afterwards (the pathnames allow the system runsnake to display all relevant source, so it doesn't need to run in the same python as where the profile comes from).
So something along these lines would probably give the best of both worlds:
if os.path.exists( os.environ['SAGE_LOCAL']+'/bin/runsnake'): subprocess.call(["runsnake",tmpfile]) else os.system("""sh -c " LD_LIBRARY_PATH=$SAGE_ORIG_LD_LIBRARY_PATH; export LD_LIBRARY_PATH if [ `uname` = 'Darwin' ]; then DYLD_LIBRARY_PATH=$SAGE_ORIG_DYLD_LIBRARY_PATH fi unset PYTHONHOME; unset PYTHONPATH; runsnake %s " """%tempfile)
This is assuming that the runsnake script itself addresses the python it wants by full name, which installed scripts usually do. We definitely need to reset the PYTHON variables, since those will be referring to sage-specific python modules, which may be incompatible with the system python. And we also do not want the sage libraries get in the way.
optional: catch if running the command led to an error condition and then print some message what the possible avenues are for getting runsnake installed.
comment:24 Changed 6 years ago by
I like your proposal from the last comment, but some comments:
You never need os.system("sh -c ...")
since os.system()
already uses a Shell.
And like I said before, use sage-native-execute
instead of manually messing with environment variables.
Instead of os.path.exists()
, you could instead use the more Pythonic
try: subprocess.call(os.path.join(SAGE_LOCAL, "bin", "runsnake"...) except OSError: ...
comment:25 follow-up: ↓ 26 Changed 6 years ago by
To elaborate on os.path.exists(file)
:
- it checks only whether something by that name exists, not that it's an ordinary file. So at least use
os.path.isfile(file)
instead.
- it doesn't check whether the file is executable. Removing executable bits from a program is a valid way to temporarily "disable" it, so you should also check
os.access(file, X_OK)
.
But really, the try
/except
style is preferred by Python...
comment:26 in reply to: ↑ 25 ; follow-up: ↓ 27 Changed 6 years ago by
Replying to jdemeyer:
But really, the
try
/except
style is preferred by Python...
Yes, I know that as a general rule, pythonistas seem to promote it, but one shouldn't apply this blindly. The problem is that checking that it's hard to write and "except" class that guarantees it only catches the particular OSError resulting from the relevant file not existing.
A priori, there may be loads of reasons why os.system might return an OSError, and I only want to try something else if runsnake
is not installed in sage; otherwise I'd prefer to see an error. I certainly get that effect by testing the presence of runsnake
. If $SAGE_LOCAL/bin/runsnake
exists but is not executable, an error is also more appropriate than silently trying something else, so not testing executability also leads to better error reporting.
I think it's good to be aware that try/except
can be quite efficient in python and for some operations is less prone to race conditions, but one shouldn't apply it blindly. It's simply too hard to get a sufficiently fine filtering in the except clause.
comment:27 in reply to: ↑ 26 ; follow-up: ↓ 28 Changed 6 years ago by
Replying to nbruin:
A priori, there may be loads of reasons why os.system might return an OSError
You mean subprocess.call
, not os.system
. Yes, this is an important difference!
I would say it's highly unlikely that subprocess.call
raises an OSError
which is not because the file does not exist or is not executable. The only thing I can think of is running out of memory...
If
$SAGE_LOCAL/bin/runsnake
exists but is not executable, an error is also more appropriate
I completely disagree with this, see previous comment.
comment:28 in reply to: ↑ 27 ; follow-up: ↓ 29 Changed 6 years ago by
Replying to jdemeyer:
If
$SAGE_LOCAL/bin/runsnake
exists but is not executable, an error is also more appropriateI completely disagree with this, see previous comment.
OK, executable lookup indeed ignores non-executable files that occur earlier in the path, so we should too. Strictly speaking the check should be
sage_runsnake=os.path.join(os.eviron("SAGE_LOCAL"),"bin","runsnake") if os.path.isfile(sage_runsnake) and os.access(sage_runsnake,os.X_OK): ...
to emulate normal PATH behaviour (where lookup skips directories with the relevant name). Also, there's a comment on os.access
that it tests against real uid rather than effective gid. Sigh ... it's almost worth running the risk of an improperly caught OSError.
comment:29 in reply to: ↑ 28 Changed 6 years ago by
Replying to nbruin:
Also, there's a comment on
os.access
that it tests against real uid rather than effective gid.
That's true, but I would think it's unlikely that people run Sage as suid. But it's certainly odd that there is no system call to check whether a file is executable for the current user.
comment:30 Changed 6 years ago by
- Description modified (diff)
See also #17735 that was primarily about docs, checks and preparsing for runsnake()
, but discuss similar issues.
comment:31 Changed 6 years ago by
- Commit changed from f2654aea5ade94a5f43d87dc3b83906c57054545 to b5f96c35a44f83dbad81a74ed1fbaaddecdd8cf3
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b0178cf | trac #9386: improve environment variable saving infrastructure for sage-env/sage-native-execute
|
c4fdc67 | trac #9386: stricter error checking on parameters in sage_export and other cleanup
|
b5f96c3 | trac #14414: let runsnake properly test for SAGE_LOCAL install of runsnake and use sage-native-execute otherwise
|
comment:32 Changed 6 years ago by
- Dependencies set to #9386
- Status changed from needs_work to needs_review
OK, the current branch implements the proposed strategy
edit: D'oh. sage wasn't waiting for the runsnake process to complete before, and there's no reason to change that. Should be fixed now (just call Popen rather than call)
comment:33 Changed 6 years ago by
- Commit changed from b5f96c35a44f83dbad81a74ed1fbaaddecdd8cf3 to 8a8a7cee9292c039034f58b43d1070a999879619
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8a8a7ce | trac #14414: let runsnake properly test for SAGE_LOCAL install of runsnake and use sage-native-execute otherwise
|
Thanks for investigating this. Please someone go ahead and make a patch!
Cheers,