Repository navigation
[fsdp] fix: preserve Hugging Face fp32-keep modules in FSDP2 - #7165
chengcuiping wants to merge 4 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Hey, so I just saw this substantial work here on this specific issue... I independently reproduced the original precision-loss bug and confirmed that the overall isolation/loading approach works on both one and two GPUs. I found one merge-blocking semantic issue: the FP32 child policy currently uses The matched parameters are already stored in FP32, so This changes the motivating Inkling behavior directly. Inkling’s short-convolution module records its incoming activation dtype, computes internally in FP32, and converts its output back to the original dtype. In an A100 reproduction with PyTorch 2.8:
The current distributed test misses this because it runs outside I prepared a focused correction that:
Corrective commit: Validation:
The correction is based on current |
|
Thanks @MagellaX — I reviewed the PyTorch 2.8 mixed-precision semantics and your corrective diff, and I agree this is a merge-blocking issue. My original reasoning conflated the all-gathered parameter dtype with the activation and execution dtype at the FSDP2 unit boundary. The keep-aware build cast already preserves the matched parameters in FP32, so The existing distributed test did not catch this because its forward hook observed I will integrate
I will also audit direct/non-autocast forward callers and extend the activation-boundary parity coverage to FP16. The acceptance criterion will be transparency against the unsharded reference under the same execution context; I will not use an FSDP boundary cast to make an otherwise unsupported path behave differently from that reference. The Transformers-version assertion fix is independently valid and will be included. I will also update the implementation comments, test terminology, and PR description to distinguish the all-gathered parameter dtype from the activation/compute dtype and remove the claim that the matched module is necessarily computed in FP32. Thank you for the independent reproduction and for preparing a focused, well-validated correction. I will post the exact revalidation results here after updating the branch. |
|
@MagellaX, thank you for identifying and correcting the activation-boundary semantics. Your original fix Caller audit: Independent validation:
The final contract is deliberately narrow: FP32-keep parameters remain stored/materialized/all-gathered in FP32, while the FSDP2 child boundary does not change module activation input/output dtypes. Final PR head: |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
verl never consulted Hugging Face's `_keep_in_fp32_modules` / `_keep_in_fp32_modules_strict`, so modules a model declares as fp32-keep were silently degraded under FSDP2 in two ways: 1. `FSDPEngine._build_module` casts with `module.to(torch_dtype)` right after `from_pretrained`, which had just materialised those modules in fp32. The blanket cast rounds them through bf16/fp16 irrecoverably. 2. `fully_shard` all-gathers every parameter of a unit in `mp_policy.param_dtype`, so in verl's default configuration (fp32 storage plus bf16 compute) an fp32-keep module still computes in bf16. The stored values stay bit-exact and only the compute dtype is wrong, which is what makes the bug silent. Resolve the keep names using HF's per-list dtype gate (`_keep_in_fp32_modules` for fp16 only, `_keep_in_fp32_modules_strict` for fp16 and bf16), verified identical in transformers 4.56.1 and 5.11.0, reading only the top-most declaration as both versions do. Matching follows the installed version: 4.x anchors on whole path segments, 5.x searches unanchored and expands globs. Cast only the non-keep floating states at build time, keeping matched parameters and buffers in fp32, and make each topmost matched module its own `fully_shard` unit with `param_dtype=torch.float32` so it is all-gathered in fp32. Only `param_dtype` is overridden; reduce/output dtype and `cast_forward_inputs` keep the caller's semantics. A keep unit absorbs its whole subtree from the standard wrap-target list, so nothing is wrapped twice, and a container without its own `forward` is skipped in favour of its callable children, whose hooks actually fire. `fsdp2_load_full_state_dict` aligns source dtypes to the destination before broadcast, covering every alias of a tied weight, in a shallow copy that leaves the caller's state dict untouched. Four conditions fail closed before anything is wrapped, so the model is never left partially wrapped: a keep parameter no unit can own, one tied across unit boundaries, a unit mixing fp32 keep parameters with lower-precision ones, and a unit whose keep parameters are already degraded. FSDP1 is left unchanged: its flat parameters require a uniform dtype per group and cannot represent the result. Models declaring no keep list are unaffected. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: chengcuiping <chengcuipingswu@163.com>
Keep Hugging Face fp32 child parameters in their original all-gather dtype without forcing floating inputs to fp32. Exercise the FSDP path under engine-style autocast, pin Inkling-style activation boundary parity, and make the matcher test portable across the supported Transformers range. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: yash solanki <alphacr792@gmail.com> (cherry picked from commit dbefb12)
Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: chengcuiping <chengcuipingswu@163.com>
Run the existing BF16/FP16 reference-parity matrix with forward prefetch both disabled and enabled so fp32 child FSDP units are exercised with the upstream prefetch chain. Co-authored-by: OpenAI Codex <codex@openai.com> Signed-off-by: chengcuiping <chengcuipingswu@163.com>
f28d3de to
f621a30
Compare
|
Hi @eric-haibin-lin @vermouth1992 — PR #7165 has been rebased and pushed at The rebase preserves the Local validation passed: 85 CPU tests, the FP32-keep regression on 1 and 2 GPUs, the 2-GPU CPU-offload and pinned-transfer regressions, and full pre-commit checks. These ran on A100 with PyTorch 2.8.0+cu128 and Transformers 4.56.1; the current CI environment remains unvalidated. All 36 workflow runs for this head are currently |
What does this PR do?
Fixes #7092.
verl's FSDP2 path did not honor Hugging Face's
_keep_in_fp32_modulesand_keep_in_fp32_modules_strictdeclarations. As a result, parameters and floating-point buffers that Hugging Face intentionally keeps in FP32 could be silently reduced during model construction, FSDP2 parameter materialization/all-gather, or full-state-dict loading.This PR preserves the Hugging Face FP32-keep parameter contract throughout the FSDP2 lifecycle:
param_dtype=None, so their parameters materialize/all-gather in their original FP32 dtype.FSDP1 behavior is unchanged. This PR introduces no user-facing API, CLI, or configuration changes.
AI assistance was used in preparing this change; I reviewed every changed line and independently executed all validation reported below.
Checklist Before Starting
Searched for similar open PRs using the issue number and
keep_in_fp32_moduleskeywords; no existing PR implements this contract.Reviewed related PR #4275, which adds dtype-aware FSDP2 wrapping for LoRA modules but does not implement Hugging Face
_keep_in_fp32_modules/_keep_in_fp32_modules_strictsemantics.Formatted the title as
[{modules}] {type}: {description}:[fsdp] fix: preserve Hugging Face fp32-keep modules in FSDP2Test
pytest tests/utils/test_keep_in_fp32_utils_on_cpu.py -qpytest tests/utils/test_fsdp_wrap_policy_on_cpu.py tests/workers/test_fsdp_gradient_accumulation_sync_on_cpu.py tests/utils/test_fsdp2_peft_wrapping.py -qCUDA_VISIBLE_DEVICES=0 torchrun --standalone --nproc-per-node=1 tests/special_distributed/test_fsdp2_keep_in_fp32.pyCUDA_VISIBLE_DEVICES=0,1 torchrun --standalone --nproc-per-node=2 tests/special_distributed/test_fsdp2_keep_in_fp32.pyCUDA_VISIBLE_DEVICES=0,1 torchrun --standalone --nproc-per-node=2 tests/special_distributed/test_fsdp2_cpu_offload_state_dict.pyparam_dtype=torch.float32in an isolated worktreepre-commit run --all-files --show-diff-on-failure --color=alwaysgit diff --checkfor the worktree and complete PR diffTest environment:
Transformers 5.10.0 was reported as yanked by the installer, but the requested wheel was installed successfully and its CPU helper suite completed with 67 passed tests.
Two distinct regressions are covered:
weight.dtypeforward hook observes only the materialized/all-gathered parameter dtype; it is not evidence of an operator's execution dtype.param_dtype=torch.float32policy forces a different activation boundary. The isolated negative control changes the matched module's BF16 activation input/output to FP32 and fails reference-transparency assertions as expected.The distributed regression coverage includes:
API and Usage Example
No user action is required. Existing Hugging Face declarations are honored automatically when verl uses FSDP2:
With
strategy=fsdp2and a low-precisionmixed_precision.param_dtype, eligible matched parameters remain stored in FP32 and materialize/all-gather in FP32. The FSDP2 boundary does not redefine module activation input/output or execution dtypes; those remain determined by the caller's autocast context and the module's own casting logic.Design & Code Changes
Honor Hugging Face declaration semantics
The resolver applies the same dtype gates as Hugging Face Transformers:
_keep_in_fp32_modulesapplies when the model dtype is FP16._keep_in_fp32_modules_strictapplies when the model dtype is FP16 or BF16.Matching follows the installed Transformers capability: whole-segment matching for supported 4.x versions and unanchored glob matching for supported 5.x versions. The helper suite was executed with Transformers 4.56.1, 5.3.0, 5.5.3, and 5.10.0.
Preserve FP32 before sharding
FSDPEngine._build_moduleuses a keep-aware cast for FSDP2. Matched floating-point parameters and buffers remain FP32, including all aliases of tied tensors. Other floating-point states are cast to the configured model dtype.The FSDP1 construction path is unchanged.
Isolate FP32 parameter units without changing activation boundaries
Each topmost eligible matched module is wrapped before its parent using:
The keep-aware build cast has already preserved the matched parameters in FP32. Leaving the child policy's
param_dtypeunset makes FSDP2 materialize/all-gather those parameters in their original FP32 dtype without inserting an FP32 activation conversion at the child boundary. Activation input/output and internal execution dtypes remain controlled by the caller's autocast context and the module's own casting logic. All other policy fields are retained.Reject unsafe or ambiguous layouts
A selected FP32 child absorbs its subtree from the standard wrap-target list to avoid duplicate wrapping. Containers without their own callable
forwardresolve to eligible callable descendants.The implementation raises a descriptive error before wrapping for layouts that cannot safely preserve the requested precision, including:
This fail-closed behavior prevents silent precision loss.
Make destination dtype authoritative during state-dict loading
fsdp2_load_full_state_dictaligns incoming parameters and persistent buffers with their destination dtypes before broadcast and distribution.The loader:
remove_duplicate=False.Validation Scope and Limitations
PreTrainedModel; no large FP32-keep checkpoint was downloaded or executed.Checklist Before Submitting
pre-commit run --all-files --show-diff-on-failure --color=always.tests/special_distributed/run_all.sh.ci-requestSlack channel when the PR is ready for CI, or use the Feishu group if Slack is unavailable.