Remove the newton_kamino preset from Reach tasks - #6941
Conversation
The Kamino solver preset was declared on the shared ReachPhysicsCfg, so every Reach task advertised physics=newton_kamino even though the preset was never validated for these environments. Drop the preset from ReachPhysicsCfg. Isaac-Reach-Franka, Isaac-Reach-Franka-OSC and Isaac-Reach-UR10 now expose only isaacsim_physx, newton_mjwarp and ovphysx, with newton_mjwarp remaining the default. Update the illustrated environment table, the generated comprehensive table and the environment browser data to match.
Greptile SummaryThe PR removes the unvalidated
Confidence Score: 5/5The PR appears safe to merge, with the configuration removal and all affected documentation surfaces aligned. The removed preset is scoped to the intended Reach configurations, the remaining default is unchanged, and no current caller or generated documentation surface was found relying on the deleted field. Important Files Changed
Reviews (1): Last reviewed commit: "Remove the newton_kamino preset from Rea..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The preset removal and documentation updates are internally consistent, but the PR immediately removes a public, documented selector without the required prior deprecation and categorizes the breaking changelog entry under the wrong heading.
- Design and architecture: Narrowing Reach tasks to validated physics backends is reasonable, but removing the shared
ReachPhysicsCfg.newton_kaminopreset must be staged through deprecation rather than delivered as an immediate breaking removal. - API:
ReachPhysicsCfg.newton_kaminois both a public configuration attribute and a documentedphysics=selector for three Reach tasks. Its deletion makes existing commands and derived configurations fail immediately, contrary to the repository's deprecation-first requirement. Retain and deprecate it for a release before removal. - Implementation: The source and documentation selector lists are synchronized, but the
.major.rstfragment places a**Breaking:**entry underRemoved. Repository policy requires breaking entries underChanged; move the entry and retain its migration guidance there.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| from dataclasses import MISSING | ||
|
|
||
| from isaaclab_newton.physics import KaminoSolverCfg, MJWarpSolverCfg, NewtonCfg | ||
| from isaaclab_newton.physics import MJWarpSolverCfg, NewtonCfg |
There was a problem hiding this comment.
🟡 Warning · Api — Public preset removed without prior deprecation
ReachPhysicsCfg.newton_kamino is a public config attribute and a documented physics= selector value for three Reach tasks. Dropping it (and the KaminoSolverCfg import) makes existing commands and derived configs error immediately, while the repository rule requires public symbols to be deprecated in a prior release before removal. Keep the preset for one release with a deprecation warning, then remove it.
| @@ -0,0 +1,8 @@ | |||
| Removed | |||
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Breaking entry should sit under Changed
The changelog rules state that breaking changes belong under Changed, prefixed with **Breaking:**. This fragment uses the **Breaking:** prefix but places the entry under a Removed heading, so the compiled release notes will categorize the change inconsistently with the policy. Move the entry (and its migration guidance) under a Changed heading.
Signed-off-by: Kelly Guo <kellyg@nvidia.com>
Description
The Kamino solver preset was declared on the shared
ReachPhysicsCfg, so every Reach task advertisedphysics=newton_kaminoeven though the preset was never validated for these environments.This drops
newton_kaminofromReachPhysicsCfg.Isaac-Reach-Franka,Isaac-Reach-Franka-OSCandIsaac-Reach-UR10now expose onlyisaacsim_physx,newton_mjwarpandovphysx;newton_mjwarpremains the default. The now-unusedKaminoSolverCfgimport is removed as well.Documentation is updated in the three places that carry the selector list:
docs/source/overview/environments.rstdocs/source/_static/css/environment-browser.jsThe prose task lists in
kamino-solver.rstandhydra.rstname only the Cartpole/Ant tasks, so they need no change.Type of change
physics=newton_kaminoon the three Reach tasks now errors instead of selecting Kamino. A.major.rstchangelog fragment with migration guidance is included.Screenshots
N/A
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists thereNotes on the checklist: no new tests are added — this removes a configuration entry rather than adding behavior, and the selector list is already covered by the registry-backed documentation check. The changelog is provided as a fragment under
source/isaaclab_tasks/changelog.d/, per the current policy of not editingCHANGELOG.rstorextension.tomldirectly.