Skip to content

[Bugfix][Weight Cache] Fingerprint sampled tensor bytes, not headers only - #59648

Open
he-yufeng wants to merge 4 commits into
vllm-project:mainfrom
he-yufeng:fix/weight-cache-content-fingerprint
Open

he-yufeng wants to merge 4 commits into
vllm-project:mainfrom
he-yufeng:fix/weight-cache-content-fingerprint

Conversation

@he-yufeng

@he-yufeng he-yufeng commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Overview

Fixes #59647. hash_checkpoint() fingerprinted only safetensors headers (tensor names, dtypes, shapes, offsets), so checkpoints with identical layout but different weight values produced the same WeightCacheKey, and an ipc_cache engine could silently map a daemon's stale weights. The fingerprint now also covers each shard's file size and a 1 KiB sample of every tensor's data region.

Claims

  • A base model and a fine-tune (or any two checkpoints differing only in weight values) now produce different keys, so the daemon's stale cache is rejected and the engine falls back to disk loading, per the documented safe-fallback contract.
  • Byte-identical checkpoints in different directories still share a key, preserving the copy-invariance goal of Fast Start #54921.
  • Extra startup cost is bounded at one small read per tensor plus the header, a few hundred KB per shard, negligible next to loading the weights themselves.

Validation

Reproducer from the issue (two single-tensor checkpoints with identical headers, values 0.0 vs 1.0): keys collided before this change and differ after it. Added unit tests:

Independent end-to-end reproduction of the bug on the 0.30.0 release (fp16, WSL2) by @mitoyosh, with the control experiment that a layout-mismatched checkpoint still triggers the documented disk fallback: https://homelabpostmortem.com/2026/10/03/vllm-weight-cache-serves-another-checkpoints-weights/

  • test_hash_checkpoint_distinguishes_weight_values: same layout, different values must not share a key.
  • test_hash_checkpoint_stable_across_directory_copies: byte-identical copies in different directories share a key (tensor sized past the 1 KiB sample so sampling is exercised).
  • test_hash_checkpoint_without_safetensors_returns_none: unchanged fallback to the model path.

Commands run (macOS, CPU-only env, VLLM_TARGET_DEVICE=empty):

pytest tests/model_executor/model_loader/test_weight_cache.py -q -k "hash_checkpoint or weight_cache_key or daemon_rejects"
# 5 passed

pre-commit run --files vllm/model_executor/model_loader/weight_cache/protocol.py tests/model_executor/model_loader/test_weight_cache.py
# ruff check / ruff format / mypy all passed

End-to-end daemon + engine runs need a GPU, which I do not have locally; the GPU e2e coverage already lives in test_ipc_cache_cold_start_and_warm_restart in the same file, and the key-comparison path is exercised by test_weight_cache_key_distinguishes_dp_ranks.

Details

Root cause: _safetensors_header() hashed only the length-prefixed JSON header, which by construction contains no weight bytes. The header hash was introduced in #54921 to stop identifying checkpoints by path (so identical weights in different directories match), and CodeRabbit flagged the value-blind collision there at the time without a reply before merge.

Design: per shard, hash file size, the raw header, and the first 1 KiB of each tensor's data region (data_offsets from the header). Per-tensor sampling was chosen over whole-file head/tail sampling because a small fine-tune can leave both file ends untouched while changing one interior tensor. Sorting entries by name keeps the mix order deterministic.

Tradeoffs and limitations:

  • Existing daemon caches miss once after upgrade (their stored keys use the old scheme) and are repopulated via the normal disk fallback.
  • A value change confined to bytes past the first 1 KiB of every tensor would not move the key. This is a cache-correctness fingerprint, not a security boundary (the socket protocol already enforces same-user ownership); the header plus per-tensor heads cover the realistic accidental-mismatch cases the docs' "safe fallback" promise targets.

