[Feature]: Add corrupted request metric to V1 metrics system.

[atalhens]

Signed off: [email protected]

Purpose

The purpose of this PR is to fix: #27301 and #27298. This PR implements a new Prometheus metric vllm:num_requests_corrupted to track and monitor corrupted requests in vLLM. The feature provides visibility into request corruption issues that can occur during inference, enabling better monitoring and debugging capabilities.

• New Prometheus gauge metric vllm:num_requests_corrupted
• Console logging of corrupted request counts per logging interval.
• Enabled via the env variable VLLM_COMPUTE_NANS_IN_LOGITS flag for performance control, thus it won't impact any functionality unless enabled; the default value is set to False.
• Updated documentation and updated dashboard examples config as well.

Performance Considerations:

• Corrupted request counting is optional and disabled by default
• When enabled, it adds no computational overhead

Test Plan

• Not needed as it is a by-default disabled metric.

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.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[mergify]

Documentation preview: https://vllm--27306.org.readthedocs.build/en/27306/

[markmc]

Thanks for all your work on this - it's pretty obvious that #18777 has very limited value without this

And ... I know the way SchedulerStats.num_corrupted_reqs is currently implemented as a "count of currently running requests that are corrupted" leads to using a Gauge ...

But I really think a Counter is a more natural use of a time-series metric for this information. Probably that would mean adding is_output_corrupted to FinishedRequestStats and removing num_corrupted_reqs from SchedulerStats?

[markmc]

(sorry didn't mean to approve, wrong button!)

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @atalhens.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[atalhens]

Thanks for all your work on this - it's pretty obvious that #18777 has very limited value without this

And ... I know the way SchedulerStats.num_corrupted_reqs is implemented as a "count of currently running requests that are corrupted" leads to using a Gauge ...

But I really think a Counter is a more natural use of a time-series metric for this information. Probably that would mean adding is_output_corrupted to FinishedRequestStats and removing num_corrupted_reqs from SchedulerStats?

Thanks for the suggestion. Due to the way SchedulerStats was implemented, it previously did make sense to have it as a gauge metric. But definitely for time series metrics like this, it makes more sense as a counter metric. In this case, I suggest the following changes that align with your thought of changing SchedulerStats and introducing a new field over the FinishedStats.

1. Added is_corrupted flag in FinishedStats with default as False.
2. Added the num_corrupted_request field in IterationStats, within which update_from_output () helps update the response of a request from EngineCoreOutput. However, currently, it doesn't have any context about the nans in output logits.
3. Thus, added a field num_nans_in_logits that gets populated from the already implemented requests.num_nans_in_logits.
4. Added a field in LoggingStatsLogger num_corrupted_reqs which gets populated from the iteration stats.
5. Added Prometheus metric corrupted_requests: Number of requests corrupted out of running requests.

cc: @markmc

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @atalhens.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @atalhens.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[simon-mo]

@markmc reviewed

Have concern over the code

[simon-mo]

Can you further justify the need for VLLM_COMPUTE_NANS_IN_LOGITS env var and why can't this be enabled by default? I cannot find a place where we were doing CPU <> GPU syncs in counting NaNs here.

[markmc]

Can you further justify the need for VLLM_COMPUTE_NANS_IN_LOGITS env var and why can't this be enabled by default?

Looks like it was added in #18777 in response to this comment

I cannot find a place where we were doing CPU <> GPU syncs in counting NaNs here.

Yeah, looks like the original performance overhead concern wasn't very specific

[atalhens]

Can you further justify the need for VLLM_COMPUTE_NANS_IN_LOGITS env var and why can't this be enabled by default?

Looks like it was added in #18777 in response to this comment

I cannot find a place where we were doing CPU <> GPU syncs in counting NaNs here.

Yeah, looks like the original performance overhead concern wasn't very specific

Thanks for pointing out simon,
The env variable was kept to address the overhead concern raised in the previous PR mentioned by Marc above. This PR brought the implementation to compute _get_nans_in_output_logits.

About the GPU<>CPU sync to calculate the NaNs, it is called this way.
request.num_nans_in_logits = num_nans_in_logits[req_id] is gets populated from

num_nans_in_logits = model_runner_output.num_nans_in_logits and the model runner output utilizes the

vllm/vllm/v1/worker/gpu_model_runner.py

Line 3112 in 141e6a0

def _get_nans_in_logits(

In order to get nans, as it was gatekept here:

vllm/vllm/v1/worker/gpu_model_runner.py

Line 2247 in 141e6a0

if envs.VLLM_COMPUTE_NANS_IN_LOGITS:

by the environment and disabled by default, I utilized the same variable just to stay consistent.

cc: @simon-mo , @markmc

[simon-mo]

Can we quantify this for the users?

When running model X on hardware Y, there's x% throughput regression when sending metrics requests every 1s...

So users can understand the implication of turning this on.

[atalhens]

Hi, I ran the VLLM performance benchmarks with and without the flag being enabled to see the performance impact on a 2 H100 hardware node.
From my sight, I observed that enabling the flag adds an overhead of:

• About 1-2% in latency
• Though surprisingly, there was not a very big impact on throughput. I observed less than 1% in most of the scenarios; for high-throughput scenarios, it was around 3-4%
  T
  This doc includes the benchmark result. - Doc Link

Throughput Tests

Test name	Metric	Native	With VLLM_COMPUTE_NANS_IN_LOGITS flag enabled	Change (%)
throughput_llama8B_tp1	Tput (req/s)	22.7456	22.7126	-0.14
throughput_llama8B_tp1	Tput (tok/s)	9803.83	9789.58	-0.15
throughput_llama8B_tp1	Elapsed time (s)	8.79289	8.80569	+0.15
throughput_mixtral8x7B_tp2	Tput (req/s)	6.5888	6.5919	+0.05
throughput_mixtral8x7B_tp2	Tput (tok/s)	3250.45	3251.98	+0.05
throughput_mixtral8x7B_tp2	Elapsed time (s)	30.3546	30.3403	-0.05

---

In conclusion, I think this difference is significant enough to warrant flagging this feature.

Thanks!
Snehlata

cc: @markmc @simon-mo

[simon-mo]

Thank you for the benchmarks