[3.0.0-beta2] Forward AutoMate run_w_id downstream args

[ooctipus]

Summary

• make run_w_id.py explicitly accept --viz/--visualizer and forward it to the delegated RL-Games script
• forward any additional unknown args, so other downstream launcher flags such as --headless can pass through

run_w_id.py is a convenience wrapper for AutoMate Assembly runs: it updates the selected assembly ID in the AutoMate task config, then launches the normal RL-Games train/play script through ./isaaclab.sh -p. Since the downstream script owns AppLauncher startup, visualizer selection should be forwarded rather than handled by the wrapper itself.

Validation

• git diff --check upstream/release/3.0.0-beta2..HEAD
• python3 -m py_compile source/isaaclab_tasks/isaaclab_tasks/direct/automate/run_w_id.py
• python3 source/isaaclab_tasks/isaaclab_tasks/direct/automate/run_w_id.py --help
• pre-commit run --files source/isaaclab_tasks/changelog.d/fix-automate-run-w-id-forward-args.rst source/isaaclab_tasks/isaaclab_tasks/direct/automate/run_w_id.py

[greptile-apps]

Greptile Summary

This PR improves the run_w_id.py AutoMate wrapper by extracting command construction into a testable build_command function, adding explicit --visualizer/--viz forwarding, and switching from parse_args to parse_known_args so that unrecognized flags (e.g. --headless) pass through to the downstream RL-Games script. A new unit test exercises the happy-path command construction.

• build_command extraction separates subprocess invocation from argument assembly, enabling the new test to verify the forwarded command without touching the filesystem or spawning a process.
• --visualizer/--viz is now an explicit parser argument with allow_abbrev=False guarding against prefix-match ambiguity; it is forwarded as --visualizer <value> to the downstream script.
• Unknown args passthrough via parse_known_args means flags like --headless are no longer silently dropped.

Confidence Score: 4/5

Safe to merge; the change is narrow and well-contained, with no regressions to existing behaviour.

The refactor is straightforward: command construction is correctly extracted, parse_known_args works as intended, and the --visualizer/--viz alias is wired up correctly through argparse. The test covers the primary forwarding path. The only gaps are minor: the test doesn't assert the no-visualizer path, and a long-standing missing required=True on --assembly_id remains unfixed.

No files require special attention; run_w_id.py and its test are both straightforward.

Important Files Changed

Filename	Overview
source/isaaclab_tasks/isaaclab_tasks/direct/automate/run_w_id.py	Extracts command construction into build_command, adds --visualizer/--viz forwarding, switches to parse_known_args for passthrough, and adds allow_abbrev=False; logic is correct though --assembly_id still lacks a None guard.
source/isaaclab_tasks/test/test_automate_run_w_id.py	New focused unit test for the wrapper command-construction path; covers the happy path with --viz and --headless passthrough, but is missing a negative test (no visualizer flag) and the --visualizer long-form variant.
source/isaaclab_tasks/changelog.d/fix-automate-run-w-id-forward-args.rst	One-line changelog entry accurately describing the fix; no issues.

Sequence Diagram

sequenceDiagram
    participant User
    participant run_w_id.py
    participant assembly_tasks_cfg.py
    participant isaaclab.sh

    User->>run_w_id.py: "python run_w_id.py --assembly_id=00004 --viz kit --headless"
    run_w_id.py->>run_w_id.py: "parse_known_args()<br/>args.visualizer=kit<br/>downstream_args=[--headless]"
    run_w_id.py->>assembly_tasks_cfg.py: update_task_param(cfg_path, 00004, ...)
    run_w_id.py->>run_w_id.py: build_command(args, downstream_args)
    run_w_id.py->>isaaclab.sh: "subprocess.run(command, check=True)"

Loading

_{Reviews (1): Last reviewed commit: "Forward Automate run_w_id downstream arg..." | Re-trigger Greptile}

[isaaclab-review-bot]