AI assistance (Kimi Code) was used to investigate the root cause, write the patch and tests, and run the validations above; the submitting human reviewed the final diff. Not duplicating existing work: the open PRs touching weight_cache/protocol.py (#55490, #56000, #56047, #56999, #59435) do not change hash_checkpoint, and no open PR references #59647.


Pull Request Checklist
  • I used vLLM's /pr-checklist skill. (Mandatory for agents, optional for humans).

  • AI assistance was used during the creation of this PR.

  • Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.

  • Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.

  • Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.

  • Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.

@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 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Documentation preview: https://vllm--59648.org.readthedocs.build/en/59648/

@mergify mergify Bot added documentation Improvements or additions to documentation bug Something isn't working labels Oct 1, 2026
@Isotr0py Isotr0py self-assigned this Oct 1, 2026
Comment on lines +255 to +268
header = f.read(header_len)
hasher.update(size_bytes)
hasher.update(header)
try:
entries = json.loads(header)
except json.JSONDecodeError:
return
data_start = 8 + header_len
for name in sorted(entries):
offsets = entries[name].get("data_offsets")
if not offsets:
continue
f.seek(data_start + offsets[0])
hasher.update(f.read(min(_TENSOR_HEAD_BYTES, offsets[1] - offsets[0])))

@Isotr0py Isotr0py Oct 3, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm wondering the cost of iterating and hashing each tensor entry for extremly large model like Kimi-K3. Is it possible to only hash fixed number of interleave chunks for each shard? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question, I sized this before picking the scheme. A K3-scale checkpoint is around 70k tensors across roughly 50 shards, so this works out to about 1.3k sampled reads of 1 KiB each per shard, roughly 1.3 MB total. On NVMe that is tens of milliseconds; on a spinning disk at a pessimistic 10 ms seek per read it is ~13 s worst case, against the minutes that same disk spends streaming the shard's tens of GB. It runs once per daemon load, so it is noise next to the load itself. Iterating the entries costs nothing extra since the header is already parsed.

On fixed interleaved chunks: I went per-tensor deliberately. The gap in #59647 is value-only divergence, and the realistic cases (a merged LoRA, a norm-only touch, an expert-targeted fine-tune) rewrite only some matrices. Fixed byte ranges over a ~40 GB shard can easily skip every changed region, which reproduces the exact silent-stale-weights failure this PR closes. Sampling the head of every tensor keeps the property that each tensor contributes to the key, so a value change anywhere flips it.

Honest limit either way: this is a sampled fingerprint, not a digest of every byte, so a change confined to the middle of one tensor slips through; catching that means reading everything, which defeats the preload. If seek count on slow media ever becomes a real concern, the bounded variant that keeps the guarantee is coalescing adjacent tensors into span reads (the data region lays tensors out contiguously), not dropping tensors from the key. Happy to do that here or as a follow-up if you prefer.

@Isotr0py Isotr0py Oct 5, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On NVMe that is tens of milliseconds; on a spinning disk at a pessimistic 10 ms seek per read it is ~13 s worst case, against the minutes that same disk spends streaming the shard's tens of GB.

Actually, besides local NVMe, models like K3 are more likely stored in NFS and shared across cluster nodes. So I think the bottleneck is the RPC in this scene instead. (For example, given 70K tensors and RTT=1ms, it will need 70s for sequential hashing with per-tensor seek+read)🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

NFS is exactly the case the worker pool is sized for. The fingerprint cost scales with tensor count, not model size: one 1 KiB pread per tensor, 32 in flight. A K3-scale checkpoint is about 70k tensors, so ~2.2k sequential rounds; at a 1 ms NFS RTT that is 2 to 3 seconds, and even at a congested 5 ms RTT it stays around 11 s, against the minutes NFS spends streaming the same checkpoint into memory. LAN NFS also batches better than this worst case, since 32 concurrent requests let the server coalesce.

The alternative of sampling only some tensors is where I would not go: the #59647 failure mode is a fine-tune that rewrites only part of the checkpoint, and any subsampling of tensors lets a value-only change slip through with probability proportional to the skip rate. Hashing every tensor head is what makes the key collision-free for that case. If the NFS cost still reads as too high, the knob I would rather turn is _FINGERPRINT_READ_WORKERS (64 workers halves the RTT-bound time) or a larger per-tensor read to catch mid-tensor edits, not dropping tensors from the hash.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

@Isotr0py You're right, NFS RTT is the correct cost model for cluster checkpoints, and sequential per-tensor reads do land at ~70 s for K3-scale there. Addressed in e87d982: the sampled reads now go through a small thread pool (32 in flight), so the same ~70k reads finish in a few seconds. The header parse is unchanged, and pool.map preserves order, so the digest is byte-identical to sequential sampling and keys computed by the previous head stay valid.

I kept per-tensor sampling rather than fixed interleave chunks for the reason from the earlier round: the divergences this key exists to catch (a merged LoRA, a norm-only touch, an expert-targeted fine-tune) rewrite only some matrices, and fixed byte ranges over a ~40 GB shard can skip every changed tensor. Concurrency removes the RPC wall time without giving up that property.

Added a test with 64 tensors where only one changes value, which also pins down that the digest doesn't depend on read scheduling. I verified sequential and concurrent digests match on a 200-tensor safetensors layout here; this machine has no torch, so the suite itself still needs a run where torch lives.

(AI assistance was used for the patch, disclosed per vLLM's AGENTS.md policy.)

@mergify

mergify Bot commented Oct 7, 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, @he-yufeng.

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

@mergify mergify Bot added the needs-rebase label Oct 7, 2026
@he-yufeng
he-yufeng force-pushed the fix/weight-cache-content-fingerprint branch from af36772 to 63d2e71 Compare October 8, 2026 01:32
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (8733a21) to clear the conflict mergify flagged. It was test-file only: upstream added test_weight_cache_key_distinguishes_nvfp4_activation_override at the same spot where this PR appends the hash_checkpoint tests, so both are kept now. protocol.py had no upstream drift since the branch point. Reran the unit slice on the rebased head: 7 passed (the three hash_checkpoint tests, the dp_ranks key test, and the upstream nvfp4 override test). 63d2e71 is pushed.

@mergify mergify Bot removed the needs-rebase label Oct 8, 2026

YingchaoX commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Adding measurements relevant to the network-filesystem latency discussion, and a question about an explicit opt-in to reuse an already selected preloaded instance.

Version boundary: these results are from vLLM 0.31.0, not this PR's branch. I have not tested the concurrent fingerprint-reading implementation in this PR.

Setup

  • Model: Qwen3.8-27B-FP8, local checkpoint at /mnt/models/Qwen3.8-27B-FP8. For precision, its config reports Qwen3_5ForConditionalGeneration / qwen3_5.
  • 66 safetensors shards, approximately 28.75 GiB, on OSSFS.
  • One NVIDIA H20-3e, TP=1; PyTorch 2.13.0, Python 3.12.
  • Text-only, max_model_len=4096, max_num_seqs=1, max_num_batched_tokens=512, fixed 2 GiB KV cache.
  • Both variants used --load-format ipc_cache, zero_copy, fallback=false, the same live daemon, and the same warm compilation caches. The GPU also hosted an existing service, which was left untouched; its completion counters did not change during the experiment.

Controlled A/B

A process-local import hook instrumented both variants without modifying the installed vLLM files:

  • A: execute the original hash_checkpoint() on each engine start.
  • B: return the fingerprint captured from this same live daemon's actual initial hash computation, skipping the repeated checkpoint scan. The experiment verified the daemon PID/start time before reuse. Other WeightCacheKey comparisons and GPU UUID checks remained enabled.

Runs were ordered A → B → B → A. Total startup is process launch to HTTP /health 200; daemon preparation is excluded from both variants.

Run Checkpoint fingerprint Model-loading stage Total startup
A1 32.374 s 38.042 s 79.492 s
B1 0.000120 s 5.565 s 47.253 s
B2 0.000106 s 5.623 s 46.452 s
A2 31.851 s 37.275 s 78.098 s

Mean startup: 78.79 s → 46.85 s, saving 31.94 s / 40.5%. In A, the instrumented header reads accounted for 32.319 s and 31.791 s, reading only 203,664 bytes across 66 files. The remaining startup overhead was essentially unchanged. These are nested timings: the fingerprint time is included in model loading.

All four engines logged successful zero-copy mapping of 980 tensors, and a short deterministic completion smoke test produced identical output. This is a small, environment-specific experiment, not a general performance or accuracy claim.

One detail relevant to the earlier discussion: in 0.31.0, the engine calls _fetch_entries() → WeightCacheKey.from_model_config() → hash_checkpoint(), so this cost is paid on each engine restart, not only when preparing the daemon.

Possible separate feature

Would an explicit opt-in to trust a selected preloaded instance be a reasonable separate feature proposal? An operator could select an immutable cache identity / daemon instance and accept its resident weights as authoritative, avoiding checkpoint-file rescans while retaining architecture, dtype, quantization, parallel-layout, version, and GPU compatibility checks. Source-checkpoint changes would then require an explicit new preload rather than being rediscovered on every restart.

The default validation behavior could remain unchanged. I am asking about a separate operator-controlled mode, not asking this correctness-fix PR to absorb that scope.

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Clean experiment, and the A → B → B → A ordering plus keeping the GPU UUID and other WeightCacheKey checks live in variant B makes the 31.9 s attribution convincing. 203 KB of header reads costing 32 s is pure latency-bound I/O, and your note that 0.31.0 pays it on every engine restart through _fetch_entries() is the part that matters operationally.

On the opt-in question: yes, I think that's a reasonable separate feature, and it should stay separate. The scope split is clean. This PR is about correctness: when the fingerprint is consulted, it must reflect the actual checkpoint bytes. Your proposal is an operator-controlled mode that deliberately trades exactly that detection away for startup time, with explicit re-preload as the invalidation path. Those two pull in opposite directions, and merging them would make both worse.

Design-wise, what you'd keep is the cheap half: architecture, dtype, quantization, parallel layout, version, and GPU compatibility are all config-derived comparisons, no file I/O, so an opt-in trust mode loses nothing by retaining them. What you'd skip is only the content hash. The risk boundary is that an in-place edit of the source checkpoint becomes invisible to the engine, so the mode needs to be loud: explicit flag, a startup log line stating which cache identity is being trusted and that source changes require a new preload. Operators on immutable model stores (which OSSFS deployments often are in practice) get your 40% back; everyone else keeps the default validation.

One honest caveat on how the two interact: this PR's concurrent fingerprint reader should cut your 32 s substantially on a latency-bound FS, since parallelism is exactly what hides per-request latency, but your B variant shows the floor is ~0, so for operators willing to own explicit invalidation, opt-in trust wins outright. Worth opening as its own issue so the semantics debate (flag naming, what "immutable identity" means, how preload invalidation is signaled) happens there rather than on a correctness PR. Happy to chime in once it's up.

Prepared with AI assistance (Kimi Code).

@he-yufeng

Copy link
Copy Markdown
Contributor Author

Rebased onto current main; the conflicts were adjacent additions from the RoPE-parameters fingerprint that landed since (upstream's hash_rope_parameters and test_ipc_rope_matching both kept, my sampled-bytes fingerprint and its tests sit alongside). No behavior change to the reviewed diff; the four fingerprint tests pass locally on this head, and neutering the sampled-byte mixing (header-only hashing, the pre-PR behavior) makes the two distinguishing tests fail again.

@liusy58 liusy58 mentioned this pull request Oct 20, 2026
13 of 34 tasks
@mergify

mergify Bot commented Oct 21, 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, @he-yufeng.

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

@mergify mergify Bot added the needs-rebase label Oct 21, 2026
he-yufeng and others added 2 commits October 21, 2026 20:08
…only

Fixes vllm-project#59647.

hash_checkpoint() hashed only each shard's safetensors header (names,
dtypes, shapes, offsets), so two checkpoints with the same layout but
different weight values, e.g. a base model and its fine-tune, produced
the same WeightCacheKey. An engine started with --load-format ipc_cache
against a daemon holding the other checkpoint then mapped the stale
weights with no warning, while the CLI and API reported the new path.

Mix the file size and the head (1 KiB) of every tensor's data region
into the per-shard hash. The key stays stable for byte-identical copies
in different directories (the vllm-project#54921 property), while any real
fine-tune moves it. Reads stay at a few hundred KB per shard, negligible
next to weight loading.

Co-authored-by: Kimi Code <kimi-code@moonshot.cn>
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
On NFS each sampled pread costs one RPC RTT, so sequential per-tensor
sampling of a K3-scale checkpoint (~70k tensors) takes over a minute at
1 ms RTT. Issue the sampled reads through a small thread pool (32 in
flight) and mix them in sorted-name order as before. pool.map preserves
order, so the digest is byte-identical to sequential sampling and keys
already in the wild stay valid; wall time drops to a few seconds.

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Co-authored-by: Kimi Code <kimi-code@moonshot.cn>
@he-yufeng
he-yufeng force-pushed the fix/weight-cache-content-fingerprint branch from bd0c1d5 to 80a015e Compare October 21, 2026 12:11
@mergify mergify Bot removed the needs-rebase label Oct 21, 2026
@mergify

mergify Bot commented Oct 25, 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, @he-yufeng.

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

@mergify mergify Bot added the needs-rebase label Oct 25, 2026
@he-yufeng

Copy link
Copy Markdown
Contributor Author

Merged current main (ed1a7307950a) and pushed ec29900cbb70. The only conflict was the import block: both upstream's time and this PR's ThreadPoolExecutor are required. I also tightened the preload documentation and fingerprint docstrings to state the actual coverage, the first 1 KiB of each tensor, rather than implying that every possible weight-value change is detected. The final diff has been reviewed line by line by the submitting human.

Validation on the final head: 44 fingerprint, cache-key, daemon-reject, and RoPE tests pass. The complete test_weight_cache.py run is 62 passed, 6 skipped, 1 failed. The failure is test_ipc_cache_cold_start_and_warm_restart_tp2_ep, which aborts during engine startup because this Mac exposes one allowed NUMA node while TP2 requires two CPU workers; it is not being reported as a green GPU or distributed run. pre-commit run --files passes for the implementation, tests, and documentation.

@mergify mergify Bot removed the needs-rebase label Oct 27, 2026
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
@he-yufeng
he-yufeng force-pushed the fix/weight-cache-content-fingerprint branch from ec29900 to c4bf23e Compare October 27, 2026 02:36
@he-yufeng

Copy link
Copy Markdown
Contributor Author

DCO follow-up: c4bf23eeba64 has the identical tree to ec29900cbb70; only the latest documentation commit message gained the required Signed-off-by trailer. No source or documentation content changed. The DCO check is now SUCCESS.

This branch has not been deployed

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

Labels

bug Something isn't working documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Fast Start WeightCacheKey hashes only safetensors headers, so an ipc_cache engine can silently map another checkpoint's weights

3 participants