[CI] Cross-platform — Part 4: Multi-GPU pytest workflow with cuda:1 coverage

[hujc7]

1. Summary

• CI: Adds a dedicated multi-GPU pytest workflow that runs one shard per non-default cuda:N, with a shared atomic work queue, per-file reports, and end-of-run reconciliation.
• isaaclab: Adds composable DeviceScope flags and test_devices() for scope ∩ runtime parametrization while retaining exact custom string masks.
• isaaclab: Uses ISAACLAB_TEST_DEVICES as the single source for both test parametrization and explicit Kit launch-device selection; AppLauncher has no implicit test-environment override.
• isaaclab: Migrates 19 device-parametrized test modules across the core, Newton, OV PhysX, and PhysX suites while preserving the existing cpu + cuda:0 single-GPU behavior.

2. Device selection

from isaaclab.test.utils import DeviceScope, test_devices

test_devices()                                                  # cpu + every GPU
test_devices(DeviceScope.CUDA)                                  # every GPU
test_devices(DeviceScope.CPU | DeviceScope.NON_DEFAULT_CUDA)    # composed scope
test_devices("101X")                                            # exact custom mask

• ISAACLAB_TEST_DEVICES limits which devices a run may use; unset preserves the historical cpu + cuda:0 runtime.
• The multi-GPU lane discovers non-default-capable scopes and narrows each shard to one concrete device.

3. Dependencies

All prerequisite changes have merged into develop, so this PR no longer carries them — it is now scoped to the device-selection infrastructure and the multi-GPU pytest workflow only:

• [CI] Cross-platform — Part 1: pytest markers and shared scaffolding (base) #5695 — Cross-platform Part 1 (base pytest markers / shared scaffolding).
• [MGPU] Sim: honor device kwarg over sim_cfg.device in build_simulation_context #5881 — Sim honors the device kwarg over sim_cfg.device in build_simulation_context.
• [MGPU] App: default Kit renderer to one GPU #5933 — Kit renderer defaults to one GPU + adds ISAACLAB_FABRIC_USE_GPU_INTEROP; the multi-GPU lane sets ISAACLAB_FABRIC_USE_GPU_INTEROP=0.
• [Newton] Fix multi-GPU initialization on non-default devices #6322 — Newton multi-GPU initialization on non-default CUDA devices (cuda:1 and higher).

4. Test plan

