Skip to content

feat(router): add dynamic worker taint updates - #12620

Merged
tmonty12 merged 16 commits into
mainfrom
tmonty12/dyn-10703-update-model-taints
Aug 11, 2026
Merged

tmonty12 merged 16 commits into
mainfrom
tmonty12/dyn-10703-update-model-taints

Conversation

@tmonty12

@tmonty12 tmonty12 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add the narrow update_model_taints(endpoint, taints) Python API for base-model worker routing metadata
  • replace only caller-managed taints, reject the reserved dynamo.topology/* namespace, and regenerate topology taints from immutable topology metadata
  • add existing-record replacement for KV discovery and same-ID value-change propagation for Kubernetes and mock discovery
  • wake and re-check pending scheduler requests when worker configs change
  • register POST /engine/update/model_taints across vLLM, SGLang, and TensorRT-LLM worker modes, including realtime, embedding, multimodal, Omni, and diffusion variants
  • register the same route in the shared Rust common worker so TokenSpeed and future unified backends receive it without engine-specific opt-in
  • document the Kubernetes port-forward/curl flow
  • enforce the immutable model-card boundary and verify taint-only changes do not alter mdcsum()

This is Part 1 of #10703. Endpoint-local pool balancing remains deferred to #12050. LoRA MDC runtime configs are not consumed by router runtime-config discovery, so the update API intentionally targets only the base-model MDC.

Validation

  • cargo test -p dynamo-runtime model_taint_update_tests
  • cargo test -p dynamo-runtime model_taint_updates_replace_existing_value_and_emit_added
  • cargo test -p dynamo-runtime changed_model_value_is_an_added_upsert
  • cargo test -p dynamo-kv-router worker_taint_update_wakes_constrained_pending_request
  • cargo test -p dynamo-llm runtime_taints_do_not_change_mdcsum
  • cargo test -p dynamo-backend-common (137 passed)
  • cargo test -p dynamo-backend-common --features integration handoff_precedes_registration_and_populates_namespaced_registry
  • cargo test -p dynamo-backend-common --features integration model_taint_update_route_updates_registered_base_model
  • cargo check -p dynamo-llm
  • cargo check --manifest-path lib/bindings/python/Cargo.toml
  • cargo clippy -p dynamo-runtime -p dynamo-kv-router -p dynamo-llm --lib --tests -- -D warnings
  • cargo clippy -p dynamo-backend-common --lib --tests -- -D warnings
  • .venv/bin/python -m pytest components/src/dynamo/common/tests/test_model_taints.py (5 passed)
  • Ruff check across all changed backend modules
  • Python bytecode compilation across all changed backend modules
  • cargo fmt --all -- --check
  • cargo fmt --manifest-path lib/bindings/python/Cargo.toml -- --check

The focused SGLang route-registration regression is included but could not run in the local lightweight environment because torch is not installed; CI has the backend dependencies.

Draft follow-ups

  • exercise the update path against live etcd and Kubernetes discovery deployments
  • confirm the mutable-boundary enforcement location and mixed-version behavior with reviewers
  • align semantic request-validation failures with a reusable typed /engine/* 4xx error path

Summary by CodeRabbit

  • New Features
    • Added an HTTP API and Python interface for replacing worker routing taints.
    • Worker taint changes now propagate through discovery and routing, enabling dynamic worker selection without restarting services.
    • Supported workers across language, embedding, multimodal, image, video, and realtime workloads.
  • Bug Fixes
    • Prevented malformed, duplicate, or reserved topology taints from being applied.
    • Improved routing updates so newly eligible workers receive pending requests.
  • Documentation
    • Added deployment guidance for updating taints, rollout ordering, and security considerations.

@copy-pr-bot

copy-pr-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

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 github-actions Bot added feat documentation Improvements or additions to documentation backend::vllm Relates to the vllm backend labels Aug 4, 2026
@datadog-official

This comment has been minimized.

@github-actions github-actions Bot added backend::sglang Relates to the sglang backend backend::trtllm Relates to the trtllm backend multimodal labels Aug 4, 2026
@github-actions github-actions Bot added the router Relates to routing, KV-aware routing, etc. label Aug 4, 2026

@tmonty12 tmonty12 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Scoped review: five remaining concerns. All other previously discussed findings are intentionally omitted.

Comment thread lib/runtime/src/storage/kv.rs Outdated
Comment thread examples/backends/vllm/deploy/README.md
Comment thread lib/runtime/src/discovery/mod.rs Outdated
Comment thread lib/runtime/src/discovery/kv_store.rs
Comment thread lib/kv-router/src/scheduling/local.rs
@tmonty12
tmonty12 force-pushed the tmonty12/dyn-10703-update-model-taints branch from af2e1f9 to a075cae Compare August 5, 2026 16:52
@tmonty12
tmonty12 marked this pull request as ready for review August 5, 2026 17:28
@tmonty12
tmonty12 requested review from a team as code owners August 5, 2026 17:28

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 3 potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

Comment thread components/src/dynamo/sglang/request_handlers/handler_base.py
Comment thread lib/runtime/src/discovery/mod.rs
Comment thread lib/runtime/src/storage/kv/nats.rs Outdated
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The change adds worker-local model-taint update routes, discovery support for taint-only events, atomic persistence operations, runtime propagation, public APIs, worker wiring, and router end-to-end coverage.

Changes

Model taint update flow

Layer / File(s) Summary
Discovery contract and reconciliation
lib/runtime/src/discovery/*, lib/llm/src/model_card.rs
Discovery validates mutable taints, emits ModelTaintsUpdated, persists updates, and reconciles snapshots.
Storage compare-and-replace
lib/runtime/src/storage/*, lib/runtime/src/transports/etcd.rs
Storage backends add atomic compare-and-replace operations. Watch delivery now applies cancellation-aware backpressure.
Event propagation and routing refresh
lib/llm/src/*, lib/kv-router/src/scheduling/local.rs
Runtime consumers process taint events, and successful worker changes refresh routing queues.
Worker route and public API
components/src/dynamo/common/*, lib/backend-common/src/worker.rs, lib/bindings/python/*, components/src/dynamo/{sglang,trtllm,vllm}/*
Workers register update/model_taints. The route validates, deduplicates, sorts, and applies taints. Rust and Python APIs expose the update operation.
Router integration and validation
tests/router/*, examples/backends/vllm/deploy/README.md
Test workers accept initial taints and system ports. End-to-end coverage verifies taint replacement and routing propagation.

Estimated code review effort: 5 (Critical) | ~120 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description provides detailed scope and validation, but it omits the required Related Issues selection and reviewer-start section. Add the required Related Issues section with either the issue-closing entry or no-issue confirmation, and identify the files where reviewers should start.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: dynamic worker taint updates for the router.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
components/src/dynamo/sglang/request_handlers/handler_base.py (1)

887-891: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reserve update/model_taints from configured engine routes.

register_model_taint_route installs update/model_taints. The collision check only covers built_in_routes. A configured route with the same path is registered after this call. The configured handler can shadow the taint handler or cause startup failure.

Reject MODEL_TAINT_ROUTE before registering configured routes.

Based on the shared route contract, MODEL_TAINT_ROUTE resolves to update/model_taints.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@components/src/dynamo/sglang/request_handlers/handler_base.py` around lines
887 - 891, Prevent configured routes from registering the reserved
MODEL_TAINT_ROUTE path (`update/model_taints`) after register_model_taint_route
installs it. Validate each configured route in the configured_routes loop and
reject or skip entries matching MODEL_TAINT_ROUTE before calling
runtime.register_engine_route, while preserving registration of other configured
routes.
🧹 Nitpick comments (8)
tests/router/counter_worker.py (1)

117-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Set CUDA_VISIBLE_DEVICES before importing device-sensitive modules.

CounterWorkerProcess does not set this variable, and direct CLI usage derives it from device_type. Update every invocation path, including the documented CLI path, before moving lines 117–128 to module scope; otherwise CPU workers can inherit a GPU-visible environment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/router/counter_worker.py` around lines 117 - 128, Ensure every
CounterWorkerProcess invocation path, including the documented CLI entrypoint,
derives and sets CUDA_VISIBLE_DEVICES from device_type before importing
device-sensitive modules such as dynamo.llm and dynamo.runtime. Apply this
initialization before promoting the imports around register_model_taint_route
and DistributedRuntime to module scope, while preserving CPU workers’ GPU-hidden
environment.

Sources: Coding guidelines, Path instructions

lib/runtime/src/storage/kv.rs (1)

430-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Document the meaning of the returned StoreOutcome revision.

The four implementations return different revision values on success: the etcd bucket returns Created(0), the file bucket returns Created(1), the memory bucket returns the incremented in-memory revision, and the NATS bucket returns the JetStream revision. The only current caller ignores the value. State in the contract that the returned revision is backend-specific and not comparable across backends, so a future caller does not treat it as a monotonic version.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/storage/kv.rs` around lines 430 - 441, Update the
`compare_and_replace` trait contract documentation to state that a successful
`StoreOutcome` revision is backend-specific and must not be compared across
backends or treated as a monotonic version. Keep the existing atomicity,
missing-key, and retry semantics unchanged.
lib/runtime/src/discovery/kube.rs (1)

486-496: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reduce the info-level payload for emitted discovery events.

Line 489 logs ?event at info level. For DiscoveryEvent::Added(DiscoveryInstance::Model { .. }) this prints the entire card_json, which can be several kilobytes per event. The initial-sync log at line 405 prints only instance_id. Log the event discriminant and instance ID at info level, and keep the full event at debug level.

♻️ Proposed change
                         for event in events {
                             tracing::info!(
                                 stream_id = %stream_id,
-                                ?event,
+                                event_kind = match &event {
+                                    DiscoveryEvent::Added(_) => "added",
+                                    DiscoveryEvent::ModelTaintsUpdated(_) => "model_taints_updated",
+                                    DiscoveryEvent::Removed(_) => "removed",
+                                },
                                 "Emitting discovery event"
                             );
+                            tracing::debug!(stream_id = %stream_id, ?event, "Discovery event detail");
                             if event_tx.send(Ok(event)).is_err() {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/discovery/kube.rs` around lines 486 - 496, Update the event
logging in the loop around event_tx.send to remove the full ?event payload from
the info-level record; log only the event discriminant and instance ID, matching
the concise initial-sync logging, while emitting the complete event at debug
level.
lib/runtime/src/transports/etcd.rs (1)

332-358: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider collapsing the read and the transaction into one etcd round trip.

The method issues a get and then a txn, so every attempt costs two round trips. A single transaction guarded on both existence and value achieves the same result:

let txn = Txn::new()
    .when(vec![Compare::value(key, CompareOp::Equal, expected.as_ref().to_vec())])
    .and_then(vec![TxnOp::put(key, value.as_ref().to_vec(), Some(put_options))])
    .or_else(vec![TxnOp::get(key, None)]);

The existing or_else branch already distinguishes Missing from Conflict by inspecting whether the returned kvs are empty, so the classification logic needs no change. The current form is correct; this is a latency and simplicity improvement only.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/transports/etcd.rs` around lines 332 - 358, Collapse the
initial get and subsequent transaction in the compare-and-put method into one
transaction. Remove the preliminary connector get and guard the transaction with
Compare::value using expected.as_ref().to_vec(), while preserving the existing
lease-backed TxnOp::put, or_else TxnOp::get, and Missing/Conflict classification
logic.
lib/runtime/src/storage/kv/nats.rs (1)

338-346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Delete the per-run JetStream bucket at the end of the test.

Each run creates a bucket named compare_and_replace_<uuid> and never removes it. Repeated integration runs accumulate buckets on the shared NATS server. The etcd equivalent avoids this by reusing a fixed bucket and randomizing only the key. Either delete the bucket after the assertions, or use a fixed bucket name with a UUID-suffixed key as the etcd test does.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/storage/kv/nats.rs` around lines 338 - 346, Update the test
creating the per-run bucket through get_or_create_bucket to clean up that bucket
after all assertions complete, ensuring deletion occurs before the test returns
even when feasible within the existing flow.
lib/runtime/src/storage/kv/file.rs (2)

435-466: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate the four duplicated temp-file cleanup blocks.

Lines 438-440, 444-446, 451-453, and 458-460 repeat the same fs::remove_file plus tracing::warn! pair. Extract one helper and call it on each early-return path.

♻️ Proposed helper

Add to impl Directory:

fn discard_temp_file(temp_path: &Path) {
    if let Err(remove_error) = fs::remove_file(temp_path) {
        tracing::warn!(
            path = %temp_path.display(),
            %remove_error,
            "Failed to remove unused FileStore temp file"
        );
    }
}

Then replace each cleanup block:

         let current = match fs::read(&full_path) {
             Ok(current) => current,
             Err(error) if error.kind() == ErrorKind::NotFound => {
-                if let Err(remove_error) = fs::remove_file(&temp_path) {
-                    tracing::warn!(path = %temp_path.display(), %remove_error, "Failed to remove unused FileStore temp file");
-                }
+                Self::discard_temp_file(&temp_path);
                 return Err(StoreError::MissingKey(key.to_string()));
             }
             Err(error) => {
-                if let Err(remove_error) = fs::remove_file(&temp_path) {
-                    tracing::warn!(path = %temp_path.display(), %remove_error, "Failed to remove unused FileStore temp file");
-                }
+                Self::discard_temp_file(&temp_path);
                 return Err(to_fs_err(error));
             }
         };
         if current.as_slice() != expected.as_ref() {
-            if let Err(remove_error) = fs::remove_file(&temp_path) {
-                tracing::warn!(path = %temp_path.display(), %remove_error, "Failed to remove unused FileStore temp file");
-            }
+            Self::discard_temp_file(&temp_path);
             return Err(StoreError::Retry);
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/storage/kv/file.rs` around lines 435 - 466, Consolidate the
repeated temporary-file cleanup in the affected Directory implementation by
adding a discard_temp_file helper that removes the supplied Path and emits the
existing warning on failure. Replace all four duplicated
fs::remove_file/tracing::warn! blocks in the read, mismatch, and rename-error
early-return paths with calls to this helper, preserving their current error
returns.

206-218: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

flock(LOCK_EX) blocks the runtime worker thread for an unbounded time.

acquire performs a blocking flock with LOCK_EX. The callers insert, compare_and_replace, and delete are async fn and run on Tokio worker threads. The wait duration depends on another process holding the directory lock, not on local disk latency, so a stalled peer process blocks a Tokio worker thread indefinitely. The existing fs::read and fs::rename calls in these methods are also blocking, but their duration is bounded by disk I/O.

Move the locked critical sections to tokio::task::spawn_blocking, or acquire with LOCK_EX | LOCK_NB and retry on EWOULDBLOCK with an async sleep and an attempt cap.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/storage/kv/file.rs` around lines 206 - 218, Update
DirectoryMutationLock::acquire and the async callers insert,
compare_and_replace, and delete so the blocking flock(LOCK_EX) and associated
locked file operations execute inside tokio::task::spawn_blocking, preventing
Tokio worker threads from waiting on another process indefinitely. Preserve the
existing lock lifetime, error propagation, and mutation behavior.
lib/runtime/src/discovery/kv_store.rs (1)

511-537: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Bound the compare-and-replace retry loop.

The loop retries indefinitely on StoreError::Retry. Each iteration issues a get and a compare_and_replace against the store. If another writer keeps changing the same key, this worker issues unbounded store requests and the caller's HTTP request never completes. Add an attempt cap and return StoreError::Retry to the caller when the cap is reached, so the route can surface a retryable status instead of hanging.

♻️ Proposed change
-        loop {
+        const MAX_ATTEMPTS: usize = 8;
+        for _ in 0..MAX_ATTEMPTS {
             let existing_json = bucket
                 .get(&key)
                 .await?
                 .ok_or_else(|| kv::StoreError::MissingKey(key.to_string()))?;
@@
             match bucket
                 .compare_and_replace(&key, existing_json, candidate_json)
                 .await
             {
                 Ok(_) => return Ok(()),
                 Err(kv::StoreError::Retry) => continue,
                 Err(error) => return Err(error.into()),
             }
         }
+
+        Err(kv::StoreError::Retry.into())
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/runtime/src/discovery/kv_store.rs` around lines 511 - 537, Bound the
retry loop in the compare-and-replace flow around bucket.get and
compare_and_replace by tracking attempts and enforcing a finite cap. When
StoreError::Retry reaches the cap, return StoreError::Retry to the caller;
preserve the existing success, missing-key, identity-mismatch, and non-retry
error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@components/src/dynamo/sglang/request_handlers/handler_base.py`:
- Line 887: Authorize the model-taint route registered by
register_model_taint_route in the handler initialization flow, ensuring
unauthorized /engine/* requests are rejected before taints or discovery routing
can be modified. Enforce this at the system status server or ingress using the
existing authorization mechanism, and add a test that verifies unauthorized
requests are denied.

In `@lib/runtime/src/storage/kv/file.rs`:
- Around line 8-10: Update the FileStore module inclusion in kv.rs to gate
file.rs and its Unix-specific implementation behind Unix support, or provide an
equivalent Windows implementation that replaces OsStrExt, AsRawFd, and
libc::flock usage. Ensure dynamo-runtime continues to compile on Windows.

---

Outside diff comments:
In `@components/src/dynamo/sglang/request_handlers/handler_base.py`:
- Around line 887-891: Prevent configured routes from registering the reserved
MODEL_TAINT_ROUTE path (`update/model_taints`) after register_model_taint_route
installs it. Validate each configured route in the configured_routes loop and
reject or skip entries matching MODEL_TAINT_ROUTE before calling
runtime.register_engine_route, while preserving registration of other configured
routes.

---

Nitpick comments:
In `@lib/runtime/src/discovery/kube.rs`:
- Around line 486-496: Update the event logging in the loop around event_tx.send
to remove the full ?event payload from the info-level record; log only the event
discriminant and instance ID, matching the concise initial-sync logging, while
emitting the complete event at debug level.

In `@lib/runtime/src/discovery/kv_store.rs`:
- Around line 511-537: Bound the retry loop in the compare-and-replace flow
around bucket.get and compare_and_replace by tracking attempts and enforcing a
finite cap. When StoreError::Retry reaches the cap, return StoreError::Retry to
the caller; preserve the existing success, missing-key, identity-mismatch, and
non-retry error behavior.

In `@lib/runtime/src/storage/kv.rs`:
- Around line 430-441: Update the `compare_and_replace` trait contract
documentation to state that a successful `StoreOutcome` revision is
backend-specific and must not be compared across backends or treated as a
monotonic version. Keep the existing atomicity, missing-key, and retry semantics
unchanged.

In `@lib/runtime/src/storage/kv/file.rs`:
- Around line 435-466: Consolidate the repeated temporary-file cleanup in the
affected Directory implementation by adding a discard_temp_file helper that
removes the supplied Path and emits the existing warning on failure. Replace all
four duplicated fs::remove_file/tracing::warn! blocks in the read, mismatch, and
rename-error early-return paths with calls to this helper, preserving their
current error returns.
- Around line 206-218: Update DirectoryMutationLock::acquire and the async
callers insert, compare_and_replace, and delete so the blocking flock(LOCK_EX)
and associated locked file operations execute inside
tokio::task::spawn_blocking, preventing Tokio worker threads from waiting on
another process indefinitely. Preserve the existing lock lifetime, error
propagation, and mutation behavior.

In `@lib/runtime/src/storage/kv/nats.rs`:
- Around line 338-346: Update the test creating the per-run bucket through
get_or_create_bucket to clean up that bucket after all assertions complete,
ensuring deletion occurs before the test returns even when feasible within the
existing flow.

In `@lib/runtime/src/transports/etcd.rs`:
- Around line 332-358: Collapse the initial get and subsequent transaction in
the compare-and-put method into one transaction. Remove the preliminary
connector get and guard the transaction with Compare::value using
expected.as_ref().to_vec(), while preserving the existing lease-backed
TxnOp::put, or_else TxnOp::get, and Missing/Conflict classification logic.

In `@tests/router/counter_worker.py`:
- Around line 117-128: Ensure every CounterWorkerProcess invocation path,
including the documented CLI entrypoint, derives and sets CUDA_VISIBLE_DEVICES
from device_type before importing device-sensitive modules such as dynamo.llm
and dynamo.runtime. Apply this initialization before promoting the imports
around register_model_taint_route and DistributedRuntime to module scope, while
preserving CPU workers’ GPU-hidden environment.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: dbdd615e-cf16-4ddf-a75d-8930b5731866

📥 Commits

Reviewing files that changed from the base of the PR and between 5f34677 and a075cae.

📒 Files selected for processing (46)
  • components/src/dynamo/common/model_taints.py
  • components/src/dynamo/common/tests/test_model_taints.py
  • components/src/dynamo/sglang/init_diffusion.py
  • components/src/dynamo/sglang/init_embedding.py
  • components/src/dynamo/sglang/init_multimodal.py
  • components/src/dynamo/sglang/request_handlers/handler_base.py
  • components/src/dynamo/sglang/tests/test_sglang_unit.py
  • components/src/dynamo/trtllm/workers/image_diffusion_worker.py
  • components/src/dynamo/trtllm/workers/llm_worker.py
  • components/src/dynamo/trtllm/workers/video_diffusion_worker.py
  • components/src/dynamo/vllm/omni/main.py
  • components/src/dynamo/vllm/omni/realtime_utils.py
  • components/src/dynamo/vllm/omni/stage_router.py
  • components/src/dynamo/vllm/worker_factory.py
  • examples/backends/vllm/deploy/README.md
  • lib/backend-common/src/worker.rs
  • lib/bindings/python/rust/lib.rs
  • lib/bindings/python/rust/llm/fpm.rs
  • lib/bindings/python/src/dynamo/_core.pyi
  • lib/bindings/python/src/dynamo/llm/__init__.py
  • lib/kv-router/src/scheduling/local.rs
  • lib/llm/src/direct_zmq_fan_in.rs
  • lib/llm/src/discovery/endpoint_card.rs
  • lib/llm/src/discovery/kv_source_watch.rs
  • lib/llm/src/discovery/runtime_configs.rs
  • lib/llm/src/discovery/watcher.rs
  • lib/llm/src/kv_dc_relay/discovery.rs
  • lib/llm/src/kv_router/indexer/recovery/direct_zmq.rs
  • lib/llm/src/local_model.rs
  • lib/llm/src/model_card.rs
  • lib/runtime/src/component/client.rs
  • lib/runtime/src/discovery/kube.rs
  • lib/runtime/src/discovery/kv_store.rs
  • lib/runtime/src/discovery/metadata.rs
  • lib/runtime/src/discovery/mock.rs
  • lib/runtime/src/discovery/mod.rs
  • lib/runtime/src/discovery/utils.rs
  • lib/runtime/src/storage/kv.rs
  • lib/runtime/src/storage/kv/etcd.rs
  • lib/runtime/src/storage/kv/file.rs
  • lib/runtime/src/storage/kv/mem.rs
  • lib/runtime/src/storage/kv/nats.rs
  • lib/runtime/src/transports/etcd.rs
  • lib/runtime/src/transports/event_plane/dynamic_subscriber.rs
  • tests/router/counter_worker.py
  • tests/router/test_router_e2e_with_mockers.py

Comment thread components/src/dynamo/common/model_taints.py
Comment thread components/src/dynamo/sglang/request_handlers/handler_base.py
Comment thread lib/runtime/src/storage/kv/file.rs Outdated

tmonty12 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Responses to the review-body-only items (the corresponding inline items 1–5 were answered in their threads):

  1. No change. counter_worker.py keeps the Dynamo imports inside main() after CUDA_VISIBLE_DEVICES is derived from device_type, so both direct CLI and process-wrapper paths preserve the required import order.

  2. Addressed in 958ff92ac0. The Bucket::compare_and_replace contract now documents that successful revisions are backend-specific and neither cross-backend comparable nor globally monotonic.

  3. Addressed in 958ff92ac0. Kubernetes discovery logs only the event kind and instance ID at info level; the full event is emitted at debug level.

  4. Deferred. The current two-round-trip etcd CAS is correct, and collapsing it to one transaction is a performance-only optimization unrelated to the model-taint MVP. This should be handled as a focused follow-up.

  5. Addressed in 958ff92ac0. The NATS integration test now deletes its per-run JetStream bucket after assertions; the focused NATS test passed against the local service.

  6. Deferred. Consolidating the duplicated FileStore temp-file cleanup is cosmetic and independent of the model-taint behavior; keeping it out avoids expanding this PR's refactor surface.

  7. Addressed in 958ff92ac0. Async FileStore mutations now use LOCK_EX | LOCK_NB with bounded async retries instead of blocking a Tokio worker indefinitely. A current-thread regression test proves timers continue to make progress while another process holds the mutation lock.

  8. Addressed in 958ff92ac0. Model-taint CAS conflicts are capped at eight attempts and return StoreError::Retry after exhaustion. A deterministic conflicting-bucket test verifies the exact attempt bound and returned error.

@tmonty12
tmonty12 force-pushed the tmonty12/dyn-10703-update-model-taints branch 2 times, most recently from 1f0dec3 to 0f612ac Compare August 7, 2026 16:33
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@PeaBrane PeaBrane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI-assisted router-only review. I left five inline findings covering discovery authority and event ordering, route reservation, and scheduler update amplification.

Comment thread lib/runtime/src/storage/kv/mem.rs
Comment thread lib/runtime/src/discovery/kube.rs Outdated
Comment thread lib/runtime/src/storage/kv/file.rs
Comment thread lib/backend-common/src/worker.rs
Comment thread lib/kv-router/src/scheduling/local.rs

@PeaBrane PeaBrane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@PeaBrane PeaBrane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compatibility note for posterity and future AI-assisted work: this PR and #12890 are semantically compatible and fit the same facts → derived-state pattern well.

#12890 treats structural model discovery/materialization and E/P/D rendezvous as level-triggered projections of current committed worker facts. This PR treats worker taints as a separate mutable runtime-routing projection: ModelTaintsUpdated updates current per-worker configuration without changing mdcsum(), WorkerSet materialization identity, or rendezvous topology.

The expected integration is small: the structural controller explicitly ignores ModelTaintsUpdated, while the runtime-config watcher consumes it and wakes scheduler re-evaluation. No major semantic conflict resolution is foreseen; the two paths have separate authority domains and compose cleanly.

Cross-reference: #12890

@tmonty12
tmonty12 force-pushed the tmonty12/dyn-10703-update-model-taints branch from 46db6ab to 378a672 Compare August 10, 2026 22:15
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
Signed-off-by: Thomas Montfort <tjmontfort12@gmail.com>
@tmonty12
tmonty12 force-pushed the tmonty12/dyn-10703-update-model-taints branch from 378a672 to c6ab547 Compare August 10, 2026 22:18

@hhzhang16 hhzhang16 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

approved as GMS codeowner

@nv-anants nv-anants left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

reviewed deps changes, lgtm!

@KrishnanPrash KrishnanPrash left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approved to unblock

@tmonty12
tmonty12 merged commit 3c7f878 into main Aug 11, 2026
124 checks passed
@tmonty12
tmonty12 deleted the tmonty12/dyn-10703-update-model-taints branch August 11, 2026 17:07
keivenchang added a commit that referenced this pull request Aug 11, 2026
Regenerated with Python 3.13 to match the CI interpreter.

These four pages are main's drift, not this PR's: 3c7f878
(feat(router): add dynamic worker taint updates, #12620) and later
commits changed curated Python API source, and the freshness gate
attributes them here because BASE_SHA stays pinned to this PR's
original base while the job runs against refs/pull/N/merge, which
already contains current main.

frontend.mdx is unchanged by this commit; it was already correct.

Generated with docs/fern/scripts/gen_python_api.py; no hand edits.

Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
MatejKosec added a commit to glamr-agent/dynamo that referenced this pull request Aug 12, 2026
I added libc to [dev-dependencies] in the previous commit to make this test
compile, and both the fix and its explanation were wrong.

The explanation first. I wrote that libc had compiled before because something
else in dynamo-runtime's graph pulled it in. Rust never resolves a libc:: path
through a transitive dependency. libc was a direct dependency of lib/runtime
until ai-dynamo#12620 deleted it, which is why the branch stopped compiling once I merged
main.

The section was wrong too. lib/runtime/src/nvtx.rs and
lib/runtime/src/pipeline/network/egress/tcp_client.rs both call libc:: outside
tests, under the nvtx and tcp-low-latency features. A dev-dependency does not
link into those builds, so my change would not have helped them.

This test needs no new dependency at all. socket2 0.5 is already a dependency of
this crate, socket2::SockRef::from accepts anything implementing AsFd, and
tokio's TcpListener implements it. SockRef::shutdown issues the same syscall
with no unsafe block and no raw descriptor, so I reverted lib/runtime/Cargo.toml
and Cargo.lock to main.

I also corrected the test's platform handling. It asserted the shutdown returned
0, and its doc comment predicted that a platform refusing the call would leave
the test passing. Darwin refuses shutdown on a listening socket with ENOTCONN,
so the test would have failed there rather than passing. It now returns early
when the platform refuses, because a listener that never broke cannot exercise
the rebind path.

libc missing from lib/runtime's [dependencies] still breaks the nvtx and
tcp-low-latency feature builds. That break came from ai-dynamo#12620 and I have not
fixed it here.

I could not build this workspace locally. CI on this pull request is the check.

Signed-off-by: Matej Kosec <mkosec@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend::sglang Relates to the sglang backend backend::trtllm Relates to the trtllm backend backend::vllm Relates to the vllm backend documentation Improvements or additions to documentation feat multimodal router Relates to routing, KV-aware routing, etc. size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants