[distributed] ProcessGroupXCCL has no one-sided window API (new_window / mem_allocator), so test_c10d_window.py cannot run on XPU
- Dominant language
- Python
- Stars
- 113
- Forks
- 128
- Avg merge
- 5d 9h
- Merged PRs (30d)
- 112
Description
### 🐛 Describe the bug
`test/distributed/test_c10d_window.py` is a backend-agnostic harness for the c10d
one-sided window API (`Backend.new_window` / `Window.put` / `signal` / `wait_signal` /
`map_remote_tensor`), parameterised over `(backend, device)` pairs:
```python
WINDOW_BACKENDS = [
("nccl2", "cuda"),
]
```
XCCL implements none of that surface: `grep -rniE "mem_allocator|new_window" src/xccl/`
in this repo returns nothing. On the PyTorch side the implementation lives only in
`torch/csrc/distributed/c10d/nccl2/WindowNCCL.cpp` (plus `ProcessGroupNCCL.cpp` /
`ProcessGroupNCCLUtils.cpp`).
The test already self-skips for backends without support:
```python
if not dist._supports_window() or not self.backend.supports_window:
self.skipTest(f"{self.backend_name} does not support windows")
```
so adding `("xccl", "xpu")` to `WINDOW_BACKENDS` today would only manufacture 11 more
skips rather than gain coverage. Current XPU state: `Ran 11 tests ... OK (skipped=11)`.
### What XPU needs
`ProcessGroupXCCL` needs to expose `mem_allocator` and implement the window surface
(`new_window`, `Window.put` / `signal` / `wait_signal` / `map_remote_tensor`) and report
`supports_window`.
Encouragingly the allocator half already exists -- `torch.xpu.MemPool` and
`torch.xpu.use_mem_pool` are both present, and the test builds its pool as
`torch.cuda.MemPool(self.backend.mem_allocator)`, so the missing piece is the
backend-side allocator/window API rather than XPU memory-pool support.
Once that lands, enabling the test is a one-line change to `WINDOW_BACKENDS` (the file
was explicitly written to be extended that way -- see its header comment "enabled by
extending WINDOW_BACKENDS").
Found while auditing `test/distributed/` for device-agnostic XPU support
(pytorch/pytorch#114850).
### What must pass to close this
`test/distributed/test_c10d_window.py` -- with `("xccl", "xpu")` added to
`WINDOW_BACKENDS`, the generated `XcclWindowTest` class (the file builds one class per
entry: `globals()[f"{backend_name.capitalize()}WindowTest"]`) must run all 11 tests
without hitting the `does not support windows` skip:
```bash
python test/distributed/test_c10d_window.py -v
```
- `XcclWindowTest::test_put_signal_wait_sync`
- `XcclWindowTest::test_put_signal_wait_async`
- `XcclWindowTest::test_signal_wait`
- `XcclWindowTest::test_put_dtypes_and_sizes`
- `XcclWindowTest::test_new_window_with_tensor`
- `XcclWindowTest::test_map_remote_tensor_local`
- `XcclWindowTest::test_window_attr`
- `XcclWindowTest::test_register_errors`
- `XcclWindowTest::test_put_out_of_bounds`
- `XcclWindowTest::test_put_argument_validation_is_rank_local`
- `XcclWindowTest::test_put_uses_destination_window_metadata`
Today the file reports `Ran 11 tests ... OK (skipped=11)` on XPU (the nccl2 class skipping
on a non-CUDA build). Acceptance criterion: 22 tests run, the 11 XCCL ones green and none
of them skipped. `test_window_attr` and `test_register_errors` in particular pin down the
`mem_allocator` / registration surface, so they are the ones to watch first.
### Versions
```
torch 2.15.0a0+git3c3da2a (source build, commit 3c3da2a5e8b)
device 4x Intel(R) Data Center GPU Max 1100
backend xccl
```
Contributor guide
Research direction
Start with test/distributed/test_c10d_window.py and the XCCL and NCCL entry points named in the issue: src/xccl/, torch/csrc/distributed/c10d/nccl2/WindowNCCL.cpp, ProcessGroupNCCL.cpp, and ProcessGroupNCCLUtils.cpp. Run the window test to establish the current skips, then use the listed XCCL test methods to verify the window and allocator surface. Done means adding XCCL coverage produces 22 passing tests with no XCCL skips.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, pytorch
- Domain
- backend-api-design, distributed-systems, testing
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 35/100