Skip to content

Capture diagnostics when environment tests stop making progress - #7882

Open
myurasov-nv wants to merge 1 commit into
isaac-sim:developfrom
myurasov-nv:myurasov/bound-test-subprocess-runtime
Open

myurasov-nv wants to merge 1 commit into
isaac-sim:developfrom
myurasov-nv:myurasov/bound-test-subprocess-runtime

Conversation

@myurasov-nv

Copy link
Copy Markdown
Member

Summary

Add a 20-minute pytest-journal progress deadline for the Isaac Sim PhysX, Newton, and OVPhysX environment suites, with the existing cold-camera allowance. Console noise does not reset the deadline. Collect thread stacks and system diagnostics, kill the subprocess group, and reconstruct completed results through the existing crash journal. Retain each suite’s overall runtime budget.

Motivation

Several task shards reach GitHub’s three-hour job timeout while stuck in environment tests. Their 10,000-second file budgets leave too little time for diagnostics and later test files. In run 35256390526, task shard 2 stopped reporting progress for over 2.5 hours and its hang-dump artifact was empty.

Validation

  • 19 orchestrator tests passed, including real hung subprocesses and noisy versus progressing subprocesses.
  • Regression verified against the original code.
  • All pre-commit hooks passed for changed files.
  • The standard uv run isaaclab -f command cannot resolve this Linux/Windows-only lockfile on macOS; the same pre-commit checks were run in an isolated uv environment.

The underlying simulation hang is not yet attributed; this change captures the evidence needed to diagnose it.

@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with no actionable correctness, security, or repository-rule violations identified.

Summary

This PR adds inactivity detection to the Isaac Sim PhysX, Newton, and OVPhysX environment suites while preserving their existing total runtime budgets.

  • Tracks progress through crash-journal file growth rather than console output.
  • Captures system and thread diagnostics before terminating a stalled subprocess group.
  • Applies a 20-minute inactivity threshold, including the existing cold-camera allowance where applicable.
  • Extends orchestrator coverage for hung, noisy, and genuinely progressing subprocesses.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Launch pytest subprocess] --> B[Monitor hard runtime and crash journal]
    B --> C{Journal size changed?}
    C -- Yes --> D[Reset progress deadline]
    C -- No --> E{Deadline exceeded?}
    D --> B
    E -- No --> B
    E -- Yes --> F[Capture thread stacks and system diagnostics]
    F --> G[Kill subprocess group]
    G --> H[Reconstruct results from crash journal]
Loading

Reviews (1) · Last reviewed commit: "Capture diagnostics when environment tes..."

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isaac Lab Review Bot

The journal-backed progress deadline reuses the existing diagnostics, process-group termination, and crash-result reconstruction paths, but two timing details need correction before merge: the progress clock begins during the separately budgeted startup phase, and empty-journal timeout results retain the full file budget instead of the measured elapsed time.

  • Design and architecture: Using crash-journal growth rather than console output as the liveness signal is consistent with the intended behavior and preserves a single termination pathway. However, the 1200-second progress window starts at process launch and overlaps the same suites’ 1000-second external-asset startup allowance, leaving only about 200 seconds after an otherwise permitted startup to produce the first journal entry. Start the progress window when startup completes.
  • API: capture_test_output_with_timeout preserves its signature and six-element return contract, while documenting the environment-controlled progress deadline. Reusing kill_reason == "timeout" keeps existing downstream reconstruction compatible; no public API break is introduced.
  • Implementation: Monotonic timing, journal-size polling, diagnostics capture, and process-group cleanup are implemented coherently. Reset last_progress when startup first completes, and pass measured wall_time as fallback_time_elapsed so progress-stall failures with an empty journal do not report the much larger overall file budget in synthesized results and aggregate timings.

Minor fixes needed. Posted 2 actionable findings inline.

Automated review; human maintainers own approval decisions.

Comment thread tools/conftest.py
shutdown_deadline = 0.0
progress_timeout = float(env.get("ISAACLAB_TEST_PROGRESS_TIMEOUT", "0"))
journal_file = env.get(JOURNAL_ENV_VAR, "")
last_progress = start_time

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Warning · Implementation — Progress deadline overlaps cold git-asset startup window

last_progress is seeded at process launch and only advances when the journal grows, so the 1200 s budget must also cover Kit startup and collection. The same three files declare GIT_ASSET_STARTUP_TIMEOUT (1000 s) in PER_TEST_STARTUP_TIMEOUTS for a cold external asset clone, leaving roughly 200 s for the first journal record before a healthy run is killed and reported as a timeout. Reset last_progress when startup_done first becomes true.

Comment thread tools/conftest.py
ovrtx_log_section = ovrtx_log.format_log_section(ovrtx_log.LOG_PATH, pass_file_label)

msg = f"Timeout after {ctx.timeout} seconds (retried {timeout_attempts} time(s))"
msg = f"Timeout after {wall_time:.1f} seconds (file budget {ctx.timeout}s; retried {timeout_attempts} time(s))"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Suggestion · Implementation — Synthesized timeout case still records file budget

The log and message now report the measured wall_time, but _make_crash_pass_result is still passed fallback_time_elapsed=ctx.timeout. When a progress stall kills the run at ~1200 s before any journal entry exists, the synthesized TIMEOUT case records the multi-thousand-second file budget instead, inflating shard totals and the slowest-files ranking. Pass wall_time as the fallback duration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infrastructure isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant