[optim][adagrad] group tensors in foreach to maximize perf #92362
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/92362
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 FailuresAs of commit 67a1880: NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
| if maximize: | ||
| device_grads = torch._foreach_neg(device_grads) | ||
|
|
||
| device_has_sparse_grad = any(grad.is_sparse for grad in device_grads) |
There was a problem hiding this comment.
You now unconditionally recompute this even if it was already passed in? That doesn't sound right?
There was a problem hiding this comment.
Hm, my thinking was that even if the whole batch may contain sparse grads, groups may not, so we can still use foreach to optimize these subgroups.
Is it more common for all grads to either be sparse or unsparse? If so, there may be other places I need to patch 😬
There was a problem hiding this comment.
I think it is fine if we say that we need this info. I'm just more worried about the ignored arg ;)
There was a problem hiding this comment.
I'm guessing whatever resolution we have here should be applied to https://github.com/pytorch/pytorch/pull/92338/files#r1072880613 as well?
There was a problem hiding this comment.
Yep, okay, my initial proposal is that we deprecate has_sparse_grad across the codebase.
janeyx99
force-pushed
the
adagrad-group-foreach
branch
from
4a63bed
to
67a1880
Compare
|
@pytorchbot merge |
Merge failedReason: Not merging any PRs at the moment because there is a merge blocking https://github.com/pytorch/pytorch/labels/ci:%20sev issue open at: Details for Dev Infra teamRaised by workflow job |
|
@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 |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / linux-bionic-cuda11.6-py3.10-gcc7-sm86 / test (default, 3, 4, linux.g5.4xlarge.nvidia.gpu) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "irrelevant failures" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
another one