Repository navigation
Conversation
|
👋 Hi surajssd! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
55a9699 to
f3765ff
Compare
244bb72 to
e56de77
Compare
e56de77 to
7709c48
Compare
7709c48 to
97dd70e
Compare
|
@dmitry-tokarev-nv PTAL :-) |
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at 7d9d629ab. I found no new defect.
My earlier approval named e5c2714ad6, which is no longer the head. The author force-pushed six times after it. I read the new tree as a new change rather than as a delta. All three of my earlier P2 items are fixed, and I ran code on each one.
Part of this round is self-review. I gave the direction for the batching.rs fix on the CodeRabbit thread, and 7c37a65b82 follows it. I mutation-tested that line on its own and posted the result there.
What changed since the approved tree, by patch identity
I ran git patch-id --stable over every commit on both trees. The first nine commits carry the same content as the tree I approved. Only their commit identities moved, because of the rebase. Five commits are new:
| commit | subject | what it answers |
|---|---|---|
88c8c072ae |
evict Mooncake Store identity instead of disabling hints | my P2 on the learned-edge and digest caps |
13f831d95f |
record shared-cache provenance only where a dump consumer needs it | my P2 on the radix-tree map |
7c37a65b82 |
keep mixed shared-cache eligibility in separate store events | the CodeRabbit thread on batching.rs |
58a144cad7 |
read Mooncake runtime metadata through runtime_data |
my P2 on the failing Python test |
7d9d629aba |
encode injected consolidator batches as named events | a test helper that the new wire field broke |
Those five commits touch nine files, 381 added and 125 removed lines.
The merge base also moved, from d07fcea4b8 to a67b954e74, which is 43 commits of main. Eighteen of the files this pull request touches also changed in that range. I merged the current main tip b40d169e0ad myself with git merge-tree --write-tree. It produced tree d33a2c7ecb with no conflict. Only one file this pull request touches also changed between a67b954e74 and that tip: docs/fern/pages/reference/components/frontend-configuration.mdx. No conclusion here rests on that file.
What I ran at this head, and what I did not test
On x86_64 Linux with the pinned toolchain 1.96.1:
| command | result |
|---|---|
cargo test -p dynamo-kv-router --lib |
1073 passed, 0 failed, 2 ignored |
cargo test -p dynamo-llm --no-default-features --lib kv_router:: |
411 passed, 0 failed, 1 ignored |
cargo test -p kvbm-consolidator |
51 passed, 0 failed, 1 ignored, across nine binaries |
I reverted lib/kvbm-consolidator/tests/common/mod.rs to the state before 7d9d629aba and ran it again. The dedup binary then failed 3 of 3 with invalid type: boolean false, expected u64. So that commit carries real weight, and the tests pin it.
I ran the three CODEOWNERS gates from .github/workflows/codeowners.yml on the checked-out tree. The strict full-tree gate exited 0. emit_codeowners.py rewrote CODEOWNERS and CONTRIBUTORS.md, both checksums were unchanged, and git diff --exit-code exited 0. So the generated file matches its source. The matcher unit tests failed 16 of 157 on my machine. They fail the same 16 on the base commit a67b954e74. Each failure is a git commit that exits 128 in a temporary repository. That is my environment, not this change.
Every name the new documentation uses exists in the code. SharedCacheType::MooncakeStore carries #[serde(rename = "mooncake-store")] at lib/kv-router/src/scheduling/config.rs:392. The Python --shared-cache-type choices are ["none", "hicache", "mooncake-store"]. DYN_MOONCAKE_KV_EVENTS_ENDPOINT is read at lib/llm/src/discovery/model_manager.rs:2442, and MooncakeStoreWorkerExtension is at components/src/dynamo/vllm/mooncake_store_runtime.py:395. Every new Python test file carries pytestmark with unit or integration, plus vllm, gpu_0 and pre_merge.
I did not test these three:
- Live Mooncake transfer, GPU acceptance, and the four initialized-connector cases. Those need a GPU and the pinned vLLM build.
- The full
components/src/dynamo/vllm/tests/test_runtime_metadata.pyrun. My available bindings are older than this branch. I measured the accessor contract it depends on instead, and posted that on the thread. generate_mooncake_oracle.py --check. It needs the pinned vLLM source tree. Nothing in the repository runs it, sooracle.jsoncan only go stale against vLLM, not against the Rust reader. The greenpinned_coordinator_oracle_through_normalized_local_indexerstest covers the reader.
One hypothesis I killed before it became a finding
ExtraKeyItem gained a Binary variant, and it is first in an untagged enum. I expected a MessagePack text value to match it before Hash(String). If that happens, extra_keys_are_text_only rejects every request that carries a cache salt or a LoRA name, and the hint never fires.
It does not happen. shared_cache_eligibility_requires_known_text_keys_in_maps_and_tuples runs 27 cases through both the named-map and the positional encodings. Six of them expect eligibility to be true with text extra keys. Those cases pass in the 1073-test run, so text matches Hash. Retracted.
I also read the positional decoder. lib/kv-router/src/zmq_wire/deserialize.rs never reads shared_cache_eligible from a positional payload. It computes the value from lora_id, is_eagle, block_mm_infos and extra_keys. So the new field cannot shift the slots, and a real vLLM producer that does not know the field is unaffected.
Retain positive provenance before event normalization and through publisher replay. Keep legacy and unsupported metadata usable for GPU routing without authorizing shared-cache hints. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Expose resolved connector facts through an explicit worker extension and a bounded named RPC. Validate the pinned source and rank coverage without changing serving when metadata is unavailable. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Add opt-in routing hints that require every object at a proven hybrid-cache restart endpoint. Scope contracts to admitted workers and bind event tasks to each router lifecycle. Preserve GPU routing, HiCache behavior, and cached-token accounting when hints are unavailable. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Use a weak observer so failed router construction releases the scheduler and cache state. Preserve admission, progress, and completion callbacks while the lease manager is alive. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Explain explicit activation, pinned dependencies, supported layouts, and best-effort fallback. Separate validated offline behavior from unverified initialized-connector and GPU acceptance. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Match isort 5.12.0 and Black 23.1.0 in the Mooncake worker configuration and tests so the pre-commit job passes without changing Python semantics. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Record the rustc-hash dependency of dynamo-llm in the standalone KVBM bindings workspace. This lets the locked metadata check proceed without changing resolved package versions. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Box corpus events to satisfy the large-enum-variant Clippy check. Move each event out of its box before timed dispatch, and keep replay test consumers cloning the underlying event. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Encode the outer batch tuple with named event fields so absent optional fields cannot shift the shared-cache eligibility flag into a numeric slot. Decode the fixture before sending and assert that false eligibility survives. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
The learned-edge and digest maps only grew, so the hint switched itself off for good once either reached its cap, and re-publishing the same contract did not bring it back. Keep learned edges in least-recently-used order and evict the coldest link at the cap. Keep a digest identity only while one of its objects is resident, and clear it together with residency. Reaching a residency cap now discards the evidence and rebuilds it from later events instead of disabling the hint. A conflicting external hash marks the known edge and refuses the new one; the cache is disabled only when its event subscriber stops. Re-measured at the caps on aarch64: 100.5 MiB live and 119 MiB during growth. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
…umer needs it The radix tree kept one provenance entry per cached block per worker on every router, including the default shared-cache-type none, because the ZMQ decoder reads an absent shared_cache_eligible as eligible. Make the map opt-in through KvIndexerBuilder. The frontend enables it only when a shared cache is configured. The worker-side LocalKvIndexer always enables it, because its TreeDump is the production consumer and the worker cannot know the router configuration; that map holds only its own blocks. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
… events Coalescing an eligible store with an ineligible one collapsed the merged event to ineligible, so the eligible prefix lost its shared-cache provenance and could no longer produce a reuse hint after GPU eviction. Treat eligibility as part of store compatibility so mixed chains flush as two events, and drop the eligibility accumulation from merge_data. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
The Python get_engine_specific only decodes string values, and both the Mooncake descriptor and the generate capability hold a JSON object or boolean, so all three cases raised "invalid type ... expected a string". Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
The integration test helper serialized RawKvEvent batches with the positional msgpack encoder. The positional decoder follows the vLLM field order, and the derived serializer skips absent optionals, so the two only lined up while BlockStored ended at block_size. Adding shared_cache_eligible put a boolean in the lora_id slot and failed eleven tests with "invalid type: boolean, expected u64". Encode the outer batch as a tuple and struct-like events as named maps, the same way both production publishers already do. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
|
/ok to test 3c74eeb |
The full-CI mypy job reported twelve errors in mooncake_store_runtime.py. The pre-merge hooks do not run mypy, so the file was never type-checked before the first full-CI run. Annotate the inferred collections, declare the vllm_config attribute the worker mixin relies on, and replace three _require calls that mypy cannot narrow with equivalent explicit raises. No behavior changes. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Approved at ff1b4ac9df. I found no new defect.
My last approval named 7d9d629ab, and the head no longer contains that commit. The author rebased onto 4d60b59662 and added one commit. I compared the two trees commit by commit and file by file, and I ran code on each change.
What changed since the approved tree.
git merge-base --is-ancestor fails in both directions, so this push rewrote history. git patch-id --stable matches 10 of the 14 old commits. The other four changed only to follow main. Across the 108 files, the added and removed lines of this PR are identical in 103. Five files differ:
| file | change |
|---|---|
lib/llm/src/discovery/model_manager.rs |
The Mooncake Store arm now calls the new KvEventSourceRequirement::derive(role, config, policy.inputs()) from main. |
lib/llm/src/kv_router.rs |
The shared-cache test router uses the Aggregated role and a membership watch. The builtin policy does not declare CACHE for a plain decode router. |
lib/llm/src/kv_router/indexer/ingress.rs |
An import merge. |
docs/fern/pages/reference/components/frontend-configuration.mdx |
The --load-aware note now follows the new text from main. |
components/src/dynamo/vllm/mooncake_store_runtime.py |
The new mypy commit ff1b4ac9df. |
A probe of the rebased gate.
I added a throwaway probe test to lib/llm/src/kv_router.rs. I ran it on x86_64 Linux at ff1b4ac9df. The probe used a mooncake-store configuration that passes validation. In all 40 cases of role, conditional disaggregation, and declared inputs, should_subscribe() equals the CACHE declaration of the policy. So if the policy declares CACHE and the model does not use Eagle, the arm builds the cache. As a control, the two configurations that validation rejects (use_remote_indexer=true, use_kv_events=false) break that equality.
With the builtin policy, the rebased gate matches the subscription rule of the approved tree for prefill, aggregated, and decode routers. This is true with and without conditional disaggregation.
Before the probe, cargo test -p dynamo-llm --no-default-features --lib -- kv_router:: discovery:: gave 655 passed, 0 failed, 1 ignored.
The mypy commit.
I ran the pinned mypy==1.18.2 on mooncake_store_runtime.py with the MYPYPATH of the dynamo-runtime / mypy job. On the parent 3c74eeb864, it reports the same 12 errors as that job, line for line. On ff1b4ac9df, it reports none. The second step of that job, mypy -p dynamo, did not run, because the first step failed. It is clean on the base and on the head, and a planted stub error makes it fail.
The commit message says "No behavior changes". I loaded the module from both commits and ran the rewritten branches. _require raises UnsupportedContract with the same message, so two of the three rewrites act the same. The third changes one case. If vllm.__file__ is None, the worker reply changes from {'unsupported': 'TypeError'} to {'unsupported': 'vLLM package has no source location'}. In both cases, publish_mooncake_store_runtime returns False and publishes nothing. Only the text of its warning differs.
The new vllm_config: Any annotation adds no name to dir(). vLLM reads dir() to find conflicts with a worker extension. test_mooncake_store_runtime.py passes 40 of 40 on both commits.
CI, and a merge with the current main.
The Pre Merge job rust-tests (.) ran at this head, on Merge ff1b4ac9df into cde7c9a8c5. It printed 169 test result lines, and none failed. The tests of this PR appear by name, for example 19 in vllm_mooncake_store::tests and 15 in model_manager::mooncake_tests. The full PR workflow last ran on 3c74eeb864, not on this head.
I merged main at 7472c23abf with git merge-tree --write-tree, and it merged without conflict. The three lock files at that tip are identical to those at cde7c9a8c5, which Pre Merge built with --locked. On the merged tree, emit_codeowners.py rewrites CODEOWNERS with no change, and the strict full-tree gate passes. When I removed the shared_cache/ entry of this PR from areas.yaml, the drift gate failed. So the gate can fail.
What I did not test.
The limits from my last approval still apply. I ran no live Mooncake transfer and no GPU acceptance. The four initialized-connector cases need the pinned vLLM build, and they skip in the vllm-runtime lane.
The batching fix e30c60dd39 follows the direction that I gave on the CodeRabbit thread, so that part is self-review. Its patch is identical to the patch that I approved.
Overview:
Summary
Add experimental, opt-in
--shared-cache-type mooncake-storerouting for vLLM'sMooncakeStoreConnector. The router can credit reusable prefixes held in the shared store after GPU eviction, rather than treating every GPU miss as cold. Hints require validated worker metadata, eligible request and event provenance, and evidence that every required cache object is present.Details:
Routing and residency
Storedevents before radix-tree insertion can reject an unknown parent. Reuse the precomputed request hashes rather than reproducing vLLM's Python hashing protocol.--shared-cache-typeis notnone, and the worker-side local indexer always enables it because its tree dump is the consumer and it cannot see router configuration. The wire decoders keep the distinction between MessagePack binary and text values.Worker contracts and lifecycle
dynamo_mooncake_store_descriptorRPC. Export initialized connector facts throughModelRuntimeConfig.set_engine_specific, validate rank coverage and descriptor agreement, and support composition with the existing FPM worker extension.1085b64425a9e6f5ca52876ad32e55fda5665f4e. Require one data-parallel rank per endpoint, prefill context parallelism (PCP) of one, and decode context parallelism (DCP) of one. Tensor and pipeline parallelism use the exact exported object prefixes.WorkerInputs::CACHEand the KV event subscription is required, following the builtin-policy refactor onmain.Validation
Validation ran on the complete implementation at the current head, not on each intermediate commit.
cargo test -p dynamo-kv-routercargo test -p kvbm-consolidator --no-fail-fastcargo test -p dynamo-llm --no-default-features --libdisabled=false, hits preserved; the cache no longer switches itself offtest_mooncake_store_runtime.py,test_mooncake_store_initialized.py,test_runtime_metadata.py) against the native bindingmaturin develop, macOS,dynamo-llmdefault features off)cargo fmt --check,cargo clippy --testsondynamo-kv-routeranddynamo-llm,pre-commit runon changed filesrust-testsandrust-clippyon all four matrix directories and the docs, Fern, codeowners, and DCO checks. Full CI (PRworkflow) was approved at this head and is in progress; itsPreview or publish docsjob failed on a Fern asset-upload error covering fonts and images this PR does not touch (the same failure hit another PR the previous day and passed on rerun).The
kvbm-consolidatorintegration harness now encodes injected events as named maps, the same way both production publishers do. The positional encoder only lined up with the positional decoder by accident, and adding the trailingshared_cache_eligiblefield toBlockStoredexposed that.Acceptance remains unverified for:
torchand the exact pinned vLLM installation.The Fern CLI was not run locally; the Fern configuration and broken-link checks pass in CI. The whole Rust workspace test suite runs in the
rust-tests (.)job and passes. The whole Python suite was not run locally.Measurements
The three-group, tensor-parallel-two debug benchmark averaged 13.19 ms for 4,096 blocks and 56.12 ms for 16,384 blocks over 32 runs per size, up from 8.42 ms and 39.28 ms before learned edges moved to a least-recently-used map. Each walked link is now promoted in the LRU order, and the lookup takes the state lock mutably to do so. These are not release-profile latency results. Measured backing-map allocations at the fixed caps were approximately 100.5 MiB live and 119 MiB during growth on aarch64 (rustc 1.96), of which the LRU edge map accounts for about 2 MiB over a plain hash map; these figures exclude other runtime state and are not process RSS.
Where should the reviewer start?
lib/llm/src/kv_router/shared_cache/mooncake_store_contract.rsandlib/llm/src/kv_router/shared_cache/vllm_mooncake_store.rsdefine supported layouts, contract validation, residency, and reusable-prefix lookup.components/src/dynamo/vllm/mooncake_store_runtime.pydefines the pinned worker contract and export path.components/src/dynamo/vllm/tests/generate_mooncake_oracle.pysupplies the offline coordinator reference cases.lib/llm/src/discovery/model_manager.rsandlib/llm/src/kv_router/indexer/mod.rsintegrate admitted membership and primary event observation.lib/kv-router/src/services/selection/core/run.rsissues the request-timecheck_blocksquery, with eligibility threaded fromlib/llm/src/kv_router/routing_host/kv_selection.rs.lib/kv-router/src/zmq_wire/tests.rs,lib/llm/src/kv_router/shared_cache/vllm_mooncake_store/tests.rs, andlib/llm/src/discovery/model_manager/mooncake_tests.rscover provenance, hybrid readiness, contract replacement, and lifetime regressions.Related Issues
🔗 This PR is linked to an issue:
DCP4deployment is outside the current scope, which requiresDCP=1.Summary by CodeRabbit
New Features
mooncake-storethrough router and frontend settings.Bug Fixes
Documentation
Tests