runsnake command broken

[nbruin]

#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.

Depends on #9386

CC: @sagetrac-sage-combinat @saliola @anneschilling @novoselt @nathanncohen

Component: performance

Author: Nils Bruin

Branch/Commit: public/ticket/14414 @ 8a8a7ce

Issue created by migration from https://trac.sagemath.org/ticket/14414

[nthiery]

comment:1

Thanks for investigating this. Please someone go ahead and make a patch!

Cheers,
Nicolas

[fchapoton]

comment:3

Attachment: trac-14414.patch.gz

Here is a patch, just as suggested.

[nbruin]

comment:4

--PING--

I just ran into this again. Can someone review this?

[anneschilling]

Commit: f2654ae

[anneschilling]

Author: Frederic Chapoton

[anneschilling]

Branch: public/ticket/14414

[anneschilling]

New commits:

[f2654ae](https://github.com/sagemath/sagetrac-mirror/commit/f2654ae)

trac #14414 runsnake and paths

[anneschilling]

comment:6

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.

[fchapoton]

Changed author from Frederic Chapoton to Frédéric Chapoton

[darijgr]

comment:9

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?

[nbruin]

comment:10

Replying to @darijgr:

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).

[darijgr]

comment:11

Thanks for the instructions!

I fear this will disappoint you, but I can now run the snake both with and without this branch...

[nbruin]

comment:12

Replying to @darijgr:

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 ...

[simon-king-jena]

comment:15

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).

[nbruin]

comment:16

Replying to @simon-king-jena:

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.

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.

[simon-king-jena]

comment:17

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.

[simon-king-jena]

comment:18

Replying to @simon-king-jena:

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".

[jdemeyer]

comment:20

What's the reason anyway that the system Python is used? Why not just use Sage's Python, like every other optional package?

[jdemeyer]

comment:21

In any case, the commands unset PYTHONHOME; unset PYTHONPATH; really should be moved to sage-native-execute.

[jdemeyer]

comment:22

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...

[nbruin]

comment:23

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 <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.

[jdemeyer]

comment:24

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:
    ...

[jdemeyer]

comment:25

To elaborate on os.path.exists(file):

1. 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.

2. 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...

[nbruin]

comment:26

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.

[jdemeyer]

comment:27

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.

[nbruin]

comment:28

Replying to @jdemeyer:

If $SAGE_LOCAL/bin/runsnake exists but is not executable, an error is also more appropriate

I 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.

[jdemeyer]

comment:29

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.

[sagetrac-tmonteil]

comment:30

See also #17735 that was primarily about docs, checks and preparsing for runsnake(), but discuss similar issues.

[sagetrac-git]

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

[sagetrac-git]

Changed commit from f2654ae to b5f96c3

[nbruin]

comment:32

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)

[nbruin]

Dependencies: #9386

[nbruin]

Changed author from Frédéric Chapoton to Nils Bruin

[sagetrac-git]

Changed commit from b5f96c3 to 8a8a7ce

[sagetrac-git]

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

[fchapoton]

comment:34

does not apply