refactor: move kv store to runtime #1459
Conversation
ryanolson
requested review from
a team,
GuanLuo,
PeaBrane,
alec-flowers,
biswapanda,
grahamking,
jthomson04,
kkranen,
oandreeva-nv,
paulhendricks,
rmccorm4 and
tmonty12
as code owners
WalkthroughThe changes introduce a comprehensive restructuring of etcd path handling in the distributed runtime, including a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DistributedRuntime
participant Namespace
participant Component
participant Endpoint
participant EtcdPath
User->>DistributedRuntime: create_namespace(name)
DistributedRuntime->>Namespace: new(name, parent)
Namespace->>EtcdPath: etcd_path()
EtcdPath-->>Namespace: returns path string
User->>Namespace: create_component(name)
Namespace->>Component: new(name, runtime)
Component->>EtcdPath: etcd_path()
EtcdPath-->>Component: returns path string
User->>Component: create_endpoint(name)
Component->>Endpoint: new(name, component)
Endpoint->>EtcdPath: etcd_path()
EtcdPath-->>Endpoint: returns path string
Poem
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. 🪧 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: 7
🔭 Outside diff range comments (1)
lib/runtime/src/component.rs (1)
489-499: 🛠️ Refactor suggestionConsolidate duplicate validation logic.
This
validate_allowed_charsfunction duplicates the logic inpath.rs. Consider moving it to a shared module or reusing the one frompath.rs.Move the validation function to a common module (e.g.,
validation.rs) and import it in both files to maintain DRY principles.
🧹 Nitpick comments (2)
lib/runtime/src/lib.rs (1)
47-47: Add a doc comment for the new storage module. Sincestorageis part of the public API, a brief doc comment will clarify its role.- pub mod storage; + /// Storage module exposing key-value store implementations (etcd, NATS). + pub mod storage;lib/llm/src/lib.rs (1)
21-21: Clean up the commented-out module. Rather than leaving a disabledpub mod key_value_store;, remove it to keep the codebase tidy and avoid confusion.-// pub mod key_value_store;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
lib/llm/src/discovery/model_entry.rs(1 hunks)lib/llm/src/lib.rs(1 hunks)lib/llm/src/local_model.rs(1 hunks)lib/llm/src/model_card/model.rs(1 hunks)lib/runtime/src/component.rs(12 hunks)lib/runtime/src/component/endpoint.rs(2 hunks)lib/runtime/src/distributed.rs(1 hunks)lib/runtime/src/lib.rs(1 hunks)lib/runtime/src/storage.rs(1 hunks)lib/runtime/src/storage/key_value_store.rs(2 hunks)lib/runtime/src/storage/key_value_store/etcd.rs(1 hunks)lib/runtime/src/storage/key_value_store/nats.rs(1 hunks)lib/runtime/src/transports/etcd.rs(2 hunks)lib/runtime/src/transports/etcd/path.rs(1 hunks)lib/runtime/tests/namespace_etcd_path.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Rust pre-merge checks
lib/runtime/tests/namespace_etcd_path.rs
[error] 5-5: unused imports: DistributedRuntime and Runtime. To override, add #[allow(unused_imports)].
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test - vllm
🔇 Additional comments (15)
lib/runtime/src/storage/key_value_store/etcd.rs (1)
20-20: Localize imports for consistency. ImportingSlugandClientfrom the local crate (instead ofdynamo_runtime) ensures internal coherence and removes external dependencies.lib/runtime/src/storage/key_value_store/nats.rs (1)
18-18: Switch to crate-relative imports. Usingcrate::{protocols::Endpoint, slug::Slug, transports::nats::Client}aligns with the runtime's module structure and replaces the old external references.lib/runtime/src/storage.rs (1)
4-4:Details
✅ Verification successful
Ensure
storagemodule is re-exported: The newpub mod key_value_storeshould be reachable viaruntime::storage. Confirm thatlib/runtime/src/lib.rscontainspub mod storage;.
🏁 Script executed:
#!/bin/bash # Verify that the storage module is exposed in the crate root rg -n "pub mod storage" lib/runtime/src/lib.rsLength of output: 68
storage module re-export confirmed
Confirmed that
lib/runtime/src/lib.rsincludespub mod storage;on line 47, makingruntime::storage::key_value_storeaccessible as intended.lib/runtime/src/storage/key_value_store.rs (1)
315-315:Details
✅ Verification successful
Update logging init call in tests: Using
crate::logging::init()aligns with the refactored logging module. Ensureloggingis exported from the runtime crate so tests can invoke it.
🏁 Script executed:
#!/bin/bash # Verify that logging module is publicly available rg -n "pub mod logging" lib/runtime/src/lib.rsLength of output: 68
Logging module export confirmed
Thepub mod logging;declaration is present at line 39 inlib/runtime/src/lib.rs, so usingcrate::logging::init()in tests is valid.lib/llm/src/discovery/model_entry.rs (1)
7-11: Consolidateddynamo_runtimeimports: The grouped import ofprotocols,Slug, and key-value store types fromdynamo_runtimereflects the new module location and reduces local crate coupling.lib/llm/src/local_model.rs (1)
9-12: Groupeddynamo_runtimeimports: ImportingComponent,Endpoint, and key-value store traits fromdynamo_runtimein one statement improves readability and aligns with the refactor.lib/llm/src/model_card/model.rs (1)
24-24: Updated runtime imports: The consolidated import ofSlug,Versioned, andnatsfromdynamo_runtimeaccurately reflects the relocated key-value store and transport modules.lib/runtime/src/transports/etcd.rs (1)
35-38: LGTM! Clean module organization.The addition of the
pathmodule and its public re-export follows Rust conventions and provides a clear separation of concerns for etcd path handling.lib/runtime/src/component/endpoint.rs (1)
62-65: LGTM! Method naming improves clarity.The rename from
etcd_path(lease_id)toetcd_path_with_lease_id(lease_id)makes the API more explicit and self-documenting.Also applies to: 130-134
lib/runtime/tests/namespace_etcd_path.rs (1)
8-118: Well-structured integration tests for namespace paths.The tests comprehensively cover:
- Path format validation with the "dynamo://" prefix
- Hierarchical namespace creation and path generation
- Component paths with the reserved
_component_keyword- Multiple namespace branches
Good test coverage for the new functionality.
lib/runtime/src/transports/etcd/path.rs (3)
10-17: Good design choice for reserved keywords.Using underscore-prefixed keywords (
_component_,_endpoint_) effectively prevents conflicts with user-defined names while maintaining readability.
361-615: Excellent test coverage!The unit tests comprehensively cover:
- Basic path parsing and formatting
- Hierarchical namespaces
- Component and endpoint paths
- Lease ID handling with hex formatting
- Reserved keyword validation
- Edge cases and error conditions
Well done on the thorough testing.
157-160:Details
✅ Verification successful
Consider handling negative lease IDs consistently.
The current implementation formats lease IDs as hexadecimal, but negative values might not round-trip correctly. Consider either:
- Ensuring lease IDs are always positive
- Using unsigned formatting (
{:x}on the bits representation)- Documenting the expected behavior for negative lease IDs
Also applies to: 230-236
🏁 Script executed:
#!/bin/bash # Check how lease IDs are generated in the codebase rg -A 5 "lease_id|lease\.id\(\)" --type rust | grep -E "(create_lease|lease_id.*=|id:.*i64)"Length of output: 4173
No changes needed for lease ID formatting
Leases from etcd are always non-negative int64, and Rust’s
{:x}formatter paired withi64::from_str_radix(_, 16)supports signed hex (including a leading “−”) correctly. The negative‐ID case cannot occur in practice, so no update is required here.lib/runtime/src/component.rs (2)
468-486: Well-designed hierarchical namespace implementation.The parent-child relationship and fully qualified name generation are implemented correctly. The use of
Arcfor parent references prevents ownership issues while maintaining the hierarchy.
188-191: Clean integration with the new EtcdPath abstraction.The new methods properly utilize the
EtcdPathtype for type-safe path construction. The distinction between static and dynamic endpoints (with lease IDs) is handled correctly.Also applies to: 338-370
|
The current flow of items is:
Is there any reason to support going from EtcdPath -> Typed Struct (Namespace, Component, Endpoint) ? One reason is in the original DEP it was written so that a Canoncial Identifier object was created as the first order of buisness, validated, and then you could create Typed Entities. This ensured that all validation logic was inside the Identifier and that Typed Entities were guaranteed to be valid. The only constructor of the Typed Entities was in the Canoncial Identifier object. For example: let namespace_desc = Instance::new_namespace("production.api.v1")?;
let drt = DistributedRuntime::new();
let namespace = endpoint_desc.to_namespace(&drt); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/runtime/src/transports/etcd/path.rs (1)
148-149:⚠️ Potential issue
with_extra_pathunexpectedly discards an existinglease_id(issue re-introduced)Resetting
self.lease_idtoNonewhen merely appending extra path segments silently drops information that callers reasonably expect to be preserved.
This was flagged and removed in an earlier commit (see prior discussion) but has resurfaced here.- self.lease_id = None;Unless there is a compelling, documented reason to invalidate the lease whenever extra segments are added, please remove this line (or add explicit docs + unit test for the behaviour).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/runtime/src/transports/etcd/path.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (lib/bindings/python)
- GitHub Check: Build and Test - vllm
|
Question from me. Is the Etcd Path the Canonical representation? Or will we have another? From the DEP we have this idea of an internal dynamo repr and then etcd and when we Debug format we have both printed. impl std::fmt::Debug for Path {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "Path({} @ {})", self.etcd_path(), self.to_string())
}
} |
Relocates KvStore from llm -> runtime
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests