Fix rl_games play checkpoint fallback

[ooctipus]

Summary

• add an opt-in fallback checkpoint pattern to the shared get_checkpoint_path() helper
• use that fallback from rl_games play so short runs can load the latest available checkpoint when the preferred best-checkpoint file has not been written yet
• naturally sort numbered checkpoint filenames so epoch 10 is selected after epoch 9

Validation

• python3 -m py_compile source/isaaclab_tasks/isaaclab_tasks/utils/parse_cfg.py scripts/reinforcement_learning/rl_games/play.py scripts/reinforcement_learning/rl_games/play_rl_games.py source/isaaclab_tasks/test/core/test_checkpoint_path.py
• env PYTHONPATH=source/isaaclab_tasks:source/isaaclab:source/isaaclab_rl /home/zhengyuz/Projects/IsaacLab.wt/newton-fabric-body-binding/env_isaaclab/bin/python -m pytest -q source/isaaclab_tasks/test/core/test_checkpoint_path.py
• pre-commit run --files scripts/reinforcement_learning/rl_games/play.py scripts/reinforcement_learning/rl_games/play_rl_games.py source/isaaclab_tasks/isaaclab_tasks/utils/parse_cfg.py source/isaaclab_tasks/test/core/test_checkpoint_path.py source/isaaclab_tasks/changelog.d/fix-rlgames-checkpoint-fallback.rst
• env PYTHONPATH=source/isaaclab_tasks:source/isaaclab:source/isaaclab_rl /home/zhengyuz/Projects/IsaacLab.wt/feature-unify/env_isaaclab/bin/python scripts/reinforcement_learning/rl_games/play.py --help
• env PYTHONPATH=source/isaaclab_tasks:source/isaaclab:source/isaaclab_rl /home/zhengyuz/Projects/IsaacLab.wt/feature-unify/env_isaaclab/bin/python scripts/reinforcement_learning/play.py --rl_library rl_games --help

[isaaclab-review-bot]

🤖 IsaacLab Review Bot — PR #6091

PR: Fix rl_games play checkpoint fallback
Author: @ooctipus | Base: develop | SHA: 8641eecb

---

Summary

This PR adds a checkpoint resolution helper (get_rl_games_checkpoint_path) that attempts to load the preferred "best" checkpoint and gracefully falls back to the latest available checkpoint when it hasn't been written yet. It also fixes natural sorting of numbered checkpoint filenames (so epoch 10 sorts after epoch 9).

---

Findings

🔵 Suggestion — Natural sort edge case with decimal numbers in filenames
source/isaaclab_tasks/isaaclab_tasks/utils/parse_cfg.py L247

The new natural sort key [int(token) if token.isdigit() else token for token in re.split(r"(\\d+)", m)] works correctly for integer-based ordering. However, note that for filenames like last_cartpole_ep_10_rew_1.0.pth, the .0 part splits into [".", "0", ".pth"], so 1.0 is treated as two separate numbers (1 and 0). This is fine for the current use case since epoch numbers drive the ordering and those are always integers, but it's worth documenting that this is natural integer sort, not version/decimal sort.

🔵 Suggestion — Bare from checkpoint import relies on script-level sys.path
scripts/reinforcement_learning/rl_games/play.py L28

The import from checkpoint import get_rl_games_checkpoint_path is a bare module import that works because play.py is invoked as a script (so its directory is on sys.path). This is consistent with how rl_games scripts are structured, but it means checkpoint.py cannot be imported from other locations without path manipulation. Consider whether a relative import or adding an __init__.py would be more robust — though this matches existing patterns in this directory, so low priority.

🟡 Warning — Fallback prints to stdout rather than using logging
scripts/reinforcement_learning/rl_games/checkpoint.py L21-23

The fallback notification uses print(f"[INFO] ...") rather than Python's logging module. While this is pragmatic for scripts, it:

• Cannot be filtered by log level
• Won't appear in structured log output
• Is inconsistent if other parts of the codebase use logging

If the rest of the rl_games scripts use print for info messages, this is fine as-is. Otherwise, consider logging.getLogger(__name__).info(...).

🔵 Suggestion — Test import path requires specific PYTHONPATH
scripts/reinforcement_learning/test/test_checkpoint_path_fallback.py L6

The test uses from scripts.reinforcement_learning.rl_games.checkpoint import get_rl_games_checkpoint_path which requires the repo root on sys.path and implicitly treats scripts/ as a namespace package (no __init__.py). This works with the documented PYTHONPATH setup but may confuse IDE tooling or fail if pytest is invoked without the correct working directory. Consider adding a brief comment or conftest noting the required invocation.

🔵 Suggestion — Test coverage could include the "no checkpoints at all" case
scripts/reinforcement_learning/test/test_checkpoint_path_fallback.py

