Add FSDP v2 MXFP8 mixed precision support - #6
Conversation
| @@ -56,6 +56,14 @@ def __init__( | |||
| self.device = params[0].device | |||
| self.dtype = params[0].dtype | |||
| self.requires_grad = params[0].requires_grad | |||
| self.mp_policy = mp_policy | |||
| self.is_fp8_group = self.mp_policy.is_fp8_param(params[0]) | |||
There was a problem hiding this comment.
Can dtype be used instead of is_fp8_group?
There was a problem hiding this comment.
The dtype approach probably won't work — the dtype of an FP8 tensor is actually uint8 since it's a fake tensor. We probably shouldn't use the dtype of a fake tensor as the basis for this check.
There was a problem hiding this comment.
I see, I think we can consider using an enum to represent the dtypes supported by M-FSDP. The flag is_fp8_group can be misleading, as it conflates different FP8 recipes without distinction.
| if s != "no_shard": | ||
| wbuf = self._create_buffer(self.dtype, shard_weights) | ||
| model_weight_dtype = self.mp_policy.model_weight_buffer_dtype(self.params[0]) |
There was a problem hiding this comment.
Could model_weight_buffer_dtype be implemented as a standalone function? We may need to determine the parameter data type directly from the parameters or their parent module, especially in cases where mixed formats like NVFP4 and MXFP8 are used, rather than assuming a single uniform recipe.
There was a problem hiding this comment.
This function should be functionally equivalent to the previous version. Since it accepts a param argument, we actually first determine the type based on the type of that param However, we need to ensure that the recipe is consistent within the same param group
There was a problem hiding this comment.
Could the model_weight_buffer_dtype be static method?
shjwudp
left a comment
There was a problem hiding this comment.
Looks great overall: it implements MXFP8 parameter initialization, pre‑ and post‑unshard/reshard handling, and batched distributed quantization from the main weight to the FP8 weight. It may be worth double‑checking compatibility with fine‑grained activation recompute.
In particular, this also addresses the EDP grad NaN issue, which is great work!
Let’s merge first so we don’t block progress, but there are still some details that need fixing, and we should also add a design doc to go with it.
| @@ -56,6 +56,14 @@ def __init__( | |||
| self.device = params[0].device | |||
| self.dtype = params[0].dtype | |||
| self.requires_grad = params[0].requires_grad | |||
| self.mp_policy = mp_policy | |||
| self.is_fp8_group = self.mp_policy.is_fp8_param(params[0]) | |||
There was a problem hiding this comment.
I see, I think we can consider using an enum to represent the dtypes supported by M-FSDP. The flag is_fp8_group can be misleading, as it conflates different FP8 recipes without distinction.
| if work is None: | ||
| work = weight_work | ||
|
|
||
| if self.is_fp8_group and full_weight_buffer is not None: |
There was a problem hiding this comment.
When should full_weight_buffer be None?
For the self.is_fp8_group condition, a more intuitive way to determine this would be based on whether a backward-specific weight is required. For example:
if self.requires_backward_specific_weight:
| start_offsets, | ||
| self.dp_group, | ||
| model_param_shards, | ||
| ) |
There was a problem hiding this comment.
Can we abstract L265-298 into a function and put it in mixed_precision.py?
| return self.main_grads_dtype | ||
| return torch.bfloat16 if self.is_fp8_param(tensor) else tensor.dtype | ||
|
|
||
| def initial_main_weight(self, tensor: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
Would dequantized be a better name here?
There was a problem hiding this comment.
get_dequantized_value or get_high_precision_value
| or self.model_weight_buffer is None | ||
| and self.main_weight_buffer is None | ||
| ): | ||
| continue |
| full_weight_buffer, weight_work = weight_buffer.unshard( | ||
| async_op=async_op, | ||
| bind_params=not self.is_fp8_group, | ||
| ) |
There was a problem hiding this comment.
Handle activation recompute case:
Per-layer recompute
- module [A, B] forward, unshard and bind [A, B] rowwise data
- module B backward, unshard and bind B colwise data
Fine-grained recompute
- module C forward, unshard and bind C rowwise data
- module B backward, unshard and bind B colwise data
*C is sub module of B
| return self.main_grads_dtype | ||
| return torch.bfloat16 if self.is_fp8_param(tensor) else tensor.dtype | ||
|
|
||
| def initial_main_weight(self, tensor: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
get_dequantized_value or get_high_precision_value
| if s != "no_shard": | ||
| wbuf = self._create_buffer(self.dtype, shard_weights) | ||
| model_weight_dtype = self.mp_policy.model_weight_buffer_dtype(self.params[0]) |
There was a problem hiding this comment.
Could the model_weight_buffer_dtype be static method?
What does this PR do ?
Contribution process
flowchart LR A[Pre-checks] --> B[PR Tests] subgraph Code Review/Approval C1[Expert Review] --> C2[Final Review] end B --> C1 C2 --> D[Merge]Pre-checks
Core 0.8)Code review
The following process is enforced via the CODEOWNERS file for changes into
megatron/core. For changes outside ofmegatron/core, it is up to the PR author whether or not to tag the Final Reviewer team.For MRs into `main` branch
Feel free to message or comment the @mcore-oncall to help accelerate your merge into main. The less complex your PR is, the faster it will be approved and merged!
(Step 1): Add PR label
Expert Review(Step 2): Collect the expert reviewers reviews
Expert Reviewlabel when your PR is ready for review.Final Review might get declined if these requirements are not fulfilled.
(Step 3): Final Review
Final Reviewlabel(Optional Step 4): Cherry-pick into release branch
If this PR also needs to be merged into
core_r*release branches, after this PR has been merged, selectCherry-pickto open a new PR into the release branch.For MRs into `dev` branch
The proposed review process for `dev` branch is under active discussion.MRs are mergable after one approval by either
eharper@nvidia.comorzijiey@nvidia.com.Merging your PR
Any member of core-adlr and
core-nemowill be able to merge your PR.