[MISC] Rework logger to enable pythonic custom logging configuration to be provided #4273
Conversation
vllm/logger.py
Outdated
| _root_logger = logging.getLogger("vllm") | ||
| default_log_level = os.getenv("LOG_LEVEL", _root_logger.level) | ||
| logger.setLevel(default_log_level) | ||
| for handler in _root_logger.handlers: |
There was a problem hiding this comment.
There's a change in behavior here! Previously it just gave all loggers the default handler. Now instead it copies them from the root vllm logger.
Again, unless someone is manually adding handlers to the root vllm logger, the new default behavior should match existing behavior.
There was a problem hiding this comment.
Actually, I have a question here. Do we even need handler + propgate False? If we don't set handler + propgate=True, doesn't it just use the handler from the root logger?
There was a problem hiding this comment.
Responded over here: #4273 (comment)
| logging_config: Dict = DEFAULT_LOGGING_CONFIG | ||
|
|
||
| if VLLM_LOGGING_CONFIG_PATH: | ||
| if not path.exists(VLLM_LOGGING_CONFIG_PATH): |
There was a problem hiding this comment.
I took the more exceptional route, but we could also silently fail and fallback to default logging config in some of these scenarios.
vllm/logger.py
Outdated
| logger.propagate = False | ||
| return logger | ||
|
|
||
| if VLLM_CONFIGURE_LOGGING or VLLM_LOGGING_CONFIG_PATH: |
There was a problem hiding this comment.
It's worth noting that this logic means that custom logging config can be provided and used even if VLLM_CONFIGURE_LOGGING evaluates to false.
| def init_logger(name: str): | ||
| # Use the same settings as above for root logger | ||
| logger = logging.getLogger(name) | ||
| logger.setLevel(os.getenv("LOG_LEVEL", "DEBUG")) |
There was a problem hiding this comment.
There's a change in behavior here! Previously, the requested logger's log level would always be configured. Now it is only configured if VLLM_CONFIGURE_LOGGING evaluates to true.
There was a problem hiding this comment.
This isn't relevant anymore because descendent loggers are no longer configured, and instead propagate their messages to the root logger that handles logging those messages (or not).
As such, the change in behavior is broader, but should still be fairly consistent since both approaches aimed to mimic the configuration of the root vLLM logger.
tdg5
changed the title
examples/logging_configuration.md
Outdated
| flexibility from: | ||
|
|
||
| - vLLM's default logging configuration (least flexible) | ||
| - coarse grained custom logging configuration |
There was a problem hiding this comment.
Can you add a simple example or to a link to a relevant subsection here? for coarse grained & fined grained bullet points (the definition is not very straightforward)
There was a problem hiding this comment.
I tried adding some examples of how to use the configs to achieve these states and I think there's now an example for each state. Let me know if that's sufficient. I can definitely add a link related to each option if you think it'd be beneficial. I'm optimistic that the overall scheme is simpler now and it may not be needed.
| @@ -0,0 +1,234 @@ | |||
| # Logging Configuration | |||
vllm/logger.py
Outdated
| _root_logger = logging.getLogger("vllm") | ||
| default_log_level = os.getenv("LOG_LEVEL", _root_logger.level) | ||
| logger.setLevel(default_log_level) | ||
| for handler in _root_logger.handlers: |
There was a problem hiding this comment.
Actually, I have a question here. Do we even need handler + propgate False? If we don't set handler + propgate=True, doesn't it just use the handler from the root logger?
|
@rkooo567, updates are complete, should you care to re-review at your leisure. |
|
@simon-mo I think it is good to go! |
Rework logger to enable pythonic custom logging configuration to be provided
Moved from #4038 to here
This PR aims to update
vllm/logger.pyto extend vLLM to give users more ability to inject custom logging configuration in an idiomatic python way vialogging.config.dictConfig.This PR changes existing behavior in a couple of minor ways, but in general aims to keep existing functionality intact such that there will be limited change in behavior if this code is merged. I've added comments to call out changes in behavior with explanations as to why I felt such a change was appropriate. A test was added before rework to ensure consistent behavior, so I'm fairly confidant existing behavior should be conserved.
Checkout
examples/logging_configuration.mdfor some high level coverage of what these changes enable.I'm open to any thoughts or suggestions y'all might have to offer. Let me know 😁