Stabilize dexsuite camera training defaults - #6835
Conversation
Camera actors were unable to train: they collapsed into joint-velocity-limit terminations or stopped moving entirely, plateauing near the share of episodes that start with the object already in the gripper. The cause is the magnitude of the camera encoder's updates. The KL-adaptive schedule oscillates its learning rate across the point where encoder features churn faster than the policy head can track, and the previous encoder produced a latent large enough that no stable learning rate existed for it. Pair a smaller shared encoder with a fixed learning rate, and make the image observations stationary so the encoder sees a consistent input scale. Also add the reset-state banking pieces this relies on: a success monitor that restarts episodes from the states the policy solves about half the time, a diversity feature that spreads the bank over the grasp geometry, and progress based tracking rewards that pay only for new ground gained.
OvPhysX is kitless and cannot share a process with Kit, but the guard only covered the Kit visualizer, so pairing it with the Isaac RTX renderer fell through to a raw "Failed to initialize Carbonite and load PhysX plugins" from inside the OvPhysX library. Extend the check to Kit cameras and report the supported combinations.
Greptile SummaryStabilizes Dexsuite camera-policy training and adds adaptive reset-state machinery.
Confidence Score: 4/5The progress-reward baseline must be reset when the pose command changes before this PR is safe to merge. Pose commands change multiple times within an episode, but both new progress terms retain the previous target's best error, producing incorrect rewards after each resample. Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/core/dexsuite/mdp/rewards.py; source/isaaclab_tasks/isaaclab_tasks/core/dexsuite/dexsuite_env_cfg.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Reset["Episode reset"] --> Bank["Restore sampled reset-bank state"]
Bank --> TargetA["Sample pose command A"]
TargetA --> Baseline["Seed progress baseline"]
Baseline --> RewardA["Reward new best error"]
RewardA --> Resample["Command timer expires after 2–3 s"]
Resample --> TargetB["Sample pose command B"]
TargetB --> Stale["Reuse command A baseline"]
Stale --> Wrong["Suppress valid progress or grant target-change progress"]
Reviews (1): Last reviewed commit: "Reject OvPhysX physics paired with a Kit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def _progress(self, error: torch.Tensor, gate: torch.Tensor, min_improvement: float) -> torch.Tensor: | ||
| """Return 1.0 for the environments that beat their episode best while ``gate`` holds.""" | ||
| unseeded = torch.isinf(self.best_error) | ||
| self.best_error[unseeded] = error[unseeded] | ||
| improved = gate & (error < self.best_error - min_improvement) | ||
| self.best_error[improved] = error[improved] |
There was a problem hiding this comment.
When the object-pose command is resampled during an episode, _progress() compares the new target's error against the best error recorded for the previous target. A farther target suppresses legitimate progress rewards, while a nearer target grants rewards solely because the command changed.
Knowledge Base Used: isaaclab_tasks: Task Registration and Organization
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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 camera stabilization and reset-bank changes are substantial, but the patch also introduces an undocumented Kuka Allegro axis-layout change, configures the diversity descriptor inconsistently with its stated design, removes public reward terms without deprecation, and silently changes the success metric’s availability and meaning.
- Design and architecture: The reset-bank oversampling, farthest-point thinning, and success-targeted sampling form a coherent design. However, the configured GraspTravelDistanceCfg leaves log_scale disabled even though the implementation and adjacent rationale rely on logarithmic distances to preserve near-grasp coverage. The success metric is also relocated from the reward term to the reset-bank monitor, changing both its ownership and semantics.
- API: The public position_command_error_tanh and orientation_command_error_tanh exports are removed outright, violating the repository requirement that public symbols receive a prior deprecation period. In addition, pinning Kuka Allegro joint and body ordering to mjwarp changes PhysX action and observation axis layouts, potentially invalidating existing checkpoints, without the required breaking-change changelog guidance.
- Implementation: The new Kuka Allegro ordering explicitly permutes PhysX finger axes relative to prior behavior and must be documented as a breaking migration. GraspTravelDistanceCfg should enable log_scale or the comments and distribution rationale must be corrected. Metrics/success_rate is now emitted only through SuccessMonitor and represents a bank-slot average rather than the previous resetting-episode success rate; this compatibility change should be documented or the prior metric retained under stable semantics.
Significant concerns. Posted 4 actionable findings inline.
Automated review; human maintainers own approval decisions.
| # follow each backend's native order: PhysX traverses breadth-first and MJWarp depth-first, which | ||
| # permutes the Allegro hand's four branching fingers (the serial arm is unaffected) and breaks | ||
| # sim-to-sim transfer of the ``.*`` action term and the joint/body-order observations. | ||
| robot: ArticulationCfg = KUKA_ALLEGRO_CFG.replace( |
There was a problem hiding this comment.
🟡 Warning · Api — Ordering pin changes PhysX observation layout silently
Per the added comment, unpinned ordering followed each backend's native order, so pinning to mjwarp permutes the Allegro finger joints and bodies under PhysX. That changes the .* action ordering and joint/body-ordered observations for the state presets described as unchanged, invalidating existing checkpoints. Add a **Breaking:** Changed entry to the isaaclab_tasks changelog fragment with migration guidance.
| # far tail near its natural share. Two axes need more candidates to cover than one; | ||
| # ``1.0`` turns the pass off. | ||
| "oversample_factor": 5.0, | ||
| "diversity_feature": mdp.GraspTravelDistanceCfg( |
There was a problem hiding this comment.
🟡 Warning · Implementation — Diversity descriptor configured without documented log scale
The adjacent comment states the descriptor's log scale is what makes the oversampling pay off (roughly doubling near-grasp starts), but GraspTravelDistanceCfg is instantiated without log_scale, whose default is False. Farthest-point sampling therefore runs on linear distances, producing the distribution the comment says it avoids. Either pass log_scale=True or correct the stated rationale.
| "object_ee_distance", | ||
| "orientation_command_error_tanh", | ||
| "position_command_error_tanh", | ||
| "orientation_command_progress", |
There was a problem hiding this comment.
🟡 Warning · Api — Public reward terms removed without deprecation
position_command_error_tanh and orientation_command_error_tanh are dropped from __all__ and from rewards.py in the same change, so any external config importing them fails immediately. Repository rules require deprecating public API symbols in a prior release before removal. Keep the old names for one release as shims that warn and delegate to the new progress terms.
| happen to be ending now. | ||
| """ | ||
| if self._monitor is not None: | ||
| self._env.extras.setdefault("log", {})["Metrics/success_rate"] = self._monitor.get_mean_success_rate() |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Success metric moved with changed semantics
success_reward.reset no longer logs Metrics/success_rate; it is now emitted only when a success_monitor is configured and reports the bank-slot average rather than the per-episode rate of the resetting environments. Configs without the monitor lose the metric entirely and existing dashboards silently change meaning. Document this in the changelog fragment or keep the episode metric logged unconditionally.
| # sim-to-sim transfer of the ``.*`` action term and the joint/body-order observations. | ||
| robot: ArticulationCfg = KUKA_ALLEGRO_CFG.replace( | ||
| prim_path="{ENV_REGEX_NS}/Robot", | ||
| joint_ordering="mjwarp", |
There was a problem hiding this comment.
is this meant to be hardcoded to mjwarp or should follow based on the backend?
There was a problem hiding this comment.
I guess sim to sim then becomes a bit tedious to remember you ahve to change joint ordering?
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
Requesting changes. The training evidence is useful, but the current head does not meet the repository rules or the code-quality bar for showroom tasks.
Blocking issues:
- The progress-reward baseline survives command resampling, so rewards become incorrect whenever
object_posechanges mid-episode. The existing Greptile inline thread is precise and remains unresolved. position_command_error_tanhandorientation_command_error_tanhare removed without a prior deprecation release. The existing Isaac Lab review thread correctly identifies this direct violation of the API policy.- The existing threads on the disabled
log_scale, Kuka ordering compatibility, and changed success-metric semantics also need resolution. I did not duplicate those inline comments. - The new stateful reset/sampling machinery has no automated tests, and the OvPhysX renderer guard lacks the one regression test that distinguishes this fix from the old behavior; see inline comments.
- Five CI jobs are currently failing (
isaaclab_tasksshards 1/3 and 3/3,isaaclab_ov, kitless rendering correctness, and Kit standalone demos). These need diagnosis before merge.
The comments and documentation also need a focused cleanup. Empirical tuning narratives are repeated across the PR, changelog, config comments, and public docstrings; several explain why exact values were chosen instead of stating a durable functional contract. There is also duplicated camera algorithm configuration, an inaccurate affine claim for the tanh depth transform, an over-broad state presets are unchanged claim, and missing return annotations on new public methods. The independent launcher-validation commit should be split into its own PR as offered, which will make both changes easier to test and review.
Verification: I reviewed the exact base/head diff and all current discussion. source/isaaclab_tasks/test/core/test_runtime_compatibility.py passes (22 tests), but none of those tests exercises OvPhysX + Isaac RTX without a Kit visualizer. The repository formatting hooks relevant to the changed files passed; the isolated codeload snapshot's ./isaaclab.sh -f run exited only because codeload expanded the 3.8 MB uv.lock, triggering the added-large-file hook, while hosted pre-commit passes. GPU training was not rerun.
| has_kit_visualizer = _has_kit_visualizer(scan, launcher_args) | ||
|
|
||
| if scan.has_ovphysx_physics and has_kit_visualizer: | ||
| if scan.has_ovphysx_physics and (has_kit_visualizer or scan.has_kit_camera): |
There was a problem hiding this comment.
Please add the regression test that distinguishes this fix from the old guard: presets=ovphysx,isaacsim_rtx with no Kit visualizer must raise. Both existing OvPhysX tests explicitly pass visualizer="kit", so they passed before this change and do not exercise scan.has_kit_camera. The new test should fail when this condition is reverted.
| asset.write_root_velocity_to_sim_index(root_velocity=velocities, env_ids=picked) | ||
|
|
||
|
|
||
| class SuccessMonitor: |
There was a problem hiding this comment.
This adds a stateful ring buffer, repeated-slot batching, partitioned sampling, and nonlinear weighting without any unit coverage in this PR or the existing tests. Please add CPU tests for repeated slot IDs and wraparound, partial-history rates, partition confinement, and target/temperature weighting. These are deterministic contracts and do not require simulator or training tests.
| activation="elu", | ||
| ) | ||
|
|
||
| # Camera-actor stability requires bounding the encoder update magnitude |
There was a problem hiding this comment.
This empirical stability narrative duplicates the PR/changelog and embeds thresholds the code neither names nor enforces. Keep the code comment to the durable functional constraint, and retain experiment-specific thresholds/results in the PR. Also extract the identical ALGO_CFG.replace(num_mini_batches=8, schedule="fixed", learning_rate=5.0e-5) used by both camera presets into a shared CAMERA_ALGO_CFG.
| }, | ||
| "buffer_size_per_group": 1024, | ||
| "buffer_size_per_group": 2048, | ||
| # harvest more candidates than the bank holds and keep the states spread widest over |
There was a problem hiding this comment.
Please replace this tuning narrative with a concise functional comment. It also attributes the distribution change to logarithmic scaling, while the GraspTravelDistanceCfg below leaves log_scale=False (already flagged in the existing thread). Exact distribution claims and justification for 5.0 belong in the PR's validation evidence, not permanent config comments.
|
|
||
| fingers_to_object = RewTerm(func=mdp.object_ee_distance, params={"std": 0.4}, weight=0.05) | ||
|
|
||
| # Tracking is rewarded progressively: one fixed payout per ``min_improvement`` of ground gained on |
There was a problem hiding this comment.
This duplicates _ProgressReward and overstates the configuration semantics: weight is the total payout only when initial_error / min_improvement happens to equal 30, which the reset distribution does not guarantee. Remove the tuning arithmetic here and let the reward term's functional contract document the behavior.
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Changed the dexsuite camera presets to a shared encoder and a fixed learning rate. Camera actor |
There was a problem hiding this comment.
Please keep this user-facing entry to the behavior and migration impact. The latent/update-budget rationale and exact tuning repeat the PR and code comments. Also, The state presets are unchanged is too broad: this PR changes resets, rewards, and Kuka axis ordering for state tasks. If only optimizer settings are meant, say State-policy optimizer settings were unchanged.
| ``duo_camera`` now use a ``[16, 32, 32]`` convolutional encoder with ``schedule="fixed"`` and | ||
| ``learning_rate=5e-5``; the adaptive KL schedule oscillates across the stability threshold and | ||
| does not train a camera actor. The state presets are unchanged. | ||
| * Changed the dexsuite camera observation normalization to fixed affine maps. RGB now maps to |
There was a problem hiding this comment.
fixed affine maps is inaccurate for depth: _depth_norm applies tanh(images / 2) - 0.5, which is nonlinear. Please call these fixed normalization maps, or describe the RGB affine transform and depth tanh transform separately.
| banked sample to the requested environments only — the wrapped terms and criteria never | ||
| run again. | ||
|
|
||
| Harvesting the first valid states leaves the bank shaped like the reset ranges: dense in |
There was a problem hiding this comment.
The class-level prose now duplicates contracts documented again in __call__, SuccessMonitorCfg, and GraspTravelDistanceCfg. Keep this docstring to the reset-bank lifecycle and invariants, keep parameter contracts in __call__, and remove the policy-training narrative. Maintaining the same behavior in several places invites drift.
| func=mdp.DifficultyScheduler, params={"init_difficulty": 0, "min_difficulty": 0, "max_difficulty": 10} | ||
| ) | ||
|
|
||
| def disable_observation_noise_terms(self): |
There was a problem hiding this comment.
Please add -> None to this new public method, per the repository's public type-hint rules. The docstring can also be reduced to the functional contract: disable curriculum terms whose address traverses an observation-noise configuration.
Camera actors are limited by how large an encoder update they tolerate, which ties the learning rate to the size of the latent handed to the MLP. Reducing the convolutional feature map to per-channel keypoint coordinates instead of flattening it keeps that latent at two numbers per channel whatever the map size, leaving room for a higher learning rate. Both camera presets move to the keypoint encoder at a fixed 1e-4. Measured at 4096 environments, iterations to 50% success drop 23% for a single RGB camera (three seeds) and 46% for duo depth; final success is unchanged.
The fragment still described the flattened encoder at 5e-5 that the camera presets used before the keypoint actor replaced it.
Re-seed the progress-reward baseline when the tracked command resamples, so a mid-episode goal change no longer credits against the previous goal's error. Restore position_command_error_tanh and orientation_command_error_tanh as deprecated wrappers rather than removing them outright, per the API policy. Reduce the comments and changelog to functional contracts, describe the depth transform as tanh rather than affine, narrow the state-preset claim to optimizer settings, share one algorithm config between the camera presets, and annotate the new public curriculum method.
The OvPhysX/Kit-renderer validation fix is independent of the dexsuite camera work and is easier to test and review on its own.
The bank size increase changed the states drawn at reset, which moved the dexsuite motion-vector rendering result away from its golden reference; 1024 is the size those references were captured with. Rerunning the camera sweep with the progress-reward fix in place favours 7e-5 over 1e-4: it is the only rate at which the depth128 preset trains, and it reaches higher success at a lower velocity-limit termination rate across the shipped presets.
3 participants
Description
Camera-based dexsuite policies could not be trained. They collapsed into
joint-velocity-limit terminations (
abnormal_robot) or stopped movingaltogether, plateauing at roughly the fraction of episodes that start with the
object already in the gripper, while the state-based policies on the same task
trained to high success.
The cause is the magnitude of the camera encoder's updates rather than anything
task-specific. Two things follow from that:
encoder features churn faster than the policy head can track. Its mean rate
is fine — matched-mean comparisons still fail — it is the excursions that do
the damage, and its KL signal is itself inflated by the feature churn it is
reacting to.
rate existed for it at a useful speed.
This PR pairs a smaller shared encoder with a fixed learning rate for both the
single_cameraandduo_camerapresets, and makes the image observationsstationary so the encoder sees a consistent input scale (RGB and depth now share
a fixed
[-0.5, 0.5]affine map instead of per-frame mean subtraction, whichalso restores the absolute-distance anchor for depth). State presets are
unchanged and keep the adaptive schedule, which works well for them.
Measured on
Isaac-Lift-KukaAllegro-Cameraat 4096 envs, all four camerapreset combinations now reach ~94-96% success where they previously plateaued
near 10%:
abnormal_robotsingle_camera,rgb64single_camera,depth64duo_camera,rgb64duo_camera,depth64The PR also lands the reset-state machinery the above relies on: a success
monitor that restarts episodes from the states the policy solves about half the
time, a diversity feature that spreads the reset bank over the grasp geometry,
and progress-based tracking rewards that pay only for new ground gained on the
episode-best error.
A second, independent commit fixes a backend-validation gap found while
verifying these presets across renderer/physics combinations: OvPhysX is kitless
and cannot share a process with Kit, but the guard only covered the Kit
visualizer, so pairing it with the Isaac RTX renderer fell through to a raw
Failed to initialize Carbonite and load PhysX pluginsfrom inside the OvPhysXlibrary instead of a readable error. Happy to split this into its own PR if
preferred.
Type of change
The breaking part is limited to the dexsuite reward terms:
position_command_error_tanhandorientation_command_error_tanhare replacedby
position_command_progressandorientation_command_progress. Migration iscovered in the changelog fragment: swap the term and set
min_improvementinstead of
std.Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists thereThe validation for the training-behavior change is the measured table above
rather than a unit test, since it is a config-tuning change whose effect only
appears over thousands of training iterations.
🤖 Generated with Claude Code