[RL] Add configclass support for skrl training - #7606
Conversation
Greptile SummaryThis PR introduces typed, modular skrl runner configuration classes, converts them to skrl dictionaries at the standard train, play, and LEAPP boundaries, and migrates core-task skrl registrations from YAML to Python modules.
Confidence Score: 4/5The PR is not safe to merge until the skrl benchmark train and play entrypoints convert configclass instances before treating them as dictionaries. Standard train, play, and export paths perform the new conversion, but both supported benchmark paths receive the newly registered Files Needing Attention: source/isaaclab/isaaclab/benchmark/entrypoints/backends/skrl/benchmark_train_skrl.py; source/isaaclab/isaaclab/benchmark/entrypoints/backends/skrl/benchmark_play_skrl.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
R[Core task skrl registration] --> C[SkrlRunnerCfg instance]
C --> T[Regular training]
C --> P[Regular playback]
C --> E[LEAPP export]
C --> BT[Benchmark training]
C --> BP[Benchmark playback]
T --> CT[skrl_cfg_to_dict]
P --> CP[skrl_cfg_to_dict]
E --> CE[skrl_cfg_to_dict]
CT --> Runner[skrl Runner dictionary]
CP --> Runner
CE --> Runner
BT --> X[Direct dictionary indexing fails]
BP --> X
Reviews (1): Last reviewed commit: "Add configclass support for skrl trainin..." | Re-trigger Greptile |
| "rsl_rl_cfg_entry_point": f"{agents.__name__}.rsl_rl_ppo_cfg:CartpoleDirectPPORunnerCfg", | ||
| "default_agent": "rsl_rl", | ||
| "skrl_cfg_entry_point": f"{agents.__name__}:skrl_direct_ppo_cfg.yaml", | ||
| "skrl_cfg_entry_point": f"{agents.__name__}.skrl_ppo_cfg:SkrlDirectPPORunnerCfg", |
There was a problem hiding this comment.
Benchmark entrypoints reject configclasses
Core skrl registrations now resolve to SkrlRunnerCfg objects instead of YAML dictionaries. The benchmark train and play entrypoints immediately access the result with agent_cfg["agent"] without first calling skrl_cfg_to_dict, so benchmarking any migrated core task raises TypeError: 'Skrl...RunnerCfg' object is not subscriptable before the runner is created. Both benchmark paths need the same conversion already used by the regular train and play entrypoints.
There was a problem hiding this comment.
Isaac Lab Review Bot
The configclass conversion is coherently integrated across train, play, LEAPP export, and checkpoint creation, while preserving plain-dictionary/YAML inputs. Before merge, document the migration required for consumers and Hydra overrides of the core registry configs, and restore the humanoid benchmark synchronization notice lost with the YAML removal.
- Design and architecture: The nested Skrl configclasses and centralized
skrl_cfg_to_dictboundary provide a clear separation between typed Isaac Lab configuration and skrl's runner dictionary schema. No architecture finding remains. - API: Core
skrl*_cfg_entry_pointregistrations now returnSkrlRunnerCfgobjects rather than dictionaries, and migrated override fields include changes such aslambdatogae_lambdaandstate_preprocessortoobservation_preprocessor. The changelog should identify this compatibility surface and direct dictionary consumers toskrl_cfg_to_dict, while clarifying the required override-key migration. - Implementation: Conversion is applied before dictionary-style use in the in-repository train, play, export, and checkpoint paths. However, the migrated humanoid manager config dropped the explicit maintenance notice tying it to the reinforcement-learning documentation's Training Performance table and coordinated updates across RL libraries; that constraint should be retained with the Python config.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * Changed core-task skrl agent configurations from YAML files to Python ``configclass`` objects. Custom YAML entry |
There was a problem hiding this comment.
🟡 Warning · Api — Changelog omits skrl entry-point migration guidance
Core skrl*_cfg_entry_point values now resolve to SkrlRunnerCfg instead of a dict — this diff had to patch four in-repo call sites for exactly that reason — and several override keys were renamed (lambda -> gae_lambda, state_preprocessor -> observation_preprocessor, clip_predicted_values removed). The fragment only states that custom YAML needs no migration. Per repository rules, mark the changed behavior and document the new type plus skrl_cfg_to_dict and the renamed keys.
|
|
||
|
|
||
| @configclass | ||
| class SkrlManagerPPORunnerCfg(SkrlRunnerCfg): |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Humanoid config lost docs benchmark sync notice
The deleted skrl_manager_ppo_cfg.yaml carried an explicit notice that this configuration generates the "Training Performance" table in the reinforcement learning documentation and that the other RL-library configurations must be updated alongside it. SkrlManagerPPORunnerCfg drops that constraint, so future retuning can silently desynchronize the published table. Carry the notice over as a class docstring.
Description
Adds typed, modular Isaac Lab configclasses for the skrl runner schema and converts them to the dictionary expected by skrl at the train, play, and LEAPP export boundaries. Existing YAML/dictionary configurations remain supported.
Migrates all 29 skrl YAML configurations in the core task package to 15 task-local Python configuration modules. The migration preserves task-specific tuning while updating deprecated skrl keys to the current skrl 2.1 schema.
Also adds Hydra/config-loading coverage, conversion unit tests, documentation, and changelog fragments.
No new dependencies are required.
Fixes #943
Type of change
Validation
36 passed: skrl config conversion, all 31 core registrations, and Hydra override coverage67 passed: entrypoint and preset CLI regressions18 passed: complete LEAPP export flowIsaac-Cartpoleskrl training smoke testRelease backport
developScreenshots
Not applicable.
Checklist
Docker and GPU tests run on demand. Push the commits you want tested, then
comment
run-cion the pull request.pre-commitchecks withuv run isaaclab -fsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.md