Repository navigation
Combine GEMM + SwiGLU fused MLP PRs (3890, 4071, 4095, 4219, 4311, 4324) → main #4636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 32 commits
2e618b3
d3fcd28
a78c09d
24d04f0
f8515fa
97a0a53
c6275d2
71b2913
2b33d05
b6a13ab
6512a1d
af6a39e
4cd3e8e
81d4d95
c824c66
4a38f4f
29c90ad
f3e955d
0a9d86c
2a4e41a
4812c96
23186ec
e9704da
1a99f7e
093caf1
5d66788
20cfe67
0909634
3f4cd68
4868335
46be5d1
8d3920d
d49ad27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -254,6 +254,10 @@ def __init__( | |||||||||||||||||
| # or bucket.grad_data. | ||||||||||||||||||
| self.cached_param_buffer_shard_list = [None] * len(self.buckets) | ||||||||||||||||||
| self.cached_grad_buffer_shard_list = [None] * len(self.buckets) | ||||||||||||||||||
| # Cached views remember the grad-mode context in which they were created. Rebuild them | ||||||||||||||||||
| # after a mode change so param all-gathers never mutate no_grad-created views while | ||||||||||||||||||
| # autograd is enabled. | ||||||||||||||||||
| self._cached_param_buffer_shards_grad_enabled = None | ||||||||||||||||||
|
|
||||||||||||||||||
| def reset(self): | ||||||||||||||||||
| """ | ||||||||||||||||||
|
|
@@ -417,23 +421,31 @@ def start_param_sync(self, force_sync: bool = False): | |||||||||||||||||
| # Standard distributed optimizer path: use _coalescing_manager. | ||||||||||||||||||
| # all_gather_into_tensor writes directly into a contiguous output buffer and | ||||||||||||||||||
| # does not need a copy-back step, so coalescing works correctly. | ||||||||||||||||||
| with _coalescing_manager( | ||||||||||||||||||
| self.intra_distributed_optimizer_instance_group, async_ops=async_op | ||||||||||||||||||
| ) as cm: | ||||||||||||||||||
| for idx, bucket in enumerate(self.buckets): | ||||||||||||||||||
| if self.cached_param_buffer_shard_list[idx] is None: | ||||||||||||||||||
| self.cached_param_buffer_shard_list[idx] = shard_buffer( | ||||||||||||||||||
| bucket.param_data, self.intra_distributed_optimizer_instance_size | ||||||||||||||||||
| current_grad_enabled = torch.is_grad_enabled() | ||||||||||||||||||
| if self._cached_param_buffer_shards_grad_enabled != current_grad_enabled: | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this line has been removed in dev branch, @ksivaman can you double check the logic for this part?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @zhongbozhu thanks — I traced this across
So this PR is not dangling — it carries both the init AND the read site (the read at L413 + the Recent dev work (#4560, #4563, #4649, #4609) replaced the grad-mode tracking with explicit @ksivaman can you confirm whether keeping the L413 read site +
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Connor-XY #4560 and #4649 are PRs to a temporary staging branch, so #4563 is the actual dev2main PR that we should consider if it should be shipped together this PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the pointer to #4563 (and for clarifying that #4560 / #4649 are staging-branch PRs, not dev2main candidates). On bundling #4563: I'd recommend keeping them separate.
Happy to flip the call if you'd rather bundle to amortize a single review cycle. |
||||||||||||||||||
| self.cached_param_buffer_shard_list = [None] * len(self.buckets) | ||||||||||||||||||
| self._cached_param_buffer_shards_grad_enabled = current_grad_enabled | ||||||||||||||||||
| # Param all-gather writes directly into bucket param buffers. These in-place updates | ||||||||||||||||||
| # are optimizer state movement, not differentiable work, so keep the cached | ||||||||||||||||||
| # destination views and the collective inside no_grad. | ||||||||||||||||||
| with torch.no_grad(): | ||||||||||||||||||
| with _coalescing_manager( | ||||||||||||||||||
| self.intra_distributed_optimizer_instance_group, async_ops=async_op | ||||||||||||||||||
| ) as cm: | ||||||||||||||||||
| for idx, bucket in enumerate(self.buckets): | ||||||||||||||||||
| if self.cached_param_buffer_shard_list[idx] is None: | ||||||||||||||||||
| self.cached_param_buffer_shard_list[idx] = shard_buffer( | ||||||||||||||||||
| bucket.param_data, self.intra_distributed_optimizer_instance_size | ||||||||||||||||||
| ) | ||||||||||||||||||
| local_data_view = self.cached_param_buffer_shard_list[idx][ | ||||||||||||||||||
| self.intra_distributed_optimizer_instance_rank | ||||||||||||||||||
| ] | ||||||||||||||||||
| dist_all_gather_func( | ||||||||||||||||||
| bucket.param_data, | ||||||||||||||||||
| local_data_view, | ||||||||||||||||||
| group=self.intra_distributed_optimizer_instance_group, | ||||||||||||||||||
| async_op=async_op, | ||||||||||||||||||
| ) | ||||||||||||||||||
| local_data_view = self.cached_param_buffer_shard_list[idx][ | ||||||||||||||||||
| self.intra_distributed_optimizer_instance_rank | ||||||||||||||||||
| ] | ||||||||||||||||||
| dist_all_gather_func( | ||||||||||||||||||
| bucket.param_data, | ||||||||||||||||||
| local_data_view, | ||||||||||||||||||
| group=self.intra_distributed_optimizer_instance_group, | ||||||||||||||||||
| async_op=async_op, | ||||||||||||||||||
| ) | ||||||||||||||||||
| if async_op: | ||||||||||||||||||
| self.param_gather_handle = cm | ||||||||||||||||||
| else: | ||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line of code looks already a dangling pointer in dev branch, but somehow it's still referenced in this PR, this might be related with merge conflict resolving, but we need more testing for this part
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed inline at #4636 (comment) — TL;DR: this PR is not dangling, the read site at L413 still uses this field. It is dangling on
dev(consumption side removed by later refactors), butmaindoesn't have those refactors yet, so removing the protection here would re-introduce the autograd-mid-AG issue PR #3890 was originally guarding against. Awaiting @ksivaman's call on whether to delete or keep.