Skip slow camera test and use run_subprocess() for eval_runner tests

[xyao-nv]

Summary

CI subprocess tests are slow and faced with timeout without stalling

Detailed description

• Skipped test_eval_runner_enable_cameras as cold-start camera rendering takes ~1165s, making CI exceeding the timeout.
• Replaced raw subprocess.run() with the shared run_subprocess() helper, which enforces ISAACLAB_ARENA_SUBPROCESS_TIMEOUT (900s in CI).
• Removed redundant stdout-regex failure check; the eval_runner already exits non-zero on job failure (no --continue_on_error).

[greptile-apps]

Greptile Summary

This PR skips two slow camera tests (test_eval_runner_enable_cameras and test_g1_locomanip_gr00t_closedloop_policy_runner_eval_runner) that exceed the CI subprocess timeout (~1165s vs the 900s limit), and migrates run_eval_runner and the gr00t_finetuned_model_path fixture to use the shared run_subprocess() helper instead of raw subprocess.run(). The refactoring is clean and consistent with the existing pattern for subprocess-based tests.

Confidence Score: 5/5

Safe to merge — changes are confined to test infrastructure with no production code impact.

No P0 or P1 issues found. The run_subprocess() migration is correct, timeout handling is consistent, and the only open concern (missing @pytest.mark.with_subprocess on test_eval_runner_enable_cameras) was already raised in a prior review thread. All remaining observations are P2 or lower.

No files require special attention.

Important Files Changed

Filename	Overview
isaaclab_arena/tests/test_eval_runner.py	Skips test_eval_runner_enable_cameras with @pytest.mark.skip; run_eval_runner already uses run_subprocess(). The missing @pytest.mark.with_subprocess on the skipped test was flagged in a prior review thread.
isaaclab_arena_gr00t/tests/test_gr00t_closedloop_policy.py	Migrates gr00t_finetuned_model_path fixture to run_subprocess(); adds @pytest.mark.with_subprocess + @pytest.mark.skip to test_g1_locomanip_gr00t_closedloop_policy_runner_eval_runner. Decorator ordering is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[pytest -m with_subprocess] --> B{Test has @pytest.mark.with_subprocess?}
    B -- No --> C[Silently omitted from run]
    B -- Yes --> D{Test has @pytest.mark.skip?}
    D -- Yes --> E[Collected but skipped\ne.g. test_eval_runner_enable_cameras\nin gr00t test file]
    D -- No --> F[run_eval_runner / run_subprocess]
    F --> G{Subprocess exits non-zero\nor timeout > 900s?}
    G -- Timeout --> H[SubprocessError raised\n_AT_LEAST_ONE_TEST_FAILED = True]
    G -- Non-zero exit --> I[CalledProcessError raised\n_AT_LEAST_ONE_TEST_FAILED = True]
    G -- Success --> J[Test passes]
    H --> K[CI reports failure]
    I --> K

Loading

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into xyao/ci/test_ev..." | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR improves eval_runner test reliability by replacing fragile stdout regex-based failure detection with exit-code propagation via the shared run_subprocess() helper, and skips a camera test that exceeds CI timeout due to ~1165s cold-start rendering. Clean, well-motivated simplification.

Design Assessment

Design is sound. The old approach parsed eval_runner stdout for a status table to detect failures — fragile and redundant since --continue_on_error is not passed, meaning the runner already exits non-zero on the first job failure. Delegating to run_subprocess() is the right call: it adds timeout enforcement (ISAACLAB_ARENA_SUBPROCESS_TIMEOUT), process-group isolation (start_new_session=True), and consistent error handling across the test suite. The regex removal eliminates a maintenance burden and a source of false negatives if the output format ever changes.

Findings

🟡 Warning: Unconditional skip removes camera test coverage entirely — test_eval_runner.py:149

@pytest.mark.skip is unconditional — this test will never run, including during local development where the 1000s cold-start may not apply. Consider using a CI-specific gate so developers can still run it locally:

@pytest.mark.skipif(
    os.environ.get("CI") == "true",
    reason="CI takes 1000s to cold-start camera rendering.",
)

This preserves local-dev coverage while fixing the CI timeout. If this test is genuinely too slow for any context, the unconditional skip is fine — but documenting that decision in the skip reason would help (e.g., "Always slow; run manually when camera rendering changes.").

🔵 Suggestion: Consider logging which run_subprocess timeout applies — test_eval_runner.py:42

The new run_eval_runner doesn't pass an explicit timeout_sec, so it inherits the default from ISAACLAB_ARENA_SUBPROCESS_TIMEOUT (or 600s fallback). This is fine for CI where the env var is set to 900s, but a developer running locally without the env var gets a 600s timeout — which may be surprising for heavier jobs. Not a blocker, just worth being aware of.

Test Coverage

This is a test infrastructure change, not a feature or bug fix — the tests themselves are the test coverage. The remaining 4 active eval_runner tests adequately cover multi-job, multi-environment, multi-embodiment, and existing-config scenarios. The skipped camera test is the only gap, addressed in the finding above.

• Test quality: Good — clean separation of concerns
• Coverage gaps: Camera rendering path (skipped, see finding above)

CI Status

Pre-commit check is pending.

Verdict

Ship it 🚀

Clean simplification that removes fragile regex parsing in favor of proper exit-code handling via the shared subprocess helper. The only consideration is whether the camera test skip should be CI-conditional rather than unconditional — but that's a minor point that shouldn't block merge.

[alexmillane]

Seems reasonable. Of course we'd like to re-enable this, but this seems reasonable while we work out a path to doing so.