-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Core] Enable command line logging for LLMEngine #25610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@zhuohan123 has exported this pull request. If you are a Meta employee, you can view the originating diff in D83118697. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request enables command-line logging for the LLMEngine by introducing a StatLoggerManager
and periodic logging logic. The changes are a good step towards better observability. My review focuses on improving the initialization of state related to logging to enhance code clarity and maintainability. Specifically, I've suggested moving the initialization of _last_log_time
to the __init__
method to centralize attribute setup and make the code easier to reason about.
self.logger_manager: Optional[StatLoggerManager] = None | ||
if self.log_stats: | ||
self.logger_manager = StatLoggerManager( | ||
vllm_config=vllm_config, | ||
custom_stat_loggers=stat_loggers, | ||
enable_default_loggers=log_stats, | ||
) | ||
self.logger_manager.log_engine_initialized() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better code clarity and to ensure all instance attributes are initialized in one place, it's recommended to initialize _last_log_time
here within the if self.log_stats:
block. This avoids lazy initialization in the do_log_stats_with_interval
method, makes the class's attributes more explicit, and improves overall maintainability.
self.logger_manager: Optional[StatLoggerManager] = None | |
if self.log_stats: | |
self.logger_manager = StatLoggerManager( | |
vllm_config=vllm_config, | |
custom_stat_loggers=stat_loggers, | |
enable_default_loggers=log_stats, | |
) | |
self.logger_manager.log_engine_initialized() | |
self.logger_manager: Optional[StatLoggerManager] = None | |
if self.log_stats: | |
self.logger_manager = StatLoggerManager( | |
vllm_config=vllm_config, | |
custom_stat_loggers=stat_loggers, | |
enable_default_loggers=log_stats, | |
) | |
self.logger_manager.log_engine_initialized() | |
self._last_log_time = time.time() |
def do_log_stats_with_interval(self) -> None: | ||
"""Log stats when the time interval has passed.""" | ||
now = time.time() | ||
if not hasattr(self, "_last_log_time"): | ||
self._last_log_time = now | ||
if now - self._last_log_time >= envs.VLLM_LOG_STATS_INTERVAL: | ||
self.do_log_stats() | ||
self._last_log_time = now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With _last_log_time
being initialized in __init__
, this lazy initialization check becomes redundant. Removing it simplifies the do_log_stats_with_interval
method, making it cleaner and more straightforward.
def do_log_stats_with_interval(self) -> None: | |
"""Log stats when the time interval has passed.""" | |
now = time.time() | |
if not hasattr(self, "_last_log_time"): | |
self._last_log_time = now | |
if now - self._last_log_time >= envs.VLLM_LOG_STATS_INTERVAL: | |
self.do_log_stats() | |
self._last_log_time = now | |
def do_log_stats_with_interval(self) -> None: | |
"""Log stats when the time interval has passed.""" | |
now = time.time() | |
if now - self._last_log_time >= envs.VLLM_LOG_STATS_INTERVAL: | |
self.do_log_stats() | |
self._last_log_time = now |
@zhuohan123 has exported this pull request. If you are a Meta employee, you can view the originating diff in D83118697. |
41f71a8
to
73f0a65
Compare
@zhuohan123 has exported this pull request. If you are a Meta employee, you can view the originating diff in D83118697. |
73f0a65
to
7273d9d
Compare
@zhuohan123 has exported this pull request. If you are a Meta employee, you can view the originating diff in D83118697. |
7273d9d
to
d2f9b55
Compare
Summary: Pull Request resolved: vllm-project#25610 Enable command line logging for LLMEngine Signed-off-by: Zhuohan Li <zhuohan123@gmail.com> Test Plan: All existing tests should pass Differential Revision: D83118697
@zhuohan123 has exported this pull request. If you are a Meta employee, you can view the originating diff in D83118697. |
d2f9b55
to
63e57a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Ye (Charlotte) Qi <yeq@meta.com> Signed-off-by: Zhuohan Li <zhuohan123@gmail.com> Signed-off-by: yewentao256 <zhyanwentao@126.com>
Summary: Enable command line logging for LLMEngine
Test Plan: All existing tests should pass
Differential Revision: D83118697