Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare use of sage.all in sage.structure.factory.lookup_global for modularized distributions #32586

Open
mkoeppe opened this issue Sep 29, 2021 · 4 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 29, 2021

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Sep 29, 2021
@nbruin
Copy link
Contributor

nbruin commented Sep 29, 2021

comment:1

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?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2021

comment:2

Replying to @nbruin:

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.

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2021

comment:3

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 29, 2021

comment:4

Replying to @nbruin:

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.

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

@mkoeppe mkoeppe changed the title Eliminate use of sage.all in sage.structure.factory.lookup_global Prepare use of sage.all in sage.structure.factory.lookup_global for modularized distributions Sep 29, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 14, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe modified the milestones: sage-10.0, sage-10.1 Apr 30, 2023
@mkoeppe mkoeppe removed this from the sage-10.1 milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants