Skip to content

Conversation

@qimcis
Copy link
Contributor

@qimcis qimcis commented Sep 10, 2025

Overview:

Added tests, docs, and runtime enforcement to keep the router’s radix tree consistent.

Details:

  • Router guardrails in lib/llm/src/kv_router/indexer.rs
  • Added reference vector unit test for canonical block hash.
  • vLLM components/backends/vllm/src/dynamo/vllm/args.py now logs guidance when prefix caching is enabled
  • Added sections on hashing consistency and determinism requirements in SGLang/TRT‑LLM docs
  • docs/guides/kv_events_hashing.md: Canonical hashing spec (xxh3‑64, seed=1337, LE), reference vector, engine config tips,enforcement.

Where should the reviewer start?

docs/guides/kv_events_hashing.md

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

Summary by CodeRabbit

  • New Features

    • Added configurable enforcement for KV hash stability via environment variable to escalate warnings to errors.
    • Added runtime guidance when enabling prefix caching, suggesting deterministic settings for stable KV block IDs.
  • Documentation

    • Introduced a “KV Events & Hashing” guide with deterministic hashing details, test vectors, and engine configuration tips.
    • Added hashing consistency sections to backend READMEs; updated docs navigation; minor formatting fix.
  • Tests

    • Added reference-vector tests validating canonical block-hash computation in both core and Python bindings.

@qimcis qimcis requested review from a team as code owners September 10, 2025 05:25
@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 10, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions
Copy link

👋 Hi qimcis! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions bot added the external-contribution Pull request is from an external contributor label Sep 10, 2025
@qimcis qimcis force-pushed the docs/consistent-hashing-for-kv-events branch from 690f1a8 to e9f13fa Compare September 10, 2025 05:30
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 10, 2025

Caution

Review failed

Failed to post review comments.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 905c920 and 690f1a8.

