Repository navigation
[Fix] scheduler: Publish KV metrics when idle - #45785
Conversation
|
/tag-and-rerun-ci |
This comment has been minimized.
This comment has been minimized.
ishandhanani
left a comment
There was a problem hiding this comment.
Round 1 AI review
Automated review (round 1 of up to 2 before a human review). Findings may be wrong; reply on a thread to dispute one.
This PR makes the idle-metrics path call kv_events_publisher.emit_kv_metrics() right after log_stats(). Before, a router that reads KvMetrics kept seeing the last batch's kv_active_blocks, request_active_slots and num_requests_waiting after the scheduler went idle. The one-line production change is correct and matches the two batch-report call sites. It keeps the current_scheduler_metrics_enabled gate and the 30 s rate limit, and emit_kv_metrics returns early when KV events are off. Two CPU unit tests fail at the head SHA because their test setup does not match current main; both errors appear in the base-a-test-cpu (1) and (4) logs. The Lint failure is not from this PR: it is an F821 in xpu_graph_runner.py.
New findings: 2 major
The next AI round runs after new commits are pushed.
Rename the prefill queue stub to disagg_prefill_transfer_queue, stub pre_allocated_token_counts and pool capacity fields for the decode path, and give the HiCache idle-report fixture a kv_events_publisher.
ishandhanani
left a comment
There was a problem hiding this comment.
Round 2 AI review
Automated review (round 2 of up to 2 before a human review). Findings may be wrong; reply on a thread to dispute one.
This PR makes the scheduler's idle-metrics path call kv_events_publisher.emit_kv_metrics(). Without it, a router reading KvMetrics keeps the last batch's active-block, active-slot and waiting counts while the scheduler is idle. The new commit c898c2c890 fixes the test setup only; the production change is the same one-line call reviewed in round 1. Both round-1 findings are fixed, and all 18 base-a-test-cpu shards now pass at c898c2c890. I found no new issues.
New findings: none
Earlier findings: 2 fixed, 0 no longer apply, 0 still open
AI review is done; a human reviewer takes it from here.
|
/rerun-test registered/unit/observability/test_forward_pass_metrics.py registered/unit/managers/test_disagg_idle_step_counters.py registered/unit/managers/test_scheduler_on_idle_load.py |
|
🚀 |
Upstream sgl-project#45785 moved the queue counters into _update_queue_stats() and re-added an unguarded self.stats.cache_hit_rate = cache_hit_rate in report_prefill_stats. Take the extraction and drop the assignment, so _record_prefill_cache_hit_rate stays the only writer of the gauge.
Co-authored-by: ishandhanani <82981111+ishandhanani@users.noreply.github.com> Co-authored-by: Ishan Dhanani <ishandhanani@gmail.com>
Motivation
When the scheduler becomes idle, it refreshes its local and Prometheus statistics without publishing
KvMetrics. Consumers such as Dynamo can retain the last busy-batch KV usage and continue rejecting requests after the engine is idle.Modifications
_maybe_log_idle_metrics(), preserving the existing metrics gate and idle reporting interval.Accuracy Tests
Speed Tests and Profiling
N/A: no model or kernel changes. The GPU checks above validate admission recovery.
Checklist
CI States
Latest PR Test (Base): ✅ Run #41420374872
Latest PR Test (Extra): ❌ Run #41420375008
Latest PR Test (AMD ROCm 10): ✅ Run #41420374992
Tip
Base CI only covers the core cases, and its workload may not exercise your change. Use
/rerun-testto run the tests your change touches, or copy this prompt for an agent: