Skip to content

Fixes Rlinf config path#6137

Open
mingxueg-nv wants to merge 1 commit into
isaac-sim:developfrom
mingxueg-nv:mingxue/fix_path_on_changed_isaaclab_task
Open

Fixes Rlinf config path#6137
mingxueg-nv wants to merge 1 commit into
isaac-sim:developfrom
mingxueg-nv:mingxue/fix_path_on_changed_isaaclab_task

Conversation

@mingxueg-nv

@mingxueg-nv mingxueg-nv commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes path of RLinf config yaml file caused by missing path resolver.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@mingxueg-nv mingxueg-nv requested a review from ooctipus as a code owner June 11, 2026 05:57
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Jun 11, 2026
@greptile-apps

greptile-apps Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the RLinf config YAML path resolution in both play_rlinf.py and train_rlinf.py by replacing the bare config_path or str(SCRIPT_DIR / RLINF_DIR) fallback with a call to cli_args.resolve_config_dir, which first searches the isaaclab_tasks package tree before falling back to the script directory.

  • play_rlinf.py: Swaps assignment order so config_name is available before calling cli_args.resolve_config_dir(config_name, args_cli.config_path).
  • train_rlinf.py: Same substitution using the already-imported CLI_ARGS module handle, replacing the old args_cli.config_path or str(RLINF_DIR) one-liner.

Confidence Score: 5/5

Minimal, targeted change — only the config-dir resolution expression is touched in two files; all surrounding logic is unchanged.

Both changes correctly delegate to resolve_config_dir, which gracefully handles all three cases: explicit path, package-tree search, and script-directory fallback. The fallback directory resolves to the same path that was previously hardcoded, so the fix is strictly additive.

No files require special attention.

Important Files Changed

Filename Overview
scripts/reinforcement_learning/rlinf/play_rlinf.py Replaces the inline fallback with a call to cli_args.resolve_config_dir, which additionally searches the isaaclab_tasks package tree before falling back to the script directory. The order of config_name / config_dir assignment is swapped accordingly.
scripts/reinforcement_learning/rlinf/train_rlinf.py Same one-line fix as play_rlinf.py: replaces args_cli.config_path or str(RLINF_DIR) with CLI_ARGS.resolve_config_dir(config_name, args_cli.config_path), using the already-imported cli_args module loaded via import_local_module.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Script starts
play_rlinf.py / train_rlinf.py] --> B{--config_path
provided?}
    B -- Yes --> C[Use explicit config_path]
    B -- No --> D{isaaclab_tasks
package found?}
    D -- Yes --> E[rglob config_name.yaml
within package tree]
    E --> F{Match
found?}
    F -- Yes --> G[Return parent dir
of first match]
    F -- No --> H[Fall back to
script directory]
    D -- No --> H
    C --> I[Build RLINF_CONFIG_FILE
env var path]
    G --> I
    H --> I
    I --> J[Hydra initialize_config_dir
+ compose]
Loading

Reviews (1): Last reviewed commit: "fix config path" | Re-trigger Greptile

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Verdict: Clean, focused bug fix. Looks good overall.

This PR correctly replaces the hardcoded fallback directories (SCRIPT_DIR in play_rlinf.py and RLINF_DIR in train_rlinf.py) with calls to cli_args.resolve_config_dir(), which searches the isaaclab_tasks package tree for the config YAML before falling back to the script directory. This ensures that config files located in the task packages are properly discovered — fixing path resolution for task configurations that have been moved/reorganized.

✅ What's Good

  1. Consistent pattern: Both files now use the same resolve_config_dir() resolution logic, eliminating divergent fallback behavior.
  2. Non-breaking: The resolve_config_dir function gracefully falls back to the script directory (same as before) when the config isn't found in isaaclab_tasks, so existing workflows using --config_path or local configs remain unaffected.
  3. Correct ordering: The config_name assignment is moved above config_dir since resolve_config_dir needs it as input — good catch.

📝 Minor Observations

  1. No changelog fragment: The checklist shows the changelog/version update is unchecked. For a bug fix that changes runtime path resolution, a fragment under "Fixed" (e.g., "Fixed RLinf config YAML resolution to search isaaclab_tasks package tree") would help downstream users understand why their configs now resolve correctly.

  2. Fallback behavior difference: In the old code, play_rlinf.py fell back to SCRIPT_DIR (the rlinf/ directory) while train_rlinf.py fell back to RLINF_DIR (also the rlinf/ directory, computed differently). The resolve_config_dir function uses _SCRIPT_DIR (same rlinf/ directory) as its final fallback. The behavior is preserved, but it's worth noting the unification is intentional.

Verdict

The fix is minimal, correct, and well-scoped. No functional concerns.

@mingxueg-nv mingxueg-nv changed the title fix config path Fixes Rlinf config path Jun 11, 2026
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.

2 participants