Skip to content

fix(rsl_rl): default obs_groups for new runners - #6440

Open
saime428 wants to merge 3 commits into
isaac-sim:developfrom
saime428:fix/rsl-rl-play-obs-groups
Open

fix(rsl_rl): default obs_groups for new runners#6440
saime428 wants to merge 3 commits into
isaac-sim:developfrom
saime428:fix/rsl-rl-play-obs-groups

Conversation

@saime428

@saime428 saime428 commented Jul 9, 2026

Copy link
Copy Markdown

Summary:

  • Populate a default obs_groups mapping for OnPolicyRunner configs when running with rsl-rl >= 4.0.0.
  • Preserve any explicitly configured obs_groups values.
  • Add regression coverage for missing and existing obs_groups in the rsl-rl deprecation helper.

Why:

  • rsl-rl 4.x expects obs_groups in runner configs. Existing play.py already routes configs through handle_deprecated_rsl_rl_cfg() before calling OnPolicyRunner, so filling the missing default there keeps play/eval paths compatible without changing each script separately.

Validation:

  • python -m compileall source\isaaclab_rl\isaaclab_rl\rsl_rl\utils.py
  • python -m compileall source\isaaclab_rl\test\test_rsl_rl_cfg_deprecation.py
  • python -m ruff check source\isaaclab_rl\isaaclab_rl\rsl_rl\utils.py source\isaaclab_rl\test\test_rsl_rl_cfg_deprecation.py
  • lightweight handle_deprecated_rsl_rl_cfg() dummy-runner check for default and preserved obs_groups
  • python -m pytest source\isaaclab_rl\test\test_rsl_rl_cfg_deprecation.py -q (collection is blocked in this local environment by missing Isaac Sim / Warp packages)

Notes for reviewers:

@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Jul 9, 2026
@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes compatibility with rsl-rl >= 4.0.0 by automatically populating a default obs_groups mapping on OnPolicyRunner configs when the field is left at its MISSING sentinel. The logic is placed inside handle_deprecated_rsl_rl_cfg, which is already called on every play/eval path, so existing scripts get the fix without modification.

  • _set_default_obs_groups is added to utils.py and called at the top of the >= 4.0.0 branch; it only mutates configs whose class_name is \"OnPolicyRunner\" and whose obs_groups is still MISSING, preserving any explicitly set value.
  • Two parametrized tests in TestObsGroups verify the default-fill and the no-overwrite behavior across versions 4.x and 5.x.

Confidence Score: 4/5

Safe to merge — the change is a small, well-guarded default-fill that only mutates configs missing obs_groups, and explicitly preserves any value the user already set.

The production logic in utils.py is straightforward and correctly gated by three independent guards (class_name, hasattr, and _is_missing). The only gaps are in the test file: there is no assertion that the rsl-rl < 4.0.0 path leaves obs_groups untouched, and there is a minor PEP 8 formatting issue. Neither affects correctness of the shipped code.

The test file test_rsl_rl_cfg_deprecation.py would benefit from a small additional boundary test for the < 4.0.0 path; utils.py itself needs no further attention.

Important Files Changed

Filename Overview
source/isaaclab_rl/isaaclab_rl/rsl_rl/utils.py Adds _set_default_obs_groups helper and calls it at the start of the rsl-rl >= 4.0.0 branch; logic is guarded correctly by class_name, hasattr, and _is_missing checks.
source/isaaclab_rl/test/test_rsl_rl_cfg_deprecation.py Adds TestObsGroups with two parametrized tests covering default-fill and preserve cases; missing a blank line before the following section separator, and no negative test confirming obs_groups is not set for rsl-rl < 4.0.0.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["handle_deprecated_rsl_rl_cfg(agent_cfg, version)"] --> B{version < 4.0.0?}
    B -- Yes --> C[Validate policy present\nClear new model cfgs]
    B -- No --> D["_set_default_obs_groups(agent_cfg)"]
    D --> E{class_name == 'OnPolicyRunner'\nAND hasattr obs_groups\nAND obs_groups is MISSING?}
    E -- Yes --> F["Set obs_groups = {'actor': ['policy'], 'critic': ['policy']}"]
    E -- No --> G[Leave obs_groups unchanged]
    F --> H{policy present?}
    G --> H
    H -- Yes --> I[Infer actor/critic/student/teacher from deprecated policy]
    H -- No --> J{version < 5.0.0?}
    I --> J
    J -- Yes --> K[Validate old stochastic cfg]
    J -- No --> L[Migrate to distribution_cfg]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["handle_deprecated_rsl_rl_cfg(agent_cfg, version)"] --> B{version < 4.0.0?}
    B -- Yes --> C[Validate policy present\nClear new model cfgs]
    B -- No --> D["_set_default_obs_groups(agent_cfg)"]
    D --> E{class_name == 'OnPolicyRunner'\nAND hasattr obs_groups\nAND obs_groups is MISSING?}
    E -- Yes --> F["Set obs_groups = {'actor': ['policy'], 'critic': ['policy']}"]
    E -- No --> G[Leave obs_groups unchanged]
    F --> H{policy present?}
    G --> H
    H -- Yes --> I[Infer actor/critic/student/teacher from deprecated policy]
    H -- No --> J{version < 5.0.0?}
    I --> J
    J -- Yes --> K[Validate old stochastic cfg]
    J -- No --> L[Migrate to distribution_cfg]
Loading

Reviews (1): Last reviewed commit: "fix(rsl_rl): default obs_groups for new ..." | Re-trigger Greptile

Comment thread source/isaaclab_rl/test/test_rsl_rl_cfg_deprecation.py
Comment thread source/isaaclab_rl/test/test_rsl_rl_cfg_deprecation.py
@saime428

Copy link
Copy Markdown
Author

Hi, checking in on this fix for #5363. The PR remains conflict-free, though the branch is now behind main. I can update it before review if you prefer, or make any requested changes.

@saime428

saime428 commented Aug 2, 2026

Copy link
Copy Markdown
Author

Following up on this PR. The branch is currently behind main but remains mergeable. I can update it if preferred; otherwise I will avoid an unnecessary rebase. Please let me know if any changes are needed.

@saime428
saime428 changed the base branch from main to develop August 6, 2026 13:08
@saime428
saime428 requested a review from a team August 6, 2026 13:08
@saime428
saime428 force-pushed the fix/rsl-rl-play-obs-groups branch from eb49991 to 1249c55 Compare August 6, 2026 13:17
@saime428

saime428 commented Aug 6, 2026

Copy link
Copy Markdown
Author

Retargeted this PR from main to develop and rebased onto current develop. The fix and test commits are unchanged; I also added the changelog fragment that develop requires (source/isaaclab_rl/changelog.d/fix-rsl-rl-play-obs-groups.rst, patch tier) — that mechanism does not exist on main, so the PR was missing it.

I originally opened this against main, which is likely why it never went anywhere: in a month the only check that ever ran was labeler. docs/source/refs/contributing.rst (L53 on develop) still says "Submit a pull request to the main branch", which contradicts the PR template's "Target develop for all contributions" — that is what I followed. I've opened #6940 to fix that one line so the next contributor doesn't hit it.

The fix still applies on develop: both insertion points are unchanged there and obs_groups is still never defaulted, so #5363 still reproduces.

Verified locally against develop (source/isaaclab_rl/test/test_rsl_rl_cfg_deprecation.py):

  • with the fix — 42 passed
  • reverting only the utils.py hunk while keeping the tests — 2 failed, 40 passed, both being TestObsGroups::test_defaults_missing_obs_groups_for_on_policy_runner[4.0.0/5.0.0]

So the new tests do fail without the fix rather than passing vacuously. Happy to make any changes you'd like.

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

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug Report] play.py hangs in OnPolicyRunner init with rsl-rl 4.0.0+ due to missing obs_groups

1 participant