[V1][Metrics] Allow V1 AsyncLLM to use custom logger #14661

liuzijing2014 · 2025-03-12T07:15:23Z

Content

Update V1 AsynLLM to take in custom logger that extends StatLoggerBase. This would help to achieve function on parity as V0 regarding customization support for logging.

Follow V0 style on customizing loggers:

Take in optional stat_loggers from init.
Add add_logger and remove_logger functions.

Test Plan

pytest tests/entrypoints/openai/test_metrics.py 
...
======================== 13 passed, 1 skipped in 207.14s (0:03:27) ========================


pytest tests/v1/engine/test_async_llm.py
...
======================= 10 passed, 10 warnings in 160.87s (0:02:40) =======================

github-actions · 2025-03-12T07:15:31Z

👋 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 fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 ready label to the PR or enable auto-merge.

🚀

robertgshaw2-redhat · 2025-03-12T11:24:22Z

@markmc

markmc

Ok, this quite closely matches the V0 interface, but not quite - e.g. the record() method is new, and we're using new classes like IterationStats, and SchedulerStats

Personally, I'm not a fan of having all of this be part of the public API in such an ad-hoc way ... the logger behaviors and the stats classes etc. are all liable to change in future IMO, breaking users of the API

However, if this is something we want to retain from V0, then this PR looks reasonable

markmc · 2025-03-12T12:35:20Z

vllm/third_party/pynvml.py

+import os
+import string
+import sys
+import threading


Remove this change from the PR

This is the auto format for imports ordering from isort. Is this formatting not desired?

@liuzijing2014 the formatting change is to an unrelated file so shouldn't be included in the same PR.

No, it is not desired

This code is copied from nvidia-ml-py and as per #12977:

If there are bugfixes in nvidia-ml-py in the future, we can periodically sync the code.

So, we would prefer not to modify this code from the original

We do use isort, but only using pre-commit and in .pre-commit-config.yaml we exclude this directory:

exclude: 'vllm/third_party/.*'

So, for example, if I checkout your branch and do:

$ pre-commit run --all-files isort

this file is not modified because of the exclude

Hope that helps!

markmc · 2025-03-12T12:38:15Z

vllm/v1/engine/async_llm.py

+                    self.stat_loggers[STANDARD_LOGGING_LOGGER_NAME] = (
+                        LoggingStatLogger())
+                self.stat_loggers[PROMETHEUS_LOGGING_LOGGER_NAME] = (
+                    PrometheusStatLogger(vllm_config))


How about we move all of this into the loggers module so that we just do e.g.

self.stat_loggers = loggers.setup(vllm_config, stat_loggers)

njhill · 2025-03-12T15:11:34Z

Agree with @markmc these interfaces / data structures may still be in flux so we may want to be careful exposing as part of an external API at this stage. If we do it should be clearly documented as unstable.

liuzijing2014 · 2025-03-12T17:37:03Z

Ok, this quite closely matches the V0 interface, but not quite - e.g. the record() method is new, and we're using new classes like IterationStats, and SchedulerStats

Personally, I'm not a fan of having all of this be part of the public API in such an ad-hoc way ... the logger behaviors and the stats classes etc. are all liable to change in future IMO, breaking users of the API

However, if this is something we want to retain from V0, then this PR looks reasonable

@markmc The main motivation is to allow users to have basic logging parity between V0 and V1 (v1 feedback thread ). We have some customized logging backend that we need to log stats to, and switching to V1 breaks the stats logging support for us.

@njhill Proper documentation sounds reasonable. Besides inline code comments, is there any other place that you think we need to call this part out?

mergify · 2025-03-12T19:57:55Z

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

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

markmc · 2025-03-13T13:39:01Z

See liuzijing2014#1 for some changes to finish this PR up. Hope that helps!

Commits at https://github.com/markmc/vllm/commits/5caa8fbfb00e3a23f4d461fac2f26e059c5efd7e

markmc · 2025-03-13T13:40:03Z

Agree with @markmc these interfaces / data structures may still be in flux so we may want to be careful exposing as part of an external API at this stage. If we do it should be clearly documented as unstable.

In liuzijing2014#1 I added:

class BuiltinLoggerName(str, Enum):
    """Internal names for built-in metric loggers                                                                                                                                                           
                                                                                                                                                                                                            
    These values are intended for internal use only. Accessing to                                                                                                                                           
    these built-in loggers is not considered to be part of the                                                                                                                                              
    public API.                                                                                                                                                                                             
    """
    LOGGING = "logging"
    PROMETHEUS = "prometheus"


class StatLoggerBase(ABC):
    """Interface for logging metrics.                                                                                                                                                                       
                                                                                                                                                                                                            
    API users may define custom loggers that implement this interface.                                                                                                                                      
    However, note that the `SchedulerStats` and `IterationStats` classes                                                                                                                                    
    are not considered stable interfaces and may change in future versions.                                                                                                                                 
    """

njhill

@njhill Proper documentation sounds reasonable. Besides inline code comments, is there any other place that you think we need to call this part out?

I don't think so. The main place is in the docstring for the new stat_loggers arg to AsyncLLM (and also the add/remove methods but I would prefer to just omit those for now).

njhill · 2025-03-13T13:25:09Z

vllm/third_party/pynvml.py

+import os
+import string
+import sys
+import threading


@liuzijing2014 the formatting change is to an unrelated file so shouldn't be included in the same PR.

njhill · 2025-03-13T13:33:09Z

vllm/v1/engine/async_llm.py

+        self.stat_loggers: dict[str, StatLoggerBase] = dict()
        if self.log_stats:
-            if logger.isEnabledFor(logging.INFO):
-                self.stat_loggers.append(LoggingStatLogger())
-            self.stat_loggers.append(PrometheusStatLogger(vllm_config))
+            if stat_loggers is not None:
+                self.stat_loggers = stat_loggers
+            else:
+                setup_default_loggers(vllm_config,
+                                      logger.isEnabledFor(logging.INFO),
+                                      self.stat_loggers)


Suggest moving all of this logic into the setup function:

self.stat_loggers = setup_default_loggers(vllm_config, log_stats, stat_loggers)

Also:

No need to pass in logger.isEnabledFor(logging.INFO), that can be done inside the function

It would probably be cleaner to eliminate self.log_stats. Empty self.stat_loggers can serve that purpose (or possibly None)

njhill · 2025-03-13T13:34:28Z

vllm/v1/metrics/loggers.py

+        PrometheusStatLogger(vllm_config))
+
+    if enable_stats_log:
+        stat_loggers[STANDARD_LOGGING_LOGGER_NAME] = (LoggingStatLogger())


Suggested change

stat_loggers[STANDARD_LOGGING_LOGGER_NAME] = (LoggingStatLogger())

stat_loggers[STANDARD_LOGGING_LOGGER_NAME] = LoggingStatLogger()

njhill · 2025-03-13T13:35:11Z

vllm/v1/metrics/loggers.py

+STANDARD_LOGGING_LOGGER_NAME = "logging"
+PROMETHEUS_LOGGING_LOGGER_NAME = "prometheus"


nit: unnecessarily long names

Suggested change

STANDARD_LOGGING_LOGGER_NAME = "logging"

PROMETHEUS_LOGGING_LOGGER_NAME = "prometheus"

STANDARD_LOGGER_NAME = "logging"

PROMETHEUS_LOGGER_NAME = "prometheus"

njhill · 2025-03-13T13:37:32Z

vllm/v1/engine/async_llm.py

+    def add_logger(self, logger_name: str, logger: StatLoggerBase) -> None:
+        if not self.log_stats:
+            raise RuntimeError(
+                "Stat logging is disabled. Set `disable_log_stats=False` "
+                "argument to enable.")
+        if logger_name in self.stat_loggers:
+            raise KeyError(f"Logger with name {logger_name} already exists.")
+        self.stat_loggers[logger_name] = logger
+
+    def remove_logger(self, logger_name: str) -> None:
+        if not self.log_stats:
+            raise RuntimeError(
+                "Stat logging is disabled. Set `disable_log_stats=False` "
+                "argument to enable.")
+        if logger_name not in self.stat_loggers:
+            raise KeyError(f"Logger with name {logger_name} does not exist.")
+        del self.stat_loggers[logger_name]
+


Are these methods really necessary if you can pass in the dict?

I add them here to keep the interface on parity as V0. Either way could work.

My preference would be to exclude them for now, since the API is different to V0 anyhow. We're trying to avoid as much unnecessary code in V1 as possible.

njhill

Thanks @liuzijing2014 it looks good now, just have a couple more minor comments.

vllm/v1/engine/async_llm.py

njhill · 2025-03-19T16:51:21Z

vllm/v1/engine/async_llm.py

-                self.stat_loggers.append(LoggingStatLogger())
-            self.stat_loggers.append(PrometheusStatLogger(vllm_config))
+        self.stat_loggers = setup_default_loggers(
+            vllm_config, self.log_stats, logger.isEnabledFor(logging.INFO),


Move the logger.isEnabledFor(logging.INFO) check inside the method?

@liuzijing2014 WDYT about this suggestion?

vllm/v1/metrics/loggers.py

markmc · 2025-04-24T14:48:35Z

@njhill FYI

Error: vllm/v1/metrics/loggers.py:510: error: Name "factories" already defined on line 508  [no-redef]

mergify · 2025-04-24T14:55:14Z

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

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

njhill · 2025-04-24T15:32:10Z

@njhill FYI

Error: vllm/v1/metrics/loggers.py:510: error: Name "factories" already defined on line 508  [no-redef]

@markmc oops, now fixed

njhill

@markmc @liuzijing2014 I think this is ready now?

liuzijing2014 · 2025-04-25T21:05:38Z

LGTM, thanks for all the help! @njhill @markmc

Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Zijing Liu <[email protected]>

More general than passing sub-class types, requiring sub-class constructors to conform to a protocol. Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Zijing Liu <[email protected]>

Signed-off-by: Zijing Liu <[email protected]>

Signed-off-by: Nick Hill <[email protected]> Signed-off-by: Zijing Liu <[email protected]>

) Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Nick Hill <[email protected]> Co-authored-by: Mark McLoughlin <[email protected]> Co-authored-by: Nick Hill <[email protected]>

) Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Nick Hill <[email protected]> Co-authored-by: Mark McLoughlin <[email protected]> Co-authored-by: Nick Hill <[email protected]> Signed-off-by: Agata Dobrzyniewicz <[email protected]>

) Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Nick Hill <[email protected]> Co-authored-by: Mark McLoughlin <[email protected]> Co-authored-by: Nick Hill <[email protected]> Signed-off-by: Mu Huai <[email protected]>

) Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Nick Hill <[email protected]> Co-authored-by: Mark McLoughlin <[email protected]> Co-authored-by: Nick Hill <[email protected]> Signed-off-by: Yuqi Zhang <[email protected]>

) Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Nick Hill <[email protected]> Co-authored-by: Mark McLoughlin <[email protected]> Co-authored-by: Nick Hill <[email protected]> Signed-off-by: minpeter <[email protected]>

mergify bot added the v1 label Mar 12, 2025

markmc suggested changes Mar 12, 2025

View reviewed changes

