#29195 closed defect (fixed)

do not lazy-import sagenb on python3

Reported by: dimpase Owned by:
Priority: minor Milestone: sage-9.1
Component: python3 Keywords:
Cc: chapoton, embray, mkoeppe Merged in:
Authors: Frédéric Chapoton Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 377c828 (Commits, GitHub, GitLab) Commit: 377c8281f5239c7eff02bcb4e0c4b1e0ac6dd9e1
Dependencies: Stopgaps:

Status badges

Description

currently, with py3 sage,

sage: notebook()

results in an import error.

This is because sagenb is still lazy-imported in src/sage/all.py. This lazy import should be made conditional on python version.

Change History (35)

comment:1 Changed 15 months ago by dimpase

  • Priority changed from major to minor

comment:2 Changed 15 months ago by jhpalmieri

Something like this?

  • src/sage/all.py

    diff --git a/src/sage/all.py b/src/sage/all.py
    index 97f01b4005..70021c91bd 100644
    a b from sage.manifolds.all import * 
    206206
    207207from cysignals.alarm import alarm, cancel_alarm
    208208
    209 # Lazily import notebook functions and interacts (#15335)
    210 lazy_import('sagenb.notebook.notebook_object', 'notebook')
    211 lazy_import('sagenb.notebook.notebook_object', 'inotebook')
    212 lazy_import('sagenb.notebook.sage_email', 'email')
    213 lazy_import('sage.interacts.debugger', 'debug')
    214 lazy_import('sage.interacts', 'all', 'interacts')
    215 # interact decorator from SageNB (will be overridden by Jupyter)
    216 lazy_import('sagenb.notebook.interact', 'interact')
     209from six import PY2
     210if PY2:
     211    # Lazily import notebook functions and interacts (#15335)
     212    lazy_import('sagenb.notebook.notebook_object', 'notebook')
     213    lazy_import('sagenb.notebook.notebook_object', 'inotebook')
     214    lazy_import('sagenb.notebook.sage_email', 'email')
     215    lazy_import('sage.interacts.debugger', 'debug')
     216    lazy_import('sage.interacts', 'all', 'interacts')
     217    # interact decorator from SageNB (will be overridden by Jupyter)
     218    lazy_import('sagenb.notebook.interact', 'interact')
     219del PY2
    217220
    218221from copy import copy, deepcopy
    219222

comment:3 Changed 15 months ago by dimpase

well, I trust you that six.PY2 is exactly what's needed. What's that del PY2 for? Why not just if six.PY2:, without import and del?

Last edited 15 months ago by dimpase (previous) (diff)

comment:4 Changed 15 months ago by jhpalmieri

If I use if six.PY2 without any import statement, I see this when starting Sage:

NameError: name 'six' is not defined

So then of course del PY2 is there so that PY2 is not defined when all.py is imported when Sage starts up.

six.PY2: A boolean indicating if the code is running on Python 2

comment:5 Changed 15 months ago by dimpase

$ grep -R "import six" src/sage/ | wc -l
102

why not just put import six in all.py?

comment:6 Changed 15 months ago by dimpase

also:

$ grep -R "PY2" src/sage/ | wc -l
58
$ grep -R "six\.PY2" src/sage/ | wc -l
30

so it seems that six.PY2 is a bit more popular :-)

Last edited 15 months ago by dimpase (previous) (diff)

comment:7 Changed 15 months ago by jhpalmieri

Everything in all.py is executed when Sage starts, so if I put import six there, then six is available from the command-line. The current state of affairs:

sage: six
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-1-cfa698ef8823> in <module>()
----> 1 six

NameError: name 'six' is not defined

With import six in all.py:

sage: six
<module 'six' from '/Users/jpalmier/Desktop/Sage/sage_builds/TESTING/sage-9.1.beta4/local/lib/python3.7/site-packages/six.py'>

We should not add any extra imports without good reason.

In random files in the Sage library, not everything is imported, so you can do import six and six.PY2 without running into these problems. all.py is special.

comment:8 Changed 15 months ago by dimpase

all.py imports os and sys, even though it may be argued against having them there by exactly the same argument as for six.

comment:9 Changed 15 months ago by dimpase

I think if we can remove several hundreds (count all of import six and from six, it is over 700 total) of imports from the library without causing problems, then we should do it.

comment:10 Changed 15 months ago by jhpalmieri

How do you propose doing that? If file.py imports six and all.py imports file, then adding an import to all won’t help with file, will it?

comment:11 Changed 15 months ago by jhpalmieri

Anyway, I’m going to be away from my computer for a week, so I won’t be working on this.

comment:12 Changed 15 months ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/nosagenbpy3
  • Cc embray mkoeppe added
  • Commit set to c452f654927b8a5c7934fdd757e2967f146bb563

New commits:

c452f65do not lazy-import sagenb on py3

comment:13 follow-up: Changed 15 months ago by chapoton

We have already started to remove all calls to six. This lazy import of sagenb should just be removed.

comment:14 in reply to: ↑ 13 Changed 15 months ago by dimpase

Replying to chapoton:

We have already started to remove all calls to six. This lazy import of sagenb should just be removed.

I must say I missed this. I thought the coming release is still going to be py2-compatible. Could you point me out the corresponding tickets?

comment:15 Changed 15 months ago by chapoton

So far, we are removing six when possible so that it does not break (but may slow) the python 2 version of sage.

See for example

af348cfc28 Trac #29103: get rid of part of itervalues

5abdaf4583 Trac #29077: get rid of six.iterkeys

comment:16 Changed 15 months ago by dimpase

do you propose to remove sagenb, or just the lazy import?

comment:17 follow-up: Changed 15 months ago by chapoton

well, does removing just the lazy import breaks some doctests ? (in py2 and py3) ?

My opinion is that people cannot reasonably both ask to use the latest sage, python2 and the legacy notebook.

comment:18 in reply to: ↑ 17 Changed 15 months ago by dimpase

Replying to chapoton:

well, does removing just the lazy import breaks some doctests ? (in py2 and py3) ?

My opinion is that people cannot reasonably both ask to use the latest sage, python2 and the legacy notebook.

If there is a dependency/doctest on py3 that needs any of sagenb then it is a bug.

Any tickets on this?

comment:19 Changed 15 months ago by dimpase

what's "broken" if sagenb is not (lazy) imported is sage.interacts - but this is as expected, it appears nothing in sage.interacts works without sagenb, despite some vague comments about Jupyter.

comment:20 Changed 15 months ago by dimpase

I don't understand how the tests in src/sage/interacts/library.py work on py3, without a functioning sagenb (but with these lazy imports present).

I get errors there on this branch.

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

I would suggest to just remove the lines starting with

