[CI] Cross-platform — Part 1: pytest markers and shared scaffolding (base)#5695
[CI] Cross-platform — Part 1: pytest markers and shared scaffolding (base)#5695hujc7 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Update (2026-05-20): Reviewed new commits (b5e57a6 → 9242498).
Changes Since Last Review
The PR scope has been refined - workflow files (cross-platform-ci.yaml) have been removed. This PR now focuses purely on foundational infrastructure:
- ✅ Pytest marker registration (
windows,windows_ci,arm,arm_ci) - ✅ AppLauncher argv stripping for new CI markers
- ✅ Cross-platform temp directory (
tempfile.gettempdir()) - ✅ Test files annotated with new markers
Previous Concerns Status
| Issue | Status |
|---|---|
continue-on-error double-masking |
🔵 N/A - workflow files removed from PR |
| ARM PyTorch cu128 wheel compatibility | 🔵 N/A - workflow files removed from PR |
| Stale virtualenv risk | 🔵 N/A - workflow files removed from PR |
| Missing base markers in tests | ⚪ Minor - still only _ci variants used |
✅ Current State: Looks Good
This is now a clean foundation PR. The workflow files will presumably ship in follow-up PRs (as mentioned in the changelog skip message). No blocking concerns.
This is an automated review.
Update (commit 1e32138): The new commits simplify the marker system by removing windows and arm base markers from pyproject.toml (keeping only windows_ci and arm_ci). Additionally, module-level pytestmark annotations were removed from test_scipy.py and test_torch.py.
Previous concerns:
- ⚪ "Missing base markers in tests" → ✅ Resolved by design — base markers eliminated entirely.
New issues: None. This is a clean simplification that reduces marker surface area. No regressions introduced.
Foundation for cross-platform CI. Registers four pytest markers (windows, windows_ci, arm, arm_ci), teaches AppLauncher to recognize them in argv so they do not leak into Isaac Sim's argparse, and moves the AssetConverterBase USD scratch directory from a hardcoded /tmp/IsaacLab to tempfile.gettempdir() for cross-platform compatibility. Tags source/isaaclab/test/deps/test_torch.py and test_scipy.py with the new markers so they are selectable by future cross-platform jobs. Workflow files (arm-ci.yaml, windows-ci.yaml) ship in follow-up PRs.
b5e57a6 to
9242498
Compare
Greptile SummaryThis PR lays the scaffolding for cross-platform CI on Windows and ARM/Spark by registering four new pytest markers, teaching
Confidence Score: 3/5Safe to merge with awareness of the CUDA dependency in test_torch.py; the core scaffolding changes are correct but the torch test tagging could silently break future CI runs on GPU-less runners. The tempfile fix and marker registrations are straightforward and correct. However, all six torch tests now tagged for windows_ci and arm_ci unconditionally allocate tensors on cuda:0. If either CI runner ever lacks a CUDA device the entire test_torch suite will hard-fail before a single assertion is checked, which defeats the purpose of marking them as cross-platform runnable. source/isaaclab/test/deps/test_torch.py — all tests hardcode cuda:0 despite being tagged for cross-platform CI runners. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["pytest -m windows_ci / arm_ci"] --> B["sys.argv contains -m marker"]
B --> C["AppLauncher._create_app()"]
C --> D{"marker in strip list?"}
D -- "windows_ci / arm_ci stripped" --> E["Remove -m + value from sys.argv"]
D -- "windows / arm NOT stripped" --> F["Leaks into Isaac Sim argparse"]
E --> G["SimulationApp initialised cleanly"]
H["pytest collects test_torch.py"] --> I{"CUDA available?"}
I -- "cuda:0 present" --> J["Tests pass"]
I -- "No CUDA" --> K["RuntimeError: No CUDA GPUs"]
L["asset_converter_base.py"] --> M["tempfile.gettempdir()"]
M --> N["Linux: /tmp/IsaacLab/usd_..."]
M --> O["Windows: %TEMP%/IsaacLab/usd_..."]
Reviews (1): Last reviewed commit: "[CI] Add Windows + ARM pytest markers an..." | Re-trigger Greptile |
| import torch | ||
| import torch.utils.benchmark as benchmark | ||
|
|
||
| pytestmark = [pytest.mark.windows_ci, pytest.mark.arm_ci] |
There was a problem hiding this comment.
CUDA-only tests tagged for cross-platform CI
Every test function in this file uses device="cuda:0" unconditionally (e.g. torch.rand(size, device="cuda:0")). Tagging the whole module with windows_ci and arm_ci means these tests will be collected and run on those platforms, but they will immediately fail with RuntimeError: No CUDA GPUs are available on any runner that does not expose cuda:0. If both Windows and ARM Spark runners are guaranteed to have an NVIDIA GPU, consider adding a pytest.mark.skipif(not torch.cuda.is_available(), reason="CUDA not available") guard so the suite degrades gracefully rather than hard-failing on platforms where CUDA is absent.
| if ( | ||
| "pytest" in value_for_dash_m | ||
| or "isaacsim_ci" in value_for_dash_m | ||
| or "windows_ci" in value_for_dash_m | ||
| or "arm_ci" in value_for_dash_m | ||
| ): |
There was a problem hiding this comment.
The bare
windows and arm markers are registered in pyproject.toml but are not included in the argv-stripping filter. If a developer runs pytest -m windows (or -m arm) on a test that also initialises AppLauncher, those tokens will leak into Isaac Sim's argparse — the same failure mode this block was originally written to prevent.
| if ( | |
| "pytest" in value_for_dash_m | |
| or "isaacsim_ci" in value_for_dash_m | |
| or "windows_ci" in value_for_dash_m | |
| or "arm_ci" in value_for_dash_m | |
| ): | |
| if ( | |
| "pytest" in value_for_dash_m | |
| or "isaacsim_ci" in value_for_dash_m | |
| or "windows_ci" in value_for_dash_m | |
| or "windows" in value_for_dash_m | |
| or "arm_ci" in value_for_dash_m | |
| or "arm" in value_for_dash_m | |
| ): |
| if ( | ||
| "pytest" in value_for_dash_m | ||
| or "isaacsim_ci" in value_for_dash_m | ||
| or "windows_ci" in value_for_dash_m |
There was a problem hiding this comment.
are these new annotators necessary for this check? I think this was initially added for the isaac sim pipeline to work properly by making sure the correct arguments gets passed over. will that also be impacted by the windows and arm annotations?
There was a problem hiding this comment.
It's for pytest selecting tests, which uses the same mechanism as isaacsim_ci tag. While this is not quite scalable at the moment, some investigation into argparse refactor should help. e.g. to only pass appLauncher recognized arguments to it.
There was a problem hiding this comment.
Are we not running the the whole batch of tests on windows? Would it make sense to make a specific test shard just for windows? (that would remove the annotation)
|
|
||
| markers = [ | ||
| "isaacsim_ci: mark test to run in isaacsim ci", | ||
| "windows: mark test as runnable on Windows platforms", |
There was a problem hiding this comment.
would this be a lot of maintenance to tag every test that would work on windows? I think most of our workflows should support windows already with a few exceptions. perhaps it's easier to mark the ones that are not supported instead?
There was a problem hiding this comment.
As a start, I think whitelist should be better. Windows pool size is also just 1/4 compare to linux ones. I can test on the overall duration + test support and see if a negative tag works better, but I think that will be in followup PRs given that windows still have driver + isaacsim develop access issue not resolved.
The PR serves as the common setup for followups so they can all go independently.
There was a problem hiding this comment.
I see, then is there a need to keep both windows and windows_ci tags? I was thinking if we have both, windows could be a blacklist that determines which tests are not supported on windows. and windows_ci could be a whitelist that enables a small set of tests to run
There was a problem hiding this comment.
"windows" is dropped, and only "windows_ci" is added in the latest patch. Personally, I would whitelist should be sufficient and it's more verbose at per test/file level that where the test runs.
The bare `windows` and `arm` markers were registered alongside the live `windows_ci`/`arm_ci` selectors, but no test in this PR or in the follow-up cross-platform parts uses them. The `_ci` variants are the ones consumed by conftest's CI_MARKER orchestration and by the windows-ci workflow's `pytest -m windows_ci` invocation. Registering markers without a consumer just enlarges the surface a reviewer has to evaluate. Drop the bare entries; revisit when there's an actual use case (e.g. an exhaustive platform-compat sweep that's separate from the curated `_ci` canaries).
PR 5695 lays foundation only. The two tagged tests (test_scipy.py, test_torch.py) had pytestmark applied here but no in-PR consumer — the arm-ci and windows-ci workflows that select on these markers ship in isaac-sim#5698 and isaac-sim#5700 respectively. Each downstream PR should land the marker annotations on the tests it actually wants in its canary, alongside the workflow that runs them. This keeps 5695 self-contained as scaffolding and prevents unused annotations from accumulating here.
AntoineRichard
left a comment
There was a problem hiding this comment.
I'm not sure what's best between annotated tests and a specific test shard for windows / arm.
| if ( | ||
| "pytest" in value_for_dash_m | ||
| or "isaacsim_ci" in value_for_dash_m | ||
| or "windows_ci" in value_for_dash_m |
There was a problem hiding this comment.
Are we not running the the whole batch of tests on windows? Would it make sense to make a specific test shard just for windows? (that would remove the annotation)
| "windows_ci: mark test to run on Windows platforms in CI", | ||
| "arm_ci: mark test to run on ARM platforms in CI (e.g. NVIDIA DGX Spark)", |
There was a problem hiding this comment.
Is this better than a specific shard?
There was a problem hiding this comment.
Not exactly sure what a shard means here. Does it mean selecting the tests at workflow level? If so, I am trying to make it obvious at test level all the configuration.
Summary
Base PR for cross-platform CI on Windows and NVIDIA DGX Spark (ARM/aarch64 Linux). No user-facing API change.
windows,windows_ci,arm,arm_cipytest markers in pyproject.toml.pytest -m windows_ci(etc.) does not leak into Isaac Sim's argparse./tmp/IsaacLab→tempfile.gettempdir()for cross-platform compatibility.source/isaaclab/test/deps/test_torch.py+test_scipy.pywith the new markers so future cross-platform CI jobs can select them.Prior art and credits
This work is a derivative of and builds directly on two prior PRs by other contributors:
main, 2025-10-31 — "test windows CI". Designed and introduced the cross-platform marker scheme (windows,windows_ci,arm,arm_ci), tagged ~50 existing tests across the codebase, added the AppLauncher argv-strip pattern so the new pytest-mfilters do not leak into Isaac Sim's argument parser, and pioneered the Windows CI job wiring. The marker design and AppLauncher behavior in this PR are taken directly from test windows CI #3900.main, 2025-10-27 — "Adds basic CI verification on Windows". Built the original Windows self-hosted runner infrastructure:isaaclab.batentry script, the initial.github/workflows/build.ymlWindows job, and the self-hosted-runner labeling conventions this PR's downstream Part 2/Part 3 workflows still rely on.Both PRs target
mainand have not been merged. This PR rebases that work ontodevelop(where the active development happens —mainis behind by hundreds of commits), reduces the initial test surface to a small starter set (test_torch.py+test_scipy.py) to validate the marker/runner plumbing end-to-end before re-expanding, and adds anarm_cimarker variant + ARM/Spark runner support that #3900/#3845 did not include.Credit for the architecture goes to the authors above; this PR is the rebase + minimization step.
Series
PRs prefixed with
[CI] Cross-platform —all contribute to bringing Isaac Lab CI up on Windows and NVIDIA DGX Spark runners. This PR is the base everything else builds on. Current siblings:.github/workflows/arm-ci.yaml).github/workflows/windows-ci.yaml)Siblings depend on this PR's marker scheme; the same scaffolding lines are carried in their branches and dedupe on rebase after this lands. Additional follow-up parts can be added later without renumbering existing ones.
Test plan
pytest --collect-only -m windows_ci source/isaaclab/test/deps/returns the marked tests.pytest --collect-only -m arm_ci source/isaaclab/test/deps/returns the same set../isaaclab.sh -f(pre-commit) passes.