Deprecate DataLoader pin_memory_device param #146821
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/146821
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: ✅ No FailuresAs of commit 0936a1a with merge base 97dfd8d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
6957150 to
d390aeb
Compare
|
@albanD Hello, please check changes when available, thanks! |
d390aeb to
b393a6a
Compare
|
@zeshengzong Should we also update https://github.com/pytorch/pytorch/blob/main/torch/utils/data/_utils/pin_memory.py#L18-L33 ? We don't want Also, starting CI. |
01b1d42 to
0dac805
Compare
| @@ -15,22 +15,13 @@ | |||
| from . import MP_STATUS_CHECK_INTERVAL | |||
|
|
|||
|
|
|||
| def _pin_memory_loop(in_queue, out_queue, device_id, done_event, device): | |||
| def _pin_memory_loop(in_queue, out_queue, done_event, device): | |||
There was a problem hiding this comment.
I think device_id param may not needed, the caller pass value of torch.accelerator.current_device_index() and here we set it to torch.accelerator.set_device_index(device_id), looks like its the same value, be like
current_device_index = torch.accelerator.current_device_index()
torch.accelerator.set_device_index(current_device_index)So remove the set logic in below
There was a problem hiding this comment.
But this is called from a different thread. So we have to set the device again !
There was a problem hiding this comment.
Oh, not aware of this point, restored change, thanks!
Changed |
|
@pytorchbot rebase -b main |
|
@pytorchbot started a rebase job onto refs/remotes/origin/main. Check the current status here |
|
Successfully rebased |
0dac805 to
e9798b4
Compare
| custom_device_mod.set_device(device_id) | ||
| elif device is None: | ||
| torch.accelerator.set_device_index(device_id) | ||
| current_device_index = torch.accelerator.current_device_index() |
There was a problem hiding this comment.
@zeshengzong Can we create current_device_index in the except block, since it is used only for the ExceptionWrapper?
There was a problem hiding this comment.
Changed, thanks!
|
@zeshengzong Left a minor comment, looking good otherwise. @albanD Since your suggestion sparked this PR, can you give it a look as well. Thanks! |
|
@pytorchbot rebase -b main |
|
@pytorchbot started a rebase job onto refs/remotes/origin/main. Check the current status here |
|
Successfully rebased |
2a240ce to
490c0d5
Compare
|
@albanD Hello, please help review the change, thanks! |
|
@pytorchbot rebase -b main |
|
@pytorchbot started a rebase job onto refs/remotes/origin/main. Check the current status here |
|
Successfully rebased |
9d2242a to
0936a1a
Compare
| @@ -235,7 +234,7 @@ class DataLoader(Generic[_T_co]): | |||
| drop_last: bool | |||
| timeout: float | |||
| sampler: Union[Sampler, Iterable] | |||
| pin_memory_device: str | |||
| pin_memory_device: Optional[str] | |||
There was a problem hiding this comment.
@zeshengzong Let's align the type and default of pin_memory_device here with that defined in the function definition (line 260).
Following #131858 suggestion to optimize DataLoader code
cc @albanD