Honor replicate_physics via cfg-registered replication lists#6550
Honor replicate_physics via cfg-registered replication lists#6550ooctipus wants to merge 10 commits into
Conversation
ff7f851 to
85917aa
Compare
c792488 to
7845667
Compare
|
@greptile-apps review again |
|
@greptile-apps review again |
Greptile SummaryThis PR restores
Confidence Score: 4/5The core propagation of replicate_physics from scene cfg through to the drain is correct, but the physics-filter logic in replicate() drops UsdReplicateContext from explicit cloning_contexts entries when spawn is absent, contradicting the documented USD-only guarantee and causing the new test to fail on that code path. The mGPU double-free fix and the end-to-end flag-threading are solid. The one issue is in the replicate_physics=False branch of replicate(): it zeroes out ALL resolved contexts then re-adds UsdReplicateContext only via the spawn heuristic. A cfg that explicitly sets cloning_contexts=(UsdReplicateContext, PhysicsCtx) but has no spawn loses its USD replication entirely. The accompanying test test_replicate_physics_false_keeps_usd_only exposes this because its SimpleNamespace cfg carries no spawn — the IsValid() assertion on env_1 would fail, meaning the regression guard it is supposed to provide does not actually run. source/isaaclab/isaaclab/cloner/replicate_session.py (lines 87-91, the physics-filter block) and source/isaaclab/test/sim/test_cloner.py (test_replicate_physics_false_keeps_usd_only, which needs a spawn attribute on the test cfg or a corrected filter). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Scene as InteractiveScene
participant CC as CloneCfg
participant RS as ReplicateSession
participant AB as AssetBase.__init__
participant RQ as REPLICATION_QUEUE
participant Rep as replicate()
participant Ctx as BackendCtxCls
Scene->>CC: "CloneCfg(replicate_physics=cfg.replicate_physics)"
Scene->>RS: "ReplicateSession(..., replicate_physics=cloner_cfg.replicate_physics)"
RS->>RS: __enter__: make_clone_plan(cfgs)
loop For each asset constructor
AB->>RQ: queue_replication(original_cfg)
end
RS->>Rep: __exit__: replicate(plan, stage, replicate_physics)
loop For each cfg in queue
Rep->>Rep: resolve contexts from cfg.cloning_contexts or PHYSICS_CONTEXT
alt "replicate_physics=False"
Rep->>Rep: filter keep only UsdReplicateContext
end
alt cfg.spawn set and has_kit()
Rep->>Rep: setdefault(UsdReplicateContext)
end
Rep->>Ctx: queue_mapping(sources, destinations, ...)
end
Rep->>Ctx: replicate() per sorted priority
Rep->>Scene: set_clone_plan(plan)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Scene as InteractiveScene
participant CC as CloneCfg
participant RS as ReplicateSession
participant AB as AssetBase.__init__
participant RQ as REPLICATION_QUEUE
participant Rep as replicate()
participant Ctx as BackendCtxCls
Scene->>CC: "CloneCfg(replicate_physics=cfg.replicate_physics)"
Scene->>RS: "ReplicateSession(..., replicate_physics=cloner_cfg.replicate_physics)"
RS->>RS: __enter__: make_clone_plan(cfgs)
loop For each asset constructor
AB->>RQ: queue_replication(original_cfg)
end
RS->>Rep: __exit__: replicate(plan, stage, replicate_physics)
loop For each cfg in queue
Rep->>Rep: resolve contexts from cfg.cloning_contexts or PHYSICS_CONTEXT
alt "replicate_physics=False"
Rep->>Rep: filter keep only UsdReplicateContext
end
alt cfg.spawn set and has_kit()
Rep->>Rep: setdefault(UsdReplicateContext)
end
Rep->>Ctx: queue_mapping(sources, destinations, ...)
end
Rep->>Ctx: replicate() per sorted priority
Rep->>Scene: set_clone_plan(plan)
Reviews (2): Last reviewed commit: "Rename _PHYSICS_CONTEXT to PHYSICS_CONTE..." | Re-trigger Greptile |
| if not replicate_physics: | ||
| contexts = [] | ||
| ctx_set = dict.fromkeys(contexts) | ||
| if getattr(cfg, "spawn", None) is not None and has_kit(): | ||
| ctx_set.setdefault(UsdReplicateContext, None) |
There was a problem hiding this comment.
replicate_physics=False silently drops explicit UsdReplicateContext entries
The PR description states "every context except UsdReplicateContext is dropped" when replicate_physics=False, but the implementation first clears ALL contexts unconditionally (contexts = []), then re-adds UsdReplicateContext only when cfg.spawn is not None and has_kit(). A cfg that explicitly sets cloning_contexts=(UsdReplicateContext, PhysicsCtx) but has spawn=None — exactly the shape used in test_replicate_physics_false_keeps_usd_only — ends up with no context at all: the FakePhysicsCtx is correctly dropped but UsdReplicateContext is also lost, so env_1 never gets created. The test's final assertion stage.GetPrimAtPath("/World/envs/env_1/Robot").IsValid() would fail on that code path.
The correct filter keeps only UsdReplicateContext from the resolved context list rather than zeroing it:
if not replicate_physics:
contexts = [c for c in contexts if c is UsdReplicateContext]The subsequent ctx_set.setdefault(UsdReplicateContext, None) call is then a safe no-op when UsdReplicateContext is already present, and the spawn-based heuristic still adds it for cfgs that rely on the default stack.
Greptile SummaryThis PR restores the long-broken
Confidence Score: 4/5Safe to merge after addressing one defect: any asset that sets The overall architecture is sound and the primary regression is correctly addressed. The single concrete defect is in source/isaaclab/isaaclab/cloner/replicate_session.py — the spawn-check at lines 89-91 needs to be gated on Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Scene as InteractiveScene
participant CloneCfg as CloneCfg
participant Session as ReplicateSession
participant Asset as AssetBase.__init__
participant Plan as ClonePlan.from_env_0
participant Q as REPLICATION_QUEUE
participant Rep as replicate()
Scene->>CloneCfg: "CloneCfg(replicate_physics=cfg.replicate_physics)"
Scene->>Session: "enter (replicate_physics=cloner_cfg.replicate_physics)"
Session->>Plan: make_clone_plan(cfgs)
loop For each asset constructor
Asset->>Q: queue_replication(cfg)
end
Session->>Plan: ClonePlan.from_env_0(REPLICATION_QUEUE)
Note over Plan: cfg_rows keyed by id(cfg)
Session->>Rep: replicate(plan, stage, replicate_physics)
Rep->>Q: snapshot + clear REPLICATION_QUEUE
loop For each cfg in snapshot
alt cloning_contexts is None
Rep->>Rep: "contexts = [PHYSICS_CONTEXT]"
else cloning_contexts is set
Rep->>Rep: "contexts = resolve(cloning_contexts)"
end
alt replicate_physics is False
Rep->>Rep: "contexts = [] (all cleared)"
end
alt spawn is not None AND has_kit()
Rep->>Rep: ctx_set.setdefault(UsdReplicateContext)
end
Rep->>Rep: backend_rows[Ctx].update(rows)
end
loop For each backend context (sorted by priority)
Rep->>Rep: ctx.queue_mapping(sources, dests, env_ids)
Rep->>Rep: ctx.replicate()
end
Rep->>Scene: set_clone_plan(plan)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Scene as InteractiveScene
participant CloneCfg as CloneCfg
participant Session as ReplicateSession
participant Asset as AssetBase.__init__
participant Plan as ClonePlan.from_env_0
participant Q as REPLICATION_QUEUE
participant Rep as replicate()
Scene->>CloneCfg: "CloneCfg(replicate_physics=cfg.replicate_physics)"
Scene->>Session: "enter (replicate_physics=cloner_cfg.replicate_physics)"
Session->>Plan: make_clone_plan(cfgs)
loop For each asset constructor
Asset->>Q: queue_replication(cfg)
end
Session->>Plan: ClonePlan.from_env_0(REPLICATION_QUEUE)
Note over Plan: cfg_rows keyed by id(cfg)
Session->>Rep: replicate(plan, stage, replicate_physics)
Rep->>Q: snapshot + clear REPLICATION_QUEUE
loop For each cfg in snapshot
alt cloning_contexts is None
Rep->>Rep: "contexts = [PHYSICS_CONTEXT]"
else cloning_contexts is set
Rep->>Rep: "contexts = resolve(cloning_contexts)"
end
alt replicate_physics is False
Rep->>Rep: "contexts = [] (all cleared)"
end
alt spawn is not None AND has_kit()
Rep->>Rep: ctx_set.setdefault(UsdReplicateContext)
end
Rep->>Rep: backend_rows[Ctx].update(rows)
end
loop For each backend context (sorted by priority)
Rep->>Rep: ctx.queue_mapping(sources, dests, env_ids)
Rep->>Rep: ctx.replicate()
end
Rep->>Scene: set_clone_plan(plan)
Reviews (3): Last reviewed commit: "Rename _PHYSICS_CONTEXT to PHYSICS_CONTE..." | Re-trigger Greptile |
Since the replication-session refactor, InteractiveSceneCfg.replicate_physics was ignored: scenes configured with replicate_physics=False invoked native physics replication anyway, silently discarding per-environment USD differences (identified in isaac-sim#6541). The flag now lives where the policy executes: - CloneCfg.replicate_physics is the policy's home; InteractiveScene pipes its cfg flag into it and forwards it through ReplicateSession to replicate(). - REPLICATION_QUEUE holds bare asset cfgs: construction only registers which cfgs participate (required by ClonePlan.from_env_0 in direct workflows and to distinguish clonable assets from lights/terrain). - replicate() resolves how each cfg clones at dispatch: the cfg's cloning_contexts field when set, otherwise the active backend's default stack exported as isaaclab_<backend>.cloner.REPLICATION. PhysX pairs physics replication with USD clones (collision groups are authored on the per-env prims; PhysX has no kitless mode), Newton includes USD only under Kit, and OvPhysX replicates alone since its clone replay authors USD. - With replicate_physics=False, cloning is USD-only: the physics engine parses the per-env USD prims directly; an asset whose contexts are all physics-based is simply not cloned. Asset implementations know nothing about cloning beyond registering their cfg; per-asset overrides are a cfg assignment. The per-backend queue_*_replication helpers are removed in favor of this single path.
For scenes where every source maps only to its own environment (one variant per env, fully-heterogeneous 1:1), calling rep.replicate() once per source with a single self-target intermittently triggers native heap corruption (double-free / SIGABRT) under mGPU. Detect this layout by checking that all queued rows are self-only and skip register_replicator entirely. PhysX then parses the source prims directly from the stage, which is both correct and safe. Reproducer: mgpu_repro.zip -- heterogeneous mGPU with 192 variants and 4 GPUs, replicate_physics=True.
Three test fixes: - test_usd_replicate_self_copy_skips_copy_spec: patch isaaclab.cloner.usd.Sdf instead of cloner_utils.Sdf (Sdf moved to usd module after refactor) - test_resolve_clone_plan_source_partial_coverage_raises: rename and update to assert new behavior (returns first active source, no longer raises NotImplementedError for partial coverage) - test_collect_asset_cfgs_resolves_env_regex_macros / orders_sensors_last: set scene._env_ns when bypassing __init__ (required by _collect_asset_cfgs after env_ns was promoted to a property backed by _env_ns)
Each backend asset class now declares _PHYSICS_CLONING_CONTEXT for its physics context. AssetBase.__init__ stamps it onto cfg.cloning_contexts when the user hasn't set one. replicate() auto-adds UsdReplicateContext via dict.fromkeys() when cfg.spawn is not None and has_kit(), so USD replication is never hard-coded anywhere. SensorBaseCfg.cloning_contexts default changed from explicit USD string to None — sensors with spawn get USD automatically, contact sensors (spawn=None) get nothing. OvPhysxReplicateContext keeps its monolithic shape for now; a TODO marks the future decomposition into separate USD + physics contexts.
…ner" This reverts commit fddbe75.
Replace the REPLICATION=(USD, Physics) tuples in each backend cloner with a private _PHYSICS_CONTEXT pointing only to the physics context class. replicate() looks up _PHYSICS_CONTEXT when cloning_contexts is None, then auto-adds UsdReplicateContext via dict.fromkeys() when cfg.spawn is not None and has_kit(). No asset class files touched.
contexts = [] was unconditionally zeroing the resolved list, losing any UsdReplicateContext that was explicitly set in cloning_contexts when spawn=None (so the spawn+Kit heuristic would not re-add it). Filter to keep only UsdReplicateContext entries instead.
The partial-coverage test was rewritten to expect resolve_clone_plan_source to return the first active row's source, but the matching production change was omitted, so the old NotImplementedError guard still fired in CI. Replace the guard with selection of the first matching row that has any active env, returning None when none is active so callers fall back to direct stage resolution. The destination glob resolves only to the envs that actually received the asset.
The REPLICATION tuple was removed in favor of a single backend PHYSICS_CONTEXT, with UsdReplicateContext now added per spawned cfg by isaaclab.cloner.replicate() when Kit is available, but the contrib test still imported the deleted tuple. Assert the Newton physics context directly; the Kit-gated USD behavior is covered by the replicate session tests in test_cloner.py.
02fa39e to
1403655
Compare
Description
Since the replication-session refactor (#5770),
InteractiveSceneCfg.replicate_physicshas been ignored: scenes configured withreplicate_physics=Falseinvoked native physics replication anyway, silently discarding per-environment USD differences (e.g. prestartup scale randomization). Identified by @nblauch in #6541 with a multi-GPU heterogeneous-cloning repro.This PR restores the flag as cloner-side policy directed by asset cfgs:
CloneCfg.replicate_physicsis the policy's home.InteractiveScenepipes its cfg flag into it and forwards it throughReplicateSessiontoreplicate()— the scene contains no policy code.REPLICATION_QUEUEholds bare asset cfgs. Construction only registers which cfgs participate (queue_replication(cfg)); this stays at construction becauseClonePlan.from_env_0scans the queue after constructors run in direct workflows, and because construction distinguishes clonable assets from lights/terrain.replicate()resolves how each cfg clones at dispatch: the cfg'scloning_contextsfield when set (None→ backend default,()→ not cloned), otherwise the active backend's default stack, exported under the conventional nameisaaclab_<backend>.cloner.REPLICATION:(UsdReplicateContext, PhysxReplicateContext)— USD clones are required beyond rendering (collision groups are authored on the per-env prims), and PhysX has no kitless mode.(Usd, Newton)under Kit,(Newton,)kitless — headless runs skip the USD authoring cost, with the Kit check made at import under the documented app-first timing contract.(OvPhysxReplicateContext,)— its clone replay authors USD itself.replicate_physics=False, cloning is USD-only: every context exceptUsdReplicateContextis dropped and the physics engine parses the per-env USD prims directly. An asset whose contexts are all physics-based is simply not cloned; if that matters, it surfaces at asset initialization. The Newton limitation is documented on the cfg.MPMObjectCfgpins Newton-only contexts because particle rendering syncs through Fabric).queue_*_replicationhelpers are removed in favor of the single path (changelogs carry the migration; fragments are.majorfor the affected packages).Relation to #6541: same regression, alternative mechanism, per the design discussion — policy lives in the cloner cfg rather than the dispatcher signature, and asset cfgs are the single source of truth for how an asset clones.
Validation
test_cloner.py: 52 passed — queue/drain/dedup/priority under the bare-cfg shape, a drain-level USD-only filter test, and plan-publish forwarding of the flagtest_interactive_scene.py: spy-based test asserts the PhysX replicator is registered withreplicate_physics=Trueand NOT registered withFalsewhile envs still simulate; verified to fail with the drain filter removed (regression rule); cfg-override test coverscloning_contexts(Usd, Newton)under Kit,(Newton,)kitlessType of change
Checklist
./isaaclab.sh --formatCONTRIBUTORS.mdor my name already exists there🤖 Generated with Claude Code