feat: add FabricStageCache service for shared hierarchy handles#5676
feat: add FabricStageCache service for shared hierarchy handles#5676pv-nvidia wants to merge 1 commit into
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR introduces a well-designed caching mechanism for Fabric stage attachments and hierarchy handles. The service locator pattern with SimulationContext.services is clean and type-safe.
✅ Strengths
- Clean architecture: The
FabricStageCacheservice with automatic cleanup onclear_instance()is well-designed - Good documentation: Docstrings are thorough with cross-references to related classes
- Better error handling: Replacing
assertwithRuntimeErrorfor the device check is correct (not stripped underpython -O) - Future-proofing: The dict-keyed hierarchy cache anticipates multi-GPU scenarios
- Comprehensive tests: 7 unit tests covering service lifecycle (register, retrieve, cache, close,
clear_instance, replacement)
📁 Files Changed
| File | Changes |
|---|---|
fabric_stage_cache.py |
+68 lines — new cache class with usdrt.Usd.Stage attachment and IFabricHierarchy handle caching |
fabric_frame_view.py |
+21/-8 lines — refactored to use FabricStageCache via SimulationContext.services |
test_fabric_stage_cache.py |
+101 lines — comprehensive service lifecycle tests |
fabric-stage-cache.rst |
+10 lines — changelog fragment |
✅ CI Status
- pre-commit: ✅ Passing
- changelog-check: ✅ Passing
- Build Wheel: ✅ Passing
- license-check: ⏳ Pending
- Installation Tests: ⏳ Pending
- Docs Build: ⏳ Pending
💡 Suggestions (Non-blocking)
Defensive check in get_hierarchy()
If close() is called and then get_hierarchy() is invoked (e.g., due to a stale reference), it will crash with AttributeError on self._stage.GetFabricId(). Consider:
def get_hierarchy(self):
if self._stage is None:
raise RuntimeError("FabricStageCache has been closed")
# ... rest of methodThis provides a clearer error message for debugging stale reference issues.
Consider an integration test for multi-view sharing
The unit tests verify the service lifecycle well. An integration test confirming that two FabricFrameView instances share the same hierarchy handle would strengthen confidence:
def test_multiple_frame_views_share_hierarchy(self):
"""Multiple FabricFrameView instances share the same hierarchy handle."""
view1 = FabricFrameView(prim_paths=["/World/robot1"])
view2 = FabricFrameView(prim_paths=["/World/robot2"])
assert view1._fabric_hierarchy is view2._fabric_hierarchyDependencies
- Requires #5672 (service locator on SimulationContext) to be merged first
Summary
Clean, focused PR with good architecture and test coverage. Assuming CI tests pass and #5672 merges first, this is ready to go. 👍
Automated review by Isaac Lab Review Bot — updated for SHA ae83c65
pv-nvidia
force-pushed
the
pv/fabric-stage-cache
branch
from
3875437 to
068e120
Compare
pv-nvidia
force-pushed
the
pv/fabric-stage-cache
branch
3 times, most recently
from
3ad6108 to
575be92
Compare
## Motivation `SimulationContext` is the natural lifecycle owner for backend-specific caches (e.g. UsdRT stage handles, Fabric hierarchy data). Currently these either live as class-level globals (no lifecycle, leak across stages) or get baked directly into SimulationContext (pollutes it with backend imports). ## Solution Add a lightweight typed `ServiceLocator` exposed via `SimulationContext.services`. Backends register their own singletons using subscript syntax: ```python sim.services[FabricStageCache] = FabricStageCache(stage) cache = sim.services[FabricStageCache] del sim.services[FabricStageCache] # closes and removes ``` All registered services are closed when `clear_instance()` is called. Exceptions during close are collected and raised after full teardown completes. ## Design - Keyed by service class (typed retrieval) - Services with a `close()` method are automatically closed on deletion or teardown - `close_all(caught_exceptions)` always collects — no silent failures - Purely additive — no existing behavior changes ## Downstream This is used by the Fabric stage cache PR (#5676) to manage `IFabricHierarchy` handles per stage.
pv-nvidia
force-pushed
the
pv/fabric-stage-cache
branch
3 times, most recently
from
ae83c65 to
becd3ff
Compare
There was a problem hiding this comment.
Code Review Summary — Updated for SHA becd3ff
This PR has been rebased onto develop and the implementation remains well-designed. The FabricStageCache service properly manages shared hierarchy handles across multiple FabricFrameView instances.
✅ Strengths
- Clean architecture: The
FabricStageCacheservice with automatic cleanup onclear_instance()is well-designed - Good documentation: Docstrings are thorough with cross-references to related classes
- Better error handling: Replacing
assertwithRuntimeErrorfor the device check is correct (not stripped underpython -O) - Future-proofing: The dict-keyed hierarchy cache anticipates multi-GPU scenarios
- Comprehensive tests: 7 unit tests covering service lifecycle (register, retrieve, cache, close,
clear_instance, replacement)
📁 Files Changed
| File | Changes |
|---|---|
fabric_stage_cache.py |
+67 lines — new cache class with usdrt.Usd.Stage attachment and IFabricHierarchy handle caching |
fabric_frame_view.py |
+16/-6 lines — refactored to use FabricStageCache via SimulationContext.services |
test_fabric_stage_cache.py |
+101 lines — comprehensive service lifecycle tests |
fabric-stage-cache.rst |
+10 lines — changelog fragment |
✅ CI Status
- pre-commit: ✅ Passing
- changelog-check: ✅ Passing
- Build Wheel: ✅ Passing
- license-check: ⏳ Pending
- Installation Tests: ⏳ Pending
- Docs Build: ⏳ Pending
💡 Suggestions (Non-blocking)
Defensive check in get_hierarchy()
If close() is called and then get_hierarchy() is invoked (e.g., due to a stale reference), it will crash with AttributeError on self._stage.GetFabricId(). Consider:
def get_hierarchy(self):
if self._stage is None:
raise RuntimeError("FabricStageCache has been closed")
# ... rest of methodThis provides a clearer error message for debugging stale reference issues.
Consider an integration test for multi-view sharing
The unit tests verify the service lifecycle well. An integration test confirming that two FabricFrameView instances share the same hierarchy handle would strengthen confidence:
def test_multiple_frame_views_share_hierarchy(self):
"""Multiple FabricFrameView instances share the same hierarchy handle."""
view1 = FabricFrameView(prim_paths=["/World/robot1"])
view2 = FabricFrameView(prim_paths=["/World/robot2"])
assert view1._fabric_hierarchy is view2._fabric_hierarchyDependencies
- Requires #5672 (service locator on SimulationContext) — now merged into develop ✅
Summary
Clean, focused PR with good architecture and test coverage. The rebase onto develop (which now includes #5672) means this PR is ready to land once CI passes. 👍
Automated review by Isaac Lab Review Bot — updated for SHA becd3ff
pv-nvidia
force-pushed
the
pv/fabric-stage-cache
branch
from
becd3ff to
4c262b9
Compare
There was a problem hiding this comment.
Code Review Summary — Updated for SHA 4c262b9
This PR has been rebased onto the latest develop branch. The implementation remains well-designed. The FabricStageCache service properly manages shared hierarchy handles across multiple FabricFrameView instances.
✅ Strengths
- Clean architecture: The
FabricStageCacheservice with automatic cleanup onclear_instance()is well-designed - Good documentation: Docstrings are thorough with cross-references to related classes
- Better error handling: Replacing
assertwithRuntimeErrorfor the device check is correct (not stripped underpython -O) - Future-proofing: The dict-keyed hierarchy cache anticipates multi-GPU scenarios
- Comprehensive tests: 7 unit tests covering service lifecycle (register, retrieve, cache, close,
clear_instance, replacement)
📁 Files Changed
| File | Changes |
|---|---|
fabric_stage_cache.py |
+68 lines — new cache class with usdrt.Usd.Stage attachment and IFabricHierarchy handle caching |
fabric_frame_view.py |
+16/-6 lines — refactored to use FabricStageCache via SimulationContext.services |
test_fabric_stage_cache.py |
+101 lines — comprehensive service lifecycle tests |
fabric-stage-cache.rst |
+10 lines — changelog fragment |
✅ CI Status
- pre-commit: ✅ Passing
- changelog-check: ✅ Passing
- Build Wheel: ✅ Passing
- Check for Broken Links: ❌ Failed (unrelated to this PR — likely repo-wide doc link issue)
- license-check: ⏳ Pending
- Installation Tests: ⏳ Pending
- Docs Build: ⏳ Pending
💡 Suggestions (Non-blocking)
Defensive check in get_hierarchy()
If close() is called and then get_hierarchy() is invoked (e.g., due to a stale reference), it will crash with AttributeError on self._stage.GetFabricId(). Consider:
def get_hierarchy(self):
if self._stage is None:
raise RuntimeError("FabricStageCache has been closed")
# ... rest of methodThis provides a clearer error message for debugging stale reference issues.
Consider an integration test for multi-view sharing
The unit tests verify the service lifecycle well. An integration test confirming that two FabricFrameView instances share the same hierarchy handle would strengthen confidence:
def test_multiple_frame_views_share_hierarchy(self):
"""Multiple FabricFrameView instances share the same hierarchy handle."""
view1 = FabricFrameView(prim_paths=["/World/robot1"])
view2 = FabricFrameView(prim_paths=["/World/robot2"])
assert view1._fabric_hierarchy is view2._fabric_hierarchySummary
Clean, focused PR with good architecture and test coverage. The rebase onto develop keeps this PR current. Ready to merge once CI passes. 👍
Automated review by Isaac Lab Review Bot — updated for SHA 4c262b9
|
Superseded by the consolidated PR #5728 (pv/fabric-full-stack). |
pv-nvidia
force-pushed
the
pv/fabric-stage-cache
branch
from
4c262b9 to
612274a
Compare
Introduces FabricStageCache — a lightweight cache for the usdrt stage attachment and IFabricHierarchy handles, registered as a service on SimulationContext via the service locator (set_service/get_service). Multiple FabricFrameView instances now share a single hierarchy handle instead of each creating its own. The cache is automatically closed on SimulationContext.clear_instance(). Also replaces the assert device check with a proper RuntimeError and removes the now-unused isaaclab.sim import from fabric_frame_view.py.
pv-nvidia
force-pushed
the
pv/fabric-stage-cache
branch
from
612274a to
f3d4667
Compare
1 participant
Motivation
FabricFrameViewcurrently creates its ownusdrt.Usd.Stage.Attach()andIFabricHierarchyhandle on every initialization. When multiple views exist (common in multi-asset environments), this duplicates work and creates independent hierarchy handles that don't share change-tracking state.Solution
Introduce
FabricStageCache— a service registered onSimulationContextvia the typed service locator (#5672, already merged). It caches:usdrt.Usd.Stageattachment (already synchronized to Fabric)IFabricHierarchyhandles keyed by Fabric attachment IDMultiple
FabricFrameViewinstances now share a single hierarchy handle. The cache is automatically closed whenSimulationContext.clear_instance()is called.Changes
FabricStageCache: new class inisaaclab_physx.simFabricFrameView._initialize_fabric(): refactored to useFabricStageCacheviaSimulationContext.get_service()/set_service()usdrt.Usd.Stage.Attach()andIFabricHierarchycreation fromFabricFrameViewTests
FabricStageCachelifecycle (register, retrieve, cache, close, clear_instance, replacement)