Interfaces: use more lazy imports, restore top-level functions maxima_console etc.

[jhpalmieri]

Various files in sage/interfaces/ have this pattern:

class Octave(...):
    ....

octave = Octave()

When combined with from .octave import Octave, octave in sage/interfaces/all.py, this means that an instance of Octave is created when Sage starts up. I think we should avoid this with lazy imports.

Depends on #16522

Component: interfaces

Author: John Palmieri

Branch: 09f5d92

Reviewer: Matthias Koeppe

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

[mkoeppe]

comment:1

+1

[jhpalmieri]

Branch: u/jhpalmieri/lazy_imports_for_interfaces

[sagetrac-git]

Commit: b483758

[sagetrac-git]

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

b483758

trac 34547: use lazy imports in much of sage.interfaces.all

[jhpalmieri]

comment:4

Here is a first attempt. I'll mark it as ready for review, but I am certainly open to changes. If I made changes for the imports for magma and polymake, I got problems with pickling, so I have left those as they were before.

[jhpalmieri]

comment:5

Regarding the change

-interfaces = ['gap', 'gap3', 'giac', 'gp', 'mathematica', 'gnuplot', \
-              'kash', 'magma', 'macaulay2', 'maple', 'maxima', \
-              'mathematica', 'mwrank', 'octave', 'r', \
-              'singular', 'sage0', 'sage']

Do we need to deprecate this before just removing it? I don't know what purpose it was supposed to serve, except perhaps it is the remnants of some old code that once looped through it.

[sagetrac-git]

Changed commit from b483758 to 250da03

[sagetrac-git]

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

250da03

trac 34547: use lazy imports in much of sage.interfaces.all

[nbruin]

comment:8

