Repository navigation
[Feat] hicache: Add KVCR storage backend - #45647
ziqifan617 wants to merge 16 commits into
Conversation
…points Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
… offsets Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Port the existing HiCache candidate onto upstream eb4b167. Preserve deferred host-pool registration and the rank-local backup capability gate. Source: codex/kvcr-hicache-enablement at ce7a2d19b5 and SGLang sgl-project#43635. Co-authored-by: Lin Hu <linhu@nvidia.com>
Bind salted storage pages to router hints. Confirm hybrid checkpoints through physical hinted-source delivery, retain destination-local claims through host staging, and release them on every exit path. Keep KVCR DRAM pools off THP and cover the claim lifecycle in tests. This keeps source selection with router hints and requires no additional KVCR source-inventory API. Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
df9bb0d to
0d8db33
Compare
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Reject control-port ranges that overflow on a non-owner TP rank before publishing the group's source endpoint. Keep the range calculation in the backend address resolver. Add one CPU regression test covering valid and overflowing TP2 groups at DP ranks 0 and 1. All five endpoint tests pass in an isolated CPU harness; normal package startup is unavailable locally without PyTorch. Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
nvpohanh
left a comment
There was a problem hiding this comment.
[by Claude Code] This review is done by agents. Please feel free to disagree if they do not make sense.
| """ | ||
| if ( | ||
| kv_derived_transfers | ||
| and getattr(self.storage_backend, "supports_combined_page_reads", False) |
There was a problem hiding this comment.
[by Claude Code] Severity: style | Confidence: High
Issue
This PR adds many getattr/hasattr calls for fields that always exist. HiCacheStorage now declares supports_combined_page_reads, supports_combined_page_writes, requires_lookup_confirmation, and publishes_kv_events. StorageOperation always sets storage_key_to_event_hash and kv_event_metadata. The base controller now defines release_unqueued_prefetch_claims. New getattr/hasattr calls are not allowed in SGLang.
Locations:
cache_controller.py: 1226, 1366hybrid_cache_controller.py: 1444, 1500, 1811buffer_mode/pipeline.py: 1105-1106unified_cache/storage_attachment.py: 208-209unified_radix_cache.py: 1983-1984, 2205, 2797, 3027, 3145, 3425-3428storage_key_binding.py: 26-27, 68, 71utils.py: 161kvcr_store.py: 66, 69, 515, 645
Fix
Read these attributes directly. Where storage_backend can be None, check it once, for example in one controller property. Type key as RadixKey and extra_info as HiCacheStorageExtraInfo. KVCR runs only on Linux, so use the mmap constants directly.
There was a problem hiding this comment.
[by Claude Code] The issue still exists in the latest commit b0506ad.
There was a problem hiding this comment.
[by Claude Code] The issue still exists in the latest commit 6587556.
| started = time.perf_counter() if self._metrics is not None else None | ||
| sequence = None | ||
| if is_set and self._align_storage_prefix: | ||
| # HiCache owns a different LRU. A reused parent may never be |
There was a problem hiding this comment.
[by Claude Code] Severity: style | Confidence: High
Issue
This 7-line comment is hard to read. The PR has more blocks like it: kvcr_store.py lines 395-397 and 724-728, and hybrid_cache_controller.py lines 822-825 and 1514-1516. Several of them repeat what the next lines do or give background instead of the rule. The repo comment style asks for one or two lines.
Fix
Keep one short line of rationale for each block. For example: "Refresh parent recency before and after deposit so KVCR evicts tails first." Move the longer design notes to storage/kvcr/README.md.
There was a problem hiding this comment.
[by Claude Code] The issue still exists in the latest commit b0506ad. The _align_storage_prefix comment is shorter but still four lines, and the blocks at kvcr_store.py lines 390-392 and 716-720 and hybrid_cache_controller.py lines 822-825 and 1514-1516 are unchanged.
There was a problem hiding this comment.
[by Claude Code] The issue still exists in the latest commit 6587556. The blocks at kvcr_store.py lines 390-392, 633-636 and 716-720 and hybrid_cache_controller.py lines 822-825 and 1514-1516 are still three to five lines each.
| digests[key.decode()] = digest.hexdigest() | ||
| return digests | ||
|
|
||
| def _io(self, transfers, *, is_set, extra_info): |
There was a problem hiding this comment.
[by Claude Code] Severity: style | Confidence: Medium
Issue
KVCRStore._io is about 225 lines long. Much of it is two inline logger.info(json.dumps(...)) diagnostic blocks (lines 668-705 and 745-786) with almost the same fields. perf_counter bookkeeping also runs through every path. This makes the read/write flow hard to follow, and the two log formats can drift apart.
Fix
Put the diagnostic record in one helper, for example _log_operation(stage, handle, blocks, result, timings). Move the shared-MLA owner fallback (lines 706-794) into its own method. Then _io only builds the descriptors, submits, and collects results.
There was a problem hiding this comment.
[by Claude Code] The issue still exists in the latest commit b0506ad.
There was a problem hiding this comment.
[by Claude Code] The issue still exists in the latest commit 6587556.
| from sglang.srt.observability.metrics_collector import StorageMetrics | ||
|
|
||
| self._metrics = StorageMetrics() | ||
| self._verify_transfer_bytes = self._extra.get("verify_transfer_bytes", False) |
There was a problem hiding this comment.
[by Claude Code] Severity: style | Confidence: Medium
Issue
The constructor repeats the same block seven times: read an option, check type(...) is not bool, and raise. It does this for publish_inventory, verify_transfer_bytes, align_storage_prefix, diagnostic_telemetry, combine_page_reads, combine_page_writes, and share_replicated_mla. test_kvcr_storage.py copies the pattern in four almost identical tests: test_alignment_config_rejects_truthy_non_booleans, test_diagnostic_config_rejects_truthy_non_booleans, test_combined_read_config_rejects_truthy_non_booleans, and test_combined_write_config_rejects_truthy_non_booleans.
Fix
Add a helper such as _bool_option(extra, name, default=False). Replace the four tests with one test that loops over the option names with subTest.
There was a problem hiding this comment.
Addressed in b0506ad: the seven options now share strict boolean validation with the same defaults and error messages. Added coverage across the options; retained the existing constructor-level tests for their integration coverage.
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
## Summary Add a public-source SGLang HiCache/KVCR quick start with a separate Dockerfile, alongside the vLLM guide and image. - Build KVCR from the local Docker build-context checkout, including local source changes, rather than fetching a separate KVCR revision. - Pin SGLang [#45647](sgl-project/sglang#45647) at `b0506add3eac49bbb2c303e83169ea19c907fb98` and Dynamo at `83ea37e849e883c27d7ed848e0f329961e1fb173`. - Run the state agent and publish KVCR-owned inventory for router-guided peer delivery; verify completed delivery, destination reuse, and matching output tokens. No KVCR production-code changes. - After this PR merges, the guide clones the default branch, so the KVCR build context includes changes already merged to main, including #148. ## Validation - Built the SGLang quick-start Dockerfile from KVCR PR checkout `d186752ce78092aa2bd0cc4c1d48d24a1c247354`, SGLang `b0506add3eac49bbb2c303e83169ea19c907fb98`, and Dynamo `83ea37e849e883c27d7ed848e0f329961e1fb173`. KVCR 0.2.0 was built from the checkout and the image's import/version checks passed. The unchanged Dockerfile was built with BuildKit; this was not a GPU inference test. - With the earlier SGLang `0d37a2bf4db3521a876d1b450a884b91c0e334c9` and KVCR `7b7743fe0104393ac56c6fe83ae58387268dc72b` source pair, a custom two-worker Qwen3-32B GPU test verified cross-node peer delivery, 115,968 restored tokens, and matching output IDs. This was not the guide-as-written Qwen3-0.6B test. - The earlier SGLang `4a083003f9393b758077afe9bbce3f6b1c64f790` and KVCR `7186e7c20ba6e4d71ac3aff507fac9bfd83e8a07` pair passed the public-source Qwen3-0.6B two-worker check: 1,984/2,048 peer-reused tokens, matching 16 output token IDs, and 31 blocks / 62 spans with no failed keys. - GPU end-to-end validation of the current `b0506add` SGLang pin together with the post-merge KVCR main containing #148 remains pending. Do not infer that validation from the successful image build or earlier source-pair runs. --------- Signed-off-by: Ziqi Fan <ziqif@nvidia.com>
Signed-off-by: Rudy Pei <yanrpei@gmail.com>
…anup [Fix] hicache: Release late checkpoint claims after cancellation
nvpohanh
left a comment
There was a problem hiding this comment.
[by Claude Code] This review is done by agents. Please feel free to disagree if they do not make sense.
| local_ok, local_error = False, True | ||
| accepted = torch.tensor( | ||
| [ | ||
| int(local_ok and not operation.is_terminated()), |
There was a problem hiding this comment.
[by Claude Code] Severity: perf | Confidence: Medium
Issue
This loop does not stop when the scheduler revokes the prefetch during confirmation. is_terminated() only sets accepted[0] to 0, so the loop moves to the next smaller boundary and calls confirm_lookup again. For KVCR, each call fetches the trailing Mamba/SWA checkpoint from the peer and takes a new claim, and each step adds one all-reduce. The prefetch query thread stays busy the whole time.
Example
I marked a Mamba operation with 8 legal boundaries as terminated and ran _sync_prefetch_hit_query on CPU. It called confirm_lookup 8 times (pages 8 down to 1) and then returned 0.
Fix
Add an int(not operation.is_terminated()) slot to accepted, and break when it reduces to 0. On a rank whose operation is already terminated, skip the local confirm_lookup call but still join the all-reduce.
KVCR now converts keys through encode/decode_to/from_framework and encode/decode_to/from_router. Storage keys are already KVCR keys; the router key is the page hash prefix shared by every namespace and pool, so it has no inverse.
Motivation
Add KV Cache Runner (KVCR) as an optional L3 storage backend for HiCache. This enables local DRAM caching and router-guided KV reuse between workers through SGLang's existing host-staged storage path.
Modifications
--hicache-storage-backend kvcrwith storage lookup, backup and restore operations.Build and usage guide: KVCR #114.
Accuracy Tests
Speed Tests and Profiling
Not benchmarked. Validation covers same-host functionality, not performance, cross-host RDMA, hybrid models or disaggregated P/D.
Checklist
CI States
Latest PR Test (Base): ❌ Missing
run-cilabel -- add it to run CI tests.Latest PR Test (Extra): ❌ Blocked --
run-ciis required first.Latest PR Test (AMD ROCm 10): ➖ No AMD PR run found for this commit.
Tip
Base CI only covers the core cases, and its workload may not exercise your change. Use
/rerun-testto run the tests your change touches, or copy this prompt for an agent: