Cleanup patches, Compose agent variants through canonical preset roots - #7611
Conversation
1378798 to
e77186b
Compare
Greptile SummaryThis PR consolidates environment-coupled agent selection under canonical preset roots instead of registry compatibility maps and CLI-side agent rewriting.
Confidence Score: 5/5The PR appears safe to merge, with the canonical preset composition, SKRL algorithm identity, generated browser schema, and affected registrations aligned by implementation and tests. No actionable failures remain: affected preset families match their environment contracts, SKRL callers consistently consume the resolved algorithm identity, and the documentation schema migration is positionally consistent. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
CLI["CLI: task, library, presets=name"] --> Resolver["resolve_task_config"]
Registry["Canonical library cfg entry point"] --> Resolver
Resolver --> EnvRoot["Environment PresetCfg root"]
Resolver --> AgentRoot["Agent PresetCfg root"]
EnvRoot --> EnvVariant["Matching environment variant"]
AgentRoot --> AgentVariant["Matching agent variant or compatible default"]
EnvVariant --> Runtime["Training / play / benchmark / export"]
AgentVariant --> Runtime
Reviews (1): Last reviewed commit: "Refactor agent variants into canonical p..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The canonical preset-root design consistently unifies environment and agent variant selection, but the patch immediately removes two existing user-facing compatibility surfaces: preset-specific Gym agent entry-point keys and the public setup_preset_cli(..., agent_library=...) parameter. Both require a deprecation and migration period under repository policy.
- Design and architecture: Using canonical
<library>_cfg_entry_pointpreset roots removes duplicated registry/CLI/browser dispatch logic, and the updated resolver, browser schema, SKRL algorithm resolution, and Humanoid AMP registration follow that model consistently. Transitional aliases can preserve compatibility without changing the new canonical ownership model. - API: Existing
--agentvalues such as the camera feature and showcase space-specific entry points now fail registry lookup, while out-of-tree callers passingagent_library=to the exportedsetup_preset_cliAPI now receiveTypeError. Retain deprecated compatibility aliases and accept the legacy keyword with a warning for a migration window. - Implementation: In-repository callers were migrated, SKRL algorithm identity is now derived from resolved
agent.class, and canonical AMP lookup remains reachable. However, updating only in-tree consumers does not satisfy the compatibility contract for public callers and previously valid commands; targeted deprecated shims are needed before removing those surfaces.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
e77186b to
c307886
Compare
|
run-ci |
hujc7
left a comment
There was a problem hiding this comment.
Approving. The canonical-preset-root model holds up: I verified that presets=resnet18 / theia_tiny compose the feature runner and the feature observation group for both RSL-RL and RL-Games, that all 15 showcase space presets pair their env spaces with the matching SKRL model config, and that SKRL algorithm identity now follows agent.class for AMP, IPPO and MAPPO. No comments follow on any of that.
Two inline items, both non-blocking. The first is the one I would still fix before merge: the project template was not migrated alongside the in-repo AMP tasks, so generated projects on a non-PPO algorithm no longer resolve their agent config.
…reset-composition # Conflicts: # docs/source/_static/css/environment-browser.js # source/isaaclab_tasks/isaaclab_tasks/core/cartpole/__init__.py
|
run-ci |
Description
The symptoms addressed by #7045 and the now-merged #7532 are real, but the selection mechanism grew into a parallel configuration system: task registrations described preset-to-agent relationships, the preset CLI rediscovered those relationships and rewrote
args.agent, every backend had to opt into that mutation, and the documentation browser copied the same mapping again.Because #7532 has since merged, this branch also fully removes its live sentinel, second parse, mutation guard, benchmark wiring, and associated compatibility tests. Historical compiled changelog entries remain as release history.
This PR makes the existing preset resolver the only owner of environment-coupled selection:
The Gym registry now selects a library or a genuinely independent training recipe. A library's canonical entry point owns any agent variants coupled to environment presets, so the same
presets=broadcast selects both roots in one resolution pass.The final diff is 687 additions / 787 deletions: net -100 lines.
Before and after
1. Environment-coupled agent variants
Before: repeat the pairing, then teach the CLI to translate it
The camera task registered separate raw and feature agent keys, then repeated their relationship to environment presets in another dictionary:
The showcase tasks amplified this into one registry key per observation/action-space combination—24 noncanonical SKRL keys across the two tasks—plus another compatibility map:
After: the canonical agent entry point is a preset root
The agent package owns the variants directly:
RL Games uses the same canonical-root shape with
preset(default=..., resnet18=..., theia_tiny=...). The showcase YAML family is likewise exposed behind its sole canonical key:The configurations remain, but the 26 preset-specific agent keys, three compatibility maps, and duplicate preset-name lists do not.
2. Preset CLI ownership
Before: parse, rediscover task metadata, and mutate another subsystem's argument
_auto_select_agentscanned rawpresets=tokens, intersected them with the compatibility maps, selected a different registry key, and rewroteargs.agent. #7532 later added a sentinel and a second argparse pass just to decide whether that mutation was allowed to override a parser default.After: parse preset syntax and return it unchanged
The preset CLI is again bounded to preset help and parsing. Agent configuration selection happens once, inside config resolution.
3. Humanoid AMP and SKRL algorithm identity
Before: infer both the config and algorithm from CLI/key spelling
A plain Humanoid AMP command therefore looked for the missing
skrl_cfg_entry_point. #7045 compensated with a second CLI rule: if the canonical key was absent and exactly one other key existed, mutateargs.agentto that key. The same key-parsing pattern also mislabeled showcase keys such asskrl_box_discrete_cfg_entry_pointas algorithms.After: the task owns its default; the resolved config owns its algorithm
Omitting
--algorithmnow means “use the task's canonical SKRL config,” not “assume PPO.” An explicit--algorithmstill selects an independent algorithm recipe and is checked against the resolvedagent.class. Train, play, benchmarks, and LEAPP all use the same rule.4. Downstream consumers
_AgentDescriptionBuilderand_enumerate_agentsloaded Gym metadata and rendered compatibility pairings.agentPresetCompatibility; JavaScript searched it and injected--agentinto commands.presets=directly.FEATUREorBOX_DISCRETE.--agent=rl_games_feature_cfg_entry_pointandpresets=resnet18/theia_tiny.*_cfg_entry_pointsuffix into guessed library and algorithm labels.agent_libraryso the preset CLI could mutateargs.agent; SKRL then reconstructed the algorithm from that key.agent.class.<library>_cfg_entry_pointonly for PPO, so non-PPO and all multi-agent projects depended on CLI fallback.args.agentvalue.5. Commands
Before, users either had to provide both coupled selectors explicitly or rely on the preset CLI to synthesize the
--agenthalf:The same simplification applies to RL Games and the showcase space matrix:
Humanoid AMP no longer depends on an algorithm flag or the sole-entry fallback:
--algorithm AMPremains valid when an explicit algorithm selection is desired. Generated multi-agent projects likewise use MAPPO as their canonical SKRL config, so their documented command needs no fallback:Exact lineage of the removed mechanism
The structure removed or superseded here accumulated across several PRs:
--agent=rl_games_feature_cfg_entry_pointandpresets=resnet18/theia_tiny.rsl_rl_feature_cfg_entry_pointfor pretrained camera features.--algorithmto PPO, select a registry key from that default, and reconstruct algorithm identity from explicit key suffixes.agent.classis authoritative everywhere.agent_preset_compatibilitymaps, duplicate_SPACE_PRESETS/_RAW_CAMERA_PRESETSlists,setup_preset_cli(..., agent_library=...),_AgentDescriptionBuilder,_enumerate_agents, andagent_library=plumbing in all eight unified RL train/play entry points. It also added help, docs, and tests around those pairings.agent_libraryinto the shared LEAPP export parser and added another SKRL helper that inferred the algorithm from the agent key._auto_select_agent, rawpresets=scanning, declared-domain filtering, preset-to-key matching, post-parseargs.agentmutation, and a sole-noncanonical-agent fallback for Humanoid AMP. It also wired the two SKRL benchmark entry points.agent_preset_compatibilityinto the generated environment-browser row schema and used it in JavaScript to inject a preset-specific--agentinto generated commands._AGENT_UNSET, a second argparse pass in_agent_passed_explicitly, a looser mutation guard, six RSL-RL/RL-Games/SB3 benchmark train/play wirings, and tests for rewrittenargs.agentvalues and explicit-flag spellings.This is therefore more than an alternative fix for two symptoms: it removes the parallel preset-to-agent selection system end to end.
Intentional compatibility removal
No warning-only or no-op compatibility shim is retained for:
agent_preset_compatibilityrsl_rl_feature_cfg_entry_pointand the showcase space-specific SKRL keyssetup_preset_cli(..., agent_library=...)The separately named
pretrained_checkpoint_preset_compatibilitymetadata from #7630 is intentionally retained: it declares which preset-specific checkpoint artifacts exist and never selects an agent entry point.Keeping those aliases would preserve the duplicate ownership this refactor removes. Commands using a preset-specific key should remove
--agent=<preset-specific-key>and use the canonical library selection pluspresets=<name>, as shown above.Older SKRL runs may have encoded a configuration-key suffix such as
box_discreteas the algorithm in a run directory or manifest. The corrected identity is the resolvedagent.class(PPOin that example), so automaticlatest/bestdiscovery cannot safely match those historical runs. Pass their checkpoint path explicitly.Type of change
Validation
The four-case composition regression was also run against an untouched
developworktree. All four cases failed there because the environment preset changed while the canonical agent stayed at its default; all four pass on this branch.upstream/develop, centralized structure audit, andgit diff --check: passedNo GPU simulation or full training job was run; the regression is exercised at configuration composition and entrypoint boundaries before environment construction.
The generated environment-browser refresh also incorporates the current physics selectors for the two DrLegs rows.
Release backport
developScreenshots
Not applicable.
Checklist
uv run --frozen isaaclab -fCONTRIBUTORS.md