Where do the ".all" namespaces actually get used and for what? Are they just to trigger initialization of modules? Or do people use them to access symbols too? They certainly be importing symbols from them. Especially with lazy imports, that leads to undesirable side-effects (see e.g. #16522):

sage: type(maxima_calculus)
<class 'sage.misc.lazy_import.LazyImport'>
sage: type(sage.calculus.calculus.maxima)
<class 'sage.misc.lazy_import.LazyImport'>
sage: sage.calculus.calculus.maxima is maxima_calculus  # they are the same object
True
sage: maxima_calculus(1)
1
sage: type(maxima_calculus)
<class 'sage.misc.lazy_import.LazyImport'>
sage: type(sage.calculus.calculus.maxima)
<class 'sage.interfaces.maxima_lib.MaximaLib'>

So as you can see, we end up with a LazyImport object bound in two places. Calling it removes one of the shims but not the other (it can't! it doesn't have a reference to the relevant namespace!).
This object rebinding is only tried upon the first resolution of the lazy object, but it does mean that the shim in the wrong location stays in place (and these shims are not perfect replacements for their objects)

The delayed instantiation of interfaces is laudable but, as you can see, can introduce new problems. What does work is lazy importing the object where you need it, but I'm not so sure we need these objects at the 'all' level. Certainly the from <...>.all import * done in sage.all propagates this problem. The tool clean_namespace at the branch at #16522 can clean it up after import ...

[mkoeppe]

comment:9

Replying to Nils Bruin:

Where do the ".all" namespaces actually get used and for what? Are they just to trigger initialization of modules?

sage.interfaces is a namespace package, so we're getting rid of imports from sage.interfaces.all within the Sage library, see #34201. When this is done, all that will remain is the reimport into sage.all, to fill the interactive top level as the only purpose.

[jhpalmieri]

comment:10

Right now, git grep "from.*interfaces.all.*import" src/sage only reveals the import in sage.all. There are a few matches for git grep "import.*interfaces.all" src/sage.

[jhpalmieri]

comment:11

Maybe we need to keep the one in src/sage/repl/interface_magic.py?

[nbruin]

comment:12

Replying to Matthias Köppe:

Replying to Nils Bruin:

Where do the ".all" namespaces actually get used and for what? Are they just to trigger initialization of modules?

sage.interfaces is a namespace package, so we're getting rid of imports from sage.interfaces.all within the Sage library, see #34201. When this is done, all that will remain is the reimport into sage.all, to fill the interactive top level as the only purpose.

Ah, OK, so our current infrastructure actually guarantees that LazyImports in sage.all end up broken in the toplevel. Possible fixes would be to build the global namespace piecemeal and recreate appropriate LazyImport objects rather than just referencing them, or just do the copy and then clean them up with something like clean_namespace (see #16522). The latter is much easier to implement. It looks like cleaning sage.all and the toplevel namespace upon construction would give us a lot of benefit for relatively low effort.

One reason to ensure LazyImport shims don't stay lying around, especially at top level, is that documentation is only partially transparently reported. Compare:

sage: sage.calculus.calculus.maxima? # reports doc but with wrong type, signature, and source file
sage: sage.calculus.calculus.maxima? # second time is OK

versus

sage: maxima_calculus? # never gets type, signature, and source file right

Note:

sage: maxima_calculus?? # finds source but is still missing type and signature

[mkoeppe]

comment:13

Replying to Nils Bruin:

so our current infrastructure actually guarantees that LazyImports in sage.all end up broken in the toplevel. Possible fixes would be to [...] or just do the copy and then clean them up with something like clean_namespace (see #16522). The latter is much easier to implement. It looks like cleaning sage.all and the toplevel namespace upon construction would give us a lot of benefit for relatively low effort.

Thanks for the pointer to #16522. I haven't looked at the details, but this sounds promising.

[sagetrac-git]

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

9566546

trac 34547: remove two "import sage.interfaces.all" lines

[sagetrac-git]

Changed commit from 250da03 to 9566546

[jhpalmieri]

comment:15

Is this branch okay, or do we need to do something to accommodate #16522?

[mkoeppe]

comment:16

Are these changes:

--- a/src/sage/interfaces/axiom.py
+++ b/src/sage/interfaces/axiom.py
@@ -490,7 +490,7 @@ class Axiom(PanAxiom):
         """
         EXAMPLES::
 
-            sage: axiom.__reduce__()
+            sage: Axiom().__reduce__()
             (<function reduce_load_Axiom at 0x...>, ())

needed because of #16522?

[jhpalmieri]

comment:17

I see the following with this branch:

sage: axiom.__reduce__
<built-in method __reduce__ of sage.misc.lazy_import.LazyImport object at 0x19a6991c0>
sage: Axiom().__reduce__
<bound method Axiom.__reduce__ of Axiom>
sage: axiom.__reduce__()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [7], line 1
----> 1 axiom.__reduce__()

File /usr/local/Cellar/python@3.10/3.10.8/Frameworks/Python.framework/Versions/3.10/lib/python3.10/copyreg.py:76, in _reduce_ex(self, proto)
     74 else:
     75     if base is cls:
---> 76         raise TypeError(f"cannot pickle {cls.__name__!r} object")
     77     state = base(self)
     78 args = (cls, base, state)

TypeError: cannot pickle 'LazyImport' object
sage: Axiom().__reduce__()
(<function reduce_load_Axiom at 0x19d557c70>, ())

I don't understand why this change is necessary.

[jhpalmieri]

comment:18

Maybe it is #16522.

[mkoeppe]

comment:19

Yes, I think this should be solved via #16522 instead of working around it in the doctests

[mkoeppe]

Dependencies: #16522

[nbruin]

comment:20

I don't think #16522 will prevent this problem: I don't think special methods like __reduce__, which are present of LazyImport objects, trigger resolution. According to https://docs.python.org/3/reference/datamodel.html#object.getattr, the __getattr__ method, which is the one LazyImport uses, only gets invoked when normal attribute lookup fails.

This is why most slot methods are separately wrapped to trigger resolution, because these would never fail.

[jhpalmieri]

comment:21

The __reduce__ doctests look pretty artificial. Would there be more meaningful ones that we could use as replacements?

[jhpalmieri]

comment:48

One of the failures can be avoided by deleting the loop variable pkg at the end. For the others, should we change the doctests?

[jhpalmieri]

comment:49

Oh, I see, we can just move sage.misc.session.init() after the lazy imports.

[sagetrac-git]

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

09f5d92

trac 34547: unconditionally lazily import *_console and move the

[sagetrac-git]

Changed commit from 70b7c44 to 09f5d92

[jhpalmieri]

comment:51

This works for me.

[vbraun]

Changed branch from u/jhpalmieri/lazy_imports_for_interfaces to 09f5d92

[jhpalmieri]

Changed commit from 09f5d92 to none

[jhpalmieri]

comment:54

Followup at #34927: a change to a doctest for __reduce__ in fricas.py along the same lines as above. That doctest was marked as optional, which is why I didn't catch it before.

[jhpalmieri]

comment:55

More issues: I have no idea why, but removing these lines from interfaces.all break the emacs sage-shell-mode:

-interfaces = ['gap', 'gap3', 'giac', 'gp', 'mathematica', 'gnuplot', \
-              'kash', 'magma', 'macaulay2', 'maple', 'maxima', \
-              'mathematica', 'mwrank', 'octave', 'r', \
-              'singular', 'sage0', 'sage']

If I restore this, or if I just add a line interfaces = [], then it seems to work again. I am not going to open a ticket for this right now, in part because I'm busy with other things and in part because I have no idea what's going on.

I thought that this might be related to repl.interface_magic, since that's a place that imports all from sage.interfaces.all, but I can't see why. However, the code in that file tries to register the various interfaces to create IPython magic commands like %%gap, and with the various new lazy imports, those interfaces are no longer registered. The now missing magic commands:

%%axiom
%%fricas
%%gap3
%%giac
%%kash
%%lie
%%lisp
%%macaulay2
%%maple
%%mathematica
%%mathics
%%matlab
%%mupad
%%mwrank
%%octave
%%r

Do we care? Is it worth restoring these?

[mkoeppe]

comment:56

This line seems to refer to this global Sage variable -- https://github.com/sagemath/sage-shell-mode/blob/master/emacs_sage_shell.py#L55

[jhpalmieri]

comment:57

Thank you for finding that. I do enjoy reading uncommented code...

[jhpalmieri]

comment:58

This variable is not used anywhere in the Sage library, as far as I can tell. I see three options:

1. restore it just as it was before
2. restore it and set it to a sensible value, probably asking the sage-shell people what that means
3. let the sage-shell people set it in their own code to a sensible value

1 is easiest. 2 makes more sense than 1, and 3 makes more sense than 2, at least to me. Unless we think that other people were also using this variable?

[jhpalmieri]

comment:59

To spell out the easy solution:

diff --git a/src/sage/interfaces/all.py b/src/sage/interfaces/all.py
index 92e19e3d38..53cf7c92dd 100644
--- a/src/sage/interfaces/all.py
+++ b/src/sage/interfaces/all.py
@@ -43,3 +43,9 @@ lazy_import('sage.interfaces.r', ['r', 'R', 'r_version'])
 lazy_import('sage.interfaces.read_data', 'read_data')
 lazy_import('sage.interfaces.scilab', 'scilab')
 lazy_import('sage.interfaces.tachyon', 'tachyon_rt')
+
+# The following variable is used by sage-shell-mode in emacs:
+interfaces = ['gap', 'gap3', 'giac', 'gp', 'mathematica', 'gnuplot', \
+              'kash', 'magma', 'macaulay2', 'maple', 'maxima', \
+              'mathematica', 'mwrank', 'octave', 'r', \
+              'singular', 'sage0', 'sage']

[mkoeppe]

comment:60

Yes, I think that's a good solution, at least for now.
We don't really have protocols for documenting and deprecating global variables, so we should probably accept that downstream code makes use of this variable.

[jhpalmieri]

comment:61

Done in #34935.