Skip to content

Conversation

atalhens
Copy link
Contributor

@atalhens atalhens commented Oct 5, 2025

Purpose

Fix type inconsistency in kv_events_subscriber.py example by updating block hash type from BlockHash to ExternalBlockHash to match the main KV events implementation.

The example file was using BlockHash (which is NewType("BlockHash", bytes)) instead of ExternalBlockHash (which is Union[bytes, int]) in the event class definitions. This type mismatch could cause serialization/deserialization issues when the subscriber tries to decode events published by the main vLLM engine, which uses ExternalBlockHash for backward compatibility with both byte and integer hash representations.

Test Plan

  1. Run the KV events subscriber example to ensure it can properly decode events:

    python examples/online_serving/kv_events_subscriber.py
  2. Test with a vLLM server that has KV cache events enabled to verify end-to-end functionality:

    # Start vLLM server with KV events enabled
     vllm serve meta-llama/Llama-3.2-1B --enable-prefix-caching --kv-transfer-config '{"kv_connector":"OffloadingConnector", "kv_role":"kv_both", "kv_connector_extra_config":{ "num_cpu_blocks": 5000}}' --kv-events-config '{"enable_kv_cache_events": true, "publisher": "zmq", "topic": "kv-events"}'
    
    # In another terminal, run the subscriber
    python examples/online_serving/kv_events_subscriber.py
  3. Verify that the subscriber can properly decode both BlockStored and BlockRemoved events without type errors.

Test Result

The fix ensures type consistency between the main KV events implementation (vllm/distributed/kv_events.py) and the example subscriber. The ExternalBlockHash type is designed to handle both bytes and int representations for backward compatibility, which is essential for proper event serialization/deserialization across the KV events system.

Before:

image

After

Screenshot 2025-10-06 at 5 08 56 AM
Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue ([Bug]: KV events subscriber expecting BlockHash but receives ExternalBlockHash #26264)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

Signed-off-by: atalhens <sneh.lata@nutanix.com>
Copy link

github-actions bot commented Oct 5, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors.

You ask your reviewers to trigger select CI tests on top of fastcheck CI.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

If you have any questions, please reach out to us on Slack at https://slack.vllm.ai.

🚀

@mergify mergify bot added the documentation Improvements or additions to documentation label Oct 5, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly fixes a type inconsistency in the kv_events_subscriber.py example by updating the block hash type from BlockHash to ExternalBlockHash. This change aligns the example with the main KV events implementation, preventing potential serialization and deserialization issues. The changes are accurate, well-contained, and effectively address the described problem. The PR is ready to be merged.

Copy link
Collaborator

@heheda12345 heheda12345 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this problem!

@heheda12345 heheda12345 enabled auto-merge (squash) October 7, 2025 07:26
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 7, 2025
@heheda12345 heheda12345 merged commit 46b0779 into vllm-project:main Oct 7, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants