ENH: Replace custom LazyITKModule with native PEP 562 lazy loading#6183
Conversation
github-actions
Bot
added
type:Infrastructure
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed Greptile's two P2 findings in ca131c5:
|
thewtex
force-pushed
the
python-lazy-mechanism
branch
from
ca131c5 to
9e33a81
Compare
|
Pushed four follow-up commits addressing the audit items raised on this PR (HEAD
Validated standalone that the |
hjmjohnson
force-pushed
the
python-lazy-mechanism
branch
from
c8df1a6 to
beae673
Compare
|
@thewtex I used agentic review tools to find (perceived) weaknesses in your solution. The 4 commits are the result of that investigation. If you agree with them, they could be left or squashed into the original commits. I like the idea of this PR, and it looks like the correct solution, but it is not in my typical experience to review this. I'm inclined to accept this and then make it a priority to address issues that may arise quickly. |
|
I had a quick glance, and have no objections. But I do have questions: does this impact import speed, and how much? What is the main motivation for this refactoring? |
Replace the LazyITKModule sys.modules-swap orchestration in itk/__init__.py with a native PEP 562 implementation: module-level __getattr__ and __dir__ resolve symbols on first access, gated by a single threading.RLock. itkConfig.LazyLoading is no longer consulted; lazy is the only mode. multiprocessing.RLock is deliberately avoided so the multiprocessing start method stays pickable after `import itk`. Per-submodule LazyITKModule instances and the __init_<modulename>__.py discovery hook are preserved unchanged so the build stays green; Phase 03 will migrate them. itk_base_global_lazy_attributes (consumed by support/template_class._LoadModules) is still populated with the full set of owners per attribute, alongside the new first-owner-only _lazy_attribute_to_module that drives the top-level __getattr__. Validated via py_compile and a mock-driver smoke harness covering PEP 366 __package__, dir-without-load, lazy-load-with-cache, first-owner precedence, AttributeError on miss, dunder short-circuit, and a 6-thread first-touch race.
The new PEP 562 module-level __dir__ called the unqualified `set` name,
which Python resolves through `itk.__dict__`. Once any SWIG submodule is
loaded (e.g. ITKCommon on first `itk.Image` access), `itk.set` is
populated as an `itkTemplate` binding `std::set` and shadows the
builtin. Subsequent calls to `dir(itk)` then raised
`TemplateTypeError: itk.set is not wrapped for input type None`.
This is the same alias hazard that the existing _initialize_module
already guards against by importing `_builtin_set` from `builtins`.
Switch __dir__ to a set-literal form `{*globals().keys(), ...}` which
constructs through the C-level set type with no name lookup, so it is
unaffected by names introduced into module globals at lazy-load time.
Introduce itk.support._lazy_submodule with a builder that returns a plain types.ModuleType for itk.<Module> wired with module-level __getattr__ / __dir__ closures, registered in sys.modules['itk.<Module>'], and carrying a one-line __reduce_ex__ shim for pickle / cloudpickle by-name round-trip. This is the replacement target for the legacy LazyITKModule subclass; the next commit swaps the construction site in itk/__init__.py.
Replace the LazyITKModule(types.ModuleType) construction in _initialize_module() with the PEP 562 helper added in the previous commit. Each itk.<Module> namespace is now a plain types.ModuleType with __getattr__/__dir__ closures and a sys.modules registration, so cloudpickle round-trips by dotted name (Tests/lazy.py). The shared _lazy_load_lock is passed into the helper so top-level and per-submodule lazy loads continue to serialise on a single RLock. After this change __init__.py no longer references LazyITKModule; the class itself remains in support/lazy.py for now and will be removed in a follow-up commit.
The PEP 562 mechanism is now the only lazy-loading path so the legacy itkConfig.LazyLoading flag and its ITK_PYTHON_LAZYLOADING environment variable are no-ops. Drop the flag definition, the docstring paragraph documenting it, and the three test-side overrides that toggled it. Specifically: * Wrapping/Generators/Python/itkConfig.template.in.py: drop the LazyLoading docstring paragraph and the LazyLoading bool assignment driven by ITK_PYTHON_LAZYLOADING. * Wrapping/Generators/Python/itk/__init__.py: refresh a leading comment that used LazyLoading as the canonical example of a mutable itkConfig flag; use DefaultFactoryLoading instead so the example still names a real attribute. * Wrapping/Generators/Python/Tests/lazy.py: remove the itkConfig.LazyLoading=True override at the top of the test and refresh the now-stale "PEP 366 compliance of LazyITKModule" comment to reference the per-submodule namespaces that replaced the class. * Wrapping/Generators/Python/Tests/multiprocess_lazy_loading.py: remove the itkConfig.LazyLoading=True override and the now-unused import itkConfig. The descriptive header comments referencing the *concept* of lazy loading remain intact. * Modules/Filtering/ImageIntensity/wrapping/test/itkImageFilterNumPyInputsTest.py: remove the itkConfig.LazyLoading=False override and the associated import itkConfig (otherwise the test would raise AttributeError once the flag is gone).
Tests/nolazy.py was the eager-mode counterpart to Tests/lazy.py and forced itkConfig.LazyLoading=False to exercise the non-lazy import path. With the LazyLoading flag removed in the previous commit the PEP 562 lazy mechanism is the only import path, so the eager test is no longer meaningful and would raise AttributeError on import. Delete the file and its CMake registration. * Wrapping/Generators/Python/Tests/nolazy.py: deleted. * Wrapping/Generators/Python/Tests/CMakeLists.txt: drop the itk_python_add_test(NAME PythonNoLazyModule ...) block.
Remove the legacy custom-subclass-of-types.ModuleType implementation now that the itk package and its per-submodule namespaces resolve attributes via module-level __getattr__ / __dir__. - Delete Wrapping/Generators/Python/itk/support/lazy.py (LazyITKModule, ITKLazyLoadLock, _lazy_itk_module_reconstructor, not_loaded sentinel). - Drop support/lazy from ITK_PYTHON_SUPPORT_MODULES in Wrapping/Generators/Python/CMakeLists.txt so the wheel no longer installs the deleted file. A repo-wide grep confirms no live import of from itk.support import lazy, from itk.support.lazy, support.lazy, _lazy., or LazyITKModule remains.
Extend Tests/lazy.py beyond PEP 366 + cloudpickle to assert the three remaining contracts the PEP 562 lazy mechanism must preserve: - PEP 562 __dir__: "Image" appears in dir(itk) and dir(ITKCommon) but is absent from vars(itk) / vars(ITKCommon) until first access -- the lazy attribute map is enumerable without forcing a SWIG load. - Factory hook: under DefaultFactoryLoading, accessing a class whose module declares a needed factory (ITKIOImageBase -> ImageIO) grows ObjectFactoryBase.GetRegisteredFactories(); the disabled path is already covered by nodefaultfactories.py. - stdlib pickle: pickle.loads(pickle.dumps(itk.ITKCommon)) returns the same instance, exercising the per-submodule __reduce_ex__ shim wired by _make_itk_lazy_submodule (a bare types.ModuleType is unpicklable on CPython 3.12+ without it). Remove unused symbols: - _lazy_submodule.py: docstring named the deleted LazyITKModule subclass; rephrased to describe the module's behavior directly. - multiprocess_lazy_loading.py: header comments used CamelCase "LazyLoading" as a symbol; rephrased to "lazy module loading" / "PEP 562 __getattr__ hook" while keeping the threading contract the test still enforces. - Wrapping/Generators/Python/CMakeLists.txt: directory-layout comment listed itk(...|LazyLoading|...).py as a static support file; updated to the actual filenames currently shipped. Comment- and docstring-only; no functional change. PythonLazyModule, PythonMultiprocessLazyLoad, and PythonLazyLoadingImage all still pass.
Add a section to the ITK 6 migration guide describing the removal of itkConfig.LazyLoading and the ITK_PYTHON_LAZYLOADING environment variable, which were dropped when the Python lazy-loading mechanism was rewritten on top of PEP 562. The section distinguishes per-symbol behavior: assignment to itkConfig.LazyLoading is silently ignored, any subsequent read raises AttributeError, and the environment variable is no longer consulted.
Via pixi run pre-commit-run
Two follow-ups from the Greptile review of PR 6183: * Drop the dead `loaded_modules: set[str]` declaration and its `loaded_modules.add(target)` write in `_make_itk_lazy_submodule`. The set was a vestige of `LazyITKModule.__getstate__`/`__setstate__`; the new `__reduce_ex__` shim delegates to `importlib.import_module` and never consults it. * Reword the migration-guide example for `itkConfig.LazyLoading`. The previous inline comment claimed the second line raised `AttributeError`, which was wrong: the preceding assignment had already set the attribute, so the read returned `False`. Split the example into separately commented write and read patterns and clarify that the `AttributeError` only occurs on a fresh `import itkConfig` with no prior assignment.
…heck assert is silently stripped under `python -O`, leaving the invariant unchecked. Replace with an explicit `if/raise` so the guard survives optimization mode.
Bare types.ModuleType has no reducer, so pickle.dumps(itk) raises TypeError: cannot pickle 'module' object on CPython 3.12+. Define a module-level __reduce_ex__ that delegates to importlib.import_module, matching the per-submodule shim. Cover the round-trip in Tests/lazy.py.
Synthetic submodules previously shipped with __loader__=None and no __spec__, which importlib.util.find_spec, inspect.getsourcefile, pkgutil, and IDE introspection tools treat as a broken module. Build a placeholder spec via importlib.util.spec_from_loader so the modules present a normal PEP 451 surface.
Removing the LazyLoading attribute outright is a hard API break:
downstream code that reads itkConfig.LazyLoading raises AttributeError
on import. Add a one-release shim:
- itkConfig.__getattr__ returns True for LazyLoading reads and emits
DeprecationWarning so legacy `if itkConfig.LazyLoading:` keeps
working.
- ITK_PYTHON_LAZYLOADING is no longer consulted, but its presence in
the environment now emits DeprecationWarning at itkConfig import
time so launch scripts get a single audible heads-up.
Both shims are intended for ITK 6.x and may be removed in 7.0.
hjmjohnson
force-pushed
the
python-lazy-mechanism
branch
from
beae673 to
e1c6adf
Compare
PR InsightSoftwareConsortium#6183's first PEP 562 lazy-loading conversion serialised every first-touch SWIG load through one process-wide threading.RLock. When parallel-worker code (threading.Pool, joblib threading backend, dask local cluster) does first-touch `itk.X` from one thread and first-touch `itk.Y` from another, the second thread blocks until the first's load finishes — even though X and Y share no state. Replace the single `_lazy_load_lock` with a per-SWIG-module dict of RLocks created lazily on first lookup. Per-module serialisation is preserved (template registration and factory-loading hooks remain race-free for any single SWIG module), but unrelated modules now load in parallel. Also refactors module-attribute access from `globals()[name]` / `g.update(namespace)` to `getattr(this_module, name)` / `setattr(this_module, attr, value)`. Functionally equivalent (both ultimately update the same module `__dict__`) but avoids non-literal indexing of `globals()` that static analyzers (semgrep CWE-96) flag as a code-injection foot-gun. The same refactor lands on the per-submodule path in support/_lazy_submodule.py. The `_make_itk_lazy_submodule(...)` signature changes from `lazy_load_lock` (a single RLock) to `get_module_load_lock` (a callable returning the RLock for a given target SWIG module name) so the per-submodule `__getattr__` can lock on the actual target rather than the containing submodule.
Provides a debug escape hatch missing from PR InsightSoftwareConsortium#6183 after the deprecation of `itkConfig.LazyLoading`. When the user sets `ITK_EAGER_IMPORT` to `1`, `true`, `yes`, or `on` in the environment, `import itk` walks every SWIG module in the lazy-attribute map and triggers a single first-touch `__getattr__` on each. Any import-time failure (missing C-extension dependency, broken factory, version mismatch) surfaces synchronously at `import itk` rather than at the first `itk.<thing>` attribute access in user code, which is invaluable for triaging "why does my itk import fail in this Docker image?". Mirrors SPEC 1's `EAGER_IMPORT` convention used by `scientific-python/lazy-loader`, `scikit-image`, NetworkX, MNE-Python. Default behaviour is unchanged (env var unset => fully lazy as in PR InsightSoftwareConsortium#6183). Leaves the lazy machinery in place so subsequent accesses still hit the fast cached path.
PR InsightSoftwareConsortium#6183's motivation cites cold-start improvements from the PEP 562 conversion but ships zero quantitative measurements. PEP 810 (accepted for Python 3.15) sets the ecosystem expectation at 50-70% startup-time reduction; without an in-CI gate, ITK has no way to detect a future change that accidentally re-introduces an eager SWIG load at `import itk` time. This test runs `python -X importtime -c 'import itk'` in a subprocess, parses the cumulative cost from the bundled tracer's output, and asserts: - cumulative `import itk` time stays below 5000 ms - `len(sys.modules)` after `import itk` stays below 400 Both ceilings are intentionally generous so the test does not flap on slow CI runners; their job is to surface a hard regression (e.g., a stray `import itk.ITKCommon` at module top-level that would force the SWIG load) rather than gate ordinary noise. The `ITK_EAGER_IMPORT=0` env override on each subprocess defeats any caller-set EAGER_IMPORT mode so the benchmark always measures the lazy path.
PR InsightSoftwareConsortium#6183's first PEP 562 lazy-loading conversion serialised every first-touch SWIG load through one process-wide threading.RLock. When parallel-worker code (threading.Pool, joblib threading backend, dask local cluster) does first-touch `itk.X` from one thread and first-touch `itk.Y` from another, the second thread blocks until the first's load finishes — even though X and Y share no state. Replace the single `_lazy_load_lock` with a per-SWIG-module dict of RLocks created lazily on first lookup. Per-module serialisation is preserved (template registration and factory-loading hooks remain race-free for any single SWIG module), but unrelated modules now load in parallel. Also refactors module-attribute access from `globals()[name]` / `g.update(namespace)` to `getattr(this_module, name)` / `setattr(this_module, attr, value)`. Functionally equivalent (both ultimately update the same module `__dict__`) but avoids non-literal indexing of `globals()` that static analyzers (semgrep CWE-96) flag as a code-injection foot-gun. The same refactor lands on the per-submodule path in support/_lazy_submodule.py. The `_make_itk_lazy_submodule(...)` signature changes from `lazy_load_lock` (a single RLock) to `get_module_load_lock` (a callable returning the RLock for a given target SWIG module name) so the per-submodule `__getattr__` can lock on the actual target rather than the containing submodule.
Provides a debug escape hatch missing from PR InsightSoftwareConsortium#6183 after the deprecation of `itkConfig.LazyLoading`. When the user sets `ITK_EAGER_IMPORT` to `1`, `true`, `yes`, or `on` in the environment, `import itk` walks every SWIG module in the lazy-attribute map and triggers a single first-touch `__getattr__` on each. Any import-time failure (missing C-extension dependency, broken factory, version mismatch) surfaces synchronously at `import itk` rather than at the first `itk.<thing>` attribute access in user code, which is invaluable for triaging "why does my itk import fail in this Docker image?". Mirrors SPEC 1's `EAGER_IMPORT` convention used by `scientific-python/lazy-loader`, `scikit-image`, NetworkX, MNE-Python. Default behaviour is unchanged (env var unset => fully lazy as in PR InsightSoftwareConsortium#6183). Leaves the lazy machinery in place so subsequent accesses still hit the fast cached path.
PR InsightSoftwareConsortium#6183's motivation cites cold-start improvements from the PEP 562 conversion but ships zero quantitative measurements. PEP 810 (accepted for Python 3.15) sets the ecosystem expectation at 50-70% startup-time reduction; without an in-CI gate, ITK has no way to detect a future change that accidentally re-introduces an eager SWIG load at `import itk` time. This test runs `python -X importtime -c 'import itk'` in a subprocess, parses the cumulative cost from the bundled tracer's output, and asserts: - cumulative `import itk` time stays below 5000 ms - `len(sys.modules)` after `import itk` stays below 400 Both ceilings are intentionally generous so the test does not flap on slow CI runners; their job is to surface a hard regression (e.g., a stray `import itk.ITKCommon` at module top-level that would force the SWIG load) rather than gate ordinary noise. The `ITK_EAGER_IMPORT=0` env override on each subprocess defeats any caller-set EAGER_IMPORT mode so the benchmark always measures the lazy path.
hjmjohnson
force-pushed
the
python-lazy-mechanism
branch
from
d4d359e to
9bbe050
Compare
|
A careful review is needed, but this might be ready to go. Local timing benchmark on the fix-folded tip (force-pushed
Cold RecursionError that the just-folded fix repairsThe PERF commit's refactor (semgrep CWE-96 hardening) replaced Four call sites (two in importtime tree top entriesBEFORE Test environment
|
PR InsightSoftwareConsortium#6183's motivation cites cold-start improvements from the PEP 562 conversion but ships zero quantitative measurements. PEP 810 (accepted for Python 3.15) sets the ecosystem expectation at 50-70% startup-time reduction; without an in-CI gate, ITK has no way to detect a future change that accidentally re-introduces an eager SWIG load at `import itk` time. This test runs `python -X importtime -c 'import itk'` in a subprocess, parses the cumulative cost from the bundled tracer's output, and asserts: - cumulative `import itk` time stays below 5000 ms - `len(sys.modules)` after `import itk` stays below 400 Both ceilings are intentionally generous so the test does not flap on slow CI runners; their job is to surface a hard regression (e.g., a stray `import itk.ITKCommon` at module top-level that would force the SWIG load) rather than gate ordinary noise. The `ITK_EAGER_IMPORT=0` env override on each subprocess defeats any caller-set EAGER_IMPORT mode so the benchmark always measures the lazy path.
hjmjohnson
force-pushed
the
python-lazy-mechanism
branch
from
9bbe050 to
ed35abe
Compare
|
Ran the full Python-wrapping ctest suite against the fix-folded tip and pushed one follow-up to keep the new cold-start gate in budget. Suite result: 3477 / 3477 pass ( Why 400 was too tight
The Forward-compatibility note added to the gateThe new docstring spells out the linear scaling so the next person who hits this knows to bump rather than spelunk:
Headroom at 750 vs current 524: 226 entries ≈ ~75 new SWIG modules of cushion before the gate trips again. Other test categories (all green)
Full ctest log preserved locally at |
|
I have been reviewing so much lately, I can't spare time to carefully review this. I leave it to Matt, Brad, and Simon. |
SimonRit
left a comment
SimonRit
left a comment
There was a problem hiding this comment.
My knowledge is too limited on this topic to provide a meaningful review... LGTM!
|
@hjmjohnson @dzenanz @SimonRit thanks for the reviews 🙏 @hjmjohnson thanks for the follow-ups and extended testing. This is expected to result in somewhat of a performance bump but also help with bugs as Hans' testing noted. There are additional performance gains and import issues that this should address in a more significant way such as the use case of the recent Discourse thread (use under multithreading). In terms of impact, cleaning up the module dependencies and DAG will be much higher in terms of performance. I will not have time to follow-up with issues until next week at the earliest, so I will defer merge until then. Others are welcome to merge before then as long as they follow up with other issues that arise. :-) |
4 participants
The custom
LazyITKModule(types.ModuleType)subclass that poweredITK's Python lazy-loading layer is replaced with the standard-library
PEP 562 (Python 3.7+) module-level
__getattr__/__dir__protocol.The same observable contract is preserved: lazy first-touch resolution,
thread-safe first-load via a single recursive lock, factory-loading
hook gated on
itkConfig.DefaultFactoryLoading, PEP 366__package__,and pickle / cloudpickle round-trip identity through
sys.modulesregistration.
Motivation:
LazyITKModule.__getattribute__intercepted everyattribute read just to check a sentinel; PEP 562 only fires on
miss, so steady-state reads now hit the standard module fast path
for free.
ITKLazyLoadLockcombinedthreading.RLockandmultiprocessing.RLock. Constructing the multiprocessing half pinnedthe multiprocessing start method at
import itktime, breakingdownstream callers that wanted
multiprocessing.set_start_method(...)after import. The new implementation uses only
threading.RLock;process boundary isolation already handles the cross-process case
(each child process re-runs
itk/__init__.pyagainst its ownsys.modules)._lazy_itk_module_reconstructor),__getstate__,__setstate__, and the eager-mode dispatch branchare deleted; standard module pickling and
sys.modules['itk.<Mod>']registration cover the round-trip with a single 3-line
__reduce_ex__shim per synthetic submodule (vanillapickle.dumps(types.ModuleType(...))raises on CPython 3.12+).What changes:
Wrapping/Generators/Python/itk/__init__.pyrewritten withmodule-level
__getattr__/__dir__(PEP 562) and a singlethreading.RLock. Reload-guard branch and eager-mode branchremoved.
Wrapping/Generators/Python/itk/support/_lazy_submodule.pyexports
_make_itk_lazy_submodule(module_name, lazy_attributes, lazy_load_lock)returning a plaintypes.ModuleTypecarryingPEP 562
__getattr__/__dir__/__reduce_ex__.Wrapping/Generators/Python/itk/support/lazy.pydeleted (181 lines:custom subclass, lock, reconstructor, sentinel).
Wrapping/Generators/Python/Tests/nolazy.pydeleted; the eagerbranch it exercised no longer exists.
itkConfig.LazyLoadingand theITK_PYTHON_LAZYLOADINGenvironment-variable reader removed from
itkConfig.template.in.py. Lazy is now the single mode.Tests/lazy.py,Tests/multiprocess_lazy_loading.py, andModules/Filtering/ImageIntensity/wrapping/test/itkImageFilterNumPyInputsTest.pydrop
itkConfig.LazyLoading = ...overrides.Tests/lazy.pyextended to cover PEP 562__dir__(visiblebefore load), the factory-loading hook, and stdlib
pickleround-trip on
itk.ITKCommon.Net diff: 10 files changed, +267 / −335 (net -68 lines).
Validated end-to-end with
pixi run --as-is build-pythonandpixi run --as-is ctest --test-dir build-python -R Python -E "PythonNoLazyModule"— 172/172 tests pass, including thePythonLazyModule,PythonMultiprocessLazyLoad, andPythonGILReleaseSafetyTestregression triplet.Lazy-only is now the single mode: callers that previously set
itkConfig.LazyLoading = Falseshould remove the assignment (itwill raise
AttributeError).