Repository navigation
Conversation
|
👋 Hi surajssd! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
44fa4d9 to
6fede58
Compare
12650ea to
409eab8
Compare
409eab8 to
8206988
Compare
8206988 to
9d93f93
Compare
d051b1b to
304ec15
Compare
|
/ok to test 7a15c49 |
✅ Dynamo PR CI passed — run 38956099883 (attempt 1) on
|
7a15c49 to
7591229
Compare
7591229 to
ea629c6
Compare
External PR triage (2026-10-13)Blocker: trusted CI has not run on the current head Next action (maintainer): @jthomson04 (approved CI on 2026-10-07) or @ai-dynamo/dynamo-router-codeowners @ai-dynamo/dynamo-kv-memory-codeowners @ai-dynamo/dynamo-performance-codeowners — after review, a vetter comments Supply-chain audit (head Automated triage of external contributions; this comment is edited in place on each run. Written by Devin |
jthomson04
left a comment
There was a problem hiding this comment.
Thanks for splitting this out of #14965. Before it goes in, I'd like to trim it to what the Mooncake tier will actually read:
- Nothing in tree consumes
shared_cache_eligibleyet, but the cost is on by default.LocalKvIndexer::newenables provenance on every vLLM worker, because text-only stores decode as eligible. In a releaseRadixTreemicro-bench with 524k resident blocks, that's +63% heap (27.1 → 44.1 MiB) and +65% apply time per event. Please move the radix-tree provenance and theLocalKvIndexeropt-in into the PR that adds the consumer, or drop dump provenance and let replays be ineligible, as CRTC already does. - The frontend gate in
kv_router/indexer/mod.rsdoes nothing useful. The defaultrouter_event_threads=4uses CRTC, which ignores it, and your 9599bcf notes that no observer reads a frontend dump. Please drop it. ExtraKeyItem::Binarychanges GPU-path acceptance. On main, a vLLM batch with a non-UTF-8binextra key (the prompt-embeds sha256) fails to decode and is dropped; here it's indexed. vLLM reports[0]*ntoken IDs for those requests, so unrelated requests would now share local hashes. That needs its own decision and PR.- Add
skip_serializing_if = "std::ops::Not::not". False is already the serde default, and today every store event from every backend pays 23 bytes for the key (+12.6% on a 1-block store). - Fan-out: since false is fail-closed, forcing every struct literal to spell it out adds no safety. Putting the bit on
RouterEvent/PlacementEventnext tosession_id(default + skip + a builder, compared in the coalescer the waysession_idis) avoids about 50 files of churn. - Tests: deleting both
provenance.removecalls inapply_removedkeeps every test green. Please assert the map is empty after removes.
The rest checks out: the coalescer split, the dump eligibility runs, cleanup on clear/remove_worker, and wire compatibility in both directions.
ea629c6 to
42b9b23
Compare
42b9b23 to
560dfa2
Compare
jthomson04
left a comment
There was a problem hiding this comment.
Thanks for splitting this. Everything from my last review is covered: the flag now sits next to session_id with a default, skip-if-false and a builder; the coalescer compares it; tree dumps stay ineligible; and the provenance map moved to #16432 as opt-in. Perf: nothing in production sets it to true yet, so it adds no bytes on the wire and one bool compare in the coalescer. The ExtraKeyItem::Binary concern moved to #16431 unchanged; I'll follow up there. Three non-blocking notes inline.
|
/ok to test 6fa671d |
jthomson04
left a comment
There was a problem hiding this comment.
I'm not sure we need this bit, and this applies to 2b and 2c too. As I read 2b, sce means "tokens_hash covered every extra key vLLM hashed for this block". It's only false because the decoder drops keys it doesn't map: untagged salts, native MM hashes, and future keys.
Could the decoder fold those unmapped keys into that block's hash seed instead? No request could then reach an edge learned from such a store, which is the same effect as sce = false. That would remove the field and coalescer split here, shrink 2b to the fold, and drop 2c's provenance map, since dumps would already carry the right hashes. The request-side gate stays in the frontend.
It also stops the GPU tree crediting those stores to plain requests. That changes GPU routing, so it should be its own PR. Is there a case the bit catches that this wouldn't?
|
@surajssd are you still working on this? We would like to push on this feature. Thanks for the contribution! |
Preserve eligibility through coalescing, publication, and buffered recovery. Default absent metadata to ineligible and omit false wire values. Part of ai-dynamo#14842. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
Serialize the unreleased event metadata as sce while retaining the Rust field name, false default, and omission behavior. Keep publisher metadata assignments consistent and remove duplicate coalescing test coverage. Part of ai-dynamo#14842. Signed-off-by: Suraj Deshmukh <surajd.service@gmail.com>
6fa671d to
3ad23bf
Compare
Yep |
Overview
Part 2a of the shared-cache eligibility work for #14842. Preserve an explicit eligibility decision through event publication and buffered recovery so later consumers can use it.
Summary
Carry
shared_cache_eligibleonRouterEventandPlacementEvent. The wire key issce. Missing metadata defaults tofalse, serialization omitsfalsevalues, and a true value adds five bytes in named MessagePack.KvCacheStoreDatastays unchanged.Key Changes
The patch adds 39 nonblank production lines, excluding tests and comments. The full change is nine files, 156 additions, and 33 deletions.
Validation
After rebasing onto
mainatf01ff2b075, the macOS router library suite passes 1,232 tests, with three ignored. The LLM publisher suite passes all 127 tests with default features disabled.shared_cache_eligibility_defaults_false_and_roundtripsverifies legacy defaults, JSON and named MessagePack round trips, omission of false, and the five-byte cost of true.shared_cache_eligibility_survives_coalescing_publication_and_bufferingverifies same-value coalescing, separation of different values, and preservation through publication and the recovery buffer.Local strict Clippy did not pass: the installed Rust 1.99.1 reports warnings in unchanged code, including deprecated
fetch_updateandasync_traitmust-use diagnostics. The repository pins Rust 1.96.1. This rebase adds no feature changes.Issues and Ongoing Work
Pull Request Checklist
/pr-checklistskill.