-
Notifications
You must be signed in to change notification settings - Fork 169
Sync amax & AWQ-Lite act_scale in context parallel/data parallel [OMNIML-2813] #359
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
f17131f
42519cc
264adbb
7cbe5b9
1f7d17e
71a9f7a
d02365c
5a572da
fc0bb88
95da832
34c11ef
10e3e2b
9f0691f
fa8f4c8
d1fac44
22b8b73
ca7c0e8
3f857a3
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -22,6 +22,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
import megatron.core.tensor_parallel.layers as megatron_parallel | ||||||||||||||||||||||||||||||||||||||||||||||||||||
import megatron.core.transformer.mlp as megatron_mlp | ||||||||||||||||||||||||||||||||||||||||||||||||||||
import torch | ||||||||||||||||||||||||||||||||||||||||||||||||||||
from megatron.core.parallel_state import get_data_parallel_group | ||||||||||||||||||||||||||||||||||||||||||||||||||||
from megatron.core.tensor_parallel.mappings import gather_from_sequence_parallel_region | ||||||||||||||||||||||||||||||||||||||||||||||||||||
from megatron.core.transformer import MegatronModule | ||||||||||||||||||||||||||||||||||||||||||||||||||||
from megatron.core.transformer.utils import make_sharded_tensors_for_checkpoint | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -217,9 +218,15 @@ class _MegatronParallelLinear(_ParallelLinear): | |||||||||||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
def _setup(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
data_parallel_group = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
data_parallel_group = get_data_parallel_group(with_context_parallel=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
except AssertionError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
data_parallel_group = get_data_parallel_group() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
self.parallel_state = ParallelState( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
getattr(mcore_parallel, "get_expert_data_parallel_group", "get_data_parallel_group")(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
data_parallel_group, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
mcore_parallel.get_tensor_model_parallel_group(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
jenchen13 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
mcore_parallel.get_context_parallel_group(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
224
to
233
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. Guard
Something along these lines keeps the DP-only path working: - self.parallel_state = ParallelState(
- data_parallel_group,
- mcore_parallel.get_tensor_model_parallel_group(),
- mcore_parallel.get_context_parallel_group(),
- )
+ try:
+ context_parallel_group = mcore_parallel.get_context_parallel_group()
+ except AssertionError:
+ context_parallel_group = -1
+ self.parallel_state = ParallelState(
+ data_parallel_group,
+ mcore_parallel.get_tensor_model_parallel_group(),
+ context_parallel_group,
+ ) 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
super()._setup() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
Past review concern about NaN propagation remains unresolved.
The previous review identified that NaN detection is performed locally (lines 624-627), but the synchronization call (lines 629-633) will still propagate NaNs from any rank to all ranks in the DP/CP group through the
AVG
operation. This creates an inconsistent state where:is_enabled = False
locallyAVG
operationact_scale
butis_enabled = True
Consider one of these solutions:
all_reduce
to check if ANY rank has NaN, then disable all ranks if truesync_act_scale_across_dp_cp
whenmodule.awq_lite.is_enabled
is true (but this requires collective agreement on enabled state)Based on past review comments.
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.
i now only sync the
act_scale
if no nans have been detected (it is after anelse
statement)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.
@jenchen13 Thank you for the update. While moving the sync to the
else
block prevents syncing on ranks that detect NaN locally, there's a critical distributed synchronization issue that remains:The Problem: Collective Operation Mismatch
The
dist.all_reduce()
call insidesync_act_scale_across_dp_cp
is a collective operation that requires all ranks in the DP/CP group to participate. With the current implementation:is_enabled = False
→ skips the else block → does NOT callall_reduce
else
block → callsall_reduce
This creates a deadlock because Rank B is waiting for Rank A to participate in the collective operation, but Rank A never makes the call.
Suggested Solution
You need to ensure all ranks agree on whether to sync before making the collective call:
This ensures all ranks take the same code path and avoid the collective operation mismatch.