fix: Renamed event publisher classes and configuration #1273
Conversation
alec-flowers
requested review from
a team,
GuanLuo,
PeaBrane,
biswapanda,
grahamking,
ishandhanani,
jthomson04,
kkranen,
nnshah1,
oandreeva-nv,
paulhendricks,
piotrm-nvidia,
ptarasiewiczNV,
rmccorm4,
ryanolson,
tanmayv25,
tedzhouhk and
tmonty12
as code owners
WalkthroughThis change set systematically renames the ZeroMQ event publisher classes and configuration across Rust, Python bindings, and usage sites, replacing Changes
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
launch/dynamo-run/src/subprocess/vllm_v1_inc.py(3 hunks)lib/bindings/python/rust/lib.rs(1 hunks)lib/bindings/python/rust/llm/kv.rs(3 hunks)lib/bindings/python/src/dynamo/_core.pyi(3 hunks)lib/bindings/python/src/dynamo/llm/__init__.py(1 hunks)lib/llm/src/kv_router/protocols.rs(1 hunks)lib/llm/src/kv_router/publisher.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/bindings/python/src/dynamo/llm/__init__.py (1)
lib/bindings/python/rust/lib.rs (1)
_core(39-80)
launch/dynamo-run/src/subprocess/vllm_v1_inc.py (3)
lib/bindings/python/rust/lib.rs (1)
lease_id(472-478)lib/llm/src/kv_router/publisher.rs (1)
kv_block_size(66-68)lib/llm/src/kv_router.rs (1)
block_size(154-156)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build and Test - vllm
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (.)
🔇 Additional comments (12)
lib/bindings/python/src/dynamo/llm/__init__.py (1)
35-36:✅ Verification successful
LGTM! Improved naming consistency with ZMQ prefix.
The renaming from
KvEventPublisherFromZmq/KvEventPublisherFromZmqConfigtoZmqKvEventPublisher/ZmqKvEventPublisherConfigfollows better naming conventions by placing the technology prefix first. This aligns with the broader renaming effort across the codebase.Let me verify that all references to the old class names have been updated consistently across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify all references to old ZMQ event publisher names have been updated # Search for any remaining references to old names echo "Searching for old class names that should have been renamed..." rg -i "KvEventPublisherFromZmq" --type py --type rust echo "Searching for old config names that should have been renamed..." rg -i "KvEventPublisherFromZmqConfig" --type py --type rust # Verify new names are used correctly echo "Verifying new class names are imported/used correctly..." rg -A 2 -B 2 "ZmqKvEventPublisher" --type py --type rustLength of output: 5012
ZMQ Prefix Renaming Verified and Complete
No usages of the old
KvEventPublisherFromZmqorKvEventPublisherFromZmqConfigremain, and the newZmqKvEventPublisher/ZmqKvEventPublisherConfigare correctly referenced across the codebase:
- Python binding:
lib/bindings/python/src/dynamo/llm/__init__.py- Rust-Python interop:
lib/bindings/python/rust/lib.rs- Subprocess script:
launch/dynamo-run/src/subprocess/vllm_v1_inc.py- Core implementation:
lib/llm/src/kv_router/publisher.rsNo further action required.
lib/llm/src/kv_router/publisher.rs (2)
95-95: LGTM! Struct renaming improves naming consistency.The struct name change from
KvEventPublisherFromZmqtoZmqKvEventPublisherfollows better naming conventions and is consistent with the broader renaming effort across the codebase.
103-103: LGTM! Implementation block correctly updated.The implementation block correctly reflects the struct name change. No functional changes are introduced.
lib/llm/src/kv_router/protocols.rs (1)
44-46: LGTM! Excellent documentation enhancement.The expanded comment provides valuable context by:
- Adding a reference URL to SGLang's blog post explaining data parallelism concepts
- Clarifying why data parallel rank tracking is needed (semi-independent ranks)
- Improving the field description from "backwards compatible" to more precise "Optional for backwards compatibility"
This enhancement improves code maintainability and developer understanding.
lib/bindings/python/rust/lib.rs (1)
64-65: LGTM! Python binding registration correctly updated.The module registration correctly reflects the renamed Rust structs (
ZmqKvEventPublisherandZmqKvEventPublisherConfig), ensuring consistency between the Rust implementation and Python interface. This aligns with the import changes in the Python__init__.pyfile.launch/dynamo-run/src/subprocess/vllm_v1_inc.py (2)
8-11: Update setup instructions to align with vLLM 0.9.0 compatibilityThe comments now correctly reference v0.9.0 instead of a commit hash, which improves clarity and maintainability.
38-39: Consistent renaming of ZeroMQ event publisher importsThe imports for the renamed classes
ZmqKvEventPublisherandZmqKvEventPublisherConfigreplace the oldKvEventPublisherFromZmq*names, keeping Python bindings in sync with Rust.lib/bindings/python/src/dynamo/_core.pyi (2)
582-590: Add type stubs for renamedZmqKvEventPublisherConfigThe new stub correctly defines the Python-side config class with default values for
zmq_endpointandzmq_topic.
600-608: Add type stubs forZmqKvEventPublisherThe new stub for
ZmqKvEventPublishermatches the renamed class in the Rust binding and includes theshutdownmethod.lib/bindings/python/rust/llm/kv.rs (3)
131-140: Rename Python binding struct toZmqKvEventPublisherConfigThe new
#[pyclass] pub struct ZmqKvEventPublisherConfigreplaces the oldKvEventPublisherFromZmqConfig, aligning with the upstream Rust rename.
143-156: Update constructor signature forZmqKvEventPublisherConfigThe
#[new]impl now accepts(worker_id, kv_block_size, zmq_endpoint, zmq_topic)with sensible defaults. This matches the Rust struct and Python stub.
167-174: Rename Python binding struct toZmqKvEventPublisherand update instantiationThe struct and its
#[new]method now referencellm_rs::kv_router::publisher::ZmqKvEventPublisher, replacing the oldKvEventPublisherFromZmq. The call tostart_background_taskremains correct.
grahamking
changed the title
Overview:
Address comments from #1181
Summary by CodeRabbit
KvEventPublisherFromZmq/KvEventPublisherFromZmqConfigtoZmqKvEventPublisher/ZmqKvEventPublisherConfigthroughout the Python and Rust interfaces.