📒 Files selected for processing (8)
  • components/backends/sglang/README.md (2 hunks)
  • components/backends/trtllm/README.md (1 hunks)
  • components/backends/vllm/README.md (1 hunks)
  • components/backends/vllm/src/dynamo/vllm/args.py (1 hunks)
  • docs/guides/kv_events_hashing.md (1 hunks)
  • docs/index.rst (2 hunks)
  • lib/bindings/python/tests/test_kv_bindings.py (2 hunks)
  • lib/llm/src/kv_router/indexer.rs (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-29T00:02:35.018Z
Learnt from: alec-flowers
PR: ai-dynamo/dynamo#1181
File: lib/llm/src/kv_router/publisher.rs:379-425
Timestamp: 2025-05-29T00:02:35.018Z
Learning: In lib/llm/src/kv_router/publisher.rs, the functions `create_stored_blocks` and `create_stored_block_from_parts` are correctly implemented and not problematic duplications of existing functionality elsewhere in the codebase.

Applied to files:

  • lib/llm/src/kv_router/indexer.rs
🧬 Code graph analysis (2)
lib/bindings/python/tests/test_kv_bindings.py (3)
lib/bindings/python/src/dynamo/_core.pyi (1)
  • compute_block_hash_for_seq_py (340-352)
lib/bindings/python/rust/llm/kv.rs (1)
  • compute_block_hash_for_seq_py (37-44)
lib/llm/src/kv_router/indexer.rs (1)
  • test_block_hash_ref_vector (1190-1197)
lib/llm/src/kv_router/indexer.rs (1)
lib/bindings/python/tests/test_kv_bindings.py (1)
  • test_block_hash_ref_vector (288-293)
🪛 GitHub Actions: Pre Merge Validation of (ai-dynamo/dynamo/refs/pull/2981/merge) by qimcis.
lib/bindings/python/tests/test_kv_bindings.py

[error] 1-1: isort (pre-commit hook) failed: Import order adjusted in lib/bindings/python/tests/test_kv_bindings.py; file updated automatically. Please commit the changes.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build and Test - dynamo
  • GitHub Check: pre-merge-rust (lib/runtime/examples)
  • GitHub Check: pre-merge-rust (.)
  • GitHub Check: pre-merge-rust (lib/bindings/python)

Walkthrough

Adds deterministic hashing guidance for KV-aware routing across backends; introduces a KV Events & Hashing guide and docs index updates; augments vLLM args to log determinism tips; tightens KV router handling with env-controlled warning/error on missing blocks; and adds Rust/Python tests validating the canonical block-hash vector.

Changes

Cohort / File(s) Summary
Backend README updates
components/backends/sglang/README.md, components/backends/trtllm/README.md, components/backends/vllm/README.md
Added “Hashing Consistency for KV Events” sections with deterministic hashing guidance (e.g., PYTHONHASHSEED, stable seeds, deterministic block IDs); minor Markdown fix; vLLM README includes optional prefix-caching algo and duplicates the section twice.
vLLM args guidance
components/backends/vllm/src/dynamo/vllm/args.py
When prefix caching is enabled, logs a warning if PYTHONHASHSEED is unset/random and an info suggesting a deterministic prefix hashing algo if supported. No API changes.
KV hashing guide + docs index
docs/guides/kv_events_hashing.md, docs/index.rst
New guide detailing canonical router hashing (xxh3-64, seed=1337), engine hash stability, env var enforcement, and a test vector; docs index links fixed and new guide added to toctree.
KV Router enforcement & tests
lib/llm/src/kv_router/indexer.rs
Conditional logging (warning vs error) on missing parent/block based on DYN_KV_ENFORCE_ENGINE_HASH_STABILITY; early-return behavior retained; adds unit test for reference block-hash vector.
Python test for hash vector
lib/bindings/python/tests/test_kv_bindings.py
Adds test importing compute_block_hash_for_seq_py and asserting the known test vector result.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Router
  participant KVStore

  Client->>Router: KV Event (Stored / Removed)
  Router->>KVStore: Lookup parent/block by Engine Block ID
  alt Block found
    Router->>KVStore: Apply store/remove
    Router-->>Client: Ack
  else Block missing
    Note over Router: Check DYN_KV_ENFORCE_ENGINE_HASH_STABILITY
    alt Enforcement enabled ("1"/"true")
      Router->>Router: Log ERROR with hint
    else Enforcement disabled
      Router->>Router: Log WARNING with guidance
    end
    Router-->>Client: Skip mutation (return early)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “feat: Update docs to indicate need to use consistent hashing for KV events in backend engines” accurately describes the documentation updates but omits the related runtime enforcement code changes and new tests, so it only partially reflects the full scope of the PR. Despite this, it remains clearly tied to a significant aspect of the changeset.
Description Check ✅ Passed The description adheres to the repository’s template by providing an Overview, Details, reviewer start location, and issue reference, and it clearly conveys the PR’s purpose and changes. It could be improved by removing the stray empty bullet in the Details section and explicitly noting the docs/index.rst link updates for completeness.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.

Poem

I nibble bits, not clover leaves—
Hashes hum with xxh3’s breeze.
Seeds set steady, blocks align,
Warnings hop to errors fine.
Tests go thump: 1-2-3-4!
Determinism—open door.
Carrots cached, my radix sure. 🥕✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Signed-off-by: Keiven Chang <[email protected]>
Signed-off-by: Chi McIsaac <[email protected]>
Signed-off-by: Chi McIsaac <[email protected]>
@qimcis qimcis force-pushed the docs/consistent-hashing-for-kv-events branch from 06d64ce to 385040d Compare September 10, 2025 05:44
@PeaBrane
Copy link
Contributor

Can you also make some minor updates to docs/architecture/kv_cache_routing.md?

@qimcis qimcis force-pushed the docs/consistent-hashing-for-kv-events branch from dbc993b to c232989 Compare September 10, 2025 22:49
Signed-off-by: Chi McIsaac <[email protected]>
Signed-off-by: Chi McIsaac <[email protected]>
@qimcis qimcis requested a review from PeaBrane September 11, 2025 16:16
@qimcis
Copy link
Contributor Author

qimcis commented Sep 12, 2025

@PeaBrane The env var setting wouldn't work to the best of my knowledge, it should only affect python's built in hash() randomization. I've already removed mention of this in the docs for SGLang! Thanks for reviewing

@qimcis qimcis requested a review from PeaBrane September 16, 2025 13:55
@pull-request-size pull-request-size bot added size/S and removed size/M labels Sep 18, 2025
@PeaBrane PeaBrane enabled auto-merge (squash) September 18, 2025 19:46
@PeaBrane PeaBrane disabled auto-merge September 18, 2025 19:46
@PeaBrane PeaBrane enabled auto-merge (squash) September 18, 2025 19:46
@PeaBrane
Copy link
Contributor

/ok to test 52e76be

@PeaBrane PeaBrane merged commit 3b6dbef into ai-dynamo:main Sep 18, 2025
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor feat size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants