[Startup] Avoid replicated Shadow Hand camera USD work - #7036
Conversation
Greptile SummaryThe PR reduces Shadow Hand startup work by constructing assets inside
Confidence Score: 4/5The PR should not merge until explicitly configured USD replication remains honored on the headless Newton Warp path. The optimization conflates automatic and explicit UsdReplicateContext entries, silently overriding a supported configuration that requests concrete USD clones. Files Needing Attention: source/isaaclab/isaaclab/cloner/replicate_session.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ReplicateSession queues assets and camera] --> B{Headless Newton with only newton_warp?}
B -->|No| C[Compose concrete USD destinations]
B -->|Yes| D[Build Newton model and publish clone plan]
D --> E[Camera resolves prototype paths from clone plan]
E --> F[Newton renderer creates per-environment views]
Reviews (1): Last reviewed commit: "Avoid redundant replicated camera USD wo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if replicate_physics and not automatic_usd_replication and renderer_type == "newton_warp": | ||
| contexts = [context for context in contexts if context is not UsdReplicateContext] |
There was a problem hiding this comment.
When a headless Newton Warp configuration explicitly includes UsdReplicateContext in cloning_contexts, this filter removes that context along with automatically selected contexts, causing the requested concrete per-environment USD prims to remain unauthored for stage-based consumers.
Knowledge Base Used: IsaacLab Core Simulation Layer
![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 startup optimization and ReplicateSession migration are coherent, but two cloner API contracts need correction: explicitly requested USD replication is suppressed for Newton Warp cameras, and UsdReplicateContext(stage) now defaults to reference composition instead of copy-spec semantics.
- Design and architecture: Using clone-plan prototypes for headless Newton cameras is consistent with the optimization goal. However, the dispatch layer must preserve explicit cloning contexts as an override, and reference-based replication should be an explicit backend choice rather than an implicit default for every direct context user.
- API: The filter in
replicate()contradicts the documented guarantee that explicit contexts are honored by removing a caller-suppliedUsdReplicateContext. Separately, changing the defaultUsdReplicateContextbehavior from independent copied specs to live internal references materially changes existing callers, whileusd_replicate()explicitly retains the previous behavior. - Implementation: The reorientation environments correctly move asset construction into
ReplicateSession, and the camera/Newton paths consistently consume clone-plan prototypes. Before merge, retain caller-provided USD contexts and either preserve copy-spec behavior as theUsdReplicateContextdefault or provide the required compatibility and migration treatment for the semantic change.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| @@ -87,10 +120,13 @@ def replicate(plan: ClonePlan, *, stage: Usd.Stage, replicate_physics: bool = Tr | |||
| contexts = [backend_physics_ctx] if backend_physics_ctx else [] | |||
| else: | |||
| contexts = [string_to_callable(c) if isinstance(c, str) else c for c in cfg.cloning_contexts] | |||
| renderer_type = getattr(getattr(cfg, "renderer_cfg", None), "renderer_type", None) | |||
| if replicate_physics and not automatic_usd_replication and renderer_type == "newton_warp": | |||
There was a problem hiding this comment.
🟡 Warning · Api — Explicit UsdReplicateContext silently dropped
When cloning_contexts is unset, contexts holds only the physics context, so this filter can only strip a UsdReplicateContext the caller listed explicitly. That contradicts the docstring retained at line 85 ("Explicit contexts are honored") and leaves no way to request concrete USD destinations in a headless Newton camera run. Gate only the automatic setdefault at line 129 and leave caller-provided contexts intact.
|
|
||
| replicate_priority = 100 | ||
|
|
||
| def __init__(self, stage: Usd.Stage): | ||
| def __init__(self, stage: Usd.Stage, *, use_references: bool = True): |
There was a problem hiding this comment.
🟡 Warning · Api — Public context default flips to references
UsdReplicateContext(stage) previously authored an independent Sdf.CopySpec per destination; it now composes internal references, so destination specs no longer carry copied properties and later source-layer edits propagate to clones. usd_replicate opting out with use_references=False shows the semantics differ materially. Keep use_references=False as the default and opt in at the backend dispatch site, or document the change and migration in the changelog fragment.
Spawn the Shadow Hand, cube, and camera only in the source environment so the existing clone plan owns replication. Keep regex prim paths for runtime views while avoiding redundant USD authoring at large environment counts.
96e5315 to
ce4a492
Compare
|
Reworked the fix so prototype-spawn ownership stays in the task configuration. The Shadow Hand robot, cube, and tiled camera retain their regex These are fresh measurements from the replacement implementation, not the measurements previously reported on this PR. Both runs used the same RTX 5090 / Threadripper PRO 7965WX machine, 4,096 environments, and warm local asset and kernel caches. Hub failed to launch on this machine and added an unrelated approximately 12.7-second CloudFront metadata lookup through
Raw wall-clock values were 36.11 s → 16.90 s for scene creation and 57.84 s → 33.03 s for environment creation. Profiled Run provenance:
The fixed run completed environment startup, reset, and the first simulation step. The full |
2 participants
Spawns the Shadow Hand robot, cube, and tiled camera only under
/World/envs/env_0through each asset spawnerspawn_path. The assetprim_pathremains a regex for runtime views, and the existing scene clone plan owns replication to the remaining environments.This keeps prototype-spawn ownership in task configuration and avoids task-side config rewriting or global spawner changes.
The following 4,096-environment measurements exclude
omni.client.statasset-resolution overhead. Hub failed to launch on the benchmark machine, causing an unrelated approximately 12.7-second CloudFront metadata lookup in both runs.d00e811e)ce4a492)For reproducibility, the raw wall-clock values were 36.11 s → 16.90 s for scene creation and 57.84 s → 33.03 s for environment creation. The profiled
omni.client.stattime was 12.69 s in the base run and 12.68 s in the fixed run.Measured on the same RTX 5090 / Threadripper PRO 7965WX machine with warm local asset and kernel caches:
Validation:
./isaaclab.sh -f.