Code Review: PR #6136

Thanks for this contribution! This PR improves the run_w_id.py wrapper by enabling proper argument forwarding to downstream scripts. Here's my review:

✅ What Looks Good

1. Clean Refactoring: Extracting build_command() as a separate function improves code organization and testability.

2. Proper Argument Forwarding: Using parse_known_args() with allow_abbrev=False is the correct pattern for wrapper scripts that need to pass through unknown arguments.

3. Visualizer Support: Adding explicit --visualizer/--viz support with proper forwarding to the downstream RL-Games script addresses the stated issue.

4. Helpful Epilog: The parser epilog clearly communicates that additional arguments are forwarded, which improves UX.

5. Changelog Entry: The changelog entry is clear and follows project conventions.

💬 Suggestions (Non-blocking)

Consider using consistent argument style

The --visualizer argument uses space-separated syntax when forwarding:

command.extend(["--visualizer", args.visualizer])

While other arguments use = syntax:

command.append(f"--checkpoint={args.checkpoint}")

Both work, but consistency might be nice. This is purely stylistic and not a blocking concern.

Test coverage opportunity

The PR description mentions adding a unit test, but I only see 2 files in the changeset (changelog + run_w_id.py). If a test file was intended, it may not have been included in this commit. Consider adding tests for:

• The build_command() function directly
• Visualizer argument forwarding
• Downstream argument passthrough (e.g., --headless)

This would help prevent regressions and document expected behavior.

ℹ️ Notes

• The file modification in update_task_param() happens before subprocess execution. If the subprocess fails, the config file remains modified. This is existing behavior and may be intentional for debugging, but worth noting.

• Typos in known argument names (e.g., --assmbly_id instead of --assembly_id) will now be silently passed through to the downstream script rather than raising an error. This is expected behavior with parse_known_args() but users should be aware.

---

Overall this is a clean, focused PR that solves a real usability issue.

---

Update (d1149f9): Reviewed new commits since c0cfc28. The diff is unchanged from the previous review — no new issues introduced. Previous suggestions (argument style consistency, test coverage) remain open but are non-blocking. LGTM.

---

Update (3ee9ea2): Reviewed incremental changes since d1149f9. No new issues introduced. Previous inline suggestions (use is not None for visualizer check, downstream_args ordering, test coverage) remain unaddressed but are non-blocking. LGTM.

[isaaclab-review-bot]

suggestion: if args.visualizer is not None: would be safer than truthiness — an empty string "" is falsy and would silently skip forwarding. Matches the default=None sentinel more precisely.

[isaaclab-review-bot]

note (low risk): Since --num_envs, --seed, etc. are emitted above from parsed defaults, a user who also passes --num_envs=64 as a "downstream" arg will produce a duplicate flag. Whether last-wins depends on the downstream argparser. Consider a brief comment noting this or placing downstream_args before the explicit flags.

[isaaclab-review-bot]

suggestion: Consider adding a second test case (or parametrizing) with --train instead of --log_eval to cover the training script path (train.py vs play.py).

[greptile-apps]

Missing negative and long-form visualizer test cases

The single test verifies --viz kit is forwarded correctly, but two cases are untested: (1) when neither --viz nor --visualizer is provided, the delegated command should contain no --visualizer entry at all (the default=None path through build_command); and (2) the --visualizer long-form alias. A future refactor that accidentally changes the default or the long-form dest would not be caught by the current suite.

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!

[greptile-apps]

--assembly_id can be None and silently corrupts the config

assembly_id has no required=True and no None guard — only the sentinel string "ASSEMBLY_ID" is caught. When --assembly_id is omitted entirely, args.assembly_id is None, update_task_param writes assembly_id = 'None' into assembly_tasks_cfg.py, and eval_filename becomes evaluation_None.h5, without any error being surfaced. This pre-dated the PR but the refactor is a natural point to add required=True or an explicit None check here.