Import NN directly rather than lazily throughout the Sage library
Closed this issue · 22 comments
This is extracted from #16522.
One issue raised on that ticket is that it is problematic to use lazy imports on things other than a function (stated in #16522 comment:20). It appears that using honest imports rather than lazy ones for NN should not cause too much of a slowdown, but this needs to be tested.
Component: misc
Author: John Palmieri
Branch/Commit: 539119b
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/34652
See #16522 comment:34 for some imports that should be changed.
Something strange is going on. If I make just the change
diff --git a/src/sage/rings/semirings/all.py b/src/sage/rings/semirings/all.py
index 9159407408..b150b55ebc 100644
--- a/src/sage/rings/semirings/all.py
+++ b/src/sage/rings/semirings/all.py
@@ -1,6 +1,3 @@
-
-from sage.misc.lazy_import import lazy_import
-lazy_import('sage.rings.semirings.non_negative_integer_semiring',
- ['NonNegativeIntegerSemiring', 'NN'])
+from .non_negative_integer_semiring import NN
from .tropical_semiring import TropicalSemiringthen I get a warning upon starting Sage:
/Users/jpalmier/Desktop/Sage/git/sage/src/sage/sets/non_negative_integers.py:83: UserWarning: Resolving lazy import FacadeSets during startup
Parent.__init__(self, facade = ZZ, category = InfiniteEnumeratedSets().or_subcategory(category) )
Mathematically there is an obvious connection between sage/sets/non_negative_integers.py and sage/rings/semirings/non_negative_integer_semiring.py, but from the code point of view, I don't see why this change causes this warning.
With the change above and some other changes (changing imports of NN from sage.rings.semirings.all to the sage.rings.semirings.non_negative_integer_semiring), the startup time may have gone up slightly. I ran ./sage --startuptime about half a dozen times without the change and then with the change. The average before was 949.5ms, the average after was 957.5ms. So an increase of 8ms. In my opinion that's okay.
I agree, we should not worry about that increase
Replying to John Palmieri:
Mathematically there is an obvious connection between
sage/sets/non_negative_integers.pyandsage/rings/semirings/non_negative_integer_semiring.py, but from the code point of view, I don't see why this change causes this warning.
The line in non_negative_integer_semiring.py:
from sage.sets.non_negative_integers import NonNegativeIntegers
probably explains the link.
The semiring class inherits from it and the __init__ chains to it. So that's a programmatic link. Apparently, something in the category framework for FacadeSets lazy-imports these and the initialization of NN triggers the resolution; now prior to conclusion of start-up. So that suggests there's a further lazy import that should be scuttled, because it now gets resolved upon startup anyway. I think that was the costly bit before, but by the looks of it the cost may not be so high anymore. Python's import framework has improved over the years, if I'm not mistaken and/or you've tested on a filesystem that has very little overhead when warmed up.
in fact, as far as I can find, it happens in sets_cat.py in class Sets(Category_singleton); line 1882. There's a whole bunch of class-namespace-level LazyImports there; some of them marked at_startup, which should silence the warning. You might want to double-check why those are lazy: is it to solve some otherwise circular import problem?
I suspect the trigger that actually resolves the lazy shim here is self._with_axiom('Facade'), which is probably executed somewhere in creating the category for NN or the semiring. If I recall correctly, this is one of the points where the category framework really ceases to by Python and instead is some other kind of programming language bolted onto Python infrastructure. It's certainly the kind of shenanigans that makes me never want to get near it.
Nils, thanks for the pointer to the lines in sets_cat.py. Changing those fixed the problem.
Here's a branch. I'm at home working on a slower machine, and I don't see any differences in startup time between this and my develop branch. I made limited changes here, focusing almost completely on NN. Other lazy import issues should be handled on other tickets.
Tests might pass, although I just upgraded to the latest MacOS so I am seeing failures. They should be unrelated to these changes. Please test it.
New commits:
24c56c5 | trac 34652: import NN directly rather than lazily |
Commit: 24c56c5
Author: John Palmieri
Okay, if I suppress the warning messages "ld: warning: dylib ... was built for newer macOS version (...) than being linked (...)", then all doctests pass.
The changes from lazy to eager ...
--- a/src/sage/categories/sets_cat.py
+++ b/src/sage/categories/sets_cat.py
@@ -1878,13 +1878,13 @@
cls = ImageSubobject
return cls(self, domain_subset)
- Facade = LazyImport('sage.categories.facade_sets', 'FacadeSets')
+ from sage.categories.facade_sets import FacadeSets as Facade
... break a modularization test (sagemath-objects) - see
https://github.com/sagemath/sagetrac-mirror/actions/runs/3334274863/jobs/5517058687
(sagemath-objects does not ship facade_sets; that could be changed in pkgs/sagemath-objects/MANIFEST.in, pkgs/sagemath-categories/MANIFEST.in.m4 if necessary.)
We could also try the import and except ModuleNotFoundError: do a lazy import instead. I don't know the right approach.
I think adding facade_sets to sagemath-objects is well motivated; facade parents are an aspect of the parent/element framework.
However, I wouldn't like to do the same change with metric_spaces.
I don't mind changing the metric_spaces import back to being lazy. That shouldn't affect NN, it was just something that was convenient to change at the same time.
(Not sure if I made the right changes to the manifests.)
LGTM, let's wait for the Build&Test action
Reviewer: Matthias Koeppe
Thanks!
Changed branch from u/jhpalmieri/trac34652-actively-import-NN to 539119b