[Newton] Avoid repeated model startup work - #7295
Conversation
There was a problem hiding this comment.
Isaac Lab Review Bot
The PR appropriately moves solver-specific builder registration to Newton managers, memoizes articulation target-mode resolution, and centralizes articulation-view ownership. One compatibility issue remains: get_physics_sim_view() no longer provides its previously documented mutable registration list.
- Design and architecture: Builder-schema ownership is cleanly moved out of the cloner and into active solver managers, while articulation views are canonicalized by exact root expression. However, replacing the existing view registry with snapshots changes how external assets can register views.
- API:
get_physics_sim_view()previously returned the live registry and explicitly supportedget_physics_sim_view().append(view). It now returns a newly allocated list, so existing callers can append without error but their views are not registered. Preserve append-compatible behavior or introduce an explicit registration API with a deprecation and migration path. - Implementation: The target-mode cache includes DOF names and imported stiffness and damping values, then reapplies modes using local indices. The canonical articulation-view cache is reset by
clear(), and the updated articulation and joint-wrench paths use it. The remaining implementation concern is the snapshot returned byget_physics_sim_view(), which silently discards mutations expected by its prior contract.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| """ | ||
| return cls._views | ||
| """Return the registered articulation views.""" | ||
| return list(cls._articulation_views.values()) |
There was a problem hiding this comment.
🟡 Warning · Api — get_physics_sim_view no longer accepts registrations
This method previously returned the live _views registry and documented that assets append their views so sensors can find them (as Articulation._initialize_impl did). It now returns list(cls._articulation_views.values()), a per-call snapshot, so any remaining or extension-side get_physics_sim_view().append(view) silently registers nothing and non-articulation views become undiscoverable with no error. Add an explicit registration classmethod, or keep append-compatible semantics with a deprecation path.
Greptile SummaryThis PR moves Newton builder-schema registration to the active solver managers and reduces repeated articulation startup work.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue established. The changed startup paths consistently delegate schema registration to active solver managers, reuse articulation setup by exact expression or structural signature, and retain regression coverage for the affected contracts. Important Files Changed
Sequence DiagramsequenceDiagram
participant Scene as Scene startup
participant Manager as Active Newton manager
participant Builder as ModelBuilder
participant Asset as Articulation
participant Cache as Articulation-view cache
participant Sensor as Joint-wrench sensor
Scene->>Manager: create_builder()
Manager->>Builder: register active solver attributes
Scene->>Builder: import and finalize model
Asset->>Manager: get_articulation_view(root expression)
Manager->>Cache: create or reuse canonical view
Cache-->>Asset: shared ArticulationView
Sensor->>Manager: get_articulation_view(same expression)
Cache-->>Sensor: same ArticulationView
Reviews (1): Last reviewed commit: "Cache repeated Newton articulation setup" | Re-trigger Greptile |
655977f to
947869a
Compare
## Summary This PR is now the contact/raycast part of the Newton startup work: - compile contact-sensor full-path expressions once and match them directly, without regex-to-glob conversion or stage globbing; - use Newton's current contact-sensing API names; - declare raycast collision-shape requirements before model finalization, so the model builds one correctly configured BVH instead of rebuilding it during sensor initialization. The contact-selector and sensing-API commits retain Chris's (`camevor`) original authorship. The BVH lifecycle change incorporates the review from #7296 while keeping this as Chris's PR. The related ownership work is intentionally split by component: #7292 handles explicit global ownership, and #7295 handles model/articulation startup. This PR contains no inactive-solver registration or articulation-view changes and supersedes #7296. Production code is `+29/-56` (net `-27`) against `909cc5decc5`. ## Startup benchmark RTX 5090, CUDA device 1, 4096 environments, three fresh processes per revision/task. Values are median end-to-end startup wall time. Base: `909cc5decc5`. PR: `5d0e1b1595e`. | Task | Base | PR | Change | |---|---:|---:|---:| | `Isaac-Cartpole` | 8.316 s | 8.410 s | +1.1% | | `Isaac-Velocity-Rough-UnitreeGo2` | 16.363 s | 15.682 s | -4.2% | | `Isaac-Lift-KukaAllegro-Camera` | 39.473 s | 37.605 s | -4.7% | Cartpole has no contact/raycast workload here and is neutral within process-startup noise. The sensor-heavy tasks show the intended gain: | Median phase | Go2 base | Go2 PR | Kuka base | Kuka PR | |---|---:|---:|---:|---:| | `newton_contact_sensor` | 0.10 s | 0.04 s | 1.32 s | 0.24 s | | `simulation_start` | 6.36 s | 5.91 s | 11.48 s | 10.39 s | Raw end-to-end totals: - Cartpole base: 9.368, 8.316, 8.195 s; PR: 8.410, 8.370, 8.476 s. - Go2 rough base: 19.900, 16.363, 16.091 s; PR: 15.669, 15.682, 15.984 s. - Kuka camera base: 41.328, 38.625, 39.473 s; PR: 37.580, 37.605, 38.785 s. ## Test plan - `261 passed, 8 xpassed` across the Newton manager abstraction, contact-sensor, and raycast-sensor suites. - Repository formatting and pre-commit checks pass. - Architecture checks reject the removed regex-to-glob path, deprecated sensing names, duplicate BVH state, and late BVH fallback. - The three 4096-environment benchmark tasks also provide end-to-end Newton MJWarp startup coverage. ## Type of change - Performance improvement - Bug fix --------- Co-authored-by: Octi Zhang <zhengyuz@nvidia.com>
## Summary This PR is now the contact/raycast part of the Newton startup work: - compile contact-sensor full-path expressions once and match them directly, without regex-to-glob conversion or stage globbing; - use Newton's current contact-sensing API names; - declare raycast collision-shape requirements before model finalization, so the model builds one correctly configured BVH instead of rebuilding it during sensor initialization. The contact-selector and sensing-API commits retain Chris's (`camevor`) original authorship. The BVH lifecycle change incorporates the review from isaac-sim#7296 while keeping this as Chris's PR. The related ownership work is intentionally split by component: isaac-sim#7292 handles explicit global ownership, and isaac-sim#7295 handles model/articulation startup. This PR contains no inactive-solver registration or articulation-view changes and supersedes isaac-sim#7296. Production code is `+29/-56` (net `-27`) against `909cc5decc5`. ## Startup benchmark RTX 5090, CUDA device 1, 4096 environments, three fresh processes per revision/task. Values are median end-to-end startup wall time. Base: `909cc5decc5`. PR: `5d0e1b1595e`. | Task | Base | PR | Change | |---|---:|---:|---:| | `Isaac-Cartpole` | 8.316 s | 8.410 s | +1.1% | | `Isaac-Velocity-Rough-UnitreeGo2` | 16.363 s | 15.682 s | -4.2% | | `Isaac-Lift-KukaAllegro-Camera` | 39.473 s | 37.605 s | -4.7% | Cartpole has no contact/raycast workload here and is neutral within process-startup noise. The sensor-heavy tasks show the intended gain: | Median phase | Go2 base | Go2 PR | Kuka base | Kuka PR | |---|---:|---:|---:|---:| | `newton_contact_sensor` | 0.10 s | 0.04 s | 1.32 s | 0.24 s | | `simulation_start` | 6.36 s | 5.91 s | 11.48 s | 10.39 s | Raw end-to-end totals: - Cartpole base: 9.368, 8.316, 8.195 s; PR: 8.410, 8.370, 8.476 s. - Go2 rough base: 19.900, 16.363, 16.091 s; PR: 15.669, 15.682, 15.984 s. - Kuka camera base: 41.328, 38.625, 39.473 s; PR: 37.580, 37.605, 38.785 s. ## Test plan - `261 passed, 8 xpassed` across the Newton manager abstraction, contact-sensor, and raycast-sensor suites. - Repository formatting and pre-commit checks pass. - Architecture checks reject the removed regex-to-glob path, deprecated sensing names, duplicate BVH state, and late BVH fallback. - The three 4096-environment benchmark tasks also provide end-to-end Newton MJWarp startup coverage. ## Type of change - Performance improvement - Bug fix --------- Co-authored-by: Octi Zhang <zhengyuz@nvidia.com> (cherry picked from commit 21bc111)
…model-startup # Conflicts: # source/isaaclab_newton/test/physics/test_newton_manager_abstraction.py
## Summary This is the model/articulation part of the scoped Newton startup work. It keeps startup work with the component that owns it: - each Newton manager declares only the custom builder schema used by its active solver; - articulation target modes are resolved for the prototype and copied to its replicas; - the base physics manager owns the articulation-view registry; - articulations create and register their view, while joint-wrench sensors reuse it or create it when used independently; - root expressions remain regular expressions instead of taking a lossy regex-to-glob round trip. The active-solver and reusable-view findings originated in Chris's isaac-sim#7269. This PR isolates those model/articulation changes so isaac-sim#7269 can remain the contact/raycast change under Chris's PR. This replaces the cloner's unconditional MuJoCo + Kamino registration and the duplicate joint-wrench view. There is no view scan, manager-specific cache API, compatibility fallback, or duplicate registry. The production diff against the current base is `+57/-60` (net `-3`). The explicit-global-ownership part is isaac-sim#7292. The contact/raycast part remains in isaac-sim#7269. ## Startup benchmark RTX 5090, CUDA device 1, 4096 environments, three fresh processes per revision/task. Values are median end-to-end startup wall time; raw runs are included below. Base: `86cf66651bd`. PR: `947869af173`. | Task | Base | PR | Change | |---|---:|---:|---:| | `Isaac-Cartpole` | 8.316 s | 7.655 s | -8.0% | | `Isaac-Velocity-Rough-UnitreeGo2` | 17.525 s | 14.844 s | -15.3% | | `Isaac-Lift-KukaAllegro-Camera` | 41.691 s | 36.832 s | -11.7% | Raw totals: - Cartpole base: 10.595, 8.190, 8.316 s; PR: 7.655, 7.674, 7.469 s. - Go2 rough base: 17.568, 17.525, 16.426 s; PR: 14.852, 14.844, 14.724 s. - Kuka camera base: 47.590, 41.691, 38.724 s; PR: 36.832, 36.666, 37.032 s. The measured `env_creation` medians improve from 6.473 to 5.801 s for Cartpole, 15.558 to 12.863 s for Go2 rough, and 37.481 to 32.578 s for Kuka camera. ## Test plan - `247 passed` across the physics-manager lifecycle, Newton cloner, manager abstraction, coupled-manager, and joint-wrench reuse tests. - `test_rename_builder_labels.py`: `17 passed` after removing obsolete solver-registration mocks. - Ruff check and format pass on all changed Python files. - The three 4096-environment benchmark tasks provide end-to-end Newton MJWarp startup coverage. (cherry picked from commit c4a2759)
…7299) ## Summary Backports the following merged changes to `release/3.0.0` as separate provenance-preserving cherry-picks: - #7292 — scope Newton global imports with clone plans - #7285 — stabilize the sensor/PhysX video recording test - #7269 — streamline Newton contact and raycast sensor startup - #7119 — normalize non-finite depth display values safely - #7295 — avoid repeated Newton model and articulation startup work Each source squash commit was cherry-picked with `-x` and applied without conflicts. ## Validation - Stable patch IDs match all five source squash commits exactly. - File-by-file manifests match each source squash commit. - `git diff --check upstream/release/3.0.0..HEAD` - `uv run --frozen python tools/changelog/cli.py check backport-7285-7292-base` - `SKIP=check-changelog-fragments uv run --frozen isaaclab -f` - Cloner/Newton focused tests: 98 passed - Scene global-ownership tests: 2 passed - Simulator clone-plan tests: 4 passed - Video recording regression test: 1 passed - Newton BVH lifecycle tests: 2 passed - Newton contact-selector tests: 7 passed - Newton raycast BVH test: 4 passed - Non-finite depth display tests: 4 passed - #7295 physics lifecycle, cloner, manager, and coupling tests: 248 passed - #7295 Newton joint-wrench sensor tests: 11 passed - #7295 PhysX joint-wrench sensor tests: 16 passed PR #7121 remains open and is intentionally excluded; it will be backported from its final merge commit after merging. --------- Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Co-authored-by: matthewtrepte <mtrepte@nvidia.com> Co-authored-by: camevor <camevor@nvidia.com> Co-authored-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com> Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Summary
This is the model/articulation part of the scoped Newton startup work. It keeps startup work with the component that owns it:
The active-solver and reusable-view findings originated in Chris's #7269. This PR isolates those model/articulation changes so #7269 can remain the contact/raycast change under Chris's PR.
This replaces the cloner's unconditional MuJoCo + Kamino registration and the duplicate joint-wrench view. There is no view scan, manager-specific cache API, compatibility fallback, or duplicate registry.
The production diff against the current base is
+57/-60(net-3).The explicit-global-ownership part is #7292. The contact/raycast part remains in #7269.
Startup benchmark
RTX 5090, CUDA device 1, 4096 environments, three fresh processes per revision/task. Values are median end-to-end startup wall time; raw runs are included below. Base:
86cf66651bd. PR:947869af173.Isaac-CartpoleIsaac-Velocity-Rough-UnitreeGo2Isaac-Lift-KukaAllegro-CameraRaw totals:
The measured
env_creationmedians improve from 6.473 to 5.801 s for Cartpole, 15.558 to 12.863 s for Go2 rough, and 37.481 to 32.578 s for Kuka camera.Test plan
247 passedacross the physics-manager lifecycle, Newton cloner, manager abstraction, coupled-manager, and joint-wrench reuse tests.test_rename_builder_labels.py:17 passedafter removing obsolete solver-registration mocks.