diff --git a/actions/generate_recipe.py b/actions/generate_recipe.py index 90b5dc3e2..62e5d6ce8 100644 --- a/actions/generate_recipe.py +++ b/actions/generate_recipe.py @@ -37,7 +37,6 @@ RUN_DEPS = { "gsl", - "lazy_import", "libnetcdf", "openmm", "pandas", diff --git a/doc/source/acknowledgements.rst b/doc/source/acknowledgements.rst index 104c8a9da..bd48a7f1a 100644 --- a/doc/source/acknowledgements.rst +++ b/doc/source/acknowledgements.rst @@ -475,12 +475,6 @@ The header documentation reads; imshow does not plot axis yet. make a correct documentation -lazy_import ------------ - -:mod:`sire` uses `lazy_import `__ to -lazy load the modules. This is licensed under the GPLv3. - rich ---- diff --git a/doc/source/changelog.rst b/doc/source/changelog.rst index 7365b4623..b01fafe49 100644 --- a/doc/source/changelog.rst +++ b/doc/source/changelog.rst @@ -17,6 +17,11 @@ organisation on `GitHub `__. * Please add an item to this CHANGELOG for any new features or bug fixes when creating a PR. +* Replaced the third-party ``lazy_import`` dependency (GPLv3) with a minimal, standard-library-only + ``importlib``-based implementation in ``sire.utils._lazy_import``. This also fixes a bug where + lazily-loaded modules could end up with two distinct class objects for the same module path + (e.g. via unpickling in a separate process), causing spurious ``isinstance()`` failures. + * Fixed ``sire.restraints.boresch()`` setting a dynamic ``_use_pbc`` Python attribute on the returned ``BoreschRestraints`` instead of calling ``set_uses_pbc()``, which broke pickling (e.g. for ``multiprocessing``/``ProcessPoolExecutor``) and meant the flag did diff --git a/pixi.toml b/pixi.toml index f0b424f23..99a68f992 100644 --- a/pixi.toml +++ b/pixi.toml @@ -12,7 +12,6 @@ cmake = ">=3.30.0" git = "*" pybind11 = "*" gsl = "*" -lazy_import = "*" libboost-devel = "*" libboost-python-devel = "*" libcblas = "*" diff --git a/src/sire/CMakeLists.txt b/src/sire/CMakeLists.txt index 413ddea52..7d8a0c36b 100644 --- a/src/sire/CMakeLists.txt +++ b/src/sire/CMakeLists.txt @@ -115,7 +115,7 @@ add_subdirectory (vol) install( FILES __init__.py _load.py _match.py _parallel.py _pythonize.py - _measure.py _colname.py + _measure.py _colname.py _lazy_import.py DESTINATION ${SIRE_PYTHON}/sire ) diff --git a/src/sire/__init__.py b/src/sire/__init__.py index 7ec14b306..92488d7b7 100644 --- a/src/sire/__init__.py +++ b/src/sire/__init__.py @@ -803,56 +803,77 @@ def _convert(id): __repository__ = config.sire_repository_url __revisionid__ = config.sire_repository_version[0:7] -_can_lazy_import = False - -if "SIRE_NO_LAZY_IMPORT" not in _os.environ: - try: - import lazy_import as _lazy_import - import logging as _logging - - _logger = _logging.getLogger("lazy_import") - _logger.setLevel(_logging.ERROR) - - # Previously needed to filter to remove excessive warnings - # from 'frozen importlib' when lazy loading. - # import warnings - # warnings.filterwarnings("ignore") - - _can_lazy_import = True - - except Exception as e: - print("Lazy import disabled") - print(e) - _can_lazy_import = False +from ._lazy_import import lazy_module as _lazy_module +_can_lazy_import = "SIRE_NO_LAZY_IMPORT" not in _os.environ # Lazy import the modules for speed, and also to prevent pythonizing them -# if the users wants to run in legacy mode +# if the users wants to run in legacy mode. +# +# _lazy_module() doesn't execute anything regardless of call order - it +# just registers a stub - so the order here is cosmetic; kept alphabetical. +# See the eager fallback below for the one case where order does matter. if _can_lazy_import: - analysis = _lazy_import.lazy_module("sire.analysis") - base = _lazy_import.lazy_module("sire.base") - cas = _lazy_import.lazy_module("sire.cas") - convert = _lazy_import.lazy_module("sire.convert") - cluster = _lazy_import.lazy_module("sire.cluster") - error = _lazy_import.lazy_module("sire.error") - ff = _lazy_import.lazy_module("sire.ff") - id = _lazy_import.lazy_module("sire.id") - io = _lazy_import.lazy_module("sire.io") - maths = _lazy_import.lazy_module("sire.maths") - mm = _lazy_import.lazy_module("sire.mm") - mol = _lazy_import.lazy_module("sire.mol") - morph = _lazy_import.lazy_module("sire.morph") - move = _lazy_import.lazy_module("sire.move") - options = _lazy_import.lazy_module("sire.options") - qm = _lazy_import.lazy_module("sire.qm") - qt = _lazy_import.lazy_module("sire.qt") - restraints = _lazy_import.lazy_module("sire.restraints") - search = _lazy_import.lazy_module("sire.search") - squire = _lazy_import.lazy_module("sire.squire") - stream = _lazy_import.lazy_module("sire.stream") - units = _lazy_import.lazy_module("sire.units") - utils = _lazy_import.lazy_module("sire.utils") - vol = _lazy_import.lazy_module("sire.vol") + analysis = _lazy_module("sire.analysis") + base = _lazy_module("sire.base") + cas = _lazy_module("sire.cas") + cluster = _lazy_module("sire.cluster") + convert = _lazy_module("sire.convert") + error = _lazy_module("sire.error") + ff = _lazy_module("sire.ff") + id = _lazy_module("sire.id") + io = _lazy_module("sire.io") + maths = _lazy_module("sire.maths") + mm = _lazy_module("sire.mm") + mol = _lazy_module("sire.mol") + morph = _lazy_module("sire.morph") + move = _lazy_module("sire.move") + options = _lazy_module("sire.options") + qm = _lazy_module("sire.qm") + qt = _lazy_module("sire.qt") + restraints = _lazy_module("sire.restraints") + search = _lazy_module("sire.search") + squire = _lazy_module("sire.squire") + stream = _lazy_module("sire.stream") + system = _lazy_module("sire.system") + units = _lazy_module("sire.units") + utils = _lazy_module("sire.utils") + vol = _lazy_module("sire.vol") +else: + # SIRE_NO_LAZY_IMPORT is set - import everything eagerly instead, so + # these are still bound as expected. Ordered to match + # _pythonize.py's _load_new_api_modules() (base first, then move, io, + # system, squire, mm, convert, ff, mol, analysis, cas, cluster, error, + # id, maths, morph, restraints, qt, stream, units, vol), with the + # modules pythonize doesn't force-load (options, qm, search, utils) + # appended at the end. + from . import ( + base, + move, + io, + system, + squire, + mm, + convert, + ff, + mol, + analysis, + cas, + cluster, + error, + id, + maths, + morph, + restraints, + qt, + stream, + units, + vol, + options, + qm, + search, + utils, + ) def _version_string(): diff --git a/src/sire/_lazy_import.py b/src/sire/_lazy_import.py new file mode 100644 index 000000000..881d58d54 --- /dev/null +++ b/src/sire/_lazy_import.py @@ -0,0 +1,578 @@ +""" +A minimal, standard-library-only replacement for the third-party +`lazy_import` package (GPLv3), used to defer importing submodules until +they are actually used. Package-agnostic: nothing here assumes anything +about the particular package it's wrapping, so the same module can be +(and is) shared verbatim between sire and BioSimSpace. + +This is a private module - not part of the public API, just an internal +bootstrapping helper. +""" + +import importlib.machinery as _importlib_machinery +import importlib.util as _importlib_util +import pkgutil as _pkgutil +import sys as _sys +import threading as _threading +import types as _types + +__all__ = ["lazy_module", "is_lazy_module", "force_load"] + + +class _LazyModule(_types.ModuleType): + """ + A stand-in for a module that hasn't been imported yet. The first + time any real attribute is accessed (or force_load() is called), the + actual module is imported and this proxy is replaced everywhere it's + reachable (sys.modules, and the attribute of whatever parent package + holds it) with the real thing. + + Known limitation: this replacement only reaches places that look the + name up again later (sys.modules, or a parent's own attribute) - a + name already bound to *this* stub before the load happened (e.g. + `import pkg.sub as x`, which binds `x` to whatever was in + sys.modules at that moment) keeps pointing at the stub forever. + Reads through that binding still work, via __getattr__ delegating to + the real module below - but a write (`x.SOME_ATTR = value`) lands on + the stub's own __dict__ instead, invisible to sys.modules["pkg.sub"] + or anyone else holding a reference obtained afterwards. Narrow (it + needs a pre-load `as` import specifically, then a write, not just a + read), but worth knowing: it's the same two-objects-one-name shape + this module exists to eliminate, just for writes through a stale + pre-load reference rather than for reads or for class identity. + """ + + def __init__(self, name: str, search_locations=None, spec=None): + super().__init__(name) + + # Populate the standard module attributes (__file__, __path__, + # __loader__, __spec__, __package__) straight from the spec, if we + # have one, exactly as a real (non-lazy) module would have them. + # This matters because plenty of code that has no interest in this + # module's contents still checks for these (e.g. inspect.getmodule() + # scans *every* entry in sys.modules doing hasattr(module, + # '__file__') while resolving a source location for something else + # entirely - seen for real via a third-party library's use of + # inspect deep inside an unrelated import). Without this, that one + # hasattr() call would fall through to __getattr__ below and force + # a full, real load - of every single lazily-registered module in + # the process, all at once, at whatever arbitrary moment such a + # scan happens to run. + # + # These are read directly off the spec, not by building a + # module_from_spec(spec) and copying its __dict__: ModuleType's + # own __init__() above has already pre-seeded __spec__/__loader__/ + # __package__/__doc__ as real dict entries set to None, so copying + # via dict.setdefault() (which a template's __dict__ would need, + # to avoid clobbering __name__) would silently no-op for exactly + # those three. And separately, module_from_spec() calls the + # loader's create_module(), which for a single-phase-init C + # extension (what Boost.Python wrappers are) actually runs the + # module's init function - dlopening it and executing its C-level + # registration code. Building a template purely to copy attributes + # off it would do that at registration time, for every compiled + # submodule reachable in the lazy tree, then a second time when + # something actually imports it properly - exactly the "twice is + # corruption" scenario the lock elsewhere in this class exists to + # prevent. Reading spec attributes directly avoids calling + # module_from_spec() at registration time at all. + if spec is not None: + self.__dict__["__spec__"] = spec + self.__dict__["__loader__"] = spec.loader + self.__dict__["__package__"] = spec.parent + + # spec.has_location, not "spec.origin is not None" - that's + # what CPython's own _init_module_attrs uses to decide this, + # since some loaders put a non-path marker in origin (e.g. + # 'frozen', 'built-in') rather than leaving it None. Not + # reachable via PathFinder for anything in sire's own tree, + # but __file__ is exactly what the inspect.getmodule + # protection above leans on - a wrong value there is worse + # than a merely absent one. + if spec.has_location: + self.__dict__["__file__"] = spec.origin + + if spec.submodule_search_locations is not None: + # list(...) here deliberately freezes a namespace + # package's _NamespacePath at this moment, rather than + # leaving it as the dynamic, sys.path-tracking object a + # real import would use - fine for sire (no namespace + # packages in play), but worth knowing if that ever + # changes. + self.__dict__["__path__"] = list(spec.submodule_search_locations) + + # stored directly in __dict__, so accessing these never itself + # goes through __getattr__ below (which would recurse) + self._lazy_name = name + # Keep the spec we were already handed (both call sites below - + # _register_submodules() and lazy_module() - already had to look + # it up to get this far) so _load() can reuse it directly instead + # of doing a second, redundant find_spec() call at load time. See + # _load() below for the one behavioural consequence of this: the + # spec is now pinned at registration time rather than re-resolved + # at first touch. + self._lazy_spec = spec + # The real, fully-executed module, once _load() has completed - not + # just "have we started loading" (see _load() below for why that + # distinction matters). + self._lazy_real = None + # Set to the loading thread's ident for the duration of _load()'s + # actual work (spec lookup through exec_module()), and back to None + # once it's done (successfully or not) - lets a *reentrant* call + # from the same thread (a circular import touching this module from + # inside its own exec_module()) through without deadlocking, while + # still blocking any *other* thread until the load truly finishes. + self._lazy_owner = None + # If known (i.e. this is a submodule registered by + # _register_submodules()), the parent's search path - see _load() + # below for why this needs to be captured up front rather than + # derived when the module actually loads. + self._lazy_search_locations = search_locations + # Immediate children pre-registered under this module's name by + # _register_submodules() - recorded up front purely so _load()'s + # post-exec fixup below has an exact, cheap list to walk, rather + # than needing to scan the whole of sys.modules (which can run to + # hundreds of entries) on every single module load. + self._lazy_children = [] + # guards _load() below - without this, two threads racing to + # first-touch the same lazy module could each build and exec_module() + # their *own* separate module object concurrently, corrupting + # whatever global C++-side registration state that module's import + # touches (RegisterMetaType and friends aren't designed to run + # twice at once) + self._lazy_lock = _threading.Lock() + + def _load(self): + # Fully loaded already - safe to fast-path with no lock, since + # _lazy_real is only ever set (by the thread that did the loading) + # *after* exec_module() has completely finished (see below), so + # every other thread reading it here is guaranteed to see a fully + # executed module, never a partial one. + if self._lazy_real is not None and self._lazy_owner is None: + return self._lazy_real + + # Reentrant call from the thread currently doing the load (a + # circular import reached back into this module from inside its + # own exec_module()) - hand back the partially-initialised module, + # exactly what CPython itself does for ordinary circular imports. + # Taking the lock here would deadlock against ourselves. + if self._lazy_owner == _threading.get_ident(): + if self._lazy_real is None: + # Re-entered before the module object itself exists yet - + # only reachable between this thread entering the lock + # below and self._lazy_real being set further down, a + # window module_from_spec()'s own create_module() can + # reach into for a single-phase-init C extension (its + # module init function can run arbitrary C-level code, + # including imports, before Python-level exec even + # starts). There's nothing to hand back yet - and nothing + # useful to build, since a second, independent module + # object here would defeat the point of this whole class + # - so raise something a caller stands a chance of + # diagnosing, rather than the AttributeError on None that + # returning self._lazy_real as-is would produce a few + # lines down. + raise ImportError( + f"circular import: {self._lazy_name!r} was re-entered " + "before its module object existed" + ) + + return self._lazy_real + + with self._lazy_lock: + # re-check now that we hold the lock - another thread may have + # already finished loading while we were waiting for it + if self._lazy_real is not None and self._lazy_owner is None: + return self._lazy_real + + self._lazy_owner = _threading.get_ident() + + # Computed up front, before anything that can raise - the + # except handler below needs both, and must never risk an + # UnboundLocalError masking the real failure if it happens + # before this point is otherwise reached. + parent_name, _, attr_name = self._lazy_name.rpartition(".") + parent_patched = False + + try: + # Both places that construct a _LazyModule (_register_ + # submodules() and lazy_module()) already had to look up a + # spec to get this far, and handed it to us in __init__ - + # reuse it rather than paying for a second, redundant + # lookup here, which also makes the fallback branch below + # (and its PathFinder-vs-find_spec choice) rare rather than + # eliminating it - lazy_module() still allows a stub to be + # built with spec=None if its own find_spec() failed, and + # that's handled there. + # + # This is a small behaviour change, not just a speedup: + # the spec is now resolved once, at *registration* time + # (effectively, at `import sire` time for everything under + # it), rather than at first-touch. A sys.path edit or + # importlib.invalidate_caches() between those two points + # no longer affects what actually gets loaded - it matches + # what `import sire` itself already saw, which is more + # correct, but it is a real difference from before. + if self._lazy_spec is not None: + spec = self._lazy_spec + + # module_from_spec() never consults sys.modules, and + # sys.modules[self._lazy_name] gets overwritten with + # real_module a few lines below regardless - so unlike + # the fallback branch, there's nothing here that reads + # sys.modules and needs this name popped out of it + # first. Skipping the pop also narrows the window + # where the name is absent from sys.modules entirely + # to nothing, rather than widening it. + else: + # This proxy is itself sitting in sys.modules[name] + # right now. find_spec()/import_module() both consult + # sys.modules first, so if we left it there they'd + # either hand this same proxy straight back (recursing + # forever) or fail (since it has no real __spec__) - + # pop it out first so the lookup below does a genuine + # fresh search. + _sys.modules.pop(self._lazy_name, None) + + # Use PathFinder directly with the parent's + # already-known search path, rather than + # importlib.util.find_spec(name) - for a dotted name, + # that variant automatically imports the parent + # package to get its __path__, which would re-enter a + # still-lazy parent's load from inside this module's + # own load. + if self._lazy_search_locations is not None: + spec = _importlib_machinery.PathFinder.find_spec( + self._lazy_name, self._lazy_search_locations + ) + else: + spec = _importlib_util.find_spec(self._lazy_name) + + if spec is None: + raise ImportError(f"No module named {self._lazy_name!r}") + + real_module = _importlib_util.module_from_spec(spec) + + _sys.modules[self._lazy_name] = real_module + + # also fix up the attribute on the parent package, if there + # is one, so that e.g. `sire.maths` now points at the real + # module + if parent_name: + parent = _sys.modules.get(parent_name) + + if parent is not None: + setattr(parent, attr_name, real_module) + parent_patched = True + + # Record the (still-executing) real module *before* running + # its body - exactly what Python's own import system does, + # and important for the same reason: if executing the + # module's body re-entrantly imports this same name (a + # circular import), the reentrant branch above must see + # this partially-initialised real module, not a second, + # independent one. + self._lazy_real = real_module + + spec.loader.exec_module(real_module) + + # Fix up every pre-registered child of *this* module that + # its own body didn't already set as a real attribute (see + # _register_submodules() for why that gap exists in the + # first place). Doing this here, immediately after this + # module's own exec_module() call, rather than up-front at + # registration time, matters: for any access that goes + # through the import system (`import parent.child`, or a + # bare attribute access on an already-imported parent), + # this preserves the guarantee that touching a child still + # forces its parent's __init__.py to run first, in its + # normal top-to-bottom order, exactly as a real (non-lazy) + # import would - some packages' own submodules are only + # safe to import in a specific order (e.g. because of + # shared state in a third-party library they both touch), + # and that guarantee would silently break if a child could + # be reached directly off this module without ever running + # its body. Note this guarantee comes from CPython's import + # statement resolving the parent first, not from anything + # this module does - a lookup that bypasses the import + # system entirely (e.g. sys.modules["parent.child"] + # directly) reaches the child's own stub without the + # parent ever having been touched, same as it always could. + # + # Walk self._lazy_children (recorded up front by + # _register_submodules()) rather than scanning the whole of + # sys.modules - cheap, and exact. + prefix_len = len(self._lazy_name) + 1 + + for _child_name in self._lazy_children: + _child_module = _sys.modules.get(_child_name) + + if _child_module is None: + continue + + _child_attr = _child_name[prefix_len:] + + if _child_attr not in real_module.__dict__: + setattr(real_module, _child_attr, _child_module) + except BaseException: + # Leave this stub fully retryable - CPython itself leaves a + # failed import retryable (a broken module, or one with a + # missing optional dependency, doesn't get permanently + # wedged just because someone touched it once), and without + # this a failed load here would otherwise leave sys.modules + # either missing the entry entirely (a confusing KeyError + # on next use) or holding a half-initialised module that + # looks loaded but silently lacks whatever wasn't reached + # before the failure - in both cases masking the *original* + # error on every subsequent attempt. + # + # Put *this stub* back in sys.modules, rather than just + # leaving the name missing - a plain `import sire.mol` + # retried after a failure would otherwise bypass it and go + # through CPython's own machinery instead, reintroducing + # the duplicate-module-object problem this whole scheme + # exists to prevent. + _sys.modules[self._lazy_name] = self + + # Likewise, undo the parent-attribute fixup above if it + # already ran (it happens *before* exec_module(), so a + # failure inside exec_module() would otherwise leave the + # parent pointing at the broken module directly - a plain + # attribute, not this stub - so retrying via the parent + # would silently skip _load() altogether next time). Gated + # on the parent_patched flag set above, rather than + # comparing the parent's current attribute against + # self._lazy_real - that comparison is wrong precisely when + # it matters most: a failure *before* self._lazy_real gets + # set (e.g. spec is None) leaves it as None, and a parent + # with no such attribute at all also returns None from + # .get(), so None is None would wrongly read as "yes, undo + # it" and attach the stub to a parent it was never set on. + if parent_patched: + parent = _sys.modules.get(parent_name) + + if parent is not None: + setattr(parent, attr_name, self) + + self._lazy_real = None + raise + finally: + self._lazy_owner = None + + return self._lazy_real + + def __dir__(self): + # Unlike __repr__ below, this one *does* force the load. IPython's + # tab-completion (`sr.mol.`) goes through dir(), so leaving it + # lazy here would mean nothing lazily-loaded ever tab-completes + # before something else happens to have touched it - a real + # regression from the old lazy_import package, which forced a load + # on dir() too (_pythonize.py used to lean on exactly that as its + # own force-load idiom). __repr__ stays lazy on purpose - that one + # matters for debuggers/pytest failure reporting not wanting to + # trigger a real import just to print a value. + return dir(self._load()) + + def __getattr__(self, attr): + # only real attribute lookups (i.e. ones that fail against this + # proxy's own __dict__) should trigger the load - this deliberately + # does *not* override __repr__, so introspection that just wants to + # print/log a value (e.g. a debugger, or pytest's failure + # reporting) doesn't silently trigger a real import as a side + # effect + if attr.startswith("_lazy"): + raise AttributeError(attr) + + # hasattr(module, "__path__") is the standard "is this a package?" + # idiom, and before load it's answerable from the spec without + # loading anything: a module whose spec has no + # submodule_search_locations has no __path__ yet (module_from_spec() + # itself only ever sets __path__ conditionally, for the same + # reason __init__ above only sets it conditionally). Without this, + # any code sweeping over sys.modules asking "which of these are + # packages" - the same shape of scan the __file__ handling above + # exists to protect against - would force-load every plain + # (non-package) module stub in the tree at once. + # + # Gated on self._lazy_real is None (reading that field doesn't + # itself force anything) because the spec only speaks for the + # module *before* its body has run - a plain .py module can + # still assign __path__ itself, making it act as a package (an + # unusual but legitimate pattern). Once loaded, defer to the real + # module like every other attribute does; only the pre-load case + # is answerable without it. + if ( + attr == "__path__" + and self._lazy_real is None + and self._lazy_spec is not None + and self._lazy_spec.submodule_search_locations is None + ): + raise AttributeError(attr) + + real_module = self._load() + + try: + return getattr(real_module, attr) + except AttributeError: + # 'attr' may be a submodule that lazy_module() pre-registered + # under its own dotted name in sys.modules (see below), but + # that was never actually set as an attribute of this (real, + # now-loaded) parent module. That happens when the parent's + # own body does `from . import sub` (rather than `from .sub + # import Name`): CPython's own fromlist handling can bind + # 'sub' straight from sys.modules without ever calling + # getattr() on our stub, so nothing triggered *its* load or + # did the usual parent-attribute fixup. Recover here instead, + # on demand. + full_name = f"{self._lazy_name}.{attr}" + sub_module = _sys.modules.get(full_name) + + if sub_module is None: + raise + + if isinstance(sub_module, _LazyModule): + sub_module = sub_module._load() + + setattr(real_module, attr, sub_module) + return sub_module + + def __repr__(self): + if self._lazy_real is not None and self._lazy_owner is None: + return repr(self._lazy_real) + + return f"" + + +def _register_submodules(name, search_locations): + """ + Pre-register a lazy stub for every *immediate* submodule of 'name', + discovered purely from what's on disk under 'search_locations' - no + code from any of these modules is executed to do this. + + This matters for correctness, not just convenience: if some external + code (e.g. pickle, resolving a class's __module__) imports a + submodule directly by its dotted path before anything has triggered + the parent package's lazy load, Python's import machinery takes a + fast path once it sees the parent already present in sys.modules - + it fetches the parent's __path__ (which correctly triggers our + load), but then goes on to build *and execute* the submodule itself + regardless of whether that load already did so as a side effect, + silently producing a second, distinct copy of that submodule (and + therefore of any classes it defines). Pre-registering a stub for + every immediate submodule under its own exact dotted name closes + that gap, since the direct import then finds (and loads) that same + stub instead of racing to rebuild it. + + Driving this from pkgutil.iter_modules()/PathFinder.find_spec() + rather than a hardcoded module list means it stays correct for + forks or downstream code that add new submodules, with no extra + maintenance. + + Recursive: submodules of submodules get pre-registered too, all the + way down, so a class living arbitrarily deep inside a lazily-loaded + package is protected against the same bug. Note that pre-registering + a stub here does *not* attach it as an attribute of its parent - it + only reserves the name in sys.modules. The attribute fixup happens + lazily instead, in _load() above, right after a package's own body + finishes running. That ordering is deliberate: attaching children + eagerly (before the parent's own __init__.py has ever run) would let + an access that goes through the import system reach a child straight + off the parent without ever triggering the parent's own + initialisation - and some packages' submodules are only safe to touch + in the order their __init__.py imports them in (e.g. because of shared + state in a third-party library more than one of them happens to + touch), an ordering guarantee a real, non-lazy import always preserves + and which this needs to as well, for that same class of access. This + doesn't (and can't) protect a lookup that bypasses the import system + entirely, e.g. sys.modules["parent.child"] directly - that reaches the + child's own stub without the parent ever being touched, exactly as it + always could; not this module's problem to solve. + """ + parent = _sys.modules.get(name) + + for _finder, sub_name, _is_pkg in _pkgutil.iter_modules(search_locations): + full_name = f"{name}.{sub_name}" + + if full_name in _sys.modules: + # Already registered (e.g. by some other path) - still worth + # recording against the parent below, so _load()'s fixup loop + # knows about it too. + if isinstance(parent, _LazyModule): + parent._lazy_children.append(full_name) + continue + + # Use PathFinder directly with the already-known search_locations, + # rather than importlib.util.find_spec(full_name) - that variant + # resolves the parent package to read its __path__, which would + # touch our still-lazy parent stub's __getattr__ and force it to + # load prematurely, defeating the point of doing this lazily. + try: + spec = _importlib_machinery.PathFinder.find_spec( + full_name, search_locations + ) + except (ImportError, AttributeError): + continue + + if spec is None: + continue + + _sys.modules[full_name] = _LazyModule(full_name, search_locations, spec=spec) + + if isinstance(parent, _LazyModule): + parent._lazy_children.append(full_name) + + if spec.submodule_search_locations: + _register_submodules(full_name, spec.submodule_search_locations) + + +def lazy_module(name: str): + """ + Return a proxy for the module called 'name' that defers the actual + import until one of its attributes is accessed, or force_load() is + called on it explicitly. This is a drop-in, GPLv3-free replacement for + lazy_import.lazy_module(). + + If 'name' is a package, every immediate submodule is also + pre-registered as its own lazy stub - see _register_submodules() for + why. + + Idempotent: calling this twice for the same name returns the *same* + object both times, whatever state it's in (unloaded stub, mid-load, or + fully real) - building a second, independent _LazyModule for a name + already present in sys.modules would leave whoever's holding the first + one with a second, unrelated loader for the same dotted name, which is + exactly the class of bug this module exists to prevent. + """ + existing = _sys.modules.get(name) + + if existing is not None: + return existing + + # Find the spec *before* registering our stub - find_spec() consults + # sys.modules first, and our stub has no real __spec__ of its own. + try: + spec = _importlib_util.find_spec(name) + except (ImportError, AttributeError): + spec = None + + module = _LazyModule(name, spec=spec) + _sys.modules[name] = module + + if spec is not None and spec.submodule_search_locations: + _register_submodules(name, spec.submodule_search_locations) + + return module + + +def is_lazy_module(module) -> bool: + """Return whether 'module' is a not-yet-loaded lazy module proxy.""" + return isinstance(module, _LazyModule) and module._lazy_real is None + + +def force_load(module): + """ + If 'module' is a lazy module proxy, force it to load now. This is + a no-op if 'module' is already a real, fully-loaded module. + """ + if isinstance(module, _LazyModule): + module._load() diff --git a/src/sire/_pythonize.py b/src/sire/_pythonize.py index 5eed9b85d..5f637a24e 100644 --- a/src/sire/_pythonize.py +++ b/src/sire/_pythonize.py @@ -250,75 +250,65 @@ def _load_new_api_modules(delete_old: bool = True, is_base: bool = False): _pythonize(Convert._SireOpenMM.TorchQMEngine, delete_old=delete_old) _pythonize(Convert._SireOpenMM.TorchQMForce, delete_old=delete_old) - try: - import lazy_import - - have_lazy_import = True - except ImportError: - have_lazy_import = False - - if have_lazy_import: - # Now make sure that all new modules have been loaded - # (we need to import base first) - from . import base - - if lazy_import.LazyModule in type(base).mro(): - # this module is lazily loaded - use 'dir' to load it - dir(base) - - if is_base: - # return, as we will only import base here - _is_in_loading_process = False - return - - from . import ( - move, - io, - system, - squire, - mm, - convert, - ff, - mol, - analysis, - cas, - cluster, - error, - id, - maths, - morph, - restraints, - qt, - stream, - units, - vol, - ) + from ._lazy_import import force_load + + # Now make sure that all new modules have been loaded + # (we need to import base first) + from . import base + + force_load(base) + + if is_base: + # return, as we will only import base here + _is_in_loading_process = False + return + + from . import ( + move, + io, + system, + squire, + mm, + convert, + ff, + mol, + analysis, + cas, + cluster, + error, + id, + maths, + morph, + restraints, + qt, + stream, + units, + vol, + ) - for M in [ - move, - io, - system, - squire, - mm, - convert, - ff, - mol, - analysis, - cas, - cluster, - error, - id, - maths, - morph, - restraints, - qt, - stream, - units, - vol, - ]: - if lazy_import.LazyModule in type(M).mro(): - # this module is lazily loaded - use 'dir' to load it - dir(M) + for M in [ + move, + io, + system, + squire, + mm, + convert, + ff, + mol, + analysis, + cas, + cluster, + error, + id, + maths, + morph, + restraints, + qt, + stream, + units, + vol, + ]: + force_load(M) _is_in_loading_process = False diff --git a/src/sire/mm/__init__.py b/src/sire/mm/__init__.py index 172c42fdf..37948fb44 100644 --- a/src/sire/mm/__init__.py +++ b/src/sire/mm/__init__.py @@ -221,10 +221,10 @@ def _fix_siremm(): # sire.mm is the first Sire submodule touched in a process, calling # use_new_api() before _fix_siremm is defined means that reentrant load of # sire.mol fails with 'cannot import name _fix_siremm from sire.mm', which -# then cascades into 'sire.mm could not be loaded' via the lazy_import -# wrapper. Nothing above this point depends on use_new_api() having run -# (it only pythonizes names already pulled directly from the raw legacy -# _MM module), so it is safe to defer to here. +# then causes the whole sire.mm import to fail. Nothing above this point +# depends on use_new_api() having run (it only pythonizes names already +# pulled directly from the raw legacy _MM module), so it is safe to defer +# to here. from .. import use_new_api as _use_new_api _use_new_api() diff --git a/tests/test_lazy_import.py b/tests/test_lazy_import.py new file mode 100644 index 000000000..4321338c8 --- /dev/null +++ b/tests/test_lazy_import.py @@ -0,0 +1,570 @@ +import pickle + + +def _make_and_dump(path): + """Runs in a fresh worker process. Independently triggers sire's + lazy-loading of sire.mol, then pickles a Molecule to disk.""" + import sire as sr + + mol = sr.mol.Molecule() + with open(path, "wb") as f: + pickle.dump(mol, f) + + +def _load_and_check(path): + """Runs in another, separate fresh worker process. Independently + triggers sire.mol's lazy load (before ever touching the pickle), + then unpickles the Molecule and checks that its class is identical + to (not just equal by name to) the one this process would construct + itself.""" + import sire as sr + + Molecule = sr.mol.Molecule + + with open(path, "rb") as f: + obj = pickle.load(f) + + return isinstance(obj, Molecule), type(obj) is Molecule + + +def test_lazy_import_pickle_across_processes(tmp_path): + """ + A class from a lazily-loaded module, independently lazy-loaded in two + separate worker processes, must be the same class object in both - + isinstance()/type() checks on an object pickled in one process and + unpickled in another must succeed. + """ + from multiprocessing import get_context + + ctx = get_context("spawn") + + path = str(tmp_path / "molecule.pickle") + + # Dump in one, independent, fresh worker process. + with ctx.Pool(1) as pool: + pool.apply(_make_and_dump, (path,)) + + # Load and check in a completely separate, fresh worker process. + with ctx.Pool(1) as pool: + isinstance_ok, type_is_ok = pool.apply(_load_and_check, (path,)) + + assert isinstance_ok + assert type_is_ok + + +def _check_direct_submodule_import(): + """ + Runs in a fresh process. Imports a submodule of a lazily-loaded + package directly by its dotted path, *before* anything has triggered + the parent package's own lazy load, and checks that the class + obtained this way is identical to the one obtained via the parent's + (now-loaded) attribute. + """ + import sys + + import sire # noqa: F401 + + assert type(sys.modules["sire.mol"]).__name__ == "_LazyModule" + + # Note: sire.mol._element / Element, not sire.mol._trajectory, since + # mol/__init__.py happens to also define its own unrelated function + # called `_trajectory`, which shadows the submodule attribute + # regardless of lazy loading (import a.b.c as x walks attributes, + # not sys.modules, so a later same-named function always wins) - + # that's an unrelated naming collision, not what this test targets. + import sire.mol._element as leaf + + parent = sys.modules["sire.mol"] + + return leaf.Element is parent.Element + + +def test_lazy_import_direct_submodule_import(): + """ + Importing a submodule of a lazily-loaded package directly by its + dotted path (e.g. `import sire.mol._x`, or pickle resolving a + class's __module__), before the parent has ever been touched, must + produce the same class object as accessing it via the parent's own + (now-loaded) attribute - not a second, independent copy. + """ + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + ok = pool.apply(_check_direct_submodule_import) + + assert ok + + +def _make_synthetic_package(root, pkg_name, mod_name, mod_body): + """Write a minimal, importable package to disk: root/pkg_name/__init__.py + (empty) and root/pkg_name/mod_name.py (mod_body). Returns str(root), for + inserting onto sys.path in a worker process.""" + import os + + pkg_dir = os.path.join(root, pkg_name) + os.makedirs(pkg_dir, exist_ok=True) + + with open(os.path.join(pkg_dir, "__init__.py"), "w") as f: + f.write("") + + with open(os.path.join(pkg_dir, f"{mod_name}.py"), "w") as f: + f.write(mod_body) + + return str(root) + + +def _thread_race_worker(root): + """Runs in a fresh process. Registers a lazy stub for a submodule + whose body sleeps before setting an attribute (widening the race + window deliberately), then hammers that stub's first-touch load from + many threads at once, synchronised to start together via a Barrier. + Returns (errors, results, exec_count).""" + import builtins + import sys + import threading + + sys.path.insert(0, root) + + from sire._lazy_import import lazy_module + + lazy_module("synth_race_pkg") + stub = sys.modules["synth_race_pkg.slow_mod"] + + n_threads = 16 + barrier = threading.Barrier(n_threads) + results = [] + errors = [] + results_lock = threading.Lock() + + def touch(): + barrier.wait() + try: + value = stub.VALUE + except BaseException as exc: # noqa: BLE001 - want any failure, not just AttributeError + with results_lock: + errors.append(repr(exc)) + else: + with results_lock: + results.append(value) + + threads = [threading.Thread(target=touch) for _ in range(n_threads)] + + for t in threads: + t.start() + for t in threads: + t.join() + + exec_count = getattr(builtins, "_LAZY_TEST_RACE_EXEC_COUNT", 0) + + return errors, results, exec_count + + +def test_lazy_import_thread_race(tmp_path): + """ + Many threads touching the same not-yet-loaded module for the first + time, simultaneously, must all block until the load genuinely + finishes: none may see a partially-initialised module or raise, and + the module's own body must execute exactly once, not once per + thread. The submodule's body sleeps before defining its one + attribute, to widen the race window; builtins is used as a + cross-thread (same-process) counter to confirm single execution. + """ + body = ( + "import builtins\n" + "import time\n" + "\n" + "builtins._LAZY_TEST_RACE_EXEC_COUNT = (\n" + " getattr(builtins, '_LAZY_TEST_RACE_EXEC_COUNT', 0) + 1\n" + ")\n" + "\n" + "time.sleep(0.3)\n" + "\n" + "VALUE = 42\n" + ) + root = _make_synthetic_package(str(tmp_path), "synth_race_pkg", "slow_mod", body) + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + errors, results, exec_count = pool.apply(_thread_race_worker, (root,)) + + assert errors == [] + assert results == [42] * 16 + assert exec_count == 1 + + +def _failed_load_retry_worker(root): + """Runs in a fresh process. Registers a lazy stub for a submodule + whose body raises on its first execution and succeeds on the second, + and checks that the failure is retryable rather than permanently + masked.""" + import sys + + sys.path.insert(0, root) + + from sire._lazy_import import lazy_module + + lazy_module("synth_retry_pkg") + stub = sys.modules["synth_retry_pkg.flaky_mod"] + + first_error = None + + try: + stub.VALUE + except RuntimeError as exc: + first_error = str(exc) + + # A second attempt, on the *same* stub, should retry cleanly. + second_value = stub.VALUE + + return first_error, second_value + + +def test_lazy_import_failed_load_is_retryable(tmp_path): + """ + A module whose body raises on import must be retryable afterwards - + the original exception should propagate (not a KeyError, and not a + silently half-initialised module), and a later attempt must be able + to succeed cleanly if the module's own code would now succeed. + """ + body = ( + "import builtins\n" + "\n" + "builtins._LAZY_TEST_RETRY_ATTEMPTS = (\n" + " getattr(builtins, '_LAZY_TEST_RETRY_ATTEMPTS', 0) + 1\n" + ")\n" + "\n" + "if builtins._LAZY_TEST_RETRY_ATTEMPTS == 1:\n" + " raise RuntimeError('simulated first-attempt failure')\n" + "\n" + "VALUE = 99\n" + ) + root = _make_synthetic_package(str(tmp_path), "synth_retry_pkg", "flaky_mod", body) + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + first_error, second_value = pool.apply(_failed_load_retry_worker, (root,)) + + assert first_error == "simulated first-attempt failure" + assert second_value == 99 + + +def _lazy_module_idempotent_worker(root): + """Runs in a fresh process. Calls lazy_module() twice for the same + still-unloaded name and checks both calls return the identical + object, then force-loads through one of the two references and + confirms the other sees the same real module.""" + import sys + + sys.path.insert(0, root) + + from sire._lazy_import import force_load, lazy_module + + first = lazy_module("synth_idempotent_pkg") + second = lazy_module("synth_idempotent_pkg") + + same_before_load = first is second + + force_load(first) + + # first/second (the stub) stay as they were - the real module replaces + # the stub *in sys.modules*, not in variables already holding the stub + # - so the meaningful check here is that sys.modules now holds exactly + # the module first._load() produced, not some other, independent one. + return ( + same_before_load, + first is second, + sys.modules["synth_idempotent_pkg"] is first._lazy_real, + ) + + +def test_lazy_module_is_idempotent(tmp_path): + """ + lazy_module() called twice for the same still-unloaded name must + return the identical object both times - not a second, independent + stub - and that identity must still hold once the module is loaded. + """ + root = _make_synthetic_package( + str(tmp_path), "synth_idempotent_pkg", "leaf", "VALUE = 7\n" + ) + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + same_before_load, same_after_load, matches_sys_modules = pool.apply( + _lazy_module_idempotent_worker, (root,) + ) + + assert same_before_load + assert same_after_load + assert matches_sys_modules + + +def _find_spec_on_unloaded_stub_worker(root): + """Runs in a fresh process. Registers a lazy stub, then calls + importlib.util.find_spec() on it *before* touching it at all, and + returns whatever that produced (a real ModuleSpec, or the repr of + whatever exception it raised).""" + import sys + import importlib.util + + sys.path.insert(0, root) + + from sire._lazy_import import lazy_module + + lazy_module("synth_find_spec_pkg") + + assert type(sys.modules["synth_find_spec_pkg.leaf"]).__name__ == "_LazyModule" + + try: + spec = importlib.util.find_spec("synth_find_spec_pkg.leaf") + except BaseException as exc: # noqa: BLE001 - want to see exactly what, if anything, was raised + return None, repr(exc) + + return spec is not None, None + + +def test_find_spec_on_unloaded_stub(tmp_path): + """ + importlib.util.find_spec() on a not-yet-loaded lazy module must + return a real ModuleSpec, not raise - a stub's __spec__ (and + __loader__, __package__) must be genuinely populated, not None. + """ + root = _make_synthetic_package( + str(tmp_path), "synth_find_spec_pkg", "leaf", "VALUE = 1\n" + ) + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + got_spec, error = pool.apply(_find_spec_on_unloaded_stub_worker, (root,)) + + assert error is None, f"find_spec() raised: {error}" + assert got_spec + + +def _make_extension_package(root, pkg_name, so_module_name): + """Write a minimal package (root/pkg_name/__init__.py, empty) whose + one submodule is a real, copied stdlib C extension (single-phase + init, e.g. _ctypes) rather than a .py file - named after the + original so its PyInit_ symbol still matches. Returns + str(root).""" + import importlib + import os + import shutil + + ext_module = importlib.import_module(so_module_name) + so_path = ext_module.__file__ + + pkg_dir = os.path.join(root, pkg_name) + os.makedirs(pkg_dir, exist_ok=True) + + with open(os.path.join(pkg_dir, "__init__.py"), "w") as f: + f.write("") + + shutil.copy(so_path, os.path.join(pkg_dir, os.path.basename(so_path))) + + return str(root) + + +def _extension_module_not_eagerly_initialised_worker(root): + """Runs in a fresh process. Registers a lazy stub for a package + containing a real, single-phase-init C extension submodule, and + checks that registering it doesn't dlopen/initialise it (the stub + should have none of the extension's real attributes), while touching + it afterwards does load the real thing into sys.modules.""" + import sys + + sys.path.insert(0, root) + + from sire._lazy_import import lazy_module + + lazy_module("synth_ext_pkg") + + stub = sys.modules["synth_ext_pkg._ctypes"] + stub_is_lazy_module = type(stub).__name__ == "_LazyModule" + + # Anything not one of our own private '_lazy*' bookkeeping attributes + # or a dunder here would mean the real extension's attributes were + # already present at registration time, before anyone touched it. + leaked_attrs = [ + a + for a in stub.__dict__ + if not a.startswith("_lazy") and not (a.startswith("__") and a.endswith("__")) + ] + + # Now actually touch it, and confirm the real module properly took + # over in sys.modules (not just delegation through the stub). + real_attr_count = len(dir(stub)) + real_module_in_sys_modules = type(sys.modules["synth_ext_pkg._ctypes"]).__name__ + + return ( + stub_is_lazy_module, + leaked_attrs, + real_attr_count, + real_module_in_sys_modules, + ) + + +def test_extension_module_not_eagerly_initialised(tmp_path): + """ + A single-phase-init C extension (e.g. a compiled Boost.Python + module) reachable inside a lazily-registered package must not be + dlopened/initialised at registration time - the stub must carry none + of the real extension's attributes until it's actually touched, and + touching it must then produce the real module in sys.modules, with + the real module's full attribute set. Uses a real, copied stdlib + extension (_ctypes) rather than a synthetic pure-Python module, + since this only shows up against something with real C-level + initialisation side effects. + """ + root = _make_extension_package(str(tmp_path), "synth_ext_pkg", "_ctypes") + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + ( + stub_is_lazy_module, + leaked_attrs, + real_attr_count, + real_module_type, + ) = pool.apply(_extension_module_not_eagerly_initialised_worker, (root,)) + + assert stub_is_lazy_module + assert leaked_attrs == [] + assert real_attr_count > len(leaked_attrs) + assert real_module_type == "module" + + +def _hasattr_path_reflects_load_state_worker(root): + """Runs in a fresh process. Checks hasattr(stub, '__path__') on a + not-yet-loaded module stub whose body assigns __path__ itself (an + unusual but legitimate way for a plain module to act like a + package): before load, that must be answerable without loading the + module at all; after load, it must agree with the real module.""" + import sys + + sys.path.insert(0, root) + + from sire._lazy_import import lazy_module + + lazy_module("synth_path_pkg") + stub = sys.modules["synth_path_pkg.leaf"] + + has_path_before = hasattr(stub, "__path__") + still_lazy_before = ( + type(sys.modules["synth_path_pkg.leaf"]).__name__ == "_LazyModule" + ) + + stub.VALUE # force the load + + has_path_after = hasattr(stub, "__path__") + real_module_after = type(sys.modules["synth_path_pkg.leaf"]).__name__ == "module" + + return has_path_before, still_lazy_before, has_path_after, real_module_after + + +def test_hasattr_path_reflects_load_state(tmp_path): + """ + hasattr(module, "__path__") - the standard "is this a package?" idiom + - on a not-yet-loaded module stub whose spec has no + submodule_search_locations must return False without loading the + module. Once the module has loaded, the answer must switch to + reflect what the real module actually has - including a plain + module that assigns __path__ in its own body, which the spec alone + can't predict. + """ + root = _make_synthetic_package( + str(tmp_path), + "synth_path_pkg", + "leaf", + "__path__ = ['/nonexistent']\nVALUE = 1\n", + ) + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + has_path_before, still_lazy_before, has_path_after, real_module_after = ( + pool.apply(_hasattr_path_reflects_load_state_worker, (root,)) + ) + + assert has_path_before is False + assert still_lazy_before + assert has_path_after is True + assert real_module_after + + +def _reentrant_before_real_worker(root): + """Runs in a fresh process. Simulates the one window a same-thread + reentrant import can hit before there's a module object to hand + back (only reachable in practice for a C extension whose init + routine itself imports something, which module_from_spec() can run + before _load() ever gets to set self._lazy_real) by monkeypatching + module_from_spec() to re-enter the stub's own _load() first.""" + import sys + + sys.path.insert(0, root) + + from sire import _lazy_import + from sire._lazy_import import lazy_module + + lazy_module("synth_reentrant_pkg") + stub = sys.modules["synth_reentrant_pkg.leaf"] + + original_module_from_spec = _lazy_import._importlib_util.module_from_spec + caught = [] + + def patched_module_from_spec(spec): + if spec.name == "synth_reentrant_pkg.leaf" and not caught: + try: + stub._load() + except ImportError as exc: + caught.append(str(exc)) + return original_module_from_spec(spec) + + _lazy_import._importlib_util.module_from_spec = patched_module_from_spec + try: + value = stub.VALUE + finally: + _lazy_import._importlib_util.module_from_spec = original_module_from_spec + + return caught, value + + +def test_reentrant_import_before_module_exists_raises_clearly(tmp_path): + """ + A same-thread reentrant _load() call that lands in the narrow window + before the module object itself has been built must raise a clear, + diagnosable error - not silently return None and let the caller hit + an opaque AttributeError on it further down the line. + """ + root = _make_synthetic_package( + str(tmp_path), "synth_reentrant_pkg", "leaf", "VALUE = 1\n" + ) + + from multiprocessing import get_context + + ctx = get_context("spawn") + + with ctx.Pool(1) as pool: + caught, value = pool.apply(_reentrant_before_real_worker, (root,)) + + assert len(caught) == 1 + assert "circular import" in caught[0] + assert "synth_reentrant_pkg.leaf" in caught[0] + assert value == 1