Skip to content

[fsdp, vllm] fix: drop redundant tied lm_head.weight from bucketed we… - #23

Closed
kahlun wants to merge 1 commit into
mainfrom
fix/weight-sync-tied-embedding-deadlock
Closed

kahlun wants to merge 1 commit into
mainfrom
fix/weight-sync-tied-embedding-deadlock

Conversation

@kahlun

@kahlun kahlun commented Sep 24, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

For a tied-embedding model, FSDPEngine's per-tensor weight-sync stream sends
lm_head.weight as a separate tensor even though it is byte-identical to
(aliases) model.embed_tokens.weight. The vLLM colocated rollout uses a
bucketed weight-transfer path: the sender packs tensors into fixed-size
buckets and flushes on overflow, and the receiver calls vLLM's
model.load_weights() once per bucket.

If the duplicate lm_head.weight lands in a different bucket than
model.embed_tokens.weight — likely for small, large-vocabulary tied models
where the embedding is a large fraction of the bucket size — that isolated
load_weights() call fails vLLM's tied-weight completeness check:

ValueError: 'lm_head.weight' was skipped because it is tied to
'model.embed_tokens.weight' in Qwen2ForCausalLM, but
'model.embed_tokens.weight' was not found in the checkpoint, so the tied
weight is uninitialized.

vLLM's AutoWeightsLoader never actually loads a tied lm_head.weight's bytes
— it only uses the name's presence to verify the embedding was loaded in the
same load_weights() call, then re-derives lm_head from the embedding.
Splitting the byte-identical duplicate into its own bucket is therefore pure
redundant risk. This PR drops lm_head.weight from the sync stream when
tie_word_embeddings=True, so the isolated bucket can never form.

Checklist Before Starting

  • Search for similar PRs. Queries (no duplicates found):
  • PR title formatted as [{modules}] {type}: {description} → [fsdp, vllm] fix: ...
  • No API break.

Symptom / motivation

On the bucketed colocated weight-sync path with a tied-embedding model, the
first weight update after a rollout fails the tied-weight completeness check
above. Whether this surfaces as a clean error or a hang depends on the ZMQ
receiver's error handling (see verl-project#7366): without that fix the sender can block
waiting for an ACK that never comes, producing a deterministic deadlock rather
than a fast failure.

The bug is vLLM-version-dependent, which is why it can go unnoticed:

vLLM Behavior Why
0.24.x Bug is masked Qwen2ForCausalLM.load_weights uses AutoWeightsLoader(self, skip_prefixes=["lm_head."] if tie_word_embeddings else None) — tied lm_head. is unconditionally skipped and no completeness check runs.
0.29.0 Bug reproduces skip_prefixes was dropped and a completeness check AutoWeightsLoader._check_skipped_aliases was (re)introduced in vllm/model_executor/models/utils.py, raising the exact ValueError above for an isolated tied-lm_head bucket.

Test

1. Unit test (CPU, added in this PR) —
tests/workers/test_fsdp_drop_tied_lm_head_on_cpu.py
covers: tied model drops lm_head.weight; untied model keeps it; tied model
without an lm_head.weight key is a no-op; and a config missing
tie_word_embeddings defaults to untied (keeps the key).

$ python -m pytest tests/workers/test_fsdp_drop_tied_lm_head_on_cpu.py -q
4 passed

2. Live vLLM 0.29.0 reproduction (the exact failure mechanism).
Using the stock vllm/vllm-openai:v0.29.0 image and a real tied
Qwen2ForCausalLM (Qwen2.5-0.5B-Instruct), I replayed the per-bucket
load_weights() call the bucketed receiver makes:

Scenario load_weights(...) input Result
A — unfixed isolated bucket [("lm_head.weight", w)] (embed in a prior bucket) RAISES the exact ValueError above — bug reproduced
B — with fix lm_head.weight dropped from stream; embed bucket [("model.embed_tokens.weight", w)] loads cleanly — no isolated tied bucket forms

3. Bucket-split condition confirmed in the real pipeline.
On a 4×A100, FSDP2 + colocated vLLM GRPO run (Qwen2.5-0.5B-Instruct, gsm8k),
instrumenting the bucketed sender confirmed that
model.embed_tokens.weight lands in the first bucket while the tied
lm_head.weight lands alone in the last bucket — organically, i.e. exactly
the isolated-bucket condition Scenario A raises on. With this fix applied,
2 GRPO steps complete end-to-end.

