fix: report transferred bytes on the remote-deliver path - #9
Open
kangclzjc wants to merge 1 commit into
Open
Conversation
Every other tier records blocks and bytes as a pair through
`_KVCRCore._record_transfer`, which emits `TRANSFER_BLOCKS_METRIC` and
`TRANSFER_BYTES_METRIC` for the same scope. The peer path records its
counters individually instead, and the destination only ever recorded blocks.
`TRANSFER_BYTES_METRIC` was emitted from exactly one place, hardcoded to the
`source_write` scope.
So a completed peer transfer reported:
kvcr_transfer_bytes:('source_write',) = 51380224
kvcr_transfer_blocks:('source_write',) = 7
kvcr_transfer_blocks:('remote_deliver',) = 7
with no byte counter on the destination at all. That makes the verification
step in `docs/quick-start.md` impossible to carry out as written:
their block and byte counts must agree
Only the block half could be checked.
Record the delivered byte count next to the existing block count. The
destination's `dst_descriptors` are positionally aligned with `ordered_keys`
(both are populated together at the single construction site), so summing the
spans for the completed keys covers exactly the keys the block counter
reports.
The two sides count different things by design: the source reports the bytes
NIXL moved for the transfer, the destination reports the spans it committed.
They line up on a fully completed transfer, which is the case the quick-start
tells users to verify.
Extend the existing remote-target test, which already asserted the source
byte counter and the destination block counter, to cover the new counter.
Signed-off-by: Kang Zhang <kangz@nvidia.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Thank you for the PR, @aranadive could you please take a look? |
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.
Fixes #6
Every other tier records blocks and bytes as a pair through
_KVCRCore._record_transfer(core.py:748-762). The peer path records its counters individually, and the destination only ever recorded blocks —TRANSFER_BYTES_METRICwas emitted from exactly one place (remote_fw_dram.py:1412-1416), hardcoded to thesource_writescope, reachable only from_SourceWriteOp(:318).A completed peer transfer therefore reported:
with no byte counter on the destination, making the verification step in
docs/quick-start.md:260-262— "their block and byte counts must agree" — impossible to carry out.Why the destination cannot just reuse the source path
The transfer is a source-side push.
_SourceWriteOpholds atransfer_idand reads NIXL telemetry viaprogress.poll_transfer()._TargetPullOphas notransfer_id; it learns about completion from thewrite_donecontrol message (:143). It has no NIXL telemetry to report, so its only byte source is its own descriptors.Change
Record the delivered byte count next to the existing block count.
dst_descriptorsis positionally aligned withordered_keys— both are populated together at the single construction site (remote_fw_dram.py:477-489), and the wire path validateslen(keys) == len(dst_descriptors)(:848) — so summing the spans for the completed keys covers exactly the keys the block counter reports.This covers both destination scopes,
remote_deliverandremote_fetch(:105).End-to-end verification
Built the quick-start image with this change and ran a real cross-rank transfer between two vLLM data-parallel ranks:
The two sides report identical block and byte counts. The differing
connected_remotesvalues confirm these are two distinct KVCR instances rather than one instance reported twice.51,380,224 also checks out independently: it is exactly 7 x (2 x 28 layers x 8 KV heads x 128 head_dim x 2 bytes x 64 tokens), the KV size of 7 blocks for Qwen3-0.6B.
On partial completion
The source reports the bytes NIXL moved; the destination reports the spans it committed. Both derive from the same slice of the same descriptor list — the source submits
dst_descriptors[:completed_count](:256) and propagatescompleted_countin thewrite_donenotification (:261), which the destination uses to pickordered_keys[:completed_count](:149) — so they should agree on a partial completion too.That case is not exercised end to end here, only the fully completed one shown above. The unit test's fake NIXL agent returns a constant
totalBytes=32regardless of how much was transferred, so it cannot confirm the partial case either.Test
Extended
test_remote_framework_dram_transfers_available_prefix, which already asserted the source byte counter and the destination block counter, to cover the new counter across all three parametrizations.Verification
ruff check src testspasses.pytest tests/unitmatchesmain— 152 passed, same 15 pre-existing environmental failures (SO_PEERPIDFDneeds Linux 6.5+; CI runners are unaffected).🤖 Generated with Claude Code