Opened 5 years ago

Closed 4 years ago

#23106 closed task (fixed)

Python scripts in src/bin are not ready for Sage + Python 3

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: jdemeyer, embray Merged in:
Authors: Frédéric Chapoton Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 847df2c (Commits, GitHub, GitLab) Commit: 847df2c99cbce5f35f4c7c15da1ecb26843f5324
Dependencies: Stopgaps:

Status badges

Description

Many of the scripts in src/bin start with #!/usr/bin/env python (which is Python 2) and then try to import from the Sage library. This will not work if Sage has been installed using SAGE_PYTHON3=yes.

These scripts need modification, and perhaps there are others:

  • sage-cython: from sage.env import SAGE_SRC -- could be modified to avoid this import, for example using os.getenv('SAGE_SRC')
  • sage-eval
  • sage-fixdoctests
  • sage-ipython: particularly important, since it is used to run Sage.
  • sage-list-packages
  • sage-notebook
  • sage-preparse
  • sage-rst2sws
  • sage-rst2txt
  • sage-run-cython
  • sage-runtests
  • sage-startuptime.py
  • sage-sws2rst

Change History (34)

comment:1 Changed 5 years ago by chapoton

Indeed,

git grep -c "from sage"
sage-cython:1
sage-env-config.in:1
sage-eval:3
sage-fixdoctests:2
sage-ipython:1
sage-list-packages:1
sage-notebook:5
sage-preparse:3
sage-rst2sws:3
sage-rst2txt:1
sage-run-cython:2
sage-runtests:1
sage-startuptime.py:1
sage-sws2rst:1

some of these are in fact from sagenb imports

Last edited 5 years ago by chapoton (previous) (diff)

comment:2 Changed 5 years ago by jhpalmieri

  • sage-notebook, sage-rst2sws, sage-rst2txt, sage-sws2rst: I would say that we don't touch these files. They all use SageNB, which is not (yet?) compatible with Python 3. SageNB is also slowly being phased out, which makes these a lower priority anyway.
  • sage-env-config.in is a false positive: it is a shell script and does no importing of Python modules.
  • sage-cython is easy to fix:
    • src/bin/sage-cython

      diff --git a/src/bin/sage-cython b/src/bin/sage-cython
      index 77ba303b39..ffff293ca2 100755
      a b  
      33import os, sys
      44args = sys.argv[1:]
      55
      6 from sage.env import SAGE_SRC
       6if "SAGE_SRC" not in os.environ:
       7    raise RuntimeError("The environment variable SAGE_SRC must be set.")
       8SAGE_SRC = os.environ["SAGE_SRC"]
      79
      810# args can have length 0, in case we're printing a usage message (see trac 12207)
      911pyx_file = os.path.abspath(args[-1]) if len(args) else None

The others may depend on how we approach the Sage + Python 3 runtime question. If we follow the model at #23119, we can create scripts sage3-eval, sage3-preparse, etc., and call those from the main sage script. (The model that I propose at #23119 is that there is a top-level script called "sage", as now, plus a symlink to it called "sage3". They essentially call src/bin/$0, which translates to src/bin/sage or src/bin/sage3, one of which is a symlink of the other. Then that script can call $0-eval, $0-preparse, etc.)

comment:3 Changed 4 years ago by chapoton

  • Milestone changed from sage-8.0 to sage-8.1

any progress here ?

comment:4 Changed 4 years ago by chapoton

I made #23653 for sage-cython

comment:5 Changed 4 years ago by chapoton

  • Cc jdemeyer embray added

comment:6 Changed 4 years ago by embray

Most of these just need to be fixed to use #!/usr/bin/env sage-python23 no?

comment:7 Changed 4 years ago by chapoton

  • Branch set to public/23106
  • Commit set to 5eb29bf3f711fe6593b89a04e0efc4dab56fd49f

I had the same idea, but was rather not sure of its validity. Here is a branch doing that.


New commits:

5eb29bfmodify scripts towards python3 compatibility

comment:8 Changed 4 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Status changed from new to needs_review

This seems to work smoothly. What do you think ?

comment:9 Changed 4 years ago by chapoton

Should we do the same for the remaining 4

