[Workflow] Move Isaac Lab implementation behind explicit public APIs - #7200
[Workflow] Move Isaac Lab implementation behind explicit public APIs#7200StafaH wants to merge 1 commit into
Conversation
|
Too many files changed for review (1863 files, 100 file limit). |
github-actions
Bot
added
documentation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The _src relocation and stub-driven lazy facades establish a coherent public/private split, but the transition removes previously importable concrete modules without the required deprecation period and exposes private cloner module objects under public names.
- Design and architecture: The facade architecture and relocated implementation paths are internally consistent. However, compatibility delegates are needed at established module paths so the migration complies with the repository’s deprecation policy and public module objects retain public identities.
- API: Previously importable and documented paths such as
isaaclab.assets.articulation.articulation_cfgare now intentionally unavailable without prior deprecation. In addition, publicisaaclab.cloner.pathandisaaclab.cloner.queryattributes resolve to modules whose identities remain underisaaclab._src; thin public forwarding modules should preserve both compatibility and public module identity. - Implementation: The lazy exporter correctly supports absolute named imports and caches resolved values, but exporting the private
pathandquerymodule objects makes their__name__values private and deterministically violates the existing cloner assertions. Compatibility modules at the old paths should warn and forward to the relocated implementation until a later removal.
Significant concerns. Posted 2 actionable findings inline.
The full PR diff was reviewed; some supplemental surrounding file context was omitted.
Automated review; human maintainers own approval decisions.
| import isaaclab.cloner.query # noqa: PLC0415 | ||
| from isaaclab.cloner.path import under # noqa: PLC0415 | ||
| from isaaclab.cloner.query import path_to_source # noqa: PLC0415 | ||
| import isaaclab._src.cloner.path # noqa: PLC0415 |
There was a problem hiding this comment.
🟡 Warning · Api — Preserve cloner module identity
These imports load isaaclab._src.cloner.path, whose __name__ is necessarily the private path, but the unchanged assertions require isaaclab.cloner.path. The facade likewise exports that private module object, so the test and public module identity fail deterministically. Keep public path and query delegate modules that forward their symbols instead of exposing the private module objects directly.
| def test_implementation_module_is_not_available_through_public_namespace(): | ||
| """Implementation modules should only be importable below ``isaaclab._src``.""" | ||
| code = """ | ||
| import importlib.util |
There was a problem hiding this comment.
🟡 Warning · Api — Preserve deprecated module imports
This added contract intentionally makes the existing isaaclab.assets.articulation.articulation_cfg import unavailable. Concrete modules were previously importable and documented, while repository policy requires prior deprecation before removing public APIs; a major changelog entry is not a compatibility period. Retain thin modules at the old paths that warn and re-export facade symbols, then remove them in a later release.
There was a problem hiding this comment.
Hahaha tough review. What's the timeline for this? Is this planned for GA? Lots of potential bugs in there. Maybe this is not aggressive enough? Why not move the backends modules (ov,newton,physx) to provide a similar interface. I see the code keeps the old modules and does some import re-directions is that the final plan? If that's the case I'm not sure this approach is fundamentally cleaner? Or is the PR missing the new public API?
| * Mingxue Gu | ||
| * Mingyu Lee | ||
| * Muhong Guo | ||
| * Mustafa Haiderbhai |
There was a problem hiding this comment.
Just add yourself in the maintainers :D
2 participants
Description
Moves the implementation of the
isaaclabsubpackages underisaaclab._src, following the public/private package structure used by Newton and MJWarp.Public subpackages now expose an explicit, stub-defined API through lightweight lazy facades. Internal Isaac Lab imports and downstream workspace packages use
_srconly for implementation details that are not part of those facades. Package data paths, documentation references, and source-root discovery were updated for the new layout.This is a breaking workflow/API cleanup: imports from concrete implementation modules must move to the nearest exported public package API. The changelog fragment documents the migration.
No new dependencies are required.
Type of change
Screenshots
Not applicable.
Validation
uv run isaaclab -fuv run --isolated --extra test -- make -C docs current-docsuv run --frozen pytest source/isaaclab/test/utils/test_module.py source/isaaclab/test/utils/test_configclass.py source/isaaclab/test/utils/test_string.py source/isaaclab/test/utils/test_dict.py source/isaaclab/test/utils/test_timer.py -q(91 passed)uv run --frozen python -m compileall -q source scripts tools appsisaaclabwheel and verified relocated Python modules, URDF data, and YAML dataChecklist
CONTRIBUTORS.md