lazy_import breaks CachedRepresentation

[cheuberg]

NN is a lazy_import and therefore we have

sage: NonNegativeIntegerSemiring() == NN
False

but

sage: NN == NonNegativeIntegerSemiring()
True
sage: NonNegativeIntegerSemiring() == NN._get_object()
True

This gives problems with CachedRepresentation:

sage: from sage.misc.lazy_import import LazyImport
sage: lazyZZ = LazyImport('sage.rings.integer_ring', 'ZZ')
sage: PolynomialRing(lazyZZ, 'x') is PolynomialRing(ZZ, 'x')
False

Depends on #23102

CC: @vbraun @jdemeyer @mezzarobba @simon-king-jena @nthiery

Component: coercion

Author: Jeroen Demeyer

Branch: 866eddd

Reviewer: Erik Bray

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

[cheuberg]

comment:1

The attached branch is the one which exhibits the error.

[jdemeyer]

Changed dependencies from #19152 to none

[jdemeyer]

comment:9

Should we special-case LazyImport in WithEqualityById?

[nbruin]

comment:12

There are many subtle problems with lazy_import and some of them are fairly fundamental. The basic take-away is: don't lazy_import objects, but lazy_import functions that produce objects (NN is problematic but NonNegativeIntegerSemiring() is not).

LazyImport objects try to be as transparent as possible, but obviously won't succeed perfectly. They try to remove themselves once non-trivially accessed (that is, basically when one of their attributes gets accessed), and essentially LazyImport objects should only be used to look up attributes on.

Many of our LazyImport objects get further imported erroneously. For instance, importing a LazyImport object from elsewhere via a from ... import <LazyImportObject> doesn't work properly, because the LazyImport object doesn't get an opportunity to know about the new namespace in which it gets imported. An example is NN as referenced here.

If we insert a little helper function into sage.misc.lazy_import (it has to be there because lazy_import doesn't offer a pxd):

def att(a):
    cdef LazyImport b
    b = a
    return {"_object": b._object,
            "_module": b._module,
            "_name": b._name,
            "_as_name": b._as_name,
            "_namespace": b._namespace,
            "_at_startup": b._at_startup,
            "_deprecation": b._deprecation}

then you can see from

sage: sage.misc.lazy_import.att(NN)['_namespace']['__name__']
'sage.rings.semirings.all'
sage: sage.misc.lazy_import.att(NN)['_name']
'NN'
sage: sage.misc.lazy_import.att(NN)['_as_name']
'NN'

that accessing NN in the toplevel scope will never resolve. On every access, the LazyImport object will dutifully search the sage.rings.semirings.all scope to replace its occurrence there (which only happens the first time of course), but that's not the binding through which it gets accessed afterwards.

If instead you do (and this is how NN should be imported into the toplevel if at all):

sage: lazy_import(sage.misc.lazy_import.att(NN)['_module'], sage.misc.lazy_import.att(NN)['_name'])
sage: type(NN)
<type 'sage.misc.lazy_import.LazyImport'>
sage: NN
Non negative integer semiring
sage: type(NN)
<class 'sage.rings.semirings.non_negative_integer_semiring.NonNegativeIntegerSemiring_with_category'>

As you can see, basically even

from sage.rings.semirings.non_negative_integer_semiring import NN

is an erroneous use of a LazyImport object, because it does not amount to attribute lookup.

The reason why mathematically interesting objects themselves should probably not be lazily imported, but only their access functions is because

sage: { NN : 1 }

will always be problematic: The LazyImport object doesn't get a chance to remove itself.

On the other hand, accessing the object via NonNegativeIntegerSemiring() works nicely.

Of course, NonNegativeIntegerSemiring itself is incorrectly imported.

We could clean up scopes (but that should happen in basically all __all__ files etc.) via something along the lines of:

scope=globals()
for name,value in scope.iteritems():
    if isinstance(value,sage.misc.lazy_import.LazyImport):
        T= sage.misc.lazy_import.att(value)
        if T['_namespace'] is not scope:
            scope[name] = sage.misc.lazy_import.LazyImport(T['_module'],T['_name'],T['_as_name'],scope,T['_at_startup'],T['_deprecation'])

Rather than hobbling EqualityById to accommodate for the hack that is LazyImport, we should avoid erroneous use of LazyImport. The problem with EqualityById is just a symptom of a general problem. There are many tickets about this, see e.g. #12482

[jdemeyer]

comment:13

Replying to @nbruin:

We could clean up scopes (but that should happen in basically all __all__ files etc.)

Of course, now the question becomes: which scopes? If you can hand me a list of dicts which contain all relevant lazy imports, we can indeed do something.

There are many tickets about this, see e.g. #12482

Thanks for the pointer, I fixed this.

[jdemeyer]

comment:14

One thing which we could easily do is to change cached_function() and friends to automatically dereference lazy imports. This will fix at least the problem with CachedRepresentation I think.

[jdemeyer]

Changed commit from b21aa6e to none

[jdemeyer]

Changed branch from u/cheuberg/19152-arb-misc-lazy-import to none

[vbraun]

comment:16

Still if we agree that it can't be done safely then maybe it's a bad idea?

An alternative might be to have a proxy object that, instead of trying to look like the original object, relies on the preparser to replace it? E.g. the preparser replaces NN with NonNegativeIntegerSemiring(). And library code would be forced to import from the original location.

[jdemeyer]

comment:18

Replying to @vbraun:

Still if we agree that it can't be done safely then maybe it's a bad idea?

What do the two "it"s refer to in this sentence?

Fixing cached functions can be done safely. We already pre-process arguments to cached functions in src/sage/misc/function_mangling.pyx, so it's not hard to also dereference lazy imports there. Of course this does not fix every single use-case of lazy imports, but this is still an important use-case.

Automatically "fixing" lazy imports in globals()-like dicts can be done, but there the hard problem is to determine which dicts should be processed.

An alternative might be to have a proxy object that, instead of trying to look like the original object, relies on the preparser to replace it? E.g. the preparser replaces NN with NonNegativeIntegerSemiring().

I'm not convinced that we want to involve the preparser for this. What if somebody writes

sage: NN = some_completely_unrelated_object
sage: f(NN)

you don't want this to be replaced by

sage: NonNegativeIntegerSemiring() = some_completely_unrelated_object
sage: f(NonNegativeIntegerSemiring())

[nbruin]

comment:19

Replying to @jdemeyer:

Of course, now the question becomes: which scopes? If you can hand me a list of dicts which contain all relevant lazy imports, we can indeed do something.

All scopes that from ... import a symbol that is a LazyImport. That includes many "all" files. Each of those should basically finish with a

sage.misc.lazy_import.fix_sloppily_constructed_namespace(globals())

(and then we should hope that no fancy shenanigans have happened with the bindings during module initialization)

We should consider how badly this affects startup times. In time-critical bits, it may be better to use a fresh lazy_import command instead instead of fixing the namespace afterwards.

One could run this process on [m for m in sys.modules().itervalues() if m is not None] (there are quite some entries in there that are None. I don't know what they're doing there. Vanilla python has that too), but we'd really be meddling with data that isn't ours, so I'd expect that to lead to more fragileness.

The namespaces registered under sys.modules() would certainly provide a good starting point for assessing the scale of the problem.

[jdemeyer]

comment:28

Replying to @nbruin:

You'd have to reach too deeply into python

Are you thinking of ma_lookup()? That could work, but I agree that it would be a major hack.

[nbruin]

comment:29

Replying to @jdemeyer:

Are you thinking of ma_lookup()? That could work, but I agree that it would be a major hack.

That would be one hook. But there's more.

sage: lazy_import("sage.rings.integer_ring","ZZ")
sage: id(ZZ)
140457737914168
sage: ZZ
Integer Ring
sage: id(ZZ)
140458433423744

Note that the user didn't rebind ZZ and yet its identity has changed. That can affect all kinds of things. You'd basically need LazyImport to be transparent for being put into argument lists as well. And that's just horrible (and undebuggable!). I think it would be a major maintainability penalty to try and support that kind of stuff, for relatively little gain. I would say the minor inconvenience for users that objects supported by code that is lazily imported cannot be available straight away in the top namespace is preferable.

But perhaps your experiments end up indicating something else.

[jdemeyer]

comment:30

Replying to @nbruin:

I would say the minor inconvenience for users that objects supported by code that is lazily imported cannot be available straight away in the top namespace is preferable.

So, what's your proposal then? Close this ticket as wontfix and change the lazy_import of NN (and similar objects) to a "real" import?

But perhaps your experiments end up indicating something else.

Which "experiments" do you mean? You mean with ma_lookup()?

[nbruin]

comment:31

Replying to @jdemeyer:

So, what's your proposal then? Close this ticket as wontfix and change the lazy_import of NN (and similar objects) to a "real" import?

Yes. Or if NN is too expensive to import at startup, adapt the places where NN is accessed (the count above shows about 10 modules (including __main__ and I think some double counting) that reference it via a lazy import) in a way that triggers the import as soon as NN is accessed (i.e., either by accessing NN through a namespace that's lazily imported or via an accessor function). I don't see a reasonable alternative while maintaining a semantics model that at least resembles that of python.

The other thing we should do is clean up the mess of mis-lazy_imported objects. As the list above shows, a lot of lazy_imports don't have a chance of ever clearing themselves up. The penalty of accessing an object through a lazy_import isn't that high, but the problem is these things compound. We already have lazy_imports that point to lazy_imports.

I propose:

• adding a function that gives access to the lazy_import cdef attributes (probably just by returning a dict that has their values)
• including a doctest that tests if lazy_import objects in sys.modules dicts reference their own scope.

but perhaps that's better handled on a different ticket.

[jdemeyer]

Branch: u/jdemeyer/lazy_import_breaks_cachedrepresentation

[jdemeyer]

Commit: d501601

[jdemeyer]

New commits:

d501601

WithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

[jdemeyer]

Author: Jeroen Demeyer

[sagetrac-git]

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

2f5aead

WithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

[sagetrac-git]

Changed commit from d501601 to 2f5aead

[jdemeyer]

Dependencies: #23102

[sagetrac-git]

Changed commit from 2f5aead to 759da06

[sagetrac-git]

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

256bf19	Move richcmp stuff to new file richcmp.pyx
b591b78	Support `__richcmp__` in Python classes
47f97ec	Implement RealSet comparisons using __richcmp__
acfd0aa	Typo
759da06	WithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

[sagetrac-git]

Changed commit from 759da06 to 866eddd

[sagetrac-git]

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

49b6f76	Support `__richcmp__` in Python classes
d343d9d	Implement RealSet comparisons using __richcmp__
3aac3cc	Typo
86e9fcf	Install slot wrappers for rich comparison methods
4d1021b	Further fixes and tests for richcmp_method
866eddd	WithEqualityById.__richcmp__: bail out if "other" is no instance of WithEqualityById

[embray]

comment:41

I haven't read all the comments on this ticket, but the problem statement is clear and the proposed fix makes perfect sense.

[embray]

Reviewer: Erik Bray

[vbraun]

Changed branch from u/jdemeyer/lazy_import_breaks_cachedrepresentation to 866eddd

[fchapoton]

Changed commit from 866eddd to none