Opened 15 months ago
Closed 15 months ago
#29195 closed defect (fixed)
do not lazyimport sagenb on python3
Reported by:  dimpase  Owned by:  

Priority:  minor  Milestone:  sage9.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: 
Description
currently, with py3 sage,
sage: notebook()
results in an import error.
This is because sagenb is still lazyimported 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
 Priority changed from major to minor
comment:2 Changed 15 months ago by
comment:3 Changed 15 months ago by
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
?
comment:4 Changed 15 months ago by
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
$ 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
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 :)
comment:7 Changed 15 months ago by
Everything in all.py
is executed when Sage starts, so if I put import six
there, then six
is available from the commandline. The current state of affairs:
sage: six  NameError Traceback (most recent call last) <ipythoninput1cfa698ef8823> 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/sage9.1.beta4/local/lib/python3.7/sitepackages/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
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
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
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
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
 Branch set to u/dimpase/nosagenbpy3
 Cc embray mkoeppe added
 Commit set to c452f654927b8a5c7934fdd757e2967f146bb563
New commits:
c452f65  do not lazyimport sagenb on py3

comment:13 followup: ↓ 14 Changed 15 months ago by
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
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 py2compatible. Could you point me out the corresponding tickets?
comment:15 Changed 15 months ago by
comment:16 Changed 15 months ago by
do you propose to remove sagenb, or just the lazy import?
comment:17 followup: ↓ 18 Changed 15 months ago by
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
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
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
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 followup: ↓ 22 Changed 15 months ago by
I would suggest to just remove the lines starting with
lazy_import('sagenb
comment:22 in reply to: ↑ 21 Changed 15 months ago by
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

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 * 207 207 from cysignals.alarm import alarm, cancel_alarm 208 208 209 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')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 213 lazy_import('sage.interacts.debugger', 'debug') 214 214 lazy_import('sage.interacts', 'all', 'interacts') 215 215 # interact decorator from SageNB (will be overridden by Jupyter) 216 lazy_import('sagenb.notebook.interact', 'interact')216 #lazy_import('sagenb.notebook.interact', 'interact') 217 217 218 218 from copy import copy, deepcopy
does not break any doctest in interacts (under py3).
comment:24 Changed 15 months ago by
 Branch changed from u/dimpase/nosagenbpy3 to public/ticket/29195
 Commit changed from c452f654927b8a5c7934fdd757e2967f146bb563 to c3602f43aa094a3b63bd178e2e8f5db69c180f5d
comment:25 Changed 15 months ago by
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
 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
Indeed, here is the list
sage3$ git grep c "notebook()" src/ src/doc/de/tutorial/interactive_shell.rst:4 src/doc/en/faq/faqusage.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/macapp/sageREADMEmacOS.txt:1 src/sage/interfaces/sage0.py:2
comment:28 Changed 15 months ago by
 Commit changed from c3602f43aa094a3b63bd178e2e8f5db69c180f5d to 4f3be228341d36f3af2755253b632ca1285e15e1
Branch pushed to git repo; I updated commit sha1. New commits:
4f3be22  some of the needed chnages in docs to deal with notebook() going away

comment:29 Changed 15 months ago by
 Commit changed from 4f3be228341d36f3af2755253b632ca1285e15e1 to 540992ef794cba16f8341d70d0b8518b7b33d35a
Branch pushed to git repo; I updated commit sha1. New commits:
540992e  more cleaining up of notebook()

comment:30 Changed 15 months ago by
 Commit changed from 540992ef794cba16f8341d70d0b8518b7b33d35a to 5e97e73605c1228aa64b544161f1d6db442483bf
Branch pushed to git repo; I updated commit sha1. New commits:
5e97e73  fix an RST typo

comment:31 Changed 15 months ago by
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
 Commit changed from 5e97e73605c1228aa64b544161f1d6db442483bf to 27ced7929d15c110dcca417e7dde0c65cc28a7b4
Branch pushed to git repo; I updated commit sha1. New commits:
27ced79  updated Sage banners to remove notebook()

comment:33 Changed 15 months ago by
 Commit changed from 27ced7929d15c110dcca417e7dde0c65cc28a7b4 to 377c8281f5239c7eff02bcb4e0c4b1e0ac6dd9e1
Branch pushed to git repo; I updated commit sha1. New commits:
377c828  French doc update

comment:34 Changed 15 months ago by
 Status changed from needs_review to positive_review
we can fix the more exotic languages later.
comment:35 Changed 15 months ago by
 Branch changed from public/ticket/29195 to 377c8281f5239c7eff02bcb4e0c4b1e0ac6dd9e1
 Resolution set to fixed
 Status changed from positive_review to closed
Something like this?
src/sage/all.py