lazy_import('sagenb

comment:22 in reply to: ↑ 21 Changed 15 months ago by dimpase

Replying to chapoton:

I would suggest to just remove the lines starting with

lazy_import('sagenb

this, as well as the branch on the ticket, causes doctest errors in sage.interacts.library, no?

comment:23 Changed 15 months ago by chapoton

  • src/sage/all.py

    diff --git a/src/sage/all.py b/src/sage/all.py
    index 97f01b4005..80ac1d8372 100644
    a b from sage.manifolds.all import * 
    207207from cysignals.alarm import alarm, cancel_alarm
    208208
    209209# Lazily import notebook functions and interacts (#15335)
    210 lazy_import('sagenb.notebook.notebook_object', 'notebook')
    211 lazy_import('sagenb.notebook.notebook_object', 'inotebook')
    212 lazy_import('sagenb.notebook.sage_email', 'email')
     210#lazy_import('sagenb.notebook.notebook_object', 'notebook')
     211#lazy_import('sagenb.notebook.notebook_object', 'inotebook')
     212#lazy_import('sagenb.notebook.sage_email', 'email')
    213213lazy_import('sage.interacts.debugger', 'debug')
    214214lazy_import('sage.interacts', 'all', 'interacts')
    215215# interact decorator from SageNB (will be overridden by Jupyter)
    216 lazy_import('sagenb.notebook.interact', 'interact')
     216#lazy_import('sagenb.notebook.interact', 'interact')
    217217
    218218from copy import copy, deepcopy

does not break any doctest in interacts (under py3).

Last edited 15 months ago by chapoton (previous) (diff)

comment:24 Changed 15 months ago by chapoton

  • Branch changed from u/dimpase/nosagenbpy3 to public/ticket/29195
  • Commit changed from c452f654927b8a5c7934fdd757e2967f146bb563 to c3602f43aa094a3b63bd178e2e8f5db69c180f5d

Here is a branch. Launching my bot on it.


New commits:

c3602f4remove the "notebook" command

comment:25 Changed 15 months ago by kcrisman

But sage -notebook will (eventually) launch Jupyter, correct? We are just removing the possibility of launching a notebook from within a Sage session here - I guess it's not possible to do that with Jupyter, or so I recall.

This business of interact depending on sagenb is tricky. Not having interactive stuff in Jupyter is a real regression; I constantly use Sage cell server instances (and sometimes still Sage notebook) with interacts, and of course CoCalc has them.

comment:26 Changed 15 months ago by dimpase

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

grep -R "notebook()" src/doc/ returns few hits, they need to be sorted out.

comment:27 Changed 15 months ago by chapoton

Indeed, here is the list

sage3$ git grep -c "notebook()" src/
src/doc/de/tutorial/interactive_shell.rst:4
src/doc/en/faq/faq-usage.rst:1
src/doc/en/installation/source.rst:1
src/doc/en/thematic_tutorials/explicit_methods_in_number_theory/introduction.rst:1
src/doc/en/tutorial/interactive_shell.rst:4
src/doc/fr/tutorial/interactive_shell.rst:4
src/doc/ja/tutorial/interactive_shell.rst:4
src/doc/pt/tutorial/interactive_shell.rst:4
src/doc/ru/tutorial/interactive_shell.rst:4
src/mac-app/sage-README-macOS.txt:1
src/sage/interfaces/sage0.py:2

comment:28 Changed 15 months ago by git

  • Commit changed from c3602f43aa094a3b63bd178e2e8f5db69c180f5d to 4f3be228341d36f3af2755253b632ca1285e15e1

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

4f3be22some of the needed chnages in docs to deal with notebook() going away

comment:29 Changed 15 months ago by git

  • Commit changed from 4f3be228341d36f3af2755253b632ca1285e15e1 to 540992ef794cba16f8341d70d0b8518b7b33d35a

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

540992emore cleaining up of notebook()

comment:30 Changed 15 months ago by git

  • Commit changed from 540992ef794cba16f8341d70d0b8518b7b33d35a to 5e97e73605c1228aa64b544161f1d6db442483bf

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

5e97e73fix an RST typo

comment:31 Changed 15 months ago by dimpase

In src/doc/*/tutorial/interactive_shell.rst, 3 out of 4 appearences of notebook() are just in Sage banners, so that's trivial to fix without knowning the language. The 4th is harder, and I did it in English, German, and Russian. I haven't touched French, Portugese, and Japanese files.

comment:32 Changed 15 months ago by git

  • Commit changed from 5e97e73605c1228aa64b544161f1d6db442483bf to 27ced7929d15c110dcca417e7dde0c65cc28a7b4

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

27ced79updated Sage banners to remove notebook()

comment:33 Changed 15 months ago by git

  • Commit changed from 27ced7929d15c110dcca417e7dde0c65cc28a7b4 to 377c8281f5239c7eff02bcb4e0c4b1e0ac6dd9e1

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

377c828French doc update

comment:34 Changed 15 months ago by dimpase

  • Status changed from needs_review to positive_review

we can fix the more exotic languages later.

comment:35 Changed 15 months ago by vbraun

  • Branch changed from public/ticket/29195 to 377c8281f5239c7eff02bcb4e0c4b1e0ac6dd9e1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.