lazy_import doesn't resolve properly when indirectly imported
Closed this issue · 61 comments
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).
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
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.
Description changed:
---
+++
@@ -42,4 +42,4 @@
sage: type(sage.calculus.calculus.maxima)
<class 'sage.interfaces.maxima_lib.MaximaLib'>
```
-So, the proposed fix: in `calculus.all`, import `maximalib` directly and lazily, rather than indirectly from `sage.calculus.calculus`. LazyImports can't handle indirect imports.
+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).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.
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.
Branch pushed to git repo; I updated commit sha1. New commits:
419e8cb | tool to clean misplaced lazy_imports plus some applications of it |
What is the actual problem? The first lines of the ticket description are very cryptic.
Description changed:
---
+++
@@ -1,4 +1,8 @@
-We have the problem:
+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
@@ -17,6 +21,8 @@
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 and upon each access this object will rebind `sage.calculus.calculus.maxima`. Thus, not only do we suffer from indirection overhead, we're even suffering extraneous dictionary modifications.
+
If instead we do:
```Replying to @videlec:
What is the actual problem? The first lines of the ticket description are very cryptic.
I tried to decrypt it.
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 |
Branch pushed to git repo; I updated commit sha1. New commits:
9911717 | clean sage.all and user_globals upon initialization |
rebased branch; now we just introduce the requisite helper routines in lazy_import and implement cleaning of sage.all and user_globals upon initialization. I think these are the two places where it's really hard to mandate that lazy_imports are done explicitly.
Similar cleaning operations could be placed in the construction of other namespaces too, but with a bit diminishing returns.
On 9.7.rc2 I get with:
from sage.misc.lazy_import import LazyImport,attributes #that's the helper
function
def misbound_lazies(S):
return [k for k,v in S.items() if type(v) is LazyImport
and attributes(v)['_namespace'] is not S
and attributes(v)['_namespace'] is not None]
M=[(k,len(misbound_lazies(m.__dict__))) for k,m in sys.modules.items() if m is not None]
the following result:
sage: [m for m in M if m[1] > 0]
[('sage.categories.groups', 1),
('sage.misc.all', 1),
('sage.combinat.integer_vector', 1),
('sage.rings.all', 15),
('sage.categories.all__sagemath_objects', 1),
('sage.categories.all', 1),
('sage.algebras.all', 7),
('sage.combinat.composition', 1),
('sage.graphs.path_enumeration', 1),
('sage.modular.arithgroup.congroup_generic', 1),
('sage.modular.all', 4),
('sage.schemes.all', 16),
('sage.groups.all', 9),
('sage.calculus.all', 1),
('sage.combinat.partition_tuple', 1),
('sage.combinat.partition', 1),
('sage.combinat.non_decreasing_parking_function', 1),
('sage.combinat.set_partition_ordered', 1),
('sage.combinat.integer_vectors_mod_permgroup', 1),
('sage.combinat.all', 33),
('sage.geometry.all', 4),
('sage.dynamics.all', 8),
('sage.tensor.all', 1),
('sage.all_cmdline', 426)]
That's not that bad! perhaps a lot of these can be resolved. On sage.all_cmdline we have the result of a from sage.all import *, but I think that dict only gets used for a initialize_globals, which may be a better place to fix this. We could clean sage.all_cmdline too, but if that namespace is never accessed, then the state of the shims isn't even relevant.
The other objects are perhaps worth investigating further. (Note that, for instance, structure/sage_object.pyx has some objects that have a None namespace. In that case, the shim wouldn't try to remove itself, so we shouldn't rewrite it either. clean_namespace already handles that properly)
The Build & Test run shows some failures related to NN (which IIRC has a strange lazy-import status) https://github.com/sagemath/sagetrac-mirror/actions/runs/3078385872/jobs/4973904579
Other than that, it looks good to me
I think we can add this call to clean_namespace to all the .all* modules. Not sure about the other modules that your script found
Replying to Matthias Köppe:
The Build & Test run shows some failures related to
NN(which IIRC has a strange lazy-import status) https://github.com/sagemath/sagetrac-mirror/actions/runs/3078385872/jobs/4973904579Other than that, it looks good to me
Oh cool! That's a textbook example of why LazyImport of anything else than a function is bound to cause trouble. See this line:
from sage.rings.semirings.all import NN
and then later the test:
if n is None or n is NN or n is NonNegativeIntegers():
so NN is a very specific LazyImport shim (that will not be resolved, not even if it gets used properly, because only its identity is used: attribute lookups never happen on it) and this code only ever worked because the global namespace had that exact shim. It never actually tested for the actual NN object.
So this is actually a separate bug of something that has never worked.
(I think NN was originally lazy_imported because it plays a lot of category shenanigans, which is very expensive for startup. Hence, it was felt its initialization should be delayed. But that means you end up with an object that has a meaningful identity, but that identity can change. In fact, if people feel it should have a LazyImport shim around it that delays its initialization, it should probably be one with namespace==None. Then at least its id will be consistent (but its type info will be wrong, and one would have to mandate that people only use the shim if they want to test for identity ...)
That's why we need NaturalNumbers() instead of NN [EDIT: scratch that! 0 is in NN in sage, apparently. Must be the french influence. The thing is actually called NonNegativeIntegerSemiring(). I guess "NN" for "NonNegative" is not a bad abbreviation :-)] if delayed init is important ... or custom-write the delayed init for NN so that identity and type info is correct(ish) from the start.
The current use in partitions is absolutely garbage.
Branch pushed to git repo; I updated commit sha1. New commits:
2d16678 | fix LazyImport shim problems in identity tests in partition.py |
Branch pushed to git repo; I updated commit sha1. New commits:
f27e980 | use `_get_object` so that an import actually gets triggered (attribute lookup is not sufficient) |
I think I don't understand the subtleties here. In partition.py, why not just from sage.rings.semirings.non_negative_integer_semiring import NN?
Replying to John Palmieri:
I think I don't understand the subtleties here. In
partition.py, why not justfrom sage.rings.semirings.non_negative_integer_semiring import NN?
That would be fine semantically, but it incurs the cost of importing NN at startup. As I recall, the reason why this is undesirable is because the facade shenanigans played is super-heavy on the category framework and costs a lot of initialization time. In order to avoid that import at startup, we can instead lazy import a constructor function. Calling the function will trigger the import, so n is NonNegativeIntegerSemiring() triggers the import, whereas n is NN with a lazily imported NN does not.
Alternatively, we could do a runtime import when NN is needed, but imports do have (had?) a measurable cost. I don't know if Partition needs to be efficient on construction.
I tried to localize the change as much as possible, fixing the original erroneous behaviour.
That makes sense, but maybe partitions should be lazily imported instead?
Replying to John Palmieri:
That makes sense, but maybe partitions should be lazily imported instead?
I'd say that's not the worst issue here. The problem is people apparently want NN in the top-level namespace without paying the price at startup time. But that means that NN starts out as a LazyImport shim. But the shim cannot be fully transparent: type(NN) and id(NN) will be wrong.
So routines that may get NN passed will have to be prepared to unpack the shim. That's a much more invasive change than the use of NonNegativeIntegerSemiring() to get a hold of a reference object in partitions.py itself, and it does not get alleviated by lazily importing Partitions.
Other solutions I see:
- incur the startup penalty and just have NN as a fully formed object
- customize NN to a class that has the right identity and type, but defers the expensive parts of its initialization until needed. At least for a while, things like matrices deferred the construction of their parents for efficiency reasons (the parents may never be needed and are expensive to construct!) so there's a precedent for optimizing this way
- do away with NN and have people call
NonNegativeIntegerSemiring()or some nicer-sounding alias if they need it. (apparently ZZ is way cheaper to construct)
To get an idea of the cost of initializing NN:
sage: %time NN(1)
CPU times: user 5.99 ms, sys: 11 µs, total: 6 ms
Wall time: 5.42 ms
1
sage: %time NN(1)
CPU times: user 22 µs, sys: 2 µs, total: 24 µs
Wall time: 27.7 µs
1
Can we spend something on the order 6ms (probably more on older machines) to remove a major headache? Just NOT making NN lazy would be by far the lowest effort solution. Then the singleton can just be available everywhere. One of the drawbacks: NN is hardly ever used, so we're losing 6ms to a rare scenario.
EDIT: it's not 6ms just for importing NN. This must be measuring a lot of initialization of the coercion framework as well. %time c=NN._get_object() triggers just the import and seems to take a little less than 2ms on this machine. See comment:30 below for more info
Normally that would definitely get the answer "No you can't have your constant premade. Construct it with a constructor when you need it." But given how fundamental NN is to the foundations of mathematics, I could see how people would be partial to having it available, in apparent analogy to ZZ, QQ that really do get used everywhere.
Description changed:
---
+++
@@ -21,7 +21,7 @@
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 and upon each access this object will rebind `sage.calculus.calculus.maxima`. Thus, not only do we suffer from indirection overhead, we're even suffering extraneous dictionary modifications.
+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:
It makes sense to me to just honestly import NN at the start, especially as you say, since it is so foundational to mathematics. Then it also makes sense to lazily import lots of other things, like the interfaces to various optional packages or other rarely used items — the more specialized or rarely used, the more we should try to avoid initializing them at startup. I would hope that the savings from lazily importing other things would outweigh the added costs of initializing NN.
Replying to John Palmieri:
It makes sense to me to just honestly import
NNat the start
+1
I think changing NN to be initialized (and hence normally imported everywhere rather than lazily) deserves its own ticket, because it has a whole bunch of performance assessment it will need. Once that and/or this ticket is closer to merge, we can figure out which one will be based on which (since the fix to Partitions here should probably be done differently if NN never comes in a LazyImport shim -- although the code proposed now would still be correct and probably doesn't cause much overhead compared to what Partitions is already doing).
To get a bit of an idea of the costs involved in Partitions (the following are each done in a fresh session):
It looks like NN isn't actually used by bare Partitions:
sage: NN
Non negative integer semiring
sage: %time _=Partitions()
CPU times: user 22 µs, sys: 2 µs, total: 24 µs
Wall time: 28.4 µs
sage: %time _=Partitions()
CPU times: user 24 µs, sys: 2 µs, total: 26 µs
Wall time: 29.6 µs
So it's actually crazy that Partitions(NN) is even supported. It's just slower, with the same result:
sage: %time c=Partitions(NN)
CPU times: user 1.85 ms, sys: 0 ns, total: 1.85 ms
Wall time: 1.62 ms
sage: NN
Non negative integer semiring
sage: %time c=Partitions(NN)
CPU times: user 171 µs, sys: 0 ns, total: 171 µs
Wall time: 185 µs
An argument forces its construction of NN to compare the argument to it:
sage: %time c=Partitions(4)
CPU times: user 2.01 ms, sys: 2 µs, total: 2.01 ms
Wall time: 1.98 ms
sage: NN
Non negative integer semiring
sage: %time c=Partitions(4)
CPU times: user 448 µs, sys: 47 µs, total: 495 µs
Wall time: 498 µs
sage: %time c=Partitions(5)
CPU times: user 156 µs, sys: 16 µs, total: 172 µs
Wall time: 175 µs
sage: %time c=Partitions(7)
CPU times: user 81 µs, sys: 8 µs, total: 89 µs
(and as you can see, there's a lot of other overhead as well on the first call -- but NN is pretty noticeable!)
In fact, the pure init of NN doesn't seem as expensive as 6ms quoted above. That must have a lot of coercion discovery on it too. It varies quite wildly (and you can't put it in a %timeit):
sage: type(NN)
<class 'sage.misc.lazy_import.LazyImport'>
sage: %time _=NN._get_object()
CPU times: user 730 µs, sys: 982 µs, total: 1.71 ms
Wall time: 1.56 ms
sage: type(NN)
<class 'sage.rings.semirings.non_negative_integer_semiring.NonNegativeIntegerSemiring_with_category'>
sage: type(NN)
<class 'sage.misc.lazy_import.LazyImport'>
sage: %time _=NN._get_object()
CPU times: user 1.8 ms, sys: 0 ns, total: 1.8 ms
Wall time: 1.66 ms
so it's probably more like 2ms than 6ms.
But a big take-away: supporting Partitions(NN) is just a way of having a slower way of constructing the same thing and generating more errors. The routine should just NOT take a parent as first argument: None works much better. As you can see, allowing n=NN as well as n=None (the default value) is actually very expensive. So not only is this causing a major headache, it's also a bad idea to have NN referenced in Partitions at all.
Branch pushed to git repo; I updated commit sha1. New commits:
b4f6f3d | Change order of cases in `Partitions` to put cheaper tests first |
By changing the order of testing in Partitions, we're at least ONLY triggering the import and construction of NN if it's likely necessary (i.e., only if n is not an integer AND not None -- thanks to shortcut evaluation for or.
# Analysis of misbound lazies
In sage.categories.groups:
from sage.categories.cartesian_product import CartesianProductsCategory, cartesian_product
and the definition there:
# Moved to avoid circular imports
lazy_import('sage.categories.sets_cat', 'cartesian_product')
so perhaps we can just change it to
from sage.categories.sets_cat import cartesian_product
sage.combinat.integer_vector : NN
sage.combinat.composition :
from sage.categories.cartesian_product import cartesian_product
sage.combinat.path_enumeration :
from sage.categories.cartesian_product import cartesian_product
sage.modular.arithgroup.congroup_generic :
from sage.groups.matrix_gps.all import MatrixGroup
where it is:
lazy_import('sage.groups.matrix_gps.finitely_generated', 'MatrixGroup')
sage.combinat.partition_tuple : NN
sage.combinat.partition : NN
sage.combinat.non_decreasing_parking_function : NN
sage.combinat.set_partition_ordered : cartesian_product
sage.combinat.integer_vectors_mod_permgroup : NN
Replying to Nils Bruin:
I think changing NN to be initialized (and hence normally imported everywhere rather than lazily) deserves its own ticket, because it has a whole bunch of performance assessment it will need.
Is there a ticket for this yet?
Replying to John Palmieri:
Is there a ticket for this yet?
No! Go ahead and make one! comment:34 is probably a good starting point. It looks like it's a relatively manageable problem (in the sense that it's not that many cases where lazy NN wrappers get sticky. However, it could still be that, as in partitions previously, some code uses the NN wrappers in such a way they don't get resolved (e.g., looking at their Id).
Okay, it's #34652.
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 |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Dropped workarounds for NN in partitions
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.
Branch pushed to git repo; I updated commit sha1. New commits:
05ea54e | src/sage/misc/lazy_import.pyx: Codestyle fixes |
Branch pushed to git repo; I updated commit sha1. New commits:
9837dec | Change order of cases in `Partitions` to put cheaper tests first |
Replying to Nils Bruin:
I think the change to
Partitionsin 406e6f8 makes sense regardless of the import problems onNN.
Done
Author: Nils Bruin, Matthias Koeppe
Reviewer: Matthias Koeppe, ...
+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.
Yes, I have already reviewed your changes.
Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Nils Bruin
Changed branch from u/mkoeppe/lazy_import_doesn_t_resolve_properly_when_indirectly_imported_ to 9837dec