sage-notebook
sage-rst2sws
sage-rst2txt
sage-sws2rst

?

comment:10 Changed 4 years ago by vdelecroix

naive question: why is python pointing to python2 if Sage has been built with SAGE_PYTHON3=yes!?

comment:11 follow-up: Changed 4 years ago by chapoton

I think Volker once said that one should rather always let python point to python2.

See https://trac.sagemath.org/ticket/22582#comment:31

comment:12 follow-up: Changed 4 years ago by jhpalmieri

I view sage-python23 as a build-time tool. Actually, I view the setting of SAGE_PYTHON3 as something which only affects the Sage build, not necessarily how things are run. The Sage community has not made any decisions about how to actually run Sage using Python 3. As a result, I am somewhat wary of just using sage-python23 everywhere. I tried to explain this in comment:2.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by embray

Replying to chapoton:

I think Volker once said that one should rather always let python point to python2.

See https://trac.sagemath.org/ticket/22582#comment:31

I don't think that makes any sense. It won't be *that* long before Python 2 is deprecated and removed entirely. python should just be whatever the default python is.

comment:14 in reply to: ↑ 12 Changed 4 years ago by embray

Replying to jhpalmieri:

I view sage-python23 as a build-time tool. Actually, I view the setting of SAGE_PYTHON3 as something which only affects the Sage build, not necessarily how things are run. The Sage community has not made any decisions about how to actually run Sage using Python 3. As a result, I am somewhat wary of just using sage-python23 everywhere. I tried to explain this in comment:2.

That's a fair point. I think you're right--better it just be /usr/bin/env python. I don't think it makes sense to have side-by-side Python 2 and Python 3 installations of Sage, or the ability to switch between on the fly. That may be slightly convenient for working on Python 3 compatibility development, but that only affects a few people who can probably afford to have two separate builds for those purposes. One thing I am working with "abstract package" support is a better way (better than setting SAGE_PYTHON3=yes) to select the default Python at ./configure time, but changing that selection still requires a rebuild of anything that depends on Python.

comment:15 in reply to: ↑ 13 Changed 4 years ago by jhpalmieri

Replying to embray:

Replying to chapoton:

I think Volker once said that one should rather always let python point to python2.

See https://trac.sagemath.org/ticket/22582#comment:31

I don't think that makes any sense. It won't be *that* long before Python 2 is deprecated and removed entirely. python should just be whatever the default python is.

To clarify, it's not just that Volker once said it, it's PEP 394.

comment:16 Changed 4 years ago by embray

Boy, that PEP is a mess. But I agree it makes sense, for shebang lines at least, that python2 or python3 is always specified....

comment:17 follow-up: Changed 4 years ago by chapoton

This would now start to be really very useful (as sage starts with python3 using the experimental branch).

comment:18 in reply to: ↑ 17 Changed 4 years ago by jhpalmieri

Replying to chapoton:

This would now start to be really very useful (as sage starts with python3 using the experimental branch).

I think we need to make a design decision before we can make progress on this ticket. Once you set SAGE_PYTHON3=yes (or set the appropriate configure flag, some time in the future), does that forever "pollute" the Sage scripts so that they use python3, or do we want to somehow allow python2 and python3 builds to live side by side? If the former, we need to either save the setting so that future incremental builds in the same directory continue to use SAGE_PYTHON3=yes, or we let users shoot themselves in the foot by building with Python 3 and then a few weeks later upgrading matplotlib and forgetting to set the variable, so it tries to use Python 2 and something goes horribly wrong.

There are a few Sage packages which depend on Python and build libraries which do not reside in local/lib/python$VERSION/, and those present obstacles to side-by-side builds.

comment:19 Changed 4 years ago by embray

From a development perspective there's a convenience to allowing simultaneous Python 2 and Python 3 installations to live side-by-side. But currently this doesn't even work since some libraries, such as Pynac, can't be be installed with Python 2 and Python 3 support simultaneously (this could be fixed, for example, by attaching the Python version to the SONAME of the library, but I don't think that's currently supported).

