[Bugfix]: Fix Promethus spec decode counter sum-of-sums #15415
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
The Prometheus spec decode counters (draft/accepted/emitted token counts) are incremented by the values in spec_decode_metrics. However, those values are aggregates since startup. Therefore, the Prometheus counters are effectively a sum-of-sums instead of just a sum. If a high-traffic vLLM is left on for a few hours those counters start to suggest absurdly high values like a TPS in the tens of millions. Signed-off-by: Adam Lugowski <adam.lugowski@parasail.io>
markmc
left a comment
markmc
left a comment
There was a problem hiding this comment.
Good catch. I'm personally focused on V1 so I'm not super familiar with V0, but it does appear from the code that these values are cumulative
I've made some pretty minor suggestions for tweaking your change
Thanks!
| self.last_local_log = time.time() | ||
| self.local_interval = local_interval | ||
| self.spec_decode_metrics: Optional[SpecDecodeWorkerMetrics] = None | ||
| self.last_spec_decode_metrics: Optional[SpecDecodeWorkerMetrics] = None |
There was a problem hiding this comment.
This state could be on PrometheusStatLogger since it's not used by LoggingStatLogger
| if self.spec_decode_metrics is not None: | ||
| # The counters in self.spec_decode_metrics are aggregates. | ||
| # The Prometheus Counters must be incremented with deltas. | ||
| # Keep track of the previous value so we can compute deltas. |
There was a problem hiding this comment.
I'd be inclined to do something like this:
def log_counter_from_cumulative(metric, cumulative, previous):
self._log_counter(metric, cumulative - previous)
The method name helps the code to be self-documenting, requiring less comments
and then:
self._log_from_cumulative(
self.metrics.counter_spec_decode_num_accepted_tokens,
self.spec_decode_metrics.accepted_tokens,
self.spec_decode_prev_num_accepted)
self.spec_decode_prev_num_accepted = self.spec_decode_metrics.accepted_tokens
since we don't need all of SpecDecodeWorkerMetrics
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has merge conflicts that must be resolved before it can be |
github-actions
bot
added
unstale
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
github-actions
bot
added
stale
|
The affected v0 code has since been removed |
3 participants
The Prometheus spec decode counters (draft/accepted/emitted token counts) are incremented by the values in
spec_decode_metrics. However, those values are aggregates since startup. Therefore, the Prometheus counters are effectively a sum-of-sums instead of just a sum.If a high-traffic vLLM is left on for a few hours those counters start to suggest absurdly high values like a TPS in the tens of millions.
The values in
spec_decode_metricsare used by other stat reporters such as the command-line logger, so those cannot be converted to only store deltas. Instead this PR modifiesPrometheusStatLoggerto compute the deltas itself and properly increment the PrometheusCountermetrics.