Note: on vLLM 0.24 the same 4-GPU run passes with and without the fix
(the loader masks the bug, per the table above), so end-to-end reproduction
requires vLLM 0.29+.

API and Usage Example

No API, config, or CLI change. The fix is transparent — tied-embedding models
simply no longer transmit the redundant lm_head.weight during weight sync.

Design & Code Changes

Single-file change in
verl/workers/engine/fsdp/transformer_impl.py:

  • New helper _drop_redundant_tied_lm_head(params, hf_config) pops
    lm_head.weight from the param dict in place when
    hf_config.tie_word_embeddings is truthy (defaults to untied / no-op if the
    attribute is absent, and a no-op if the key isn't present).
  • Called in both weight-emission paths after convert_weight_keys(...):
    1. the standard state_dict() path in get_per_tensor_param(), and
    2. the merged-LoRA generator path _merged_lora_per_tensor_param().
def _drop_redundant_tied_lm_head(params: dict, hf_config) -> None:
    """Drop lm_head.weight in place when it's tied to the input embedding."""
    if getattr(hf_config, "tie_word_embeddings", False):
        params.pop("lm_head.weight", None)

Checklist Before Submitting

  • Read the Contribute Guide.
  • Apply pre-commit checks: pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always
  • Documentation: n/a (no user-facing surface change).
  • Add unit or end-to-end test(s): CPU unit test added
    (tests/workers/test_fsdp_drop_tied_lm_head_on_cpu.py). The isolated-bucket
    integration failure requires a GPU + vLLM 0.29 and a colocated rollout, so
    it is not added to CI; the unit test covers the drop logic and the live
    repro is documented in the Test section above.
  • Send ci-request message when ready for CI.
  • recipe submodule: n/a.

…ight sync

get_per_tensor_param() sends lm_head.weight as a separate tensor even
though it's tied to (byte-identical to) model.embed_tokens.weight. The
bucketed weight-transfer sender packs tensors into fixed-size buckets and
flushes on overflow; for small, large-vocabulary tied-embedding models
(e.g. Qwen2.5-0.5B-Instruct) the embedding is a large fraction of the
bucket size, so the duplicate lm_head.weight can land alone in a later
bucket. The receiver calls vLLM's model.load_weights() once per bucket,
so that isolated call fails vLLM's tied-weight completeness check:

    ValueError: 'lm_head.weight' was skipped because it is tied to
    'model.embed_tokens.weight' in Qwen2ForCausalLM, but
    'model.embed_tokens.weight' was not found in the checkpoint, so the
    tied weight is uninitialized.

vLLM's AutoWeightsLoader never actually loads lm_head.weight's bytes for
a tied model -- it only uses the name's presence to check the embedding
was loaded in the same call, then re-derives lm_head from it. Sending it
is pure redundant risk, so drop it instead.

Verified on real B60 hardware: colocated-RM + FSDP2 2-D HSDP
(world_size=4) + transfer_queue.enable=True previously deadlocked
deterministically (separate, pre-existing ack-on-error gap in the ZMQ
receiver, tracked upstream in verl-project#7366); with that combination and this fix,
2 real GRPO steps complete end-to-end.

WIP test-mule commit for cross-machine testing; message/attribution to
be finalized before opening the actual PR.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The shared exporter may leave independent output heads stale for non-vLLM backends.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Removes redundant tied lm_head.weight tensors from FSDP synchronization to prevent bucketed vLLM loading failures.

Changes:

  • Filters tied weights in standard and merged-LoRA exports.
  • Adds CPU tests for tied, untied, and default configurations.
File Summary
verl/​workers/​engine/​fsdp/​transformer_impl.py Filters tied output weights during export.
tests/​workers/​test_fsdp_drop_tied_lm_head_on_cpu.py Tests the filtering behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +127 to +128
if getattr(hf_config, "tie_word_embeddings", False):
params.pop("lm_head.weight", None)
@kahlun

kahlun commented Sep 25, 2026

Copy link
Copy Markdown
Owner Author

invalid bug. fixed during verl update vllm to 0.29.0. bug happend due to forget to rebase or mege with main updates.

@kahlun kahlun closed this Sep 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants