Fix FSDP2 handling for tied embeddings - #2059
Conversation
Signed-off-by: realAsma <akuriparambi@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
BB: this does not look like a ModelOpt side error to me. This is related to Huggingface and PyTorch. Can you check or search how Huggingface handles tied weights with FSDP2 and report here? |
|
🐝 I’m checking how Hugging Face handles tied weights with FSDP2 and will report back here. |
Checked upstream handling. This is not a ModelOpt-originated error:
Conclusion: the grouping in this PR matches PyTorch's required workaround, but it is compensating for an upstream Accelerate/FSDP2 integration gap. I recommend filing/fixing this in Accelerate; we should keep the ModelOpt workaround only if we explicitly want a compatibility bridge until an upstream release contains the fix. |
Confirmed: this is an upstream Hugging Face/PyTorch integration issue, not ModelOpt-owned behavior. Accelerate's FSDP2 preparation independently calls Hugging Face also recently merged native FSDP2 handling that rewrites the input-embedding/output-head plan when they are tied before applying Recommendation: report/fix the legacy Trainer→Accelerate FSDP2 path upstream. I would not merge a permanent ModelOpt-specific policy copy unless we deliberately want a version-gated compatibility workaround for affected HF releases. No code changes prepared in this pass. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2059 +/- ##
==========================================
- Coverage 67.00% 67.00% -0.01%
==========================================
Files 520 520
Lines 59545 59562 +17
==========================================
+ Hits 39900 39908 +8
- Misses 19645 19654 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What does this PR do?
Type of change: Bug fix
Groups distinct input and output embedding modules that share a weight into one FSDP2 parameter group before Hugging Face Trainer and Accelerate apply their normal auto-wrapping. This prevents PyTorch from rejecting the shared parameter when distillation prepares both the student and teacher models.
The path is limited to FSDP2 models with genuinely tied input/output weights; untied models and other distributed backends are unchanged.
Usage
No user-facing API change is required. Existing FSDP2 distillation commands work without additional configuration.
Testing
pytest_pwd tests/unit/torch/opt/plugins/test_transformers_fsdp.py tests/unit/torch/distill/plugins/test_huggingface_kd.py -q(6 passed)nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc22with PyTorch 2.12.0a0, Accelerate 1.14.0, and Transformers 5.5.4:Before your PR is "Ready for review"
CONTRIBUTING.md: N/AAdditional Information
The failure occurs before the first training step when FSDP2 separately wraps the two modules that own a tied embedding weight.