Add the Factory contact-rich assembly task on Newton - #6891
Conversation
Greptile SummaryThe PR adds a preset-driven Factory assembly task for Newton and supporting actuator, collision-schema, asset-path, and Newton cloning changes.
Confidence Score: 4/5The PR should not merge until the accumulator constructs its collision analyzers correctly, because the Factory task otherwise fails during its first reset. The active Factory reset configuration passes analyzer configuration objects into code that calls their class factory with the predicate-call signature, causing incompatible constructor arguments before any reset states can be collected. Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/core/multi_task/utils/event_combinators.py and source/isaaclab_tasks/isaaclab_tasks/core/multi_task/reset_env_cfg.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Factory environment reset] --> B[ACCUMULATOR_RESET]
B --> C[Run chained scene reset]
C --> D[Evaluate collision acceptance conditions]
D --> E[Collect accepted states]
E --> F[Downsample state table]
F --> G[Sample reset state per environment]
Reviews (1): Last reviewed commit: "Add skip fragments for the test-only bac..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The Factory task and supporting actuator/collision changes are substantial, but several merge-time issues remain: the task’s asset root is source-checkout-specific, public Newton mesh-simplification APIs are removed without deprecation, the lazy-export stub advertises an undefined symbol, and subset collision checks index packed SDF data incorrectly. There are also a broken type-only import and an avoidable dependency from generic multi-task utilities into the lift task.
- Design and architecture: The preset-driven task structure and moving collision approximation authority from the Newton cloner to asset spawners are coherent. However, the domain-agnostic reset combinators directly depend on
core.lift.mdp.events_cfg.SuccessMonitorCfg, coupling shared infrastructure to an unrelated task package. Move that shared configuration into the multi-task utility layer and update both consumers. - API:
ImplicitActuatornow preserves independent rated and solver effort limits while retaining the one-field fallback, andCollisionBaseCfg.mesh_collision_propertyexposes the existing schema dispatch. In contrast,NewtonCfg.simplify_meshesand related callable parameters are removed without the required prior deprecation.LOCAL_ASSET_PATH_DIRconstructs a repository-only path that will not resolve in installed distributions, and themdplazy-export stub lists an undefinedRESET_STRATEGIESsymbol. - Implementation: The SDF collision analyzer packs collider slots and counts using the global environment count and absolute environment IDs, but the query kernel indexes them using the queried subset size and local subset index. Collision validation therefore reads incorrect environments for partial or reordered
env_ids; the kernel must use the global stride and absolute ID. Theobservations.pytype-onlyOffsetimport also targets a nonexistentmdp.utilmodule and should referencemulti_task.utils.pose_offset.
Significant concerns. Posted 6 actionable findings inline.
Automated review; human maintainers own approval decisions.
d297e60 to
6a1dd6d
Compare
1295350 to
3eb84ef
Compare
f28550c to
7e6e97a
Compare
AntoineRichard
left a comment
There was a problem hiding this comment.
AI-generated review
This PR is not ready to merge. The latest head still has functional blockers in task startup and collision validation, and it carries substantial unused/copied infrastructure that should be removed before this becomes a showroom task.
Merge blockers
- The task hard-codes NVIDIA's internal
isaac-devNucleus, so external users cannot load its assets. SuccessMonitorCfgis imported fromcore/lift/mdp/events_cfg.py, which has been deleted on currentdevelop; the merged tree will fail to import this task.- Reset precollection calls
CollisionAnalyzerwith the predicate signature instead of constructing it from its config. - The SDF kernel addresses globally packed collider data with subset-local strides/indices.
- The process-global collider cache retains stage-owned prims and an old environment count across environment reconstruction.
The prior automated comments about the analyzer constructor, SDF subset indexing, nonexistent .util.pose_offset import, undefined RESET_STRATEGIES, and lift-layer coupling remain valid at this head.
Required PR reduction and consolidation
- Remove/defer the 514-line Warp sampler and the Frontier strategy: no production config in this PR enables
warp=Trueor uses Frontier; only their tests do. Keep the Torch Beta/Uniform path used by Factory. - Remove the terrain/multi-task command observations and unused reward/termination terms copied into
contrib.nist; they are not referenced by the Factory config and create a false public surface. - Generate the repeated socket/plug asset configs through a small typed helper, consolidate identical keypoint/preset classes, and delete commented-out alternatives.
- Fold the duplicated Hydra import test into the existing shared test and remove config-only smoke/import tests already covered by the shared Factory runtime test and other unit tests. Keep the two scheduler behavior tests in the curriculum test module.
- Add focused regressions for reordered/subset
env_idsand for analyzer reconstruction across stages/environment counts. Those tests catch the concrete failures; many current tests only assert trivial properties or implementation strings.
The comments/docstrings also need a full editorial pass: keep them functional and durable, remove author tags/debugging narratives/value-selection postmortems, and delete stale references to terrain, connectors, old layouts, and “backwards compatibility” for new symbols.
I did not run project code, pre-commit, or GPU tests locally because benchmarks are running on this machine. Static git diff --check passed. GitHub CI at this head is currently blocked with failures in two isaaclab_tasks shards, docs, isaaclab_rl, and visualizers.
4ac941d to
2f7dda8
Compare
d3bd89e to
8c66461
Compare
Treat effort_limit as the actuator-facing rated force or torque and effort_limit_sim as the physics-solver clamp. Preserve existing one-field fallbacks while allowing both fields to carry distinct values, mirroring the velocity-limit semantics introduced in isaac-sim#6481.
Dropping the cloner's blanket approximation pass leaves every collider at its USD-authored approximation, and USD defaults that to none. A sweep of all 132 registered tasks, comparing each collider's resolved Newton shape before and after, found three tasks whose colliders changed: the lift multi-asset object and the reorient dex cube, both spawned as tessellated mesh primitives with no approximation authored. Author convexHull on their shared collision props, which the multi-asset spawner propagates to every variant, restoring the shapes those tasks resolved before.
The Lift object preset has two variants: a multi-asset spawner of Mesh* primitives, and a plain CuboidCfg that spawns an analytic UsdGeom.Cube. Only the former is a mesh, so only the former was ever hulled by the cloner -- authoring a mesh-collision approximation on the cube claims a property the prim cannot have. The collider sweep confirms it was doing nothing: the tasks the hull is meant to protect resolve identical shapes without it.
Add IsaacContrib-Factory-Franka, a manager-based assembly task covering NIST taskboard M16 nut-threading and round-peg insertion with a Franka Panda. Assembly, robot, reset strategy and curriculum are each a PresetCfg, so a run is selected with presets=franka,nut_thread_m16,newton_mjwarp instead of a bespoke env class per assembly.
The task registers one environment, IsaacContrib-Factory-Franka, and one robot. Registering that robot as a `franka` variant meant every command had to repeat a selector the task name already implies. Bind the robot-specific fields to each preset's `default` instead, so a run selects only what actually varies -- the assembly and the physics backend. Adding a second robot still works the same way: give it a named variant alongside the default.
Every scene variant pinned num_envs=2, so a training run that forgot --num_envs looked healthy while collecting almost no experience. Set the shared scene base to 4096, matching the other manager-based tasks, and let the variants inherit it -- they only need to differ in env_spacing. play resolves the same config and applies no cap of its own, so play_mode takes it back down to something you can actually watch.
The task is a contributed task family, so it belongs under contrib rather than core. Nothing in core imported it, so the dependency direction stated in isaaclab_tasks (core never depends on contrib) is preserved. Name the package after the NIST taskboard rather than the task: contrib already ships an unrelated factory package, and the old multi_task name described the directory's history rather than its contents. The gym id IsaacContrib-Factory-Franka is unchanged.
The gear variants put two gears on the board besides the pair being assembled, and neither the accumulator nor the reset acceptance gate knew about them. The bank restored only nistboard, fixed_asset, held_asset and robot, so a replayed state left the spare gears wherever the previous episode ended while the board moved underneath them. The held asset was screened for collisions against the board, table, robot and fixed asset but not against those gears, so a state seating it inside one was banked as valid. PhysX pushes such a state apart; MuJoCo/Newton diverges to NaN a few steps later, which is why only the Newton runs died. Scope both lists per assembly so the spare gears are restored and screened.
8c66461 to
fddc02f
Compare
|
Extracted the implicit-actuator effort-limit semantics into #7078, the effort-limit mirror of #6481, and rebased this branch on that single prerequisite commit. The direct diff from #7078 to this head now touches only Revalidated after the split:
|
Description
Important
Depends on #7078 — that PR must be merged first. This branch is stacked on the effort-limit mirror of #6481, so the diff includes its single actuator commit. Review the Factory commits on top; after #7078 lands, rebasing will remove the parent commit from this diff.
Adds
IsaacContrib-Factory-Franka, a preset-driven contact-rich assembly task underisaaclab_tasks.contrib.nist.The task covers 12 NIST taskboard variants:
Assembly geometry, reset strategy, curriculum, robot, and physics are composed through presets rather than separate environment classes. The task supports explicit
isaacsim_physxandnewton_mjwarppresets, withphysxretaining automatic backend selection.Dependency
#7078 separates the two implicit-actuator effort fields:
effort_limitretains the actuator-facing rated force or torque.effort_limit_simcontrols the physics-solver clamp.The Factory Franka configuration intentionally authors distinct rated and solver limits, so it cannot construct on unmodified
develop.Changes
Type of change
Validation
uv run isaaclab -fisaacsim_physxtraining run completed