Repository navigation
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Report the measured KV-cache transfer latency and byte count instead of a static estimate, falling back to the estimate when no measurement is available. Apply the replica factor only on the fallback path to avoid double-counting measured bytes for MLA. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dummy CP ranks transfer no KV and never bootstrap, so return the zero-default metric early to avoid a spurious warning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hunhokim
requested review from
ByronHsu,
HaiShaw,
ShangmingCai,
Ying1123,
fzyzcjy,
hnyls2002,
merrymercy,
sogalin,
sufeng-buaa and
xiezhq-hermann
as code owners
July 22, 2026 02:16
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Rephrase the regression-test comments to describe present-tense invariants and the mistake to avoid, instead of narrating past "buggy code" / "pre-guard code" states that are no longer visible in the tree. Also reword the lock-serialization assertion comment so the counterintuitive assertFalse reads clearly on its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
May I ask why this PR was closed? Was it replaced by a new PR? |
Contributor
Author
I had a private conversation with one of the maintainers on Slack and was told to work on the transfer engine side (e.g. Mooncake) for accurate metrics collection. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The current implementation of KV-transfer metrics can substantially over-estimate transfer speed whenever a transfer overlaps computation. For example, in a setup with Infiniband NDR NICs, KV transfer speed is observed to be well above 100 GB/s while the line rate is only 50 GB/s. This makes diagnostics on KV transfer unreliable.
Cause
This happens only when KV transfer occurs multiple times on a single request: the recorded byte count spans the whole transfer while the recorded latency covers only the last transfer, resulting in the transfer speed being inflated. Two specific examples:
The fix measures time and bytes together, on every actual transfer, so the ratio stays correct no matter how a transfer overlaps computation or splits across chunks.
Implementation
Common interfaces are introduced in
disaggregation/common/conn.pywith implementations provided for Mooncake.Inside
transfer_worker, each KV transfer is recorded into a per-request_TransferRecord. On completion,get_transfer_metric()pops the record and sources both latency and bytes from it.If no record exists, it falls back to the old index-derived byte count with
Nonelatency (unchanged behavior for backends that don't record, e.g. NIXL).Some points to consider:
prefill.pysnapshots the record onto the newReq.transfer_metricfield before the scheduler'sclear()drains it.get_kv_replica_factor(). Conflating them would reintroduce the MLA byte over-count.status_record_lockguards all sites that touch status and the record together (_record_transfer,update_status,clear()), preventing a late chunk from leaking a stale record into the next request reusing the samebootstrap_room.Unit tests
E2E tests
Environments are as follows:
All the values are from prometheus metrics.
Before the patch, transfer speeds show unrealistic values.
After the patch, transfer speeds show reasonable values.
Chunked prefill
prompt_tokens= 8192+1655 (chunked)Value = KV transfer speed (GiB/s)
Early-send of cached prefix
prompt_tokens= 6554Value = KV transfer speed (GiB/s)
CI States
Latest PR Test (Base): ⏳ Run #29968160092
Latest PR Test (Extra): ⏳ Run #29968159916