Skip to content

[RL] Add configclass support for skrl training - #7606

Open
StafaH wants to merge 1 commit into
isaac-sim:developfrom
StafaH:feature/skrl-configclass
Open

[RL] Add configclass support for skrl training#7606
StafaH wants to merge 1 commit into
isaac-sim:developfrom
StafaH:feature/skrl-configclass

Conversation

@StafaH

@StafaH StafaH commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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

  • New feature (non-breaking change which adds functionality)
  • Documentation update

Validation

  • 36 passed: skrl config conversion, all 31 core registrations, and Hydra override coverage
  • 67 passed: entrypoint and preset CLI regressions
  • 18 passed: complete LEAPP export flow
  • One-iteration Isaac-Cartpole skrl training smoke test
  • Warning-free documentation build
  • All pre-commit checks passed

Release backport

  • Backport this pull request to the active release branch after it merges into develop

Screenshots

Not applicable.

Checklist

Docker and GPU tests run on demand. Push the commits you want tested, then
comment run-ci on the pull request.

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with uv run isaaclab -f
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my feature works
  • I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package
  • My name already exists in CONTRIBUTORS.md

@StafaH
StafaH requested a review from a team September 6, 2026 05:40
@github-actions github-actions Bot added documentation Improvements or additions to documentation enhancement New feature or request isaac-lab Related to Isaac Lab team labels Sep 6, 2026
@greptile-apps

greptile-apps Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This 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.

  • Adds typed model, agent, memory, experiment, trainer, and runner configuration classes.
  • Migrates core skrl task configurations while retaining dictionary/YAML support.
  • Adds conversion, registry-loading, Hydra-override, and export-related coverage.
  • The skrl benchmark entrypoints were not updated for the new config object type and therefore fail for migrated tasks.

Confidence Score: 4/5

The 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 SkrlRunnerCfg objects and immediately subscript them, causing deterministic startup failures for migrated core tasks.

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

Filename Overview
source/isaaclab_rl/isaaclab_rl/skrl.py Adds the typed skrl configuration hierarchy and recursive conversion to the runner dictionary schema.
source/isaaclab_rl/isaaclab_rl/entrypoints/backends/train_skrl.py Converts resolved skrl configurations before training-time dictionary access and runner construction.
source/isaaclab_rl/isaaclab_rl/entrypoints/backends/play_skrl.py Converts resolved skrl configurations before playback setup and runner construction.
scripts/reinforcement_learning/leapp/skrl/export.py Adds configclass conversion at the LEAPP export boundary.
source/isaaclab_tasks/isaaclab_tasks/core/cartpole/init.py Redirects cartpole skrl registrations to configclasses, exposing the missing conversion in benchmark consumers.
source/isaaclab_tasks/test/core/test_skrl_agent_cfg.py Covers loading all core registrations and applying a Hydra override, but does not exercise benchmark consumers.

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
Loading

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",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_dict boundary provide a clear separation between typed Isaac Lab configuration and skrl's runner dictionary schema. No architecture finding remains.
  • API: Core skrl*_cfg_entry_point registrations now return SkrlRunnerCfg objects rather than dictionaries, and migrated override fields include changes such as lambda to gae_lambda and state_preprocessor to observation_preprocessor. The changelog should identify this compatibility surface and direct dictionary consumers to skrl_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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant