Address SQA bug: list variations without policy.

[alexmillane]

Summary

Address issue that listing variations used to fail without a policy specified.

Detailed description

• Addresses 6491076

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

Small, well-scoped fix: --policy_type becomes optional (default=None) so --list_variations no longer requires a policy, and the short-circuit is moved before the policy lookup into a dedicated list_variations helper. The change is correct and does what it says. One small style point and a missing regression test.

Findings

🟡 Warning: policy_runner.py:179 — assert args_cli.policy_type is not None guards a user-facing CLI flag. Arena reserves assert for internal invariants and uses raised errors for external/user input; parser.error(...) also prints usage and exits cleanly (asserts vanish under -O). Suggested inline.

Test Coverage

This is a bug fix, and test_variations_in_policy_runner.py already has the subprocess harness (run_policy_runner, --headless) that would make a regression test straightforward: run the runner with --list_variations and no --policy_type, assert exit 0 and that the variations catalogue is printed. Without it, nothing guards against --policy_type being made required again. Worth adding a small @pytest.mark.with_subprocess test.

Verdict

Minor fixes needed

[greptile-apps]

Greptile Summary

This PR fixes a bug where --list_variations failed when --policy_type was not supplied, by moving the variations short-circuit before policy loading and making --policy_type optional (default=None).

• list_variations() is extracted as a standalone function that builds the environment parser and prints the catalogue without touching policy arguments, and is called early in main() before any policy-type handling.
• --policy_type is changed from required=True to default=None in add_policy_runner_arguments, with a manual check added after the --list_variations branch to ensure the argument is present for normal policy runs.

Confidence Score: 4/5

The change is logically correct and fixes the reported bug; the only weak point is a user-facing validation guard that can be silenced by Python's optimizer.

The refactoring cleanly separates the --list_variations path from policy loading, and making --policy_type optional with a manual check is the right approach. However, the guard is implemented as an assert statement, which Python's -O flag strips at runtime — leaving get_policy_cls(None) to be called without any check in an optimized environment. Using args_parser.error() would be the correct idiom here.

The assert on line 179 of policy_runner.py is the only line that needs attention.

Important Files Changed

Filename	Overview
isaaclab_arena/evaluation/policy_runner.py	Extracts list_variations() as a standalone function and short-circuits before policy loading; uses assert for user-facing CLI validation instead of args_parser.error().
isaaclab_arena/evaluation/policy_runner_cli.py	Changes --policy_type from required=True to default=None to allow --list_variations to run without specifying a policy; straightforward and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[main] --> B[get_isaaclab_arena_cli_parser]
    B --> C[add_policy_runner_arguments\n--policy_type default=None]
    C --> D[parse_known_args]
    D --> E{args_cli.list_variations?}
    E -- Yes --> F[list_variations\nargs_parser]
    F --> F1[get_isaaclab_arena_environments_cli_parser]
    F1 --> F2[parse_known_args]
    F2 --> F3[get_arena_builder_from_cli]
    F3 --> F4[print variations catalogue]
    F4 --> G[return]
    E -- No --> H{policy_type is None?}
    H -- Yes --> I[args_parser.error\n--policy_type is required\n currently uses assert]
    H -- No --> J[get_policy_cls]
    J --> K[get_isaaclab_arena_environments_cli_parser]
    K --> L[get_arena_builder_from_cli]
    L --> M[rollout_policy]

Loading

_{Reviews (1): Last reviewed commit: "Address SQA bug, list variations without..." | Re-trigger Greptile}