That said, from a more general user perspective I don't think there's much advantage. I think it will be much, much simpler if SAGE_PYTHON3 is treated solely as a built-time variable that indicates what Python should be used to build and run Sage. And of course it should really be replaced with a ./configure flag. I think that makes the whole affair much, much simpler.

One thing that should still be possible is re-configuring with a different --with-python= flag, and rebuilding with that Python. I had some success making that work in my experimental ticket #23465.

comment:20 Changed 4 years ago by chapoton

  • Milestone changed from sage-8.1 to sage-8.2
  • Priority changed from critical to major

comment:21 Changed 4 years ago by embray

I've fixed quite a number of these in my branch just by changing the shebang lines to use sage-python23. There might be some cases where this isn't appropriate, but I've found it to b the right way to go in most cases.

comment:22 Changed 4 years ago by embray

Certainly the fixes in the branch attached to this ticket are a good start. I think we should go ahead and merge it, and leave bigger questions for a separate ticket.

comment:23 Changed 4 years ago by chapoton

so, positive review ?

comment:24 Changed 4 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

For me, yeah. I just wasn't sure if there were other objections.

comment:25 follow-up: Changed 4 years ago by jhpalmieri

To the extent that we want to separate the build process from the runtime process, this should be a temporary solution, right? We either shouldn't be calling sage-python23 from runtime pieces (since sage-python23 lives in build/bin) or we should move it or have a new script in src/bin which does something similar.

comment:26 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long --warn-long 78.8 src/sage/misc/gperftools.py
**********************************************************************
File "src/sage/misc/gperftools.py", line 244, in sage.misc.gperftools.Profiler._executable
Failed example:
    prof._executable()
Expected:
    '.../python'
Got:
    '/mnt/disk/home/release/Sage/local/bin/python2'
**********************************************************************
1 item had failures:
   1 of   4 in sage.misc.gperftools.Profiler._executable
    [36 tests, 1 failure, 0.94 s]

comment:27 Changed 4 years ago by git

  • Commit changed from 5eb29bf3f711fe6593b89a04e0efc4dab56fd49f to 25dddc53d545ad2400023000d62d33bb990879aa

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

2c8d97fMerge branch 'public/23106' in 8.2.b2
25dddc5trac 23106 detail

comment:28 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

New commits:

2c8d97fMerge branch 'public/23106' in 8.2.b2
25dddc5trac 23106 detail

New commits:

2c8d97fMerge branch 'public/23106' in 8.2.b2
25dddc5trac 23106 detail

comment:29 Changed 4 years ago by embray

  • Status changed from needs_review to needs_work

That fix won't work on Python 3--just put .../python... (and consider squashing)

comment:30 Changed 4 years ago by git

  • Commit changed from 25dddc53d545ad2400023000d62d33bb990879aa to 847df2c99cbce5f35f4c7c15da1ecb26843f5324

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

847df2cmodify scripts towards python3 compatibility

comment:31 Changed 4 years ago by chapoton

done


New commits:

847df2cmodify scripts towards python3 compatibility

New commits:

847df2cmodify scripts towards python3 compatibility

comment:32 Changed 4 years ago by embray

  • Status changed from needs_work to positive_review

Thanks!

comment:33 in reply to: ↑ 25 Changed 4 years ago by embray

Replying to jhpalmieri:

To the extent that we want to separate the build process from the runtime process, this should be a temporary solution, right? We either shouldn't be calling sage-python23 from runtime pieces (since sage-python23 lives in build/bin) or we should move it or have a new script in src/bin which does something similar.

I don't particularly care where the scripts live in the source code--I think that scripts are needed at build time living in src/bin makes sense, but don't have strong opinions about it.

All that matters to me is what happens at installation. I think all scripts in build/bin should be installed to $SAGE_LOCAL/bin. Similarly I'm of the opinion that most other files in build/ should be installed somewhere under $SAGE_LOCAL so that, for example, one can install new packages for Sage at runtime without having a full sage source tree lying around. That is, package installations and upgrades fully become a "runtime" feature of Sage. That's all a much bigger topic beyond this though.

comment:34 Changed 4 years ago by vbraun

  • Branch changed from public/23106 to 847df2c99cbce5f35f4c7c15da1ecb26843f5324
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.