Skip to content

[Bugfix] Invalidate KDA merged conv-weight cache on weight reload - #57638

Closed
mustafayildirim wants to merge 8 commits into
vllm-project:mainfrom
mustafayildirim:fix-kda-stale-merged-conv
Closed

mustafayildirim wants to merge 8 commits into
vllm-project:mainfrom
mustafayildirim:fix-kda-stale-merged-conv

Conversation

@mustafayildirim

Copy link
Copy Markdown
Contributor

Purpose

Fixes #55087. GLM-5.3 KDA merges the q/k/v short-conv weights into one cached tensor (_merged_conv_weight), built lazily on first forward. Online-RL workflows warm up with dummy weights (cache built), then refit q_conv1d/k_conv1d/v_conv1d — but the cached merge is never invalidated, so inference silently keeps using the pre-refit convolution weights.

Approach

Weight loading dispatches on the param's stamped weight_loader (getattr(param, "weight_loader", default_weight_loader) in AutoWeightsLoader). So the cache can be invalidated at exactly the point the weights change: in Glm5NextLinearAttention.__init__, wrap each conv weight's stamped loader so any write clears _merged_conv_weight. The next forward rebuilds it from the current params. No new hooks, no changes to the reload machinery.

Coverage / limitations

  • ✅ Initial load (loaders run before first forward; cache built after anyway)
  • ✅ Checkpoint-format refit: reload_weights → model.load_weights → stamped loaders → cache cleared
  • ✅ Layerwise reload: re-dispatches through the same stamped loaders
  • ❌ Kernel-format refit (param.copy_ bypass) writes params without loaders — the cache is NOT invalidated there. That path also skips every other derived-weight rebuild in the tree (same class of limitation); noted for follow-up if it matters in practice.

A _version-based approach was considered and rejected: the default loader writes via param.data.copy_, which bumps neither param._version nor param.data._version (verified on torch 2.10/ROCm), so version tracking cannot see the main refit path.

Verification

Ad-hoc functional test on MI350X against a real Glm5NextLinearAttention (gloo TP1, minimal Glm5NextConfig):

Step Result
Build layer, populate merge cache OK
Refit q_conv1d through its real stamped weight loader cache invalidated
Next merge uses refit values OK
No refit → cache persists (no spurious invalidation) OK

Pre-commit: all hooks pass. No GPU-unit test added (KDA layer requires a full VllmConfig + mamba cache plumbing; happy to add one if maintainers want it).

GLM-5.3 KDA lazily merges q/k/v conv weights into a single cached tensor.
Online-RL refits update q_conv1d/k_conv1d/v_conv1d through their weight
loaders but never invalidate the cache, so inference keeps serving the
pre-refit (e.g. dummy-warmup) merge.

Wrap each conv weight's stamped weight_loader so any write clears
_merged_conv_weight; it is rebuilt from the new params on the next
forward. Covers checkpoint-format refits (weight-loader path, including
layerwise reload which re-dispatches through the stamped loader).

Fixes vllm-project#55087

Signed-off-by: Mustafa YILDIRIM <mustafa@character.ai>

@claude claude Bot 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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added the bug Something isn't working label Sep 18, 2026
@mustafayildirim

Copy link
Copy Markdown
Contributor Author

Ping for review — fixes the #55087 stale-weights bug for GLM-5.3 KDA in online-RL refit loops. Verified on MI350X against a real Glm5NextLinearAttention: cache invalidates on any loader-mediated weight write, rebuilds from the new values, and no spurious invalidation on untouched layers. The gap (raw param.copy_ refits bypassing loaders) is documented in the PR.

@gdhyi @dllehr-amd

@mergify

mergify Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @mustafayildirim.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working needs-rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: GLM-5.3 KDA keeps stale merged convolution weights after refit

1 participant