Skip to content

Allow configuring DP update interval with an env var - #48027

Closed
omera-nv wants to merge 1 commit into
vllm-project:mainfrom
omera-nv:feat/configurable_dp_update_interval
Closed

omera-nv wants to merge 1 commit into
vllm-project:mainfrom
omera-nv:feat/configurable_dp_update_interval

Conversation

@omera-nv

@omera-nv omera-nv commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Purpose

When benchmarking a DP deployment with a burst of requests, if it takes longer than 100ms to submit the burst a race condition occurs between the core client's internal bookkeeping and the dp coordinator's periodic updates:

  • The coordinator sends and update
  • At the same time, the client submits a request to an engine and updates its internal bookkeeping
  • The client then recvs the update, overriding .lb_engines and losing the internal bookkeeping
  • Now the client will submit the next request to the same engine according to the old states

This creates an imbalance between DP ranks during the burst. In our experience, when benchmarking with uniform ISL:OSL, this leads to a perf hit that is especially felt in decode-heavy workloads (we've observed up to 10% difference in throughput between balanced and imbalanced benchmarks).

Ideally, with a burst of uniform requests, we want all ranks to work on the same number of requests (round robin), but this is currently only achievable with an external load balancer.

With this PR, round robin is achievable by using --api-server-count=1 and setting VLLM_DP_COORDINATOR_UPDATE_INTERVAL_MS to a large enough value that ensures the burst is submitted before a state update is published.

Test Plan

It seems the dp coordinator is not heavily tested, and adding special tests just for this minimal change seemed extreme. The old default value is kept so all tests and use cases that don't set the new env var should continue working as normal.

Test Result

N/A


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

Signed-off-by: Omer Ullman Argov <118735753+omera-nv@users.noreply.github.com>
@omera-nv
omera-nv requested a review from njhill as a code owner July 8, 2026 17:35

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@mergify mergify Bot added the v1 label Jul 8, 2026
@tomeras91

Copy link
Copy Markdown
Member

The code LGTM! Leaving it to @njhill for his thoughts on the approach..

@njhill njhill self-assigned this Jul 13, 2026
@njhill

njhill commented Jul 14, 2026

Copy link
Copy Markdown
Member

Thanks @omera-nv!

It would ideally be best to solve this without the env var, to make the balancing just work in this case.

There was actually a balancing fix merged very recently which should help with this #47420. Do you know whether this was included in the vLLM you used in your benchmark? If not it could be good to try that.

Also, I would recommend using the rust frontend (VLLM_USE_RUST_FRONTEND=1), which only ever has a single process rather than the possibly multiple python api server processes. The balancing done by that should also be tighter.

Finally, I do think the python logic can be improved here to better avoid such skew, I am working on a PR for that and will open it soon.

@omera-nv

omera-nv commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

There was actually a balancing fix merged very recently which should help with this #47420. Do you know whether this was included in the vLLM you used in your benchmark? If not it could be good to try that.

I was running all my tests with vllm nightly so this PR should have been included in my runs. It seems to attempt to tackle the same issue I'm facing but it doesn't quite fix my issue - I can see why it would help over several bursts, but I'm currently benchmarking a workload with very long decodes and even imbalance over a single burst degrades performance substantially. This happens since the effective number of tokens gets padded to the nearest cuda graph, so if I happen to send a single burst that spread 65/64/64/63, this imbalance+padding will make all DP ranks run with effectively 72 tokens each, leading to a 10% perf penalty.

Also, I would recommend using the rust frontend (VLLM_USE_RUST_FRONTEND=1), which only ever has a single process rather than the possibly multiple python api server processes. The balancing done by that should also be tighter.

This also doesn't solve the issue, since I'm already using --api-server-count=1. If the rust frontend also doesn't use the coordinator and its bookkeeping is self-contained then it will help - is this the case?

Finally, I do think the python logic can be improved here to better avoid such skew, I am working on a PR for that and will open it soon.

Thanks! I'll stay tuned :)

@BugenZhao

BugenZhao commented Jul 15, 2026

Copy link
Copy Markdown
Member

Also, I would recommend using the rust frontend (VLLM_USE_RUST_FRONTEND=1), which only ever has a single process rather than the possibly multiple python api server processes. The balancing done by that should also be tighter.

This also doesn't solve the issue, since I'm already using --api-server-count=1. If the rust frontend also doesn't use the coordinator and its bookkeeping is self-contained then it will help - is this the case?

Yes, the Rust frontend does bookkeeping itself, since we're sure there is always a single frontend process in such a deployment. When determining which engine to route to, the lower bound of the number of inflight requests is always preserved instead of being overwritten by possibly stale scheduler stats. IIUC this should mitigate the imbalance issue here.

Related: Inferact/vllm-frontend-rs#138

/// Compute the routing score used to pick the least-loaded engine.
///
/// Scheduler stats can raise the load estimate above the frontend-local
/// view, but they should not lower it below requests this frontend has
/// already admitted. Waiting requests still get the same extra penalty
/// as the original `waiting * 4 + running` score.
fn routing_score(&self) -> usize {
const WAITING_WEIGHT: usize = 4;
let Some(stats) = self.last_scheduler_stats else {
return self.inflight;
};
let scheduler_total = stats.running + stats.waiting;
self.inflight.max(scheduler_total) + stats.waiting * (WAITING_WEIGHT - 1)
}

@njhill

njhill commented Jul 18, 2026

Copy link
Copy Markdown
Member

@omera-nv could I ask what model you were testing with? Was it MoE?

@omera-nv

Copy link
Copy Markdown
Contributor Author

@njhill yes, it was nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-NVFP4 and nvidia/NVIDIA-Nemotron-3-Ultra-550B-A55B-NVFP4

@omera-nv

Copy link
Copy Markdown
Contributor Author

Wanted to update that I tested the rust backend and it seems to both solve the DP imbalance issue AND give a throughput boost on top of that, so I guess this PR can be closed, thanks!

@omera-nv omera-nv closed this Jul 19, 2026
@njhill

njhill commented Jul 20, 2026

Copy link
Copy Markdown
Member

Awesome, thanks @omera-nv!

@njhill

njhill commented Jul 20, 2026

Copy link
Copy Markdown
Member

@omera-nv FWIW I've now also opened #49204 with some additional refinements to the load balancing (which also fixes non-MoE balancing that was completely broken :-/)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants