lazy_import doesn't resolve properly when indirectly imported

[nbruin]

We have several places where lazy_import objects are used in a way that prevents them from behaving as designed.

The original idea of LazyImport proxies is that they have a pointer to the namespace in which they are bound, so that once the import gets triggered, the proxy object redirects the binding in the namespace to point straight to the proxied object. Once this redirection has happened, the proxy object should not play a role anymore and no performance impact should happen at all.

The problem occurs from statements such as:

calculus/all.py:1:from calculus import maxima as maxima_calculus

This doesn't work, because this is a LazyImport proxy, which needs to know the namespace in which it is bound to do the proper replacement. This one is tied to sage.calculus.calculus.maxima, so it can't rebind the global maxima_calculus. Indeed:

sage: type(sage.calculus.calculus.maxima)
<type 'sage.misc.lazy_import.LazyImport'>
sage: type(maxima_calculus)
<type 'sage.misc.lazy_import.LazyImport'>
sage: hash(maxima_calculus)
-7971541566211231133
sage: type(sage.calculus.calculus.maxima)
<class 'sage.interfaces.maxima_lib.MaximaLib'>
sage: type(maxima_calculus)
<type 'sage.misc.lazy_import.LazyImport'>

The binding of maxima_calculus in the global namespace (and the one in calculus.all too) remains to the LazyImport proxy. Thus we suffer from indirection overhead [one might worry we'd suffer repeated extraneous dictionary modifications, but LazyImport is smart enough to only attempt to rebind sage.calculus.calculus.maxima only on the first access] as well as problems that things like id(LazyImportShim) and type(LazyImportShim) are not what they're supposed to model.

If instead we do:

sage: lazy_import('sage.interfaces.maxima_lib','maxima','maxima_calculus')

we see that things do resolve:

sage: type(maxima_calculus)
<type 'sage.misc.lazy_import.LazyImport'>
sage: hash(maxima_calculus)
-7971541566211231133
sage: type(maxima_calculus)
<class 'sage.interfaces.maxima_lib.MaximaLib'>

Other bindings need their own chance to resolve, but do:

sage: type(sage.calculus.calculus.maxima)
<type 'sage.misc.lazy_import.LazyImport'>
sage: hash(sage.calculus.calculus.maxima)
-7971541566211231133
sage: type(sage.calculus.calculus.maxima)
<class 'sage.interfaces.maxima_lib.MaximaLib'>

The obvious fix: in calculus.all, import maximalib directly and lazily, rather than indirectly from sage.calculus.calculus only kicks the can further, since in sage.all we have from sage.calculus.all import * (which I think is where it really gets placed in the global sage namespace).

CC: @kcrisman @embray @tscrim

Component: misc

Author: Nils Bruin, Matthias Koeppe

Branch/Commit: 9837dec

Reviewer: Matthias Koeppe, Nils Bruin

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

[nbruin]

comment:2

Ouch. This is difficult to fix. Sure, we can put in sage/calculus/all.py:

from sage.misc.lazy_import import lazy_import
#this is sage.calculus.calculus.maxima. It needs to be lazily imported.
lazy_import("sage.interfaces.maxima_lib","maxima","maxima_calculus")

but that only solves one step of the problem. Next we get in sage/all.py:

from sage.calculus.all   import *

so if we want to solve this problem, we'd have to do so manually unless we come up with very smart hack in lazy_import. There are other lazy_imports of this type:

sage: type(Riemann_Map)
<type 'sage.misc.lazy_import.LazyImport'>
sage: %time hash(Riemann_Map)
CPU times: user 42 ms, sys: 5 ms, total: 47 ms
Wall time: 45.2 ms
8795750155546
sage: %time hash(Riemann_Map)
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 7.87 µs
8795750155546
sage: type(Riemann_Map)
<type 'sage.misc.lazy_import.LazyImport'>

Exactly the same story. Luckily, the lazy_import proxy objects are pretty transparent when the import has already happened, so it's not too much of an issue when they're in the way (unless you're in a very tight loop). I'll be upgrading the severity and scope of this ticket.

[jdemeyer]

Dependencies: #22752

[nbruin]

comment:5

Following up on the code in comment
#19628:12
and the discussion below it, we can consider cleaning the some of the namespaces of some of the improperly placed lazy_imports. It looks like cleaning them after the fact isn't very expensive, so perhaps that's easier than to avoid improper lazy_imports in the first place.

[nbruin]

Changed dependencies from #22752 to none

[nbruin]

Branch: u/nbruin/lazy_import_doesn_t_resolve_properly_when_indirectly_imported_

[nbruin]

comment:7

Seeing how many improperly placed LazyImport (i.e., LazyImport objects that do have a namespace set, but it doesn't match the namespace in which they are bound, or the name they are bound to doesn't match the _as_name attribute):

from sage.misc.lazy_import import LazyImport, attributes, clean_namespace
def misbound_lazies(S):
    return [k for k,v in S.items() if
        type(v) is LazyImport and
        attributes(v)['_namespace'] is not None and
            (attributes(v)['_namespace'] is not S or
             attributes(v)['_as_name'] != k)]
M=[(k,len(misbound_lazies(m.__dict__))) for k,m in sys.modules.iteritems() if m is not None]

gives currently:

sage: [m for m in M if m[1] > 0]
[('sage.groups.all', 9),
 ('sage.combinat.non_decreasing_parking_function', 1),
 ('sage.groups.libgap_mixin', 1),
 ('__main__', 330),
 ('sage.categories.all', 1),
 ('sage.combinat.all', 23),
 ('sage.calculus.all', 1),
 ('sage.tensor.all', 1),
 ('sage.combinat.integer_vector', 1),
 ('sage.geometry.all', 3),
 ('sage.graphs.digraph', 1),
 ('sage.all_cmdline', 330),
 ('sage.combinat.partition_tuple', 1),
 ('sage.algebras.lie_algebras.affine_lie_algebra', 1),
 ('sage.modular.arithgroup.congroup_generic', 1),
 ('sage.schemes.all', 4),
 ('sage.rings.all', 13),
 ('sage.combinat.set_partition_ordered', 1),
 ('sage.categories.groups', 1),
 ('sage.graphs.generic_graph', 1),
 ('sage.algebras.all', 2),
 ('sage.modular.all', 4),
 ('sage.combinat.integer_vectors_mod_permgroup', 1),
 ('sage.dynamics.all', 7),
 ('sage.combinat.composition', 1),
 ('sage.combinat.partition', 1),
 ('sage.all', 314)]

Executing:

for m in sys.modules.values():
    if m is not None:
        clean_namespace(m)

naturally resolves all these. Two options to use this in practice:

• put a clean_namespace(globals()) at the end of offending modules.
• execute this on sys.modules.values() at the end of initialization in sage

Comments welcome.

[nbruin]

Commit: 716fb69

[sagetrac-git]

Changed commit from 716fb69 to 419e8cb

[sagetrac-git]

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

419e8cb

tool to clean misplaced lazy_imports plus some applications of it

[videlec]

comment:10

What is the actual problem? The first lines of the ticket description are very cryptic.

[nbruin]

comment:12

Replying to @videlec:

What is the actual problem? The first lines of the ticket description are very cryptic.

I tried to decrypt it.

[sagetrac-git]

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

3095325

trac 16522: introduce debugging routine to access LazyImport attributes and a routine to clean namespaces of misconfigured LazyImport objects

[sagetrac-git]

Changed commit from 419e8cb to 3095325

[mkoeppe]

Changed commit from b4f6f3d to 406e6f8

[mkoeppe]

comment:39

Rebased

---

New commits:

a29a25d	trac 16522: introduce debugging routine to access LazyImport attributes and a routine to clean namespaces of misconfigured LazyImport objects
1fa627b	clean sage.all and user_globals upon initialization
5764c34	fix LazyImport shim problems in identity tests in partition.py
2a352f3	use `_get_object` so that an import actually gets triggered (attribute lookup is not sufficient)
406e6f8	Change order of cases in `Partitions` to put cheaper tests first

[sagetrac-git]

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

[sagetrac-git]

Changed commit from 406e6f8 to 1fa627b

[mkoeppe]

comment:41

Dropped workarounds for NN in partitions

[sagetrac-git]

Changed commit from 1fa627b to 4acb9d7

[sagetrac-git]

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

6e384f5	src/sage/misc/lazy_import.pyx: Codestyle/markup fixes
4acb9d7	src/sage/misc/lazy_import.pyx (clean_namespace): Default for namespace like lazy_import

[nbruin]

comment:43

I think handling the more common cases like Partitions(3) first makes sense anyway. Calls to Partitions() are less likely, and Partitions(NN) even less common than that. Prioritizing more common call patterns in case determination of input is generally good. So I think the change to Partitions in 406e6f8 makes sense regardless of the import problems on NN.

[sagetrac-git]

Changed commit from 4acb9d7 to 05ea54e

[sagetrac-git]

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

05ea54e

src/sage/misc/lazy_import.pyx: Codestyle fixes

[sagetrac-git]

Changed commit from 05ea54e to 9837dec

[sagetrac-git]

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

9837dec

Change order of cases in `Partitions` to put cheaper tests first

[mkoeppe]

comment:46

Replying to Nils Bruin:

I think the change to Partitions in 406e6f8 makes sense regardless of the import problems on NN.

Done

[mkoeppe]

Author: Nils Bruin, Matthias Koeppe

[mkoeppe]

Reviewer: Matthias Koeppe, ...

[nbruin]

comment:50

+1 from me, but I think someone else than me should give a positive review: the changes here affect start-up procedures for sage, so it is a rather invasive change.

[mkoeppe]

comment:51

Yes, I have already reviewed your changes.

[nbruin]

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Nils Bruin

[vbraun]

Changed branch from u/mkoeppe/lazy_import_doesn_t_resolve_properly_when_indirectly_imported_ to 9837dec