-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Removes kit dependency at runtime for asset configuration #3756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a37d32c
to
db7d2cd
Compare
db7d2cd
to
7dd771e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
This PR refactors imports in configuration classes to use Python's TYPE_CHECKING
guard pattern, allowing configuration classes to be imported without loading heavyweight Isaac Sim kit dependencies at runtime. The changes move imports like AssetBase
, Articulation
, RigidObject
, RigidObjectCollection
, and InteractiveScene
into TYPE_CHECKING
blocks, making them available only for static type analysis while deferring their runtime loading. The from __future__ import annotations
statement enables deferred evaluation of type hints, which is essential for this pattern to work. This optimization improves import performance and enables using configuration classes in contexts where the full simulator isn't needed, such as configuration definition or manipulation without spinning up the simulation environment.
Critical Issue: In action_manager.py
line 54, the code uses self._asset: AssetBase
as a runtime type annotation, but AssetBase
is now only imported in the TYPE_CHECKING
block. This will cause a NameError
at runtime when this line executes. The annotation should be stringified as self._asset: "AssetBase"
or the import strategy needs adjustment to keep AssetBase
available at runtime.
Confidence: 2/5 - The core pattern is sound, but the runtime annotation bug in action_manager.py
is a blocking issue that will cause failures. Once fixed, this would be a clean optimization. The other files (scene_entity_cfg.py
and CONTRIBUTORS.md
) appear correct.
Additional Comments (1)
-
source/isaaclab/isaaclab/managers/action_manager.py
, line 54 (link)logic: Runtime type annotation without string quotes may cause NameError if
AssetBase
isn't imported at runtime. Consider using string annotationself._asset: "AssetBase"
or importing conditionally.
3 files reviewed, 1 comment
from dataclasses import MISSING | ||
from typing import TYPE_CHECKING | ||
|
||
if TYPE_CHECKING: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this logic to the end of the imports?
Description
This quality of life upgrade adds the ability to use the configuration classes without needing to load the kit dependencies.
Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists there