chore(bindings): Provide a binding to clear etcd namespace #3094
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grahamking
requested review from
a team,
Aphoh,
PeaBrane,
alec-flowers,
biswapanda,
hhzhang16,
indrajit96,
ishandhanani,
jasonqinzhou,
krishung5,
michaelshin,
nealvaidya,
nnshah1,
tedzhouhk and
whoisj
as code owners
Contributor
WalkthroughThe change replaces direct EtcdKvCache usage and public etcd_client access with runtime-provided APIs. A new temp_clear_namespace method is added to DistributedRuntime (Rust and Python bindings). Call sites now use runtime.temp_clear_namespace and runtime.do_not_use_etcd_client. Some example logs drop lease IDs. Type hints/tests reflect the renaming/removal. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Caller/CLI
participant PyUtil as clear_namespace (Py)
participant PyRuntime as DistributedRuntime (Py binding)
participant RsRuntime as DistributedRuntime (Rust)
participant Etcd as EtcdClient
User->>PyUtil: clear_namespace(runtime, namespace)
PyUtil->>PyRuntime: temp_clear_namespace("/<ns>/")
PyRuntime->>RsRuntime: temp_clear_namespace(name)
alt Etcd client present
RsRuntime->>Etcd: kv_get_prefix(name)
loop for each key
RsRuntime->>Etcd: kv_delete(key)
Etcd-->>RsRuntime: ok
end
RsRuntime-->>PyRuntime: Ok(())
else No etcd (static workers)
RsRuntime-->>PyRuntime: Ok(())
end
PyRuntime-->>PyUtil: Ok
PyUtil-->>User: Done (logged)
sequenceDiagram
autonumber
participant Worker as Worker process
participant PyRuntime as DistributedRuntime (Py binding)
participant Etcd as EtcdClient
participant Cfg as configure_ports_with_etcd
Worker->>PyRuntime: do_not_use_etcd_client()
PyRuntime-->>Worker: Optional[EtcdClient]
Worker->>Cfg: configure_ports_with_etcd(etcd_client, ...)
Cfg->>Etcd: read/update port info
Etcd-->>Cfg: responses
Cfg-->>Worker: ports configured
note over Worker: Flow unchanged aside from client accessor rename
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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.
Please see the documentation for more information. 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. Comment |
This removes one use of the EtcdClient binding, which is going away. Also remove some unnecessary logging of lease_id, which used the binding too. And rename `etcd_client()` to `do_not_use_etcd_client()` so no-one adds another use of it. Signed-off-by: Graham King <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
lib/bindings/python/tests/test_etcd_bindings.py (1)
18-19: Guard against None: do_not_use_etcd_client() can return None.Without a check this test will raise on None when etcd isn’t running. Add an assertion to fail fast with a helpful message.
- etcd = runtime.do_not_use_etcd_client() + etcd = runtime.do_not_use_etcd_client() + assert etcd is not None, "Etcd client not available; ensure etcd is running for this test"tests/router/test_router_e2e_with_mockers.py (1)
238-239: Add explicit check for None before using etcd in tests.If the runtime instance lacks an etcd client, this will raise. Fail fast with a clearer message.
- etcd = runtime.do_not_use_etcd_client() + etcd = runtime.do_not_use_etcd_client() + assert etcd is not None, "Etcd client not available; runtime_services must start etcd"examples/multimodal/components/worker.py (1)
423-425: Handle absence of etcd client gracefully.configure_ports_with_etcd likely expects a client; skip when None to avoid surprises in static/no-etcd runs.
- etcd_client = runtime.do_not_use_etcd_client() - await configure_ports_with_etcd(config, etcd_client) + etcd_client = runtime.do_not_use_etcd_client() + if etcd_client is not None: + await configure_ports_with_etcd(config, etcd_client) + else: + logger.debug("No etcd client; skipping port configuration via etcd")components/backends/vllm/src/dynamo/vllm/main.py (1)
72-74: Gate etcd-based port configuration on client presence.Prevents runtime errors when etcd isn’t wired up.
- etcd_client = runtime.do_not_use_etcd_client() - await configure_ports_with_etcd(config, etcd_client) + etcd_client = runtime.do_not_use_etcd_client() + if etcd_client is not None: + await configure_ports_with_etcd(config, etcd_client) + else: + logger.debug("No etcd client; skipping port configuration via etcd")components/backends/sglang/src/dynamo/sglang/utils/clear_namespace.py (1)
16-17: Normalize namespace to avoid accidental double slashes.Minor hardening to handle inputs with leading/trailing slashes.
- await runtime.temp_clear_namespace(f"/{namespace}/") - logging.info(f"Cleared /{namespace}") + ns = namespace.strip("/") + await runtime.temp_clear_namespace(f"/{ns}/") + logging.info(f"Cleared /{ns}")components/backends/trtllm/utils/clear_namespace.py (1)
17-18: Normalize namespace formatting before clearing.Prevents path anomalies if users pass "/foo/" vs "foo".
- await runtime.temp_clear_namespace(f"/{namespace}/") - logger.info(f"Cleared /{namespace}") + ns = namespace.strip("/") + await runtime.temp_clear_namespace(f"/{ns}/") + logger.info(f"Cleared /{ns}")lib/bindings/python/src/dynamo/_core.pyi (1)
44-48: Expose temp_clear_namespace in the stub to match the binding.The Rust binding adds DistributedRuntime.temp_clear_namespace, but the pyi doesn’t declare it. Add it so type-checkers and IDEs stay in sync.
class DistributedRuntime: """ The runtime object for dynamo applications """ ... + async def temp_clear_namespace(self, name: str) -> None: + """ + Remove everything under the given etcd prefix. + Temporary; will be removed once MDC auto-delete exists. + """ + ... + def namespace(self, name: str) -> Namespace: """ Create a `Namespace` object """ ...lib/bindings/python/rust/lib.rs (1)
377-382: Surface deprecation at call time.Emit a runtime warning so callers see the deprecation when they use this method.
fn do_not_use_etcd_client(&self) -> PyResult<Option<EtcdClient>> { + tracing::warn!("do_not_use_etcd_client() is deprecated and will be removed soon; prefer runtime APIs."); match self.inner.etcd_client().clone() { Some(etcd_client) => Ok(Some(EtcdClient { inner: etcd_client })), None => Ok(None), } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
components/backends/sglang/src/dynamo/sglang/utils/clear_namespace.py(1 hunks)components/backends/trtllm/utils/clear_namespace.py(2 hunks)components/backends/vllm/src/dynamo/vllm/main.py(1 hunks)components/planner/src/dynamo/planner/virtual_connector.py(1 hunks)examples/custom_backend/hello_world/hello_world.py(1 hunks)examples/multimodal/components/worker.py(1 hunks)lib/bindings/python/examples/hello_world/server.py(0 hunks)lib/bindings/python/rust/lib.rs(1 hunks)lib/bindings/python/src/dynamo/_core.pyi(2 hunks)lib/bindings/python/tests/test_etcd_bindings.py(1 hunks)lib/runtime/src/distributed.rs(1 hunks)tests/router/test_router_e2e_with_mockers.py(1 hunks)
💤 Files with no reviewable changes (1)
- lib/bindings/python/examples/hello_world/server.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-01T13:55:03.940Z
Learnt from: nnshah1
PR: ai-dynamo/dynamo#1444
File: tests/fault_tolerance/utils/metrics.py:30-32
Timestamp: 2025-07-01T13:55:03.940Z
Learning: The `dynamo_worker()` decorator in the dynamo codebase returns a wrapper that automatically injects the `runtime` parameter before calling the wrapped function. This means callers only need to provide the non-runtime parameters, while the decorator handles injecting the runtime argument automatically. For example, a function with signature `async def get_metrics(runtime, log_dir)` decorated with `dynamo_worker()` can be called as `get_metrics(log_dir)` because the decorator wrapper injects the runtime parameter.
Applied to files:
components/backends/vllm/src/dynamo/vllm/main.pycomponents/backends/trtllm/utils/clear_namespace.py
🧬 Code graph analysis (10)
lib/runtime/src/distributed.rs (1)
lib/bindings/python/rust/lib.rs (1)
temp_clear_namespace(363-368)
components/planner/src/dynamo/planner/virtual_connector.py (3)
lib/runtime/src/distributed.rs (2)
etcd_client(269-271)runtime(197-199)lib/bindings/python/rust/lib.rs (1)
do_not_use_etcd_client(377-382)lib/bindings/python/src/dynamo/_core.pyi (1)
do_not_use_etcd_client(44-49)
tests/router/test_router_e2e_with_mockers.py (2)
lib/bindings/python/rust/lib.rs (1)
do_not_use_etcd_client(377-382)lib/bindings/python/src/dynamo/_core.pyi (1)
do_not_use_etcd_client(44-49)
components/backends/vllm/src/dynamo/vllm/main.py (3)
lib/runtime/src/distributed.rs (2)
etcd_client(269-271)runtime(197-199)lib/bindings/python/rust/lib.rs (1)
do_not_use_etcd_client(377-382)lib/bindings/python/src/dynamo/_core.pyi (1)
do_not_use_etcd_client(44-49)
components/backends/sglang/src/dynamo/sglang/utils/clear_namespace.py (4)
lib/runtime/src/distributed.rs (3)
runtime(197-199)namespace(216-218)temp_clear_namespace(332-341)lib/bindings/python/src/dynamo/runtime/__init__.py (1)
dynamo_worker(36-62)components/backends/trtllm/utils/clear_namespace.py (1)
clear_namespace(16-18)lib/bindings/python/rust/lib.rs (2)
namespace(370-375)temp_clear_namespace(363-368)
lib/bindings/python/tests/test_etcd_bindings.py (2)
lib/bindings/python/rust/lib.rs (1)
do_not_use_etcd_client(377-382)lib/bindings/python/src/dynamo/_core.pyi (1)
do_not_use_etcd_client(44-49)
components/backends/trtllm/utils/clear_namespace.py (2)
lib/runtime/src/distributed.rs (3)
runtime(197-199)temp_clear_namespace(332-341)namespace(216-218)lib/bindings/python/rust/lib.rs (2)
temp_clear_namespace(363-368)namespace(370-375)
lib/bindings/python/src/dynamo/_core.pyi (1)
lib/bindings/python/rust/lib.rs (1)
do_not_use_etcd_client(377-382)
lib/bindings/python/rust/lib.rs (2)
lib/runtime/src/distributed.rs (2)
temp_clear_namespace(332-341)namespace(216-218)lib/bindings/python/src/dynamo/_core.pyi (3)
namespace(38-42)do_not_use_etcd_client(44-49)EtcdClient(57-102)
examples/multimodal/components/worker.py (3)
lib/runtime/src/distributed.rs (2)
etcd_client(269-271)runtime(197-199)lib/bindings/python/rust/lib.rs (1)
do_not_use_etcd_client(377-382)lib/bindings/python/src/dynamo/_core.pyi (1)
do_not_use_etcd_client(44-49)
⏰ 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: pre-merge-rust (lib/bindings/python)
- GitHub Check: pre-merge-rust (lib/runtime/examples)
- GitHub Check: pre-merge-rust (.)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
examples/custom_backend/hello_world/hello_world.py (1)
37-37: Good cleanup: removed lease_id from log.This aligns with discouraging direct etcd binding usage; no functional change.
components/planner/src/dynamo/planner/virtual_connector.py (1)
45-47: Switch to do_not_use_etcd_client is fine; None-path handled.Constructor guards None with a clear error; later usage is safe.
grahamking
force-pushed
the
gk-clear-namespace-binding
branch
from
e5f4a18 to
4206b3c
Compare
PeaBrane
approved these changes
Sep 17, 2025
PeaBrane
reviewed
Sep 17, 2025
whoisj
approved these changes
Sep 18, 2025
Collaborator
whoisj
left a comment
whoisj
left a comment
There was a problem hiding this comment.
Looks good to me. Assuming the tests pass, I'm happy with it.
Contributor
|
Lgtm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes one use of the EtcdClient binding, which is going away.
Also remove some unnecessary logging of lease_id, which used the binding too.
And rename
etcd_client()todo_not_use_etcd_client()so no-one adds another use of it.Summary by CodeRabbit