-
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 11 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() | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ | |||||||||||||||||||||||||||||||||||
# See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||||||
# limitations under the License. | ||||||||||||||||||||||||||||||||||||
import copy | ||||||||||||||||||||||||||||||||||||
from unittest.mock import patch | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
import pytest | ||||||||||||||||||||||||||||||||||||
import torch | ||||||||||||||||||||||||||||||||||||
|
@@ -22,7 +23,9 @@ | |||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
import modelopt.torch.opt as mto | ||||||||||||||||||||||||||||||||||||
import modelopt.torch.quantization as mtq | ||||||||||||||||||||||||||||||||||||
import modelopt.torch.quantization.model_calib as model_calib_module # needed for patching awq_lite | ||||||||||||||||||||||||||||||||||||
from modelopt.torch.quantization.backends.gemm_registry import enable_real_quant_gemm | ||||||||||||||||||||||||||||||||||||
from modelopt.torch.quantization.nn.modules.tensor_quantizer import SequentialQuantizer | ||||||||||||||||||||||||||||||||||||
from modelopt.torch.quantization.utils import is_quantized_linear | ||||||||||||||||||||||||||||||||||||
from modelopt.torch.utils import torch_to | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
|
@@ -116,40 +119,166 @@ def save_restore_test(model_cls, device, quant_config, compress=False, version=N | |||||||||||||||||||||||||||||||||||
mto.restore_from_modelopt_state(model_ref, state_dict) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
def tensor_parallel_test_helper(model, config, tp_group, dp_group): | ||||||||||||||||||||||||||||||||||||
# The input to fist layer, the column parallel should be the same across all tp ranks | ||||||||||||||||||||||||||||||||||||
def _reduce_quantizer_attr(quantizer, attr=str, op=dist.ReduceOp.MAX, group=None): | ||||||||||||||||||||||||||||||||||||
quantizer_attr = getattr(quantizer, attr).clone() | ||||||||||||||||||||||||||||||||||||
print("quantizer.attr before reduce", getattr(quantizer, attr)) | ||||||||||||||||||||||||||||||||||||
dist.all_reduce(quantizer_attr, op=op, group=group) | ||||||||||||||||||||||||||||||||||||
print("quantizer.attr after reduce", getattr(quantizer, attr)) | ||||||||||||||||||||||||||||||||||||
print("quantizer_attr after reduce", quantizer_attr) | ||||||||||||||||||||||||||||||||||||
assert torch.allclose(quantizer_attr, getattr(quantizer, attr)) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
|
def _reduce_quantizer_attr(quantizer, attr=str, op=dist.ReduceOp.MAX, group=None): | |
quantizer_attr = getattr(quantizer, attr).clone() | |
print("quantizer.attr before reduce", getattr(quantizer, attr)) | |
dist.all_reduce(quantizer_attr, op=op, group=group) | |
print("quantizer.attr after reduce", getattr(quantizer, attr)) | |
print("quantizer_attr after reduce", quantizer_attr) | |
assert torch.allclose(quantizer_attr, getattr(quantizer, attr)) | |
def _reduce_quantizer_attr(quantizer, attr: str, op=dist.ReduceOp.MAX, group=None): | |
quantizer_attr = getattr(quantizer, attr).clone() | |
# Optional: guard debug prints or remove | |
# if dist.is_initialized() and dist.get_rank() == 0: | |
# print("quantizer.attr before reduce", getattr(quantizer, attr)) | |
dist.all_reduce(quantizer_attr, op=op, group=group) | |
# if dist.is_initialized() and dist.get_rank() == 0: | |
# print("quantizer.attr after reduce", getattr(quantizer, attr)) | |
# print("quantizer_attr after reduce", quantizer_attr) | |
assert torch.allclose(quantizer_attr, getattr(quantizer, attr)) |
🤖 Prompt for AI Agents
In tests/_test_utils/torch_quantization/quantize_common.py around lines 122-129,
the function signature incorrectly uses attr=str (making the default the str
type) and unconditionally prints from every rank; change the signature to
annotate attr as a string (e.g. def _reduce_quantizer_attr(quantizer, attr: str,
op=dist.ReduceOp.MAX, group=None):) so attr is typed properly (no stray
default), and remove or gate the print statements behind a single rank (e.g.
only print when dist.is_initialized() and dist.get_rank() == 0) to avoid
spamming across ranks; keep the clone, all_reduce, and assertion logic
unchanged.
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.
Forward the AWQ-Lite kwargs in the patch
The _debug_awq_lite
wrapper drops every extra keyword argument that callers pass to awq_lite
(e.g., tensor_parallel_group
, data_parallel_group
, max_calib_steps
). The upstream API explicitly accepts **kwargs
, so the first call that includes one of those options will now raise a TypeError
, breaking AWQ-Lite calibration in the very paths this PR is exercising. Please mirror the original signature and forward **kwargs
to original_awq_lite
while forcing debug=True
.
-def _debug_awq_lite(model, forward_loop, alpha_step=0.1, debug=True):
- """Function to mock awq_lite function to always use debug=True for testing"""
- return original_awq_lite(model, forward_loop, alpha_step, debug=True)
+def _debug_awq_lite(model, forward_loop, alpha_step=0.1, debug=True, **kwargs):
+ """Force awq_lite debug mode during tests without dropping optional args."""
+ return original_awq_lite(
+ model,
+ forward_loop,
+ alpha_step=alpha_step,
+ debug=True,
+ **kwargs,
+ )
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def _debug_awq_lite(model, forward_loop, alpha_step=0.1, debug=True): | |
"""Function to mock awq_lite function to always use debug=True for testing""" | |
return original_awq_lite(model, forward_loop, alpha_step, debug=True) | |
def _debug_awq_lite(model, forward_loop, alpha_step=0.1, debug=True, **kwargs): | |
"""Force awq_lite debug mode during tests without dropping optional args.""" | |
return original_awq_lite( | |
model, | |
forward_loop, | |
alpha_step=alpha_step, | |
debug=True, | |
**kwargs, | |
) |
🤖 Prompt for AI Agents
In tests/_test_utils/torch_quantization/quantize_common.py around lines 134 to
137, the _debug_awq_lite wrapper drops any extra keyword arguments callers pass
to awq_lite which causes TypeError when upstream calls include options like
tensor_parallel_group or max_calib_steps; update the wrapper to mirror the
original awq_lite signature by accepting *args and **kwargs (or the same
explicit params plus **kwargs) and forward them to original_awq_lite while
forcing debug=True (i.e., call original_awq_lite(..., debug=True, **kwargs) so
all upstream options are preserved).
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.
@realAsma For TP, I only test fc1 (column parallel) act scale during awq lite, because fc2 row parallel will fail. For DP/CP I can test both column + row parallel act scale. I'm assuming row parallel fails because it's split across the c_in
dimension in activation ... is this right?
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.