• Device-selection unit suite passes.
• Changed Python modules compile; multi-GPU shell scripts pass bash -n.
• Full documentation build (./isaaclab.sh -d).
• Full pre-commit suite (./isaaclab.sh -f).
• GitHub Actions green on latest develop (one rendering-correctness-kitless flake cleared on rerun; fixed upstream on develop by [Fix] Bump test_rendering_shadow_hand.py timeout to 1500s #6431).

[isaaclab-review-bot]

Overall

This is a well-designed PR that introduces a clean, env-driven device parametrization helper for multi-GPU testing. The mask grammar is elegant and the documentation in the module docstring is excellent. The workflow setup and test migrations look correct.

However, there's a critical issue that will prevent the tests from running.

---

Critical: Missing __init__.py

The new isaaclab.testing package is missing its __init__.py file. Without it, Python won't recognize source/isaaclab/isaaclab/testing/ as a package, and all the imports like:

from isaaclab.testing import cuda_test_devices

will fail with ModuleNotFoundError.

Fix: Add source/isaaclab/isaaclab/testing/__init__.py with at minimum:

# Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md).
# All rights reserved.
#
# SPDX-License-Identifier: BSD-3-Clause

"""Testing utilities for Isaac Lab."""

from .devices import cuda_test_devices

__all__ = ["cuda_test_devices"]

---

Minor Suggestions

1. Workflow: Consider using matrix strategy for future extensibility

The pytest target list is explicit (good for control), but as you add more files in P1/P2, consider whether a matrix or glob pattern might be cleaner. Current approach is fine for now.

2. devices.py line 114: Consider caching available devices

Calling torch.cuda.device_count() on every test parametrization is cheap but redundant. A module-level cache (computed once at import) would be cleaner:

@functools.lru_cache(maxsize=1)
def _list_available_devices() -> tuple[str, ...]:
    ...

(Return tuple for hashability, or just compute at module level.)

3. Workflow line 57: Pin Isaac Sim version handling

The fallback '6.0.0' is hardcoded. If vars.ISAACSIM_BASE_VERSION is the source of truth, consider failing explicitly when unset rather than silently falling back, to avoid version drift.

4. Consider adding a simple unit test for cuda_test_devices itself

A few tests for the helper (mask parsing, strict vs non-strict, edge cases like empty mask, invalid chars) would catch regressions in the helper logic itself.

---

Summary

• Blocking: Add the missing __init__.py for isaaclab.testing
• Non-blocking: Minor polish suggestions above

Once the __init__.py is added, this looks good to merge. Nice work on the mask grammar design! 🎯

[isaaclab-review-bot]

Overall Assessment

This is a well-designed PR that introduces an elegant, env-driven device parametrization system for multi-GPU testing. The mask grammar is intuitive (e.g., 110 → [cpu, cuda:0], 001 → [cuda:1]), the documentation is excellent, and the backward-compatible default ensures zero impact on existing single-GPU CI runs.

The PR successfully addresses:

• ✅ New isaaclab.testing module with cuda_test_devices() helper
• ✅ Auto-discovery workflow that picks up opt-in test files automatically
• ✅ Migration of 19 test files (~280 parametrize sites)
• ✅ Clear P1/P2 scope documentation for follow-ups

---

Findings

1. Missing Unit Tests for the Helper Function (Medium)

Location: source/isaaclab/isaaclab/testing/devices.py

The cuda_test_devices() function has non-trivial mask parsing logic with multiple edge cases (trailing X, strict vs non-strict, empty masks, invalid characters), but there are no dedicated unit tests for the helper itself.

Recommendation: Add a test file like source/isaaclab/test/testing/test_cuda_test_devices.py with cases covering:

• Default mask resolution
• Explicit mask parsing ("110", "001", "00X", "X")
• Strict mode raising on unavailable devices
• Non-strict mode returning empty list gracefully
• Invalid mask characters raising ValueError

This protects against regressions in the helper logic itself.

---

2. Workflow SKIP List Pattern Could Be More Robust (Low) ✅ RESOLVED

Resolved in f66b101 — The new include-files approach is cleaner and avoids the heredoc pattern entirely.

---

3. Consider Caching Device List (Nit)

Location: source/isaaclab/isaaclab/testing/devices.py line 113

_list_available_devices() is called on every cuda_test_devices() invocation. While torch.cuda.device_count() is cheap, caching the result with @functools.lru_cache would be cleaner and more efficient for test collection.

---

Summary

This is a solid foundation for multi-GPU testing infrastructure. The design choices are well-reasoned and the migration is clean.

Actionable items:

1. (Recommended) Add unit tests for cuda_test_devices() to catch edge-case regressions
2. (Optional) Minor robustness improvements noted above ✅ Resolved

Nice work on the mask grammar design! 🎯

---

Update (d6b2934): New commit adds explicit pip install pytest step to the workflow — a good defensive fix ensuring runner independence. No new issues. Previous suggestions remain optional improvements for future iterations.

---

Update (f1fa016): Good improvements in this commit:

• Extended pip install to include pytest-mock and flaky with clear explanation
• Added is_ok() helper treating pytest exit code 5 (no tests collected) as success — correct for device-gated tests
• Extended SKIP list with 4 newton tests and improved documentation

No new issues. Previous optional suggestions (unit tests for helper, robustness improvements) remain relevant but non-blocking.

---

Update (042c409): This commit expands the skip list with well-documented categories explaining why each test is skipped (FabricFrameView deadlocks, SIGSEGV failures, cuda:1 hangs, etc.). Good practice to document the reasoning. Also adds test_simulation_context.py and physx rigid-object tests to the skip list. No new issues — this is maintenance housekeeping for multi-GPU stability.

---

Update (1546520): 🎉 Excellent fix! This commit addresses the root cause of the cuda:1 test failures:

• app_launcher.py: Added ISAACLAB_SIM_DEVICE env var support to override the default device when not explicitly passed. Clean implementation with good documentation.
• Workflow: Sets ISAACLAB_SIM_DEVICE=cuda:1 to boot Kit with active_gpu=1 from process start. This correctly targets PhysX/Warp to cuda:1 before SimulationApp locks it.
• Result: 7 tests removed from the skip list and now run successfully on cuda:1!

This is a proper fix rather than just skipping problematic tests. No new issues introduced.

---

Update (feb8e21): Skip list maintenance — expanded documentation with categorized skip reasons:

1. Upstream Kit limits (FabricFrameView)
2. Pre-existing test/API breakage (newton contact_sensor)
3. Newton cuda:1 — Warp allocator failure in mujoco_warp collision driver
4. PhysX cuda:1 — hangs specific to AWS L40 runner (passes on other hardware)

Added 7 files back to skip list (4 Newton, 3 PhysX asset tests) with clear root-cause explanations. Good documentation practice for future debugging. No new issues.

---

Update (6eeda64): Added changelog entry (jichuanh-multi-gpu-ci.minor.rst) documenting the new cuda_test_devices() helper and ISAACLAB_SIM_DEVICE env var. Good housekeeping. No new issues.

---

Update (e3d328b): Major infrastructure improvement — switched from bare-runner installation to Docker-based execution:

• Added config job that loads isaacsim image config from config.yaml via sparse checkout
• Added ecr-build-push-pull action to pull the same per-commit CI image used by regular CI (dep matrices now match exactly)
• Tests run inside container with workspace volume-mounted, removing need for pip install steps on runner
• Cleaned up SKIP list comments (removed obsolete contact_sensor reference)
• Proper symlink setup (rm -f _isaac_sim && ln -s /isaac-sim _isaac_sim) inside container

This is a solid approach — reusing the regular CI's Docker image ensures environment consistency and removes runner-specific pip quirks. Good fallback documentation if ECR cache misses. No new issues.

---

Update (3c5248d): Good documentation and consistency improvements:

• Added clear comments explaining --entrypoint bash (overrides isaac-sim base image's default ENTRYPOINT that would swallow the script)
• Added explanation for --ignore=tools/conftest.py (matches regular CI's behavior)
• Consolidated ignore paths into ignore_args variable and applied to both pure-python and per-file pytest runs
• Cleaner shell structure: --entrypoint bash + -c instead of inline bash -c

No new issues. Ready for merge. 🚀

---

Update (9ff909d): Fixes local image availability issue — when ecr-build-push-pull takes the deps-cache-hit path, it creates an ECR registry-side alias but leaves no image in local Docker. This commit adds an "Ensure image is local" step that:

• Creates isolated DOCKER_CONFIG with credsStore disabled (workaround for broken runner credential helper)
• Re-authenticates to ECR via EC2 IAM role
• Explicitly pulls and tags the image for subsequent container steps

Good defensive fix with clear comments explaining the edge case. No new issues.

---

Update (6706690): Good fixes for container permission issues:

• Added --user "$(id -u):$(id -g)" to run container as host uid:gid — fixes write permission failures on volume-mounted workspace
• Created writable $HOME (/tmp/isaaclab-ci-home) with XDG_CACHE_HOME for Warp/numpy/pip caches
• Well-commented: explains the issue (image's default isaaclab uid 1000 ≠ runner's github-runner user)

Proper solution for the permission mismatch. No new issues.

---

Update (328878a): Skip list expanded with ovphysx tests. Good documentation explaining the issue: module-level pytest.mark.skipif collects 0 items + 1 skip, which isaaclab.sh CLI wrapper interprets as exit 1 (doesn't distinguish from real failures). Added 3 ovphysx asset tests to skip list. Clean maintenance commit. No new issues.

---

Update (665f0c3): Added temporary run_docker_tests: 'false' override in build.yaml to free GPU runners during PR iteration. The TEMP comment is well-documented ("revert before final review"). ⚠️ Remember to revert this before merge — the comment is a good reminder. No other changes, no new issues.

---

Update (f66b101): 🎉 Excellent refactor! This commit makes major architectural improvements:

Changes:

• New extra-env-vars input added to both run-tests and run-package-tests actions — clean mechanism to forward env vars into the container with proper parsing (handles comments, whitespace, KEY=value format)
• Workflow refactored to use run-package-tests action instead of ~150 lines of inline bash. Much cleaner and more maintainable.
• Separate build job added to pre-populate the ECR image before test job runs (mirrors the build→test split in build.yaml)
• Include-list approach replaces the SKIP list pattern — explicitly listing 9 known-working tests is safer than auto-discovery with exclusions
• Path triggers expanded to include action files, so workflow changes properly trigger CI

My previous Finding #2 (SKIP list robustness) is now obsolete — the new include-files approach is cleaner and avoids the heredoc pattern entirely. ✅

No new issues. This is a significant quality improvement that makes the multi-GPU workflow consistent with the rest of the CI infrastructure.

⚠️ Before merge: Still need to revert the run_docker_tests: 'false' in build.yaml (from 665f0c3).

---

Update (3c985eb): Minor update for FabricFrameView retest iteration:

Changes in build.yaml:

• Updated TEMP comment: now focused on "FabricFrameView cuda:1 retest" (previously was "Multi-GPU pytest docker conversion")
• Streamlined comment (removed redundant "freeing the single-GPU runner pool" explanation)
• run_docker_tests: 'false' remains set — still iterating

Changes in test-multi-gpu-pytest.yaml:

• Added test_views_xform_prim_fabric.py to the include-files list — attempting to enable the FabricFrameView test on cuda:1

This is a WIP commit testing whether the FabricFrameView deadlock issue (documented in earlier skip lists) has been resolved. ⚠️ run_docker_tests: 'false' still needs to be reverted before merge.

No new architectural issues. Just experimental test expansion.

---

Update (bdcb527): ✅ Final Cleanup Complete

The latest commit removes the temporary run_docker_tests: 'false' override that was used during iteration, restoring the proper change-detection logic:

-      run_docker_tests: 'false'
+      run_docker_tests: ${{ steps.detect.outputs.run_docker_tests }}

This was the expected final step — the temporary bypass was necessary to test the FabricFrameView cuda:1 retest path, and it's now correctly reverted.

Status: All outstanding items addressed. PR is ready for merge. 🚀

[isaaclab-review-bot]

Update (5d29bb0): ✅ Ready for Merge

The final commit reverts the temporary `run_docker_tests: 'false' override — the exact action my previous review flagged as blocking. All outstanding items are now addressed:

✅ Missing __init__.py — Added in earlier commit
✅ **run_docker_tests: 'false' reverted** — This commit ✅ **Workflow refactored** to use run-package-tests` action (f66b101)
✅ Include-list approach replaces SKIP heredoc pattern
✅ Changelog fragment added

---

Summary

This PR is now complete and ready for merge. The multi-GPU testing infrastructure is solid:

• Clean cuda_test_devices() helper with elegant mask grammar
• ISAACLAB_SIM_DEVICE env var for AppLauncher device override
• Proper integration with existing CI via run-package-tests action
• 9 test files migrated with known-working cuda:1 paths

No new issues introduced. Nice work! 🚀

---

Update (d6d69c4): ⚠️ Iteration Checkpoint — Not Merge-Ready

This commit re-introduces the temporary run_docker_tests: 'false' override in .github/workflows/build.yaml`. The PR author is iterating and has added a note:

"TEMP (revert before final review / before landing): force run_docker_tests=false while iterating PR #5823"

Status: The PR was previously ready for merge, but is now in an iteration state. This temporary flag must be reverted before merging — the author has clearly marked it as such.

📋 Blocking for merge:

• Revert `run_docker_tests: 'false' override (same fix as commit 5d29bb0)

No functional regressions otherwise — this is expected iteration behavior.

---

Update (ea919b7): Module Relocation

This commit relocates the testing utilities from isaaclab.testing to isaaclab.test.utils:

Changes:

• Renamed source/isaaclab/isaaclab/testing/ → source/isaaclab/isaaclab/test/utils/
• Updated imports across 16 test files in isaaclab, isaaclab_newton, isaaclab_ovphysx, and isaaclab_physx packages
• Workflow path trigger updated from source/isaaclab/isaaclab/testing/** → source/isaaclab/isaaclab/test/utils/**
• Changelog fragment updated to reflect new import path

✅ This is a clean refactor that consolidates test utilities under the isaaclab.test namespace. All import statements are consistently updated.

📋 Blocking for merge:

• Revert `run_docker_tests: 'false' override (status unchanged from previous update)

Otherwise, no new issues introduced.

---

Update (0118ea7): 🎯 Nice Refinements

This commit adds several quality-of-life improvements:

Changes:

1. run-tests/action.yml — Smarter env var parsing

• Now preserves mid-line # characters (e.g., IMAGE_TAG=v1.0#nightly survives)
• Only treats full-line comments (where first non-whitespace is #) as comments
• Strips YAML block indent whitespace properly

2. test-multi-gpu-pytest.yaml — Auto-discovery of opt-in tests

• Replaces hardcoded include-list with grep -rl 'cuda_test_devices' discovery
• Adding a new test to multi-GPU CI now requires no workflow edit — just use the helper
• Clean design: opt-in via code, not via workflow maintenance

3. cuda_test_devices() helper — New skip parameter

• Added skip: dict[str, str] for per-device skips with reasons
• Wraps skipped devices in pytest.param(..., marks=pytest.mark.skip(reason=...))
• CI shows SKIPPED with reason instead of silent omission
• Changed strict default from True to False (friendlier for CPU-only dev machines)

4. test_views_xform_prim_fabric.py — Migrated to helper

• Replaced @pytest.mark.parametrize("device", ["cpu", "cuda:0"]) with cuda_test_devices()
• Removed manual ISAACLAB_TEST_MULTI_GPU skipif decorators
• Uses mask="001", strict=False for cuda:1-only tests

✅ All changes are clean refinements that improve maintainability and developer experience.

📋 Blocking for merge:

• Revert run_docker_tests: 'false' override (status unchanged)

Nice incremental polish! 🔧

---

Update (101a47a): 🔧 Newton Device-Pinning & Test Tolerance Fixes

This commit addresses cuda:1 context issues in Newton manager and fixes device-specific test tolerances:

Changes:

1. newton_manager.py — Device context pinning (issue #5132)
Added explicit torch.cuda.set_device() + wp.set_device() calls in two critical locations:

• start_simulation() — Before Warp/Newton allocations to ensure mujoco_warp collision pipeline allocates on the correct CUDA device
• initialize_solver() — Mirrors the guard from start_simulation() for solver build and collision-pipeline init

This fixes null pointer returns when mujoco_warp allocates against a primary CUDA context that was never made current. The guard is idempotent if already pinned.

2. wp.ScopedCapture(device=device) — Explicit device for CUDA graph capture
Previously, CUDA graph capture didn't specify a device; now explicitly passes device to avoid capturing on wrong GPU.

3. Test tolerance fixes (test_rigid_object.py in both newton & physx packages)
Changed if device == "cuda:0" to if device.startswith("cuda") for tolerance selection. This ensures cuda:1 (and any other CUDA device) uses the relaxed GPU tolerance (1e-2) instead of the strict CPU tolerance (1e-5).

✅ Solid fixes:

• Device-pinning guards reference the related issue (#5132)
• Comments explain why the guards are needed (mujoco_warp context behavior)
• Test tolerance fix is the correct pattern for any CUDA device
• CUDA graph capture now device-aware

📋 Blocking for merge:

• Revert run_docker_tests: 'false' override (status unchanged)

Otherwise, these are important correctness fixes for multi-GPU setups. 👍

[greptile-apps]

Greptile Summary

This PR introduces a complete multi-GPU pytest CI lane: a test_devices() helper resolving device parametrization as scope ∩ runtime, an ISAACLAB_SIM_DEVICE env var for boot-device selection without per-test call-site edits, a Newton physics fix pinning torch + Warp to the target device before allocation/graph capture, and a new test-multi-gpu-pytest.yaml workflow running N parallel pytest subshells (one per non-default GPU) inside a single container with a shared atomic work queue.

• test_devices() / mask grammar: Cleanly separates test-author concern (scope) from operator concern (runtime); _expand handles the trailing-X wildcard; _list_available_devices deliberately uses torch.cuda (not warp) at collection time to avoid Warp init-order fragility before Kit boots.
• 1-docker N-shard work queue: POSIX-atomic os.rename claim pattern in conftest.py, reconciled by the host launcher's orphan/unclaimed detector; per-slug JUnit report paths prevent cross-shard collisions on same-basename files.
• Newton + build_simulation_context fixes: torch.cuda.set_device / wp.set_device before solver init + wp.ScopedCapture(device=device) resolve null-pointer faults on non-default CUDA devices; build_simulation_context now correctly honors an explicit device kwarg even when sim_cfg is also provided.

Confidence Score: 5/5

Safe to merge; all findings are edge-case concerns that do not affect the normal single-GPU or multi-GPU test paths.

The core logic — mask intersection, atomic work-queue claim, shard fan-out, Newton device pin, and build_simulation_context device-kwarg fix — is well-designed and backed by dedicated unit tests. The three flagged concerns (XR + ISAACLAB_SIM_DEVICE stale dict key, single-quoted discovery gap, theoretical slug collision) all require uncommon conditions to trigger and none affect the intended multi-GPU CI path or the single-GPU invariant.

source/isaaclab/isaaclab/app/app_launcher.py (XR + env-device key sync) and .github/workflows/test-multi-gpu-pytest.yaml (discovery grep quote handling) are the two spots that could benefit from a quick follow-up, though neither blocks merging.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/test/utils/devices.py	New test_devices() utility implementing scope ∩ runtime mask intersection; logic is clean, well-tested, and correctly defers CUDA enumeration to torch to avoid Warp init-order issues at collection time.
tools/conftest.py	Adds directory-based work-queue integration (claim/inflight/done via atomic POSIX rename), per-slug JUnit report naming, and mgpu_shard_select plugin injection; slug collision is a latent theoretical issue for paths containing double-underscore components.
.github/actions/multi-gpu/multi_gpu_host_launcher.sh	Host-side orchestrator: seeds the work queue, detects MIG vs discrete GPU topology, launches one container for all N shards, re-prints per-shard logs, and reconciles orphaned/unclaimed entries as a silent-drop guard.
.github/actions/multi-gpu/multi_gpu_shard_runner.sh	Container-side shard fan-out: derives shard count from nvidia-smi, cross-checks with torch, fans out one pytest subshell per non-default GPU with per-shard HOME isolation, aggregates exit codes without fast-fail.
.github/workflows/test-multi-gpu-pytest.yaml	New workflow with config→build→test shape; auto-discovery grep only matches double-quoted test_devices(...) masks, silently missing single-quoted variants. Build correctly pins to the multi-GPU pool so the ECR cache is populated on the right pool.
source/isaaclab/isaaclab/app/app_launcher.py	Adds ISAACLAB_SIM_DEVICE implicit-default and ISAACLAB_PIN_KIT_GPU multi-GPU renderer isolation; minor inconsistency when both ISAACLAB_SIM_DEVICE and XR mode are active leaves launcher_args["device"] stale after the XR override.
source/isaaclab/isaaclab/sim/simulation_context.py	Changes build_simulation_context device parameter default from "cuda:0" to None; when device is passed explicitly it now overrides sim_cfg.device in both branches, fixing a latent kwarg-shadowing bug.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	Pins torch + Warp to the target device before Newton model finalization and solver initialization, and passes device=device to wp.ScopedCapture; correctly fixes the null-pointer collision-pipeline fault on non-default CUDA devices.
.github/actions/multi-gpu/mgpu_shard_select.py	pytest plugin that deselects non-device-parametrized tests and out-of-scope device variants on multi-GPU shards; correctly handles NO_TESTS_COLLECTED → OK exit code remapping.
.github/actions/multi-gpu/aggregate_test_summary.py	Post-run aggregator reading JUnit XMLs and the done// work distribution; produces per-file and per-shard tables to stdout and GitHub Step Summary markdown.

Sequence Diagram

sequenceDiagram
    participant WF as workflow yaml
    participant HL as host_launcher.sh
    participant DC as Docker container
    participant SR as shard_runner.sh
    participant CF as conftest.py
    participant Q as Work Queue

    WF->>WF: "Discover opt-in test files"
    WF->>HL: "bash multi_gpu_host_launcher.sh"
    HL->>Q: "Seed queue/ with file slugs"
    HL->>DC: "docker run --gpus all"
    DC->>SR: "multi_gpu_shard_runner.sh"
    SR->>SR: "nvidia-smi DEV_COUNT + torch cross-check"
    loop "For each cuda:N (N=1..DEV_COUNT-1)"
        SR->>CF: "pytest subprocess (ISAACLAB_SIM_DEVICE + ISAACLAB_TEST_DEVICES)"
        CF->>Q: "claim: os.rename queue to inflight"
        CF->>CF: "Run per-file pytest with mgpu_shard_select plugin"
        CF->>Q: "done: os.rename inflight to done"
    end
    SR-->>DC: "Aggregate exit codes"
    DC-->>HL: "docker exit code"
    HL->>HL: "Reconciler: assert queue + inflight empty"
    HL-->>WF: "exit (docker_rc or 2 on orphan)"
    WF->>WF: "aggregate_test_summary.py to Step Summary"

Loading

_{Reviews (2): Last reviewed commit: "Merge remote-tracking branch 'origin/dev..." | Re-trigger Greptile}

[greptile-apps]

strict=True default raises at collection time on hosts without cuda:0

With the default mask "110" and strict=True, calling cuda_test_devices() on a host that reports only ["cpu"] (no CUDA) immediately raises ValueError during pytest collection — before any test runs. Before this change, the same tests (parametrized with ["cuda:0", "cpu"]) would collect normally; only the cuda:0 variant would fail at runtime. Tests like test_episode_data.py and test_math.py have no AppLauncher dependency, so they are plausible targets for collection on CPU-only dev machines. Passing strict=False to the default call-sites, or documenting the constraint, would preserve the old graceful-degrade behaviour for contributors without full GPU setups.

[greptile-apps]

Comment-stripping and xargs trimming silently corrupt values containing # or single quotes

line="${line%%#*}" strips everything from the first # in a line, so a future caller passing a value like IMAGE_TAG=v1.0#nightly would silently become IMAGE_TAG=v1.0. Similarly, $(echo "$line" | xargs) collapses internal whitespace and fails on values containing unbalanced single quotes. For the two values used today (001 and cuda:1) this is harmless, but it is a latent trap for any future extra-env-vars user. Documenting the # restriction in the input description, or switching to a safer stripping approach, would prevent silent data loss.

[isaaclab-review-bot]

Update (437a6d2): 🔧 Build Job Pool Alignment

New commit reviewed (9d159f3 → 437a6d2):

ECR Cache Locality Fix

# Before:
runs-on: [self-hosted, gpu]

# After:
runs-on: [self-hosted, linux, x64, multi-gpu]

Problem solved: The ECR cache repo is resolved per runner pool:

• gpu pool → gitci-docker-cache
• multi-gpu pool → multigpu-docker-cache

Building on [self-hosted, gpu] pushed the image to gitci-docker-cache, which the multi-GPU test job cannot see, causing a full rebuild (~27 min) on the scarce multi-GPU runner.

Fix: Align the build job to the same pool (multi-gpu) as the test job so both hit the same cache. This is the correct approach — build and test should share the same ECR namespace.

✅ LGTM for this commit. Clean infrastructure fix with excellent inline documentation explaining the cache topology.

---

📋 Status Summary:

• Previous reviews covered: runner label fix (gpu removal), script extraction, work-queue architecture
• No new concerns in this commit

📜 Previous review history

Update (9d159f3): Script Extraction & Runner Targeting Fix

• Removed conflicting gpu label (was routing to 1-GPU boxes)
• Extracted ~300 lines of inline shell/Python to multi_gpu_host_launcher.sh and aggregate_test_summary.py

Earlier: Container isolation, test_devices() API, Newton device-pinning, lazy pxr imports

[pbarejko]

Why are we using torch api here? This smells like tech debt. Shouldn't we switch to use warp? i.e.:

>>> import warp
>>> warp.init()
Warp 1.12.0 initialized:
   CUDA Toolkit 12.9, Driver 13.0
   Devices:
     "cpu"      : "x86_64"
     "cuda:0"   : "NVIDIA RTX PRO 6000 Blackwell Workstation Edition" (95 GiB, sm_120, mempool enabled)
   Kernel cache:
     /home/pbarejko/.cache/warp/1.12.0
>>> warp.get_cuda_devices()
['cuda:0']

[hujc7]

Checked. using warp requires initializing runtime which might have side-effect to kit. torch does not have that.

[kellyguo11]

we would always have warp loaded for our APIs, right? what are the side effects that impact kit?

[hujc7]

Correction: it's the warp behavior on non-default devices such that torch.cuda.set_device must be called first before wp.init. The same issue was discovered by Octi before.

[pbarejko]

Based on function name cuda_test_devices have we started to exclude cpu device tests. If so - why?

[hujc7]

renamed to test_devices

[pbarejko]

Name of this global variable makes no sense, besides it's used in one place only. This unnecessary indirection makes code harder to read.

[hujc7]

Renamed.

[pbarejko]

I don't understand the premise behind strictness in this function. I read it as maybe run on GPU 1, if unavailable then run on what's available. This introduces testing ambiguity, Just by looking at this line I don't know what you are trying to test.

In my opinion we would be better without cuda_test_devices function and keep list explicit, instead of resolved at runtime. In testing you want to keep constraints explicit.

[hujc7]

Strict is removed.
The goal of this is to dynamically configure the parameterization at test time so we run it where suitable. On regular test, cpu and cuda:0; and cuda:X for multi-gpu. The overall goal is to mark all tests needing device parameterization with this.

[isaaclab-review-bot]

Update (6a0e473): 🐳 Major Refactor — 1-Docker N-Shard Architecture + External Runner Script

(Incremental follow-up to previous review — 45+ commits since a1fa279)

---

Key Architectural Change: 1-Docker, N-Shard

Before: Each GPU shard ran in its own isolated Docker container
After: One container hosts all pytest shards as parallel subshells

┌─────────────────────────────────────────────────────────┐
│  Single Docker Container (--gpus all)                    │
│  ┌──────────────┐ ┌──────────────┐ ┌──────────────┐     │
│  │ Shard cuda:1 │ │ Shard cuda:2 │ │ Shard cuda:3 │     │
│  │   (subshell) │ │   (subshell) │ │   (subshell) │     │
│  └──────────────┘ └──────────────┘ └──────────────┘     │
│       ↓                ↓                ↓                │
│       └────────────────┴────────────────┘                │
│                        │                                 │
│             Shared PYTHONUSERBASE                        │
│         (junitparser, pytest, etc.)                      │
└─────────────────────────────────────────────────────────┘

Benefits:

• Eliminates cross-container races on shared workspace mount
• ~30s saved per shard (Docker init overhead removed)
• Shared pip install (single junitparser etc. install vs N)
• Simpler debugging (all shards in one container)

---

New: tools/multi_gpu_shard_runner.sh

The inside-container logic is now extracted to a standalone script:

Responsibilities:

1. Sets up HOME / PYTHONUSERBASE on tmpfs
2. Installs pytest deps into shared site-packages
3. Auto-discovers GPU count via nvidia-smi -L
4. Cross-checks against torch.cuda.device_count()
5. Fans out 1 pytest subshell per non-default GPU
6. Per-shard isolation: separate HOME, ISAACLAB_TEST_DEVICES
7. Wait-all-then-aggregate exit codes

✅ Good extraction — versioned, shellcheck-able, reusable for local probing

---

Other Notable Changes

1. Kit GPU Pinning (ISAACLAB_PIN_KIT_GPU=1)

• Pins Kit renderer to single GPU when set
• Prevents multi-GPU race conditions in Kit's resource allocation
• Added to app_launcher.py and enabled in workflow

2. Directory-based Work Queue

• Uses atomic os.rename() for claim contention
• Reconciler step catches silent-drops

3. MIG Detection

• Handles both discrete GPUs and MIG slices
• nvidia-smi -L is authoritative (torch under-counts MIG)

4. DO-NOT-MERGE Gate

• Docs/install-ci workflows skip when PR title contains DO-NOT-MERGE

---

Remaining Blockers

📋 Before merge:

• Revert run_docker_tests: 'false' override in build.yaml — still hardcoded

The PR title contains DO-NOT-MERGE suggesting this is intentional for iteration, but the actual production-readiness blocker is that line in build.yaml.

---

🏁 Verdict: Looking good for the CI scope. The 1-docker architecture is a solid improvement. Just need to revert the run_docker_tests override before merge.

[isaaclab-review-bot]

Update (e6689f0): 📊 Aggregated Test Summary & Filtered Live Stdout

(Incremental follow-up to previous review)

---

1. New Aggregated Test Summary Step

A new always()-conditional step parses JUnit XMLs and outputs:

• Per-file results table: file path, shard, status, pass/total, skipped, failed, walltime
• Per-shard aggregate: files, pass/total, skipped, failed, test time
• Grand total row

Dual output:

• Plain text to stdout (for workflow log readability)
• Markdown to $GITHUB_STEP_SUMMARY (renders at top of run page)

✅ Excellent addition:

• Provides at-a-glance CI results without scrolling through logs
• Per-shard breakdown helps diagnose load imbalance
• Status emojis in markdown (✅ PASS, ❌ FAIL, ⚪ EMPTY) improve scannability
• Clean separation of stdout (tabular text) vs Summary (markdown tables)

---

2. Filtered Live Stdout

The pytest output pipeline now includes a grep -aE filter that retains only high-signal lines:

Retained:

• 🚀 emoji markers (test boundaries)
• Test result lines (PASSED, FAILED, ERROR, SKIPPED, XFAIL, XPASS)
• Summary stats (Total:, Passing:, Failing:, etc.)
• Error markers (^E , Traceback, ^FAILED)
• File references (^ +File )
• Pytest section dividers (^=+, ^~~~~)

Filtered out:

• Kit init chatter, plugin registration
• omni.usd Transfer logs
• Verbose pytest collection output

✅ Good improvement:

• Reduces live log noise while preserving actionable info
• Full output still available in $shard_log (printed in grouped sections at job end)
• stdbuf -oL ensures line-buffered output (no partial line issues)
• -a flag on grep handles binary (e.g., emoji bytes) correctly

---

3. Timeout Raised to 60 Minutes

timeout-minutes: 60  # was 45

The diagnostic comment was removed, and timeout raised from 45 → 60 minutes.

ℹ️ Context: The previous 45-min timeout was flagged as diagnostic; 60 min gives more headroom for the parallel shards. Once load-balancing data is collected, this can likely be tightened back down.

---

4. Debug Comments (Review Notes)

Several #rj: comments were added throughout the workflow:

• #rj: need to test using gpu runner. see if ecr is connected.
• #rj: this is useless here. should be just before running tests
• #rj: what happens if test file only has a few test_devices()...
• #rj: is this needed for all docker? or what's the problem here?
• #rj: does test fail if either queue or inflight is not empty?
• #rj: this probably should be in action libs?

⚠️ These should be removed before merge — they're inline review notes/questions, not permanent documentation. I assume they're there for iteration/discussion and will be cleaned up.

---

5. Remaining Blockers

📋 Before merge:

• Revert run_docker_tests: 'false' override in build.yaml — still pending
• Remove #rj: debug comments — review notes, not production docs

---

🏁 Verdict: Great additions for CI observability. The aggregated summary and filtered stdout significantly improve the developer experience for debugging multi-GPU test failures. Just need to clean up the debug comments and revert the docker test override.

---

Update (c514199): No changes to the aggregated test summary or filtered stdout logic in this increment. The new work-queue distribution in conftest.py complements these features well — JUnit XML paths now use full-path slugs to avoid collisions between shards. See primary review thread for full analysis.

[kellyguo11]

we would always have warp loaded for our APIs, right? what are the side effects that impact kit?

[kellyguo11]

Could we avoid reading ISAACLAB_SIM_DEVICE directly inside _resolve_device_settings? The behavior makes sense for the multi-GPU workflow, but this makes AppLauncher feel like it is accumulating unrelated CI env vars. I think we are already doing too much of that inside the AppLauncher code, I'd really like to avoid all the CI-specific branching if we can.

[kellyguo11]

why do we need to reference CI env variables in the core code?

[hujc7]

This is for configuration the device at CI level. e.g. for multi-gpu instance, it can dispatch the test to one of the non-default gpus.

[kellyguo11]

will this conflict with pytests's test_* search for test cases? could this become more of a test-only module that's not exposed as part of the isaaclab core APIs?

[kellyguo11]

is this guaranteed to be imported after launching kit?

[hujc7]

it does not. Was it for the kit crash issue due to openblast? The issue should be resolved. I can move this to function local import.

[kellyguo11]

I think the mask idea is clever, but it's a burden on all test developers that I feel might not be very intuitive. If we expect users from the community to implement tests, they'll need to understand the masks and make sure they specify the correct values if their tests cannot support some of the options. perhaps we can have some more user friendly naming for CPU, GPU, and default cuda device options? maybe something like

test_devices(DeviceScope.ALL)
test_devices(DeviceScope.CPU_AND_DEFAULT_CUDA)
test_devices(DeviceScope.NON_DEFAULT_CUDA)

[hujc7]

Yes, I will add enum name support

dismiss bot