mergify bot added needs-rebase documentation Improvements or additions to documentation ci/build frontend multi-modality Related to multi-modality (#4194) and removed needs-rebase labels Mar 12, 2025

liuzijing2014 force-pushed the v1-custom-logger branch 2 times, most recently from 38bf2d6 to c9b95af Compare March 12, 2025 22:30

liuzijing2014 marked this pull request as ready for review March 12, 2025 22:39

liuzijing2014 requested review from WoosukKwon, robertgshaw2-redhat, njhill, ywang96, comaniac and alexm-redhat as code owners March 12, 2025 22:39

liuzijing2014 requested a review from markmc March 12, 2025 22:39

markmc mentioned this pull request Mar 13, 2025

PR feedback liuzijing2014/vllm#1

Merged

njhill reviewed Mar 13, 2025

View reviewed changes

njhill reviewed Mar 19, 2025

View reviewed changes

markmc approved these changes Mar 20, 2025

View reviewed changes

liuzijing2014 requested a review from mgoin as a code owner March 26, 2025 01:10

markmc removed ci/build multi-modality Related to multi-modality (#4194) labels Apr 23, 2025

russellb removed this from Structured Output Apr 23, 2025

njhill reviewed Apr 23, 2025

View reviewed changes

vllm/v1/metrics/loggers.py Outdated Show resolved Hide resolved

mergify bot added the needs-rebase label Apr 24, 2025

mergify bot removed the needs-rebase label Apr 24, 2025

njhill approved these changes Apr 25, 2025

View reviewed changes

njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Apr 25, 2025

liuzijing2014 force-pushed the v1-custom-logger branch 2 times, most recently from 86a1452 to 9f08bd5 Compare April 25, 2025 21:49

liuzijing2014 and others added 4 commits April 25, 2025 17:45

[V1][Metrics] Allow AsyncLLM engine to use custom stat logger

925cf5a

Signed-off-by: Zijing Liu <[email protected]> Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Zijing Liu <[email protected]>

[V1][Metrics] Define StatLoggerFactory type

1aa7283

More general than passing sub-class types, requiring sub-class constructors to conform to a protocol. Signed-off-by: Mark McLoughlin <[email protected]> Signed-off-by: Zijing Liu <[email protected]>

Update vllm/v1/metrics/loggers.py

f38f893

Signed-off-by: Zijing Liu <[email protected]>

Fix typing

303e7ef

Signed-off-by: Nick Hill <[email protected]> Signed-off-by: Zijing Liu <[email protected]>

liuzijing2014 force-pushed the v1-custom-logger branch from 9f08bd5 to 303e7ef Compare April 26, 2025 00:46

njhill merged commit 53e8cf5 into vllm-project:main Apr 26, 2025
43 checks passed

liuzijing2014 deleted the v1-custom-logger branch April 28, 2025 23:53

ckhordiasma mentioned this pull request May 14, 2025

nm vllm ent 0.8.5 sync red-hat-data-services/vllm#139

Merged

	stat_loggers[STANDARD_LOGGING_LOGGER_NAME] = (LoggingStatLogger())
	stat_loggers[STANDARD_LOGGING_LOGGER_NAME] = LoggingStatLogger()

		STANDARD_LOGGING_LOGGER_NAME = "logging"
		PROMETHEUS_LOGGING_LOGGER_NAME = "prometheus"

Uh oh!

[V1][Metrics] Allow V1 AsyncLLM to use custom logger #14661

[V1][Metrics] Allow V1 AsyncLLM to use custom logger #14661

Uh oh!

Conversation

liuzijing2014 commented Mar 12, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Content

Test Plan

Uh oh!

github-actions bot commented Mar 12, 2025

Uh oh!

robertgshaw2-redhat commented Mar 12, 2025

Uh oh!

markmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill commented Mar 12, 2025

Uh oh!

liuzijing2014 commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Mar 12, 2025

Uh oh!

markmc commented Mar 13, 2025

Uh oh!

markmc commented Mar 13, 2025

Uh oh!

njhill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njhill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markmc commented Apr 24, 2025

Uh oh!

mergify bot commented Apr 24, 2025

Uh oh!

njhill commented Apr 24, 2025

Uh oh!

njhill left a comment

Choose a reason for hiding this comment

Uh oh!

liuzijing2014 commented Apr 25, 2025

Uh oh!

Uh oh!

Uh oh!

liuzijing2014 commented Mar 12, 2025 •

edited by github-actions bot

Loading

liuzijing2014 commented Mar 12, 2025 •

edited

Loading