fix(utils): propagate non_blocking in TorchAOBaseTensor._to_copy and _get_to_kwargs - #4297
Conversation
…_get_to_kwargs ## Problem `_get_to_kwargs` explicitly discarded the `non_blocking` argument parsed from `torch._C._nn._parse_to`, with a comment saying it is "not very useful for most tensor subclasses". As a result, any call to `tensor.to(device, non_blocking=True)` on a `TorchAOBaseTensor` subclass silently became a blocking transfer at the inner-tensor level. This matters in practice for async CPU→GPU offloading workflows such as `diffusers` `enable_group_offload(use_stream=True)`: the diffusers hook schedules copies with `non_blocking=True` so that the transfer stream and the compute stream can overlap. Because the flag was dropped, all copies became blocking, negating the overlap benefit. On AMD ROCm (gfx1xxx) the missing non_blocking also interacts with a separate stream-ordering race (fixed in huggingface/diffusers#13502): the default stream can race ahead of "blocking" copies that the OS scheduler hasn't committed yet, producing device-mismatch errors in the first matmul. ## Fix 1. `_get_to_kwargs`: include `non_blocking` in the returned kwargs dict. 2. `TorchAOBaseTensor._to_copy.default`: pop `non_blocking` from kwargs and forward it to every inner `.to()` call for both `tensor_data_names` and `optional_tensor_data_names`. The change is backward-compatible: when `non_blocking=False` (the default), behaviour is identical to before. ## Tested on - 5× AMD RX 7800 XT (gfx1101), ROCm 7.1, PyTorch 2.7 - FLUX.1-dev int8 (`Int8WeightOnlyConfig`) with `enable_group_offload(use_stream=True)` - Companion fix in diffusers: huggingface/diffusers#13502
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/4297
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ❌ 1 New FailureAs of commit ce00da1 with merge base b3e0db2 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @Dev-next-gen! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Hi @jerryzh168 @andrewor14 — gentle ping 🙏 PR is ~9+ days old, CLA signed (confirmed by meta-cla bot), but the CI workflows haven't run Could a maintainer kick off the workflow approval whenever convenient? It's a small fix Companion PR on diffusers side: huggingface/diffusers#13502 — together they unblock async Happy to iterate on review feedback once CI is green. Thanks! |
|
@pytorchbot label "module: core" |
|
@jerryzh168 @vkuzo quick note for whoever reviews: the H100 failure ( |
jerryzh168
left a comment
There was a problem hiding this comment.
looks good, can you add a test for this?
Verifies the contract change in TorchAOBaseTensor._get_to_kwargs: the returned kwargs dict now includes `non_blocking`, propagated from the original `.to(device, non_blocking=...)` call. Covers three cases: explicit True, explicit False, and default (unspecified). Runs on CPU only, no @skip_if_no_cuda needed. Addresses review feedback on PR pytorch#4297.
|
@jerryzh168 added the requested test in |
|
@jerryzh168 thanks for the quick turnaround on the review! Let me know if anything else is needed from my side to land this. |
Problem
_get_to_kwargsexplicitly discarded thenon_blockingargument parsed fromtorch._C._nn._parse_to, with a comment saying it is "not very useful for most tensor subclasses". As a result, any call totensor.to(device, non_blocking=True)on aTorchAOBaseTensorsubclass silently became a blocking transfer at the inner-tensor level.This matters in practice for async CPU→GPU offloading workflows such as
diffusersenable_group_offload(use_stream=True): the diffusers hook schedules copies withnon_blocking=Trueso that the transfer stream and the compute stream can overlap. Because the flag was dropped, all copies became blocking, negating the overlap benefit.On AMD ROCm (gfx1xxx) the missing
non_blockingalso interacts with a separate stream-ordering race (fixed in huggingface/diffusers#13502): the default stream can race ahead of "blocking" copies that the OS scheduler hasn't committed yet, producing device-mismatch errors in the first matmul.Fix
_get_to_kwargs: includenon_blockingin the returned kwargs dict.TorchAOBaseTensor._to_copy.default: popnon_blockingfrom kwargs and forward it to every inner.to()call for bothtensor_data_namesandoptional_tensor_data_names.The change is backward-compatible: when
non_blocking=False(the default), behaviour is identical to before.Tested on
| GPU | 5× AMD RX 7800 XT (gfx1101) |
| ROCm | 7.1 |
| PyTorch | 2.7 |
| Model | FLUX.1-dev,
Int8WeightOnlyConfigvia torchao || Config | block-level group offload +
use_stream=True(diffusers) |Related