Conversation
bc9bb9d to
e80dca2
Compare
kellyguo11
left a comment
There was a problem hiding this comment.
Review: PR #559 — revert test
Summary: This PR re-enables 5 eval runner tests that were previously skipped with @pytest.mark.skip(reason="Skipping because of CI stalling") by replacing those skip markers with @pytest.mark.with_subprocess.
Context understood: The base branch xyao/exp/ci_stall added infrastructure to separate subprocess-spawning tests from persistent SimulationApp tests (via the with_subprocess marker and CI workflow changes). This PR is the natural follow-up that actually reverts the skips and uses the new marker.
✅ What looks good
- The change is straightforward: 5 mechanical replacements of
skip→with_subprocess. - The
with_subprocessmarker is properly registered inpytest.iniand the CI workflow already has a dedicated step that runs-m with_subprocesstests withISAACLAB_ARENA_SUBPROCESS_TIMEOUT=900. - The marker semantics are correct — these tests all use
run_eval_runner_and_check_no_failures(), which spawnseval_runner.pyviasubprocess.run().
⚠️ Potential concern: missing timeout / process group isolation in run_eval_runner_and_check_no_failures
The utility run_subprocess() in isaaclab_arena/tests/utils/subprocess.py was carefully written with:
start_new_session=Trueto isolate GPU child processestimeout=_SUBPROCESS_TIMEOUT_SECto prevent hangs
However, run_eval_runner_and_check_no_failures() in this test file uses raw subprocess.run(args, capture_output=True, text=True, check=True) — no timeout, no start_new_session=True. If the original CI stalling was caused by subprocess hangs or orphaned GPU processes, these tests could still stall even with the marker separation, since the subprocess itself has no timeout.
Suggestion: Consider either:
- Refactoring
run_eval_runner_and_check_no_failures()to userun_subprocess()from the utils module, or - At minimum, adding
timeout=_SUBPROCESS_TIMEOUT_SECandstart_new_session=Trueto thesubprocess.run()call.
This isn't a blocker if the CI workflow-level timeout-minutes: 60 is sufficient, but it would be more robust to have per-subprocess timeouts too.
PR description
The PR description still has the default template text. A brief note about reverting the skips now that the CI stalling fix is in place would help future readers.
Overall: The change itself is correct and minimal. Approving since the marker infrastructure is in place and CI will validate.
Summary
Short description of the change (max 50 chars)
Detailed description