Prepare use of sage.all in sage.structure.factory.lookup_global for modularized distributions
Opened this issue · 4 comments
This function is problematic when sage.all is not available (in modularized subset distributions):
def lookup_global(name):
"""
Used in unpickling the factory itself.
EXAMPLES::
sage: from sage.structure.factory import lookup_global
sage: lookup_global('ZZ')
Integer Ring
sage: lookup_global('sage.rings.all.ZZ')
Integer Ring
"""
name = bytes_to_str(name, encoding='ASCII')
try:
return factory_unpickles[name]
except KeyError:
pass
if '.' in name:
module, name = name.rsplit('.', 1)
all = __import__(module, fromlist=[name])
else:
import sage.all as all
return getattr(all, name)
CC: @kliem
Component: pickling
Issue created by migration from https://trac.sagemath.org/ticket/32586
It looks to me you need here the "sage global namespace". In a situation where sagelib is really just purely a python package, there is no such thing. So this code would need to be disabled in that case.
Alternatively, you build something that acts as a substitute for the "sage global namespace". Candidates: sage.repl.user_globals or, as you can see in sage.structure.category_object.CategoryObject.inject_variables you can use a hack that uses that cython code doesn't normally construct a stack frame, so that globals() in cython code refers to the globals of the callsite, not the global namespace of the module in which the routine is implemented. Those could possibly be reasonable substitutes. The idea, though, is that sage.all is the default namespace, so perhaps you don't want things to be shadowed by user redefinitions. In fact, in pickling that would be a recipe for disaster.
Quite frankly, in a modularized setup, pickles will be disastrous anyway, since this sage.all namespace will be dependent on what parts are installed at best. One solution would be to let
sage.structure.factory.lookup_global depend on the top sage package. Then the complete namespace is reliably there.
Have we found a cycle in the dependency graph that makes sagelib monolithic yet?
Replying to @nbruin:
Quite frankly, in a modularized setup, pickles will be disastrous anyway, since this
sage.allnamespace will be dependent on what parts are installed at best.
Well, the idea would be that unpickling can lead to an error. We already have good infrastructure (lazy_import with "feature") to give users advice on what package to install to get this functionality as part of the error message
Replying to @nbruin:
Have we found a cycle in the dependency graph that makes sagelib monolithic yet?
Lots! And fixed many of them already back last year in the 9.2 development cycle. See #29869, #29873, #29892, #29883, #16351, #29881, #29880, #29916, #29922
The dependencies can exist on 3 levels: Compile time, module-import time, run time. Cycles can arise at module-import time and run time.
In #29865 I have two subset distributions, sagemath-objects, sagemath-categories. By building them, one can verify that changes in Sage do not re-introduce module-import-time cycles, or module import dependencies of lower level modules on higher level modules.
In #32432 I'm prototyping another subset distribution, sagemath-polyhedra, with the goal of actually being able to pass the majority of doctests.
Replying to @nbruin:
The idea, though, is that
sage.allis the default namespace, so perhaps you don't want things to be shadowed by user redefinitions. In fact, in pickling that would be a recipe for disaster.
Thanks for the explanation, yes, I agree. So it should really be sage.all (or a subset of it -- leading to runtime errors, comment:2)
In sagemath-objects, sagemath-categories, sagemath-polyhedra, I have introduced modules sage.all__sagemath_objects, sage.all__sagemath_categories, sage.all__sagemath_polyhedra that provide the respective subsets of the global bindings. So I guess for now, if sage.all cannot be imported, I would just fall back to importing from a set of these modules. This would be quite similar to what you suggested in https://groups.google.com/g/sage-devel/c/T0A4JCOg9DY/m/yybWDYd6BAAJ, but for just this specific use case