Repository navigation
Use torch.accelerator for device, stream and memory APIs - #5450
Conversation
|
|
|
@claude review this PR |
|
Claude finished @tianyu-l's task in 2m 7s —— View job Review: Use torch.accelerator for device, stream and memory APIs
The direction looks good.
Leftovers are also clean. I could not run Python or the tests in this environment (the commands need approval), so these notes come from reading the code. Main concern: out-of-tree / privateuse1 backendsThe goal is to "work on any accelerator". But this PR moves the dependency from per-backend module functions to the
It would help to note in the PR description which backends were validated (XPU? ROCm? Neuron?). If Neuron hasn't been checked, please check it, or ping the people who added the Neuron peak-flops branch. Inconsistencies and leftovers
Smaller notes
VerificationNone of this should change numerics, but the device, stream and event setup touches every training run. Before merging:
Overall LGTM in direction. Before landing, please (1) confirm or validate privateuse1/Neuron behavior and (2) convert |
e548f95 to
031ab03
Compare
|
Thanks @tianyu-l, the review was helpful. Good catch on out-of-tree backends: device_module already covers every backend, so converting it to torch.accelerator added risk for neuron without adding anything. I've reverted those changes, so the PR now only replaces hard-coded torch.cuda calls. I also converted the first-step sync in optim.py that I'd missed and fixed up the tests. On XPU, llama3 debug run matches main exactly for 10 steps; for cuda, I will rely on CI. |
|
@claude review again |
|
Claude finished @tianyu-l's task in 1m 31s —— View job Re-review: Use torch.accelerator for device, stream and memory APIs
The revised scope is much safer.
As before, I could not run the tests here, so these notes come from reading the code. One behavior change worth fixing:
|
031ab03 to
e72ee7e
Compare
|
Thanks @tianyu-l. I fixed the optim.py guard: it now checks torch.accelerator.is_available() and device_module.is_initialized() and uses device_module.synchronize(), matching the original behavior. |
e72ee7e to
b45d7f4
Compare
|
@claude check again |
|
Claude finished @tianyu-l's task in 44s —— View job Third review: Use torch.accelerator for device, stream and memory APIs
Verdict: LGTM. The one blocking item from last time is fixed, and I found nothing new. I could not run the tests here, so this comes from reading the code.
|
|
@jemitche1 CPU failure seems only happening on this PR, could you help investigate? |
b45d7f4 to
35afe68
Compare
|
Thanks for flagging! test_optim.py was still mocking the old torch.cuda calls in the first-step block. I updated it to use torch.accelerator and device_module instead, and moved empty_cache() back outside the guard to match the original behavior. The CPU unit tests pass locally against nightly. |
|
thanks, there seems a conflict -- could you rebase |
Use torch.accelerator, torch.Stream and torch.Event so EMA CPU offload, RL weight staging and the optimizer first-step sync run on non-CUDA accelerators, and switch paged stash and precompile to the same APIs for consistency.
35afe68 to
b4e69b2
Compare
This PR replaces hard-coded torch.cuda calls with torch.accelerator, torch.Stream and torch.Event so EMA CPU offload, RL weight staging, the optimizer first-step sync and graph_trainer precompile no longer assume CUDA