Enhance cache service metrics #13
Conversation
|
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
change cache metrics to maintain fixed-sized queue Adding metrics to llm engine stats and exposed through prometheus gauge Adding metrics to llm engine stats and exposed through prometheus gauge clear metrics counters upon update
happyandslow
force-pushed
the
lexu/measurement-flags
branch
from
f93fa67 to
bc4f074
Compare
|
@happyandslow please help address comments as well. We need to ack reviewer's comments. |
vllm/engine/metrics.py
Outdated
| @@ -190,6 +190,89 @@ def __init__(self, labelnames: List[str], max_model_len: int): | |||
| labelnames=labelnames, | |||
| multiprocess_mode="sum", | |||
| ) | |||
|
|
|||
| self.gauge_cache_service_tokens_hit_rate = self._gauge_cls( | |||
| name="vllm:gauge_cache_service_tokens_hit_rate", | |||
There was a problem hiding this comment.
the naming is kind of inconsistent here. Other metric string doesn't include the metric type. for example, "vllm:gauge_cache_service_tokens_hit_rate" -> "vllm:cache_service_tokens_hit_rate"
can you update the name to be consistent?
There was a problem hiding this comment.
Check existing metrics names -- all metrics start with the metrics types they use (e.g., line vllm-project#81 self.gauge_gpu_prefix_cache_hit_rate = self._gauge_cls(...)
|
@happyandslow Could you look at the review comments? Seems the new commit didn't address them. |
addressing comments
vllm/engine/llm_engine.py
Outdated
| self.cache_service_metrics.time_load.clear() | ||
| self.cache_service_metrics.time_reshape.clear() | ||
| self.cache_service_metrics.time_unload.clear() | ||
| self.cache_service_metrics.time_update.clear() |
There was a problem hiding this comment.
Can we simply swap the corresponding lists, e.g., cache_service_time_query (initially empty) with self.cache_service_metrics.time_query, instead of of copying the whole list of elements?
| duration = copy_start.elapsed_time(copy_end) / 1000.0 | ||
| self.metrics.add_time_unload(duration) | ||
|
|
||
| torch.cuda.synchronize() |
There was a problem hiding this comment.
Is this torch.cuda.synchronize() being used to measure execution time accurately, or is it intended as a synchronization barrier between the copy_ and reshape_and_cache_flash?
No description provided.