Skip to content

Make GPU tests safe to collect on CPU-only hosts - #98

Open
ZhiyaoWen999 wants to merge 1 commit into
MoonshotAI:mainfrom
ZhiyaoWen999:fix/cpu-only-test-collection
Open

Make GPU tests safe to collect on CPU-only hosts#98
ZhiyaoWen999 wants to merge 1 commit into
MoonshotAI:mainfrom
ZhiyaoWen999:fix/cpu-only-test-collection

Conversation

@ZhiyaoWen999

Copy link
Copy Markdown

Summary

  • Defer accelerator discovery for GPU-marked tests to a pytest fixture.
  • Build device-dependent rank cases at test runtime instead of collection time.
  • Initialize device managers inside worker processes.
  • Skip GPU/NPU tests cleanly when the required accelerator is unavailable.

Root cause

GPU/NPU test modules initialized DeviceManager and evaluated device counts during module import and parameter collection. On CPU-only hosts, this could fail before pytest applied the -m "not gpu" filter.

Validation

  • pytest -v -m "not gpu" tests — 55 passed, 1 skipped, 6 deselected
  • pytest -q tests — 55 passed, 7 skipped
  • ruff check .
  • ruff format --check .

Closes #95

@ZhiyaoWen999
ZhiyaoWen999 marked this pull request as ready for review July 31, 2026 07:16
@UgaTheDev

Copy link
Copy Markdown

Independently reproduced this on a CPU-only host (macOS, Python 3.13, no CUDA/NPU/XPU). The approach is right and I think this should land. Notes below, including one thing I don't think has been said yet about why the current behavior on main is worse than it looks.

The status quo doesn't just fail loudly — it silently collects meaningless tests

main already has a mitigation in tests/test_update.py: DeviceManager() is wrapped in try/except TypeError with a fallback to a SimpleNamespace whose device_count() returns 0. That's why pytest --collect-only appears to succeed on CPU today, and I think it's why this PR may have looked lower-priority than it is.

But get_world_size() returning 0 at collection time means the parametrize block builds degenerate rank lists:

get_world_size() = 0
split       -> [[], [], [], []]
remote_err  -> [[]]
random_sub  -> [[]]

So the three test_update cases are collected, but every rank list is empty. They aren't skipped and they aren't meaningfully parametrized — on a CPU host they're placeholders that happen to survive collection. Worse, the random-subsets case collapses from N+1 parametrized subsets to exactly one empty list, so on any host where DeviceManager() fails the intended coverage silently disappears rather than erroring.

And collection succeeding is not the same as the suite passing. On current main (d1de07b), running the full suite on CPU:

6 failed, 104 passed, 7 skipped

FAILED tests/test_inplace_unpin.py::test_unpin_files - AssertionError: This test requires at least 2 GPUs.
FAILED tests/test_reuse_pin_memory.py::test_register_pin_memory - TypeError: ...
FAILED tests/test_update.py::test_update[test_no_error-rank_list0]
FAILED tests/test_update.py::test_update[test_with_remote_error-rank_list1]
FAILED tests/test_update.py::test_update[test_no_error-rank_list2]
FAILED tests/test_update.py::test_update_with_files

With this PR rebased on d1de07b, the same command:

104 passed, 13 skipped, 0 failed

All six failures become clean skips with an actionable reason (GPU/NPU runtime is unavailable: The current device type is not supported). That's the concrete before/after I'd put in the PR description — it's a stronger argument than "collection is safe", because it shows the PR fixes a suite that is currently red on CPU, not merely one that collects.

The branch is 4 commits stale, and the raw numbers look alarming because of it

Worth flagging so the diff isn't misread. This branch forks from 5ddbf0a; main has since gained #101, #99, #96 (Intel XPU) and #102 (vLLM stateless process groups). Checked out as-is the PR collects 62 tests, while main collects 117. That gap is purely branch age — the missing modules (test_device_manager.py, test_ipc_handler.py, test_p2p_guard.py, test_vllm_compat.py, test_xpu_ipc.py, test_xpu_parity.py) simply don't exist on the fork point. No tests are lost by this PR.

I rebased it locally onto d1de07b:

  • rebases cleanly, no conflicts
  • pytest --collect-only -q → 117 collected (identical to main)
  • pytest -m "not gpu" -q → 104 passed, 1 skipped, 12 deselected (identical to main)
  • pytest -q → 104 passed, 13 skipped, 0 failed
  • ruff check . → All checks passed; ruff format --check . → 30 files already formatted

So the newer XPU and device-manager tests added since the fork point coexist with the autouse fixture without issue. A rebase and a refreshed set of numbers in the description is all this needs.

The fixture generalizes beyond the files touched

Nice property that isn't called out in the description: tests/test_reuse_pin_memory.py is @pytest.mark.gpu but isn't edited by this PR, and it still converts from a hard TypeError failure to a clean skip purely via the autouse fixture in conftest.py. That means future GPU-marked tests get the right behavior for free, which is the main reason to prefer this over per-file guards.

Moving parametrize construction to runtime is the right call

build_rank_list(case, world_size) is the key change and it's correct — the previous code evaluated get_world_size() during collection, which is exactly what coupled test identity to hardware. Building rank lists inside the test body means the parametrize IDs (split-ranks, remote-error, random-subsets) are now stable and readable regardless of host, which is a real improvement over rank_list0/1/2.

Replacing assert world_size >= 2 with pytest.skip(...) is also right: "this host has fewer than 2 accelerators" is an environment condition, not a defect.

Optional nit

The fixture catches only TypeError, which does match DeviceManager._detect_device_type()'s raise for an unsupported device, so it's correct for the case at hand. The only scenario it wouldn't cover is an accelerator that is detected but whose runtime then errors during init (a broken/mismatched driver surfacing a RuntimeError from torch), which would error rather than skip. Low priority and arguably the right behavior anyway — a broken driver probably should be loud. Mentioning only so the choice is deliberate.

Summary

Verified the fix, verified it rebases clean onto current main, verified the full CPU suite goes from 6 failed to 0 failed. Only blocking-ish item is the rebase. LGTM after that.


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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make GPU/NPU test collection safe on CPU-only hosts

2 participants