Route clone backends through ClonePlan - #7462
Conversation
|
Too many files changed for review (295 files, 100 file limit). Bypass the limit by tagging |
![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 flattened ClonePlan/ReplicateSession lifecycle is coherent overall, but cfg discovery can incorrectly turn sensor visualizer configs into conflicting plan rows, camera spawning removes established redirect/existence behavior, the RayCasterCfg.spawn default change lacks migration guidance, and startup profiling still targets the removed dispatch name.
- Design and architecture: Publishing one immutable plan before construction and dispatching it to registered backends provides a single description of clone ownership. However,
_plan_cfgsalso classifies visualizer marker configs as prim authors; sensors sharing a default marker path can therefore fail planning with duplicate ownership. Visualizer-only configs should be excluded from clone-plan rows. - API: The major changelog covers the removed cloner entry points and most cfg/spawner field removals. It does not cover the
RayCasterCfg.spawndefault changing fromSensorFrameCfg()toNone. Camera construction also drops the previous physics-body-to-child redirect and existing-prim guard, changing behavior for publicCameraCfgusages that target bodies or pre-authored cameras. - Implementation: Plan construction, source-path resolution, and backend dispatch were traced. The remaining implementation integration issue is the startup profiler whitelist still matching
cloner.*:replicateafter dispatch was renamed to_replicate, causing clone-dispatch measurements to be reported as zero placeholders.
Significant concerns. Posted 4 actionable findings inline.
The full PR diff was reviewed; some supplemental surrounding file context was omitted.
Automated review; human maintainers own approval decisions.
| declared = set(field_names) | ||
| spawn = value.spawn if "spawn" in declared else None | ||
| if "prim_path" in declared and ( | ||
| spawn is not None or "collision_group" in declared or "markers" in declared |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Visualizer marker cfgs become conflicting plan rows
_plan_cfgs treats any cfg with prim_path plus markers as a prim author, so each sensor's visualizer_cfg becomes a plan row when debug_vis=True. Two sensors keeping the shared default visualizer path — pva_LF and pva_RF in scripts/demos/sensors/pva_sensor.py, which this PR migrated to ReplicateSession — then abort planning with the duplicate-destination (or duplicate-binding) error. Exclude visualizer marker cfgs from planning.
|
|
||
| spawn_target = spawn.spawn_path or self.cfg.prim_path | ||
| if sim_utils.find_first_matching_prim(spawn_target) is None: | ||
| source_paths = cloner.query.cfg_source_paths(self._clone_plan, self._source_cfg) |
There was a problem hiding this comment.
🟡 Warning · Api — Camera spawn drops redirect and existence guard
The constructor no longer redirects a camera whose prim_path names a rigid body or articulation root to a /camera child, and it now spawns unconditionally instead of skipping when the prim already exists. A CameraCfg with a spawner pointing at an existing body or pre-authored camera prim therefore changes behavior or fails. Restore the physics-body child resolution and the existence guard per planned source path.
| continue | ||
| if cfg.cloning_contexts is None: | ||
| contexts = [backend_physics_ctx] | ||
| def _replicate(plan: ClonePlan, *, replicate_physics: bool = True) -> None: |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Startup whitelist misses renamed clone dispatch
replicate was renamed to _replicate, but scripts/benchmarks/startup_whitelist.yaml still whitelists cloner.*:replicate. parse_cprofile_stats matches labels by fnmatch, and cloner.replicate_session:_replicate does not end with :replicate, so the entry now emits a zero placeholder and clone-dispatch time silently disappears from the env_creation startup profile used for the performance gate. Update the pattern.
|
|
||
| spawn: SensorFrameCfg | None = SensorFrameCfg() | ||
| """Spawn configuration for the sensor Xform prim. | ||
| spawn: SensorFrameCfg | None = None |
There was a problem hiding this comment.
🔵 Suggestion · Api — RayCaster spawn default flip lacks changelog note
RayCasterCfg.spawn changes from SensorFrameCfg() to None, so a config that previously auto-created its sensor frame at prim_path now requires the prim to exist and be covered by a plan row; in-tree callers had to add spawn=sim_utils.SensorFrameCfg(). The isaaclab changelog fragment documents the other breaking cfg changes but omits this default, leaving external users without migration guidance.
ooctipus
changed the title
5c3ba34 to
5f7619e
Compare
ooctipus
changed the title
|
PR 3 has now been split at the execution/lifecycle boundary. This PR contains only the existing Accordingly, the earlier automated inline findings on |
1 participant
Description
This is a narrow follow-up to #7453. That PR removes Newton's full post-replication entity-label pass; this PR preserves that fast path and makes the existing
ClonePlanthe single mapping consumed by USD, Newton, PhysX, and OvPhysX replication.Planning records
context_rows, which maps each simulation-owned clone-context type to the plan rows it consumes.cloner.replicate(plan)retrieves those already-registered contexts fromSimulationContext, orders them by replication priority, and passes each the same plan. The backends no longer rebuild that mapping through localqueue(...)orqueue_mapping(...)state.The public structure stays small:
ClonePlan.global_pathsremains the explicit declaration for shared scene prims; globals are not encoded as artificial zero-mask rows.ClonePlan.env_idsandpositionsremain optional for query-only plans; execution validates what it needs.cloning_contexts,cfg_rows,ReplicateSession, and the current InteractiveScene/direct construction lifecycle remain.usd_replicate(...),newton_physics_replicate(...),physx_replicate(...), andovphysx_replicate(...).Migration
Custom clone contexts now implement
replicate(plan)and must be registered withSimulationContext.get_or_create_backend(...)before dispatch. Built-in physics managers exposeclone_context_typeand register their own context during initialization. Remove context-localqueue(...)/queue_mapping(...)calls and the former backendPHYSICS_CONTEXTaliases; raw standalone replication functions remain available.The high-level
stage=argument is removed fromcloner.replicate(...)andReplicateSession; each simulation-owned context already owns its stage.Size
Both pieces of the stack are net-negative:
#7462 currently carries the exact #7453 fast-path files so it can be tested independently. They will disappear from this diff when #7453 merges and this branch is restacked.
Performance
All measurements used physical GPU 0 and the same Newton revision. The targeted benchmark warms with 128 worlds, then times replication and all required label publication for one 48-body/48-shape Newton prototype at 4,096 worlds (two samples):
The follow-up is 0.8% from #7453, within run-to-run noise, and preserves the removed-pass gain. The production Newton path still prefixes entity labels inside
ModelBuilder.replicate(...)and does not add a later whole-builder label pass.One uncontended representative warm verification used 4,096 Kuka Allegro environments, headless Newton MJWarp physics, no renderer, no interactive visualizer, and one step:
CUDA_VISIBLE_DEVICES=0 /usr/bin/time -f 'WALL_TIME_S=%e' \ uv run --no-sync isaaclab zero_agent \ --task Isaac-Lift-KukaAllegro --num_envs 4096 --max_steps 1 \ --device cuda:0 --visualizer none physics=newton_mjwarpThe targeted path and representative whole workload show no startup regression.
Type of change
Validation
git diff --checkpassedRelease backport
developChecklist
CONTRIBUTORS.md