[optim] Improve adadelta foreach, group tensors to maximize fast path #92048
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/92048
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 853066c: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
janeyx99
added
ciflow/trunk
janeyx99
force-pushed
the
group-adadelta-foreach
branch
from
949af47 to
86b9b51
Compare
janeyx99
added
the
ciflow/periodic
| time python test/run_test.py --verbose -i distributed/_shard/test_replicated_tensor | ||
| # Other tests | ||
| time python test/run_test.py --verbose -i test_cuda_primary_ctx | ||
| time python test/run_test.py --verbose -i test_optim -- -k optimizers_with_varying_tensors |
There was a problem hiding this comment.
@pytorch/pytorch-dev-infra to make sure this is okay. The total time it would add to multigpu is about 13 seconds.
albanD
left a comment
albanD
left a comment
There was a problem hiding this comment.
Looks good, just the seed needs to be change, the rest are nits
|
This PR has been accepted with the accept2ship label. Attempting to merge now. @pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
…#91896) following up to #90865 and #92048 Pull Request resolved: #91896 Approved by: https://github.com/albanD
Old behavior would have adadelta foreach sending tensors to the slow path if they were not all the same dtype nor on the same device.
This PR adds grouping for adadelta optimizer so that it would run foreach in batches, allowing more users to benefit from foreach perf.
Of course, we should ensure that the new implementation works, so there are new tests to ensure this behavior is not broken.