Skip to content

fix(rsl_rl): fix compatibility with rsl-rl-lib 5.0.1#5390

Open
mpte-official wants to merge 1 commit intoisaac-sim:mainfrom
mpte-official:fix/rsl-rl-5-compat
Open

fix(rsl_rl): fix compatibility with rsl-rl-lib 5.0.1#5390
mpte-official wants to merge 1 commit intoisaac-sim:mainfrom
mpte-official:fix/rsl-rl-5-compat

Conversation

@mpte-official
Copy link
Copy Markdown

@mpte-official mpte-official commented Apr 24, 2026

  • Add missing handle_deprecated_rsl_rl_cfg() call in benchmark_rsl_rl.py
  • Replace rsl_rl.version with importlib.metadata.version('rsl-rl-lib')
  • Fix log key 'Perf/collection time' -> 'Perf/collection_time'

Fixes #5363

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

- Add missing handle_deprecated_rsl_rl_cfg() call in benchmark_rsl_rl.py
- Replace rsl_rl.__version__ with importlib.metadata.version('rsl-rl-lib')
- Fix log key 'Perf/collection time' -> 'Perf/collection_time'

Fixes isaac-sim#5363
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Apr 24, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

🤖 Isaac Lab Review Bot

Summary

This PR fixes compatibility between the benchmark script and rsl-rl-lib 5.0.1 by adding the missing handle_deprecated_rsl_rl_cfg() call (which was already being used in the main training script), switching to importlib.metadata for version detection, and updating log key names to match the new format. The changes are straightforward and mirror patterns already established in other RSL-RL scripts.

Architecture Impact

Self-contained. Changes are limited to a single benchmark script (scripts/benchmarks/benchmark_rsl_rl.py). The modifications align this script with the existing pattern in scripts/reinforcement_learning/rsl_rl/train.py which already handles the deprecated config correctly.

Implementation Verdict

Minor fixes needed

Test Coverage

No regression tests are added. This is a bug fix PR that should ideally include a test verifying compatibility with rsl-rl-lib 5.0.1. However, given this is a benchmark script and the fix mirrors an already-tested pattern from the main training script, the risk is low. The fix can be validated by running the benchmark with rsl-rl-lib 5.0.1.

CI Status

No CI checks available yet. Cannot verify if the fix works correctly against rsl-rl-lib 5.0.1.

Findings

🔵 Improvement: benchmark_rsl_rl.py:199-200 — Import should be at module level

import importlib.metadata
installed_version = importlib.metadata.version("rsl-rl-lib")

The import importlib.metadata statement is placed inside the main() function. While this works, it's inconsistent with the rest of the codebase. Looking at scripts/reinforcement_learning/rsl_rl/train.py (not shown but referenced), imports are typically at module level. Move this import to line ~68 with the other standard library imports for consistency:

import importlib.metadata

🟡 Warning: benchmark_rsl_rl.py:200 — Missing error handling for package not found
If rsl-rl-lib is not installed (e.g., user installed the old rsl-rl package instead), importlib.metadata.version("rsl-rl-lib") will raise PackageNotFoundError. The main training script may handle this more gracefully. Consider wrapping with a try-except that provides a clear error message:

try:
    installed_version = importlib.metadata.version("rsl-rl-lib")
except importlib.metadata.PackageNotFoundError:
    raise RuntimeError(
        "rsl-rl-lib package not found. Please install it via: pip install rsl-rl-lib"
    )

🔵 Improvement: benchmark_rsl_rl.py:1-9 — Duplicate copyright headers
The file has two copyright blocks (lines 1-4 and 6-9) with different year ranges and slightly different project names. This appears to be a pre-existing issue but should be cleaned up. Keep only the 2022-2026 header:

# Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md).
# All rights reserved.
#
# SPDX-License-Identifier: BSD-3-Clause

🟡 Warning: benchmark_rsl_rl.py:235,240 — Potential KeyError if log format changes again
The log key access log_data["Perf/collection_time"] assumes the key exists. If rsl-rl-lib changes the format again in a future version, this will raise a KeyError with no context. Consider using .get() with a fallback or adding a check:

collection_time_key = "Perf/collection_time"
if collection_time_key not in log_data:
    raise KeyError(f"Expected log key '{collection_time_key}' not found. Available keys: {list(log_data.keys())}")

This would make debugging future compatibility issues easier.

🔵 Improvement: PR description incomplete
The PR description template was not filled out properly — the "Fixes #" field in the template body is empty while "Fixes #5363" is only in the commit/title metadata. The "Type of change" section wasn't updated to mark "Bug fix" as the applicable type. This is minor but makes PR tracking harder.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

This PR brings benchmark_rsl_rl.py into parity with train.py for compatibility with rsl-rl-lib 5.0.1: it adds the missing handle_deprecated_rsl_rl_cfg() call (using importlib.metadata to retrieve the installed version), and corrects two log-key references from "Perf/collection time" (space) to "Perf/collection_time" (underscore). All three fixes are straightforward and correct; the only minor nit is the function-local import importlib.metadata that could instead live at module level.

Confidence Score: 5/5

Safe to merge; all changes are targeted bug fixes with no functional regressions.

All three fixes are correct and match the patterns established in train.py. The only open item is a P2 style suggestion (import placement), which does not affect correctness or runtime behavior.

No files require special attention.

Important Files Changed

Filename Overview
scripts/benchmarks/benchmark_rsl_rl.py Adds missing handle_deprecated_rsl_rl_cfg() call (with importlib.metadata version lookup), and fixes two log-key references from "Perf/collection time" to "Perf/collection_time"; one minor style note about a function-local import.

Sequence Diagram

sequenceDiagram
    participant main
    participant importlib.metadata
    participant handle_deprecated_rsl_rl_cfg
    participant OnPolicyRunner
    participant parse_tf_logs

    main->>importlib.metadata: version("rsl-rl-lib")
    importlib.metadata-->>main: installed_version
    main->>handle_deprecated_rsl_rl_cfg: (agent_cfg, installed_version)
    handle_deprecated_rsl_rl_cfg-->>main: updated agent_cfg
    main->>OnPolicyRunner: (env, agent_cfg.to_dict(), ...)
    OnPolicyRunner-->>main: runner
    main->>parse_tf_logs: (log_dir)
    parse_tf_logs-->>main: log_data
    main->>main: log_data["Perf/collection_time"] (fixed key)
Loading

Reviews (1): Last reviewed commit: "fix(rsl_rl): fix compatibility with rsl-..." | Re-trigger Greptile

task_startup_time_end = time.perf_counter_ns()

# create runner from rsl-rl
import importlib.metadata
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.

P2 Local import should be at module level

import importlib.metadata is placed inside main(), which is unconventional and inconsistent with how train.py handles the same import (import importlib.metadata as metadata at module scope). While Python caches modules so correctness is unaffected, placing standard-library imports at the top of the file is the idiomatic Python practice and makes dependencies easier to see at a glance.

Move this import to the top-level import block and remove the inline one.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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