Repository navigation
Conversation
Signed-off-by: Harry Lee <harrli@nvidia.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe policy queue exposes ChangesPolicy queue APIs
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~15 minutes Merge Risk: ⚪ Minimal · up to This change exposes two policy queue operations and adds tests for them. No merge-blocking risk is apparent. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
| pub fn take_if(&mut self, mut predicate: impl FnMut(&T) -> bool) -> Vec<PolicyQueueEntry<T>> { | ||
| let mut removed = Vec::new(); | ||
| for class_index in 0..self.classes.len() { | ||
| removed.extend(self.take_if_in_class(class_index, &mut predicate).0); |
There was a problem hiding this comment.
take_if discards the removed_ready_head result from take_if_in_class. If a worker lane was marked blocked and this method removes that lane's head while retaining another entry, rebuild_worker_heads preserves the blocked marker and excludes the new head from candidate_worker_heads. Subsequent pop_next calls therefore return None even when the new head is dispatchable, until an unrelated worker recheck occurs. Recheck affected classes whenever removal changes a ready head.
🤖 AI Fix
Capture removed_ready_head for each class and schedule a blocked-worker recheck for that class before returning.
There was a problem hiding this comment.
Confirmed: dropping removed_ready_head can strand the retained successor. I reproduced the case and checked a local recheck fix (27 queue tests passed). That fix is not in this public head. We are withdrawing these queue helpers because the corrected A/B/C boundary no longer needs them: #15631 exposes the native policy interface, AISimulate #306 consumes it, and the simplified #15240 removes this helper dependency. Closing this PR rather than merging the defective or unnecessary API.
| pub fn take_if(&mut self, mut predicate: impl FnMut(&T) -> bool) -> Vec<PolicyQueueEntry<T>> { | ||
| let mut removed = Vec::new(); | ||
| for class_index in 0..self.classes.len() { | ||
| removed.extend(self.take_if_in_class(class_index, &mut predicate).0); |
There was a problem hiding this comment.
take_if discards the helper's removed_ready_head signal without scheduling a worker-lane recheck. If pop_next previously blocked an exact-worker lane because its head failed a request-dependent dispatch predicate, removing that head leaves the worker in blocked_workers; a dispatchable successor is then never examined until an unrelated recheck_all_workers call. Mark affected blocked lanes for lazy rechecking during removal and add a regression test covering blocked-head removal followed directly by pop_next.
🤖 AI Fix
Schedule a blocked-worker recheck whenever take_if removes a ready lane head so the next pop_next evaluates the retained successor.
There was a problem hiding this comment.
Confirmed: dropping removed_ready_head can strand the retained successor. I reproduced the case and checked a local recheck fix (27 queue tests passed). That fix is not in this public head. We are withdrawing these queue helpers because the corrected A/B/C boundary no longer needs them: #15631 exposes the native policy interface, AISimulate #306 consumes it, and the simplified #15240 removes this helper dependency. Closing this PR rather than merging the defective or unnecessary API.
|
Superseded by #15631, which is the actual independent native-policy interface consumed by AISimulate #306. This PR only adds queue helpers used by the former #15240 implementation; it does not satisfy that provider boundary. The revised consumer removes its need for these helpers. Closing this PR to keep the intended sequence: Dynamo #15631 (A) → AISimulate #306 (B) → publish a matching AISimulate nightly → Dynamo #15240 (C). #307 is merged and #15149 remains closed. The blocked-head issue identified in review is valid; a local fix and regression were checked, but no fix was pushed to this now-withdrawn implementation. |
Summary
Allow a host of Dynamo's native
PolicyQueueto recover canceled or invalidated requests and retry blocked worker lanes without dispatching requests to extract them. This is the independent native-queue prerequisite extracted from #15240; it does not call AISimulate or change any AISimulate, Python, Rust, container, or release dependency.recheck_all_workersoperation public. It marks blocked lanes for rechecking on the nextpop_nextcall.take_if, reusing existing class removal/accounting machinery to return ownership of matching entries. Retained entries keep their scheduling keys, deadlines and order; removal does not spend DRR credit. Empty classes retain the existing credit-reset behavior.retaindiscards removed payloads, whereas cancellation/retry needs their ownership. Extracting throughpop_nextwould perform dispatch and spend scheduling credit. Both methods remain useful without the replay consumer.Validation
cargo test --locked -p dynamo-kv-router scheduling::policy_queue::tests --lib: 26 passed, including deadline/accounting/credit preservation, ownership transfer across classes, and existing blocked-worker recheck regressions.rustfmt --check --edition 2024 lib/kv-router/src/scheduling/policy_queue.rsandgit diff --checkpassed.lib/kv-router/src/scheduling/policy_queue.rschanges; dependency manifests, lockfiles and Ops files are unchanged.Where should the reviewer start?
PolicyQueue::take_ifand its two contract tests inlib/kv-router/src/scheduling/policy_queue.rs.Related Issues
Relates to #15240 (consumer follow-up) and AIC-1817.
AISimulate #307 has merged. The consumer in #15240 also requires AISimulate #306 and a subsequently published matching AISimulate wheel/crate version. This small PR is independently mergeable; it does not deliver the complete conversation-aware CLI feature. #15149 remains closed.
Summary by CodeRabbit