The two test cases cover: (1) preferred checkpoint found, (2) fallback to latest. Consider adding a test that verifies ValueError is raised when no checkpoints exist at all (empty nn/ directory) and when checkpoint_file == ".*" to confirm the re-raise path works correctly.

---

Architecture Assessment

✅ The fallback pattern is clean and well-scoped — it's rl_games-specific logic extracted into its own module rather than polluting the shared get_checkpoint_path utility.

✅ The natural sort fix in parse_cfg.py benefits all callers of get_checkpoint_path, not just rl_games.

✅ Both play.py and play_rl_games.py are updated consistently.

✅ Changelog fragment included.

---

Verdict

Looks good overall. The core fix is sound — the fallback chain is correct, the natural sort addresses a real bug, and the changes are well-tested. The suggestions above are minor improvements, none blocking.

---

🤖 Generated by IsaacLab Review Bot • Event: COMMENT (bot never approves)

---

📝 Update — New commits pushed (45861f68)

Changes since last review (8641eecb):

The author refactored the approach — instead of a separate get_rl_games_checkpoint_path helper in scripts/, the fallback logic is now integrated directly into the shared get_checkpoint_path() utility via a new fallback_checkpoint parameter. This is a cleaner design:

• parse_cfg.py: Added fallback_checkpoint: str | None = None parameter to get_checkpoint_path(). When the primary checkpoint regex matches nothing, tries fallback_checkpoint before raising. Error message now mentions both patterns. Natural sort fix retained.
• play.py / play_rl_games.py: Now call get_checkpoint_path(... fallback_checkpoint=None if args_cli.use_last_checkpoint else ".*") directly — no separate helper needed.
• Changelog: Updated accordingly.
• Tests: Moved to source/isaaclab_tasks/test/core/test_checkpoint_path.py using pytest — clean, well-structured, covers preferred-found / fallback / strict-no-fallback cases.

New findings

✅ No new issues. This refactor addresses previous review suggestions:

• Eliminates the bare-module import concern (no more scripts/ helper to import)
• Test is now a proper pytest file in the package test directory (no PYTHONPATH gymnastics)
• The print("[INFO]...") vs logging note still applies (now in parse_cfg.py L253) but is consistent with surrounding code in that module

Verdict update

Improved architecture. Integrating fallback into the shared utility is the right call — it's backward-compatible (default None means no behavior change for existing callers) and avoids rl_games-specific duplication. LGTM. 👍

---

📝 Update — New commits pushed (34b7426f)

Changes since last review (45861f68):

Minor simplification to the play scripts:

• play.py / play_rl_games.py: Now pass fallback_checkpoint=".*" unconditionally (previously the conditional logic around use_last_checkpoint was more complex). When use_last_checkpoint is set, checkpoint_file is already ".*", so the fallback is redundant but harmless. When not set, it provides the desired graceful fallback. Clean and correct.
• parse_cfg.py: No functional changes — the natural sort fix and fallback_checkpoint parameter remain as before.
• test_checkpoint_path.py: Added (no changes from previous iteration).
• Changelog fragment: Added (no changes from previous iteration).

New findings

✅ No new issues. The simplification is straightforward and makes the calling code easier to reason about.

Verdict update

Still LGTM. The unconditional fallback_checkpoint=".*" is a clean simplification — no new concerns. 👍

---

📝 Update — New commits pushed (edd7f762)

Changes since last review (45861f68):

• parse_cfg.py: Changed fallback_checkpoint default from None to ".*" — fallback is now opt-out rather than opt-in. Function signature reformatted to multi-line.
• test_checkpoint_path.py: New test file at source/isaaclab_tasks/test/core/test_checkpoint_path.py covering preferred match, fallback, and strict (no-fallback) cases.
• Changelog fragment: Added.

Previous inline comments status

• ✅ Test import path (P1): Fully addressed — test is now a proper pytest file importing from isaaclab_tasks.utils, no PYTHONPATH gymnastics needed.
• ✅ Broad ValueError catch (P2): Fully addressed — the separate checkpoint.py wrapper is gone; fallback logic is integrated directly into get_checkpoint_path() with explicit pattern-based handling.

New findings

⚠️ Note (non-blocking): The default for fallback_checkpoint changed from None to ".*". This means all existing callers that don't pass this parameter will now silently fall back to any checkpoint instead of raising when the requested pattern isn't found. This is a behavioral change — callers that relied on strict matching will need to pass fallback_checkpoint=None explicitly. Verify this is the intended default for the broader codebase.

Verdict update

Still LGTM. Both previous concerns are fully addressed by the refactored design. The default change is worth a quick sanity check with the team but isn't a blocker. 👍

---

📝 Update — New commits pushed (b41bb6b6)

Changes since last review (edd7f762):

Significant simplification — the author has completely removed the --use_last_checkpoint flag and simplified the checkpoint resolution approach:

Key Changes

1. Removed --use_last_checkpoint flag entirely from:

   • scripts/reinforcement_learning/rl_games/play.py
   • scripts/reinforcement_learning/rl_games/play_rl_games.py
   • scripts/reinforcement_learning/sb3/play.py
   • scripts/reinforcement_learning/sb3/play_sb3.py

2. get_checkpoint_path() signature change in parse_cfg.py:

   • checkpoint parameter now defaults to None (was ".*")
   • When None, internally uses ".*" to match any checkpoint
   • The fallback_checkpoint parameter from previous iterations is gone

3. Simplified calling code:

   • rl_games: get_checkpoint_path(log_root_path, run_dir, other_dirs=["nn"]) — no checkpoint pattern, auto-selects latest
   • sb3: get_checkpoint_path(log_root_path, ".*", r"model.*\\.zip", sort_alpha=False) — explicit pattern to match both model.zip and model_XXXX.zip

4. Documentation updated: Tutorial example no longer uses --use_last_checkpoint

5. Unit tests: Good coverage of the new behavior (preferred match, latest fallback, strict match scenarios)

Analysis

✅ Cleaner API: Instead of a complex fallback mechanism, the approach is now simply:

• Pass checkpoint=None (or omit) → get latest checkpoint (any file)
• Pass explicit pattern → get latest matching that pattern (strict)

✅ Backward compatibility considered: Callers who passed explicit patterns will still get strict matching behavior. Only callers relying on the old default ".*" might see different behavior — but practically, ".*" matches everything anyway.

✅ Documentation consistency: The tutorial example is updated to match the new default behavior.

✅ All four play scripts updated: Consistent changes across rl_games and sb3.

Minor observations

🔵 sb3 pattern r"model.*\\.zip" in the play scripts: This matches model.zip, model_1000.zip, etc. — correct and intentional for sb3's checkpoint naming.

🔵 Natural sort retained: The fix from earlier iterations (epoch 10 after epoch 9) is preserved.

Verdict update

Still LGTM — nice simplification. The removal of --use_last_checkpoint and the cleaner default-to-latest behavior is a good UX improvement. The API is now more intuitive: omit the checkpoint arg to get the latest, pass a pattern for strict matching. 👍

---

Update — New Commits (95bfbe6)

The PR has been significantly restructured since the initial review. Previous concerns are now addressed:

1. ✅ Test import path — Tests relocated to source/isaaclab_tasks/test/core/test_checkpoint_path.py with proper imports from the installed isaaclab_tasks.utils package. No fragile script-level imports.
2. ✅ Broad ValueError catch / separate checkpoint module — Eliminated. The fallback logic is now a preferred_checkpoint parameter on get_checkpoint_path itself, removing the wrapper function and exception-based flow control.
3. ✅ stdout print instead of logging — Gone with the removed wrapper module.
4. ✅ Test coverage — Comprehensive pytest suite covering preferred-found, preferred-missing-fallback, error-when-empty, sb3 final-model preference, and sb3 fallback to latest step.

New approach review: The preferred_checkpoint parameter added to get_checkpoint_path is clean, well-documented, and backward-compatible (defaults to None). Natural sort and changelog entry look good. No new issues found.

---

📝 Update — New commits pushed (f57ee31d)

Changes since last review (95bfbe64):

Single commit consolidating the final approach:

• parse_cfg.py: checkpoint default changed from None to ".*" (explicit wildcard). preferred_checkpoint logic now tries the preferred pattern first, falls back to checkpoint — clean two-tier resolution.
• rl_games play scripts: Restored --use_last_checkpoint flag (omitted in previous iteration). When set, skips the preferred best-checkpoint; otherwise defaults to preferring the best-reward .pth.
• sb3 play scripts: Now also use preferred_checkpoint=r"model\.zip" to prefer the final model over periodic step snapshots — consistent approach across both frameworks.
• Tests: Three new test cases covering preferred-wins, preferred-missing-fallback, and sb3 final-model preference.
• Changelog: Rewritten to describe both the Added (preferred_checkpoint param) and Fixed (fallback behavior) aspects.

Assessment

✅ No new issues. The changes are a clean finalization of the approach already reviewed. The --use_last_checkpoint restoration makes sense — best vs latest is a genuine user choice for rl_games. sb3 extension is consistent. Tests are thorough. 👍

[greptile-apps]

Greptile Summary

This PR fixes RL-Games checkpoint resolution during play by adding a fallback to the latest available checkpoint when the preferred "best" checkpoint file ({name}.pth) has not yet been written. It also replaces the old filename-padding sort key (f"{m:0>15}") in get_checkpoint_path with a proper natural sort, which was silently broken for checkpoint filenames longer than 15 characters (causing ep_10 to sort before ep_9).

• checkpoint.py (new): get_rl_games_checkpoint_path wraps get_checkpoint_path and retries with ".*" when the preferred file is not found, printing an informational message and returning the naturally-sorted latest checkpoint.
• play.py / play_rl_games.py: Both files updated to call get_rl_games_checkpoint_path instead of get_checkpoint_path directly.
• parse_cfg.py: Natural sort key (re.split(r"(\d+)", m)) replaces the zero-pad approach, correctly ordering numbered filenames regardless of length.

Confidence Score: 4/5

The fallback logic and natural sort are correct; the main concern is the test file's import path, which requires the repository root to be on sys.path and will silently pass locally but may fail in a fresh CI environment.

The checkpoint fallback and natural sort changes are straightforward and correct. The test import relies on namespace-package resolution from the repo root being in sys.path — something not guaranteed by the current pytest configuration, which has no pythonpath entry and no conftest.py in the test directory. This makes the test environment-dependent and potentially brittle in CI.

scripts/reinforcement_learning/test/test_checkpoint_path_fallback.py — the import path and lack of conftest.py should be reviewed before the test is relied upon in CI.

Important Files Changed

Filename	Overview
scripts/reinforcement_learning/rl_games/checkpoint.py	New helper that wraps get_checkpoint_path with a fallback to the latest checkpoint; logic is correct but catches both "no runs" and "no checkpoints" ValueError variants
scripts/reinforcement_learning/rl_games/play.py	Swaps get_checkpoint_path for get_rl_games_checkpoint_path; straightforward one-line change with correct arguments
scripts/reinforcement_learning/rl_games/play_rl_games.py	Identical checkpoint-call update to play.py; no issues
scripts/reinforcement_learning/test/test_checkpoint_path_fallback.py	Tests cover the happy path and fallback path correctly, but the import uses a dotted namespace-package path that is fragile without a conftest.py or pythonpath config; likely to break in clean CI environments
source/isaaclab_tasks/isaaclab_tasks/utils/parse_cfg.py	Natural sort replaces the broken zero-pad-to-15-chars key; correctly orders epoch checkpoints like ep_10 after ep_9 even for long filenames where the old approach failed
source/isaaclab_tasks/changelog.d/fix-rlgames-checkpoint-fallback.rst	Changelog entry accurately describes both the fallback fix and the natural sort improvement

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[play.py / play_rl_games.py\ncalls get_rl_games_checkpoint_path] --> B{checkpoint_file\n== '.*'?}
    B -- No --> C[get_checkpoint_path\nwith preferred name]
    B -- Yes --> C2[get_checkpoint_path\nwith '.*']
    C --> D{ValueError?}
    D -- No --> E[Return preferred checkpoint]
    D -- Yes --> F{checkpoint_file\n== '.*'?}
    F -- Yes --> G[Re-raise ValueError]
    F -- No --> H[get_checkpoint_path\nwith '.*' fallback]
    H --> I{ValueError?}
    I -- Yes --> J[Re-raise ValueError\nno checkpoints at all]
    I -- No --> K[Print INFO message\nReturn latest checkpoint\nvia natural sort]
    C2 --> L{ValueError?}
    L -- Yes --> G
    L -- No --> M[Return latest checkpoint\nvia natural sort]

Loading

_{Reviews (1): Last reviewed commit: "Fix rl_games play checkpoint fallback" | Re-trigger Greptile}

[greptile-apps]

Test import path is inconsistent and environment-dependent

The test imports via the dotted namespace-package path scripts.reinforcement_learning.rl_games.checkpoint, but there are no __init__.py files in any of those directories and no pythonpath = ["."] entry under [tool.pytest.ini_options] in pyproject.toml. With pytest's default prepend importmode, only the test file's own directory (scripts/reinforcement_learning/test/) is inserted into sys.path, so this import will raise ModuleNotFoundError in a clean environment or CI unless the repository root is already on PYTHONPATH for an unrelated reason (e.g. editable install side-effects). The scripts themselves use the simpler bare from checkpoint import get_rl_games_checkpoint_path, which works because Python adds the script's directory to sys.path[0]. A conftest.py that inserts the rl_games directory, or using pythonpath = ["."] in pytest config, would make the test reliably portable.

[greptile-apps]

Broad ValueError catch covers "no runs found" and "no checkpoints found" equally

get_checkpoint_path raises ValueError for two distinct conditions: (1) no run directories match run_dir, and (2) no checkpoint files match checkpoint. Only case 2 is the intended trigger for the fallback — when the preferred "best" checkpoint hasn't been written yet. For case 1 (the run directory doesn't exist at all), the fallback unconditionally attempts a second get_checkpoint_path(…, ".*", …) call that will also raise the same "no runs" ValueError, which propagates correctly. The behavior is functionally fine, but it may be worth parsing or re-raising the "no runs" variant directly to make the intent explicit and avoid any confusion if the exception messages are inspected downstream.

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!