Defer eager submodule imports via lazy .pyi stubs#5932
Conversation
There was a problem hiding this comment.
AI Review Summary
PR Title: Defer eager submodule imports via lazy .pyi stubs
Overview
This PR correctly fixes the lazy export pattern by moving eager submodule imports from __init__.py files into their corresponding .pyi stub files. This ensures that lazy_export() works as intended - submodules are only loaded when actually accessed, not at package import time.
Changes Analysis
1. isaaclab/envs/__init__.py → __init__.pyi
✅ Correct approach: Moved from . import mdp, ui from the .py file to the .pyi stub, and added "mdp" and "ui" to __all__. This ensures these submodules are declared in the type stub for IDE support while remaining lazily loaded at runtime.
2. isaaclab/sensors/ray_caster/__init__.py
✅ Correct approach: Removed the redundant from . import patterns since the stub already declares patterns in its exports. This was causing patterns to be eagerly loaded, defeating the lazy export mechanism.
3. changelog.d/fix-fix_lazy_export.skip
✅ Appropriate: Using a .skip file for internal import hygiene changes that have no user-facing impact.
Technical Correctness
The lazy export pattern in IsaacLab relies on:
- The
.pyistub declaring the public interface (for type checkers/IDEs) - The
lazy_export()call setting up__getattr__to lazily import on first access - No eager imports in the
__init__.pythat would load submodules before they're needed
This PR correctly ensures condition #3 is met.
Verification
The author's test plan is appropriate - verifying that import isaaclab.envs no longer loads mdp/ui into sys.modules until accessed, while still allowing explicit imports like from isaaclab.envs import mdp to work.
Suggestions
-
Minor: Consider adding a comment in the code or contributing guide noting that submodule re-exports should go in
.pyistubs rather than__init__.pyto prevent future regressions. -
Question: The PR description mentions
isaaclab/utils/warp/__init__.pywas intentionally left unchanged. It would be helpful to add a brief inline comment there explaining the eager initialization is intentional, to prevent someone from "fixing" it later.
This is an automated review. A human reviewer should verify the changes align with the project's lazy loading architecture.
Greptile SummaryThis PR fixes eager submodule loading in two packages by moving the relevant
Confidence Score: 5/5Safe to merge — changes are limited to moving two The diff is narrow: two eager import lines removed, one No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User code
participant E as isaaclab.envs
participant LE as lazy_export()
participant PS as _parse_stub(__init__.pyi)
participant LL as lazy_loader.attach_stub
U->>E: import isaaclab.envs
E->>LE: lazy_export()
LE->>PS: parse envs/__init__.pyi
PS-->>LE: "filtered_body has from . import mdp, ui (needs_filter=False)"
LE->>LL: attach_stub(package_name, caller_file)
LL-->>LE: __getattr__, __dir__, __all__
LE-->>E: sets mod.__getattr__, mod.__all__
E-->>U: module ready (mdp/ui NOT yet loaded)
U->>E: isaaclab.envs.mdp (first access)
E->>LL: __getattr__("mdp")
LL-->>E: import isaaclab.envs.mdp (lazy)
E-->>U: mdp submodule
Reviews (1): Last reviewed commit: "Add changelog skip fragment for lazy-exp..." | Re-trigger Greptile |
Summary
lazy_export()packages declare their export surface in a sibling.pyistub, and relative submodule imports (from . import <sub>) belong in that stub so they resolve lazily. A few__init__.pyfiles still imported submodules eagerly, which forced the submodule (and its transitive imports) to load at package-import time and defeated the lazy stub.isaaclab/envs/__init__.py: movedfrom . import mdp, uiintoenvs/__init__.pyi(added to the stub imports and__all__).isaaclab/sensors/ray_caster/__init__.py: removed the redundantfrom . import patterns— the stub already declaredpatterns.After the change,
import isaaclab.envsno longer pullsmdp/uieagerly (verified they are absent fromsys.modulesuntil first access), whileisaaclab.envs.mdp,from isaaclab.envs import mdp,ui,ray_caster.patterns, and all named class exports still resolve correctly.Left intentionally unchanged:
isaaclab/utils/warp/__init__.pyeagerly runswp.init()and monkeypatcheswp.to_torch(itsProxyArrayimport feeds that runtime shim), so it is genuine eager initialization rather than a re-export.Test plan
import isaaclab.envs→mdp/uinot insys.modules; resolve on access.from isaaclab.envs import mdp, ManagerBasedRLEnvandimport isaaclab.sensors.ray_caster; ray_caster.patternsall resolve.pre-commit(ruff / ruff-format) passes on the changed files.