[Frontend] Avoid startup error log for models without chat template#37040
[Frontend] Avoid startup error log for models without chat template#37040njhill merged 1 commit intovllm-project:mainfrom
Conversation
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
DarkLight1337
added
the
ready
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly handles ChatTemplateResolutionError during chat template warmup by catching the specific exception and logging an informational message instead of an error with a stack trace. This improves the user experience by avoiding confusing error logs for models that intentionally lack a chat template. My review includes one suggestion regarding a local import, which points to a potential circular dependency that could be addressed to improve long-term code maintainability.
| For multi-modal requests: | ||
| - Importing libraries such as librosa triggers JIT compilation. | ||
| """ | ||
| from vllm.entrypoints.chat_utils import ChatTemplateResolutionError |
There was a problem hiding this comment.
This local import suggests a potential circular dependency between vllm.renderers and vllm.entrypoints. While this works, circular dependencies can make the code harder to maintain and refactor. A better long-term solution is to break the cycle, for example, by moving ChatTemplateResolutionError to a common exceptions file. If refactoring is out of scope for this PR, adding a comment to explain the necessity of this local import would be helpful for future maintainability.
| from vllm.entrypoints.chat_utils import ChatTemplateResolutionError | |
| # Local import to avoid a circular dependency. | |
| from vllm.entrypoints.chat_utils import ChatTemplateResolutionError |
References
- According to PEP 8, imports should usually be at the top of the file. Local imports are acceptable to resolve circular dependencies, but this often points to a need for refactoring. Adding a comment clarifies the reason for deviating from the standard. (link)
njhill
left a comment
njhill
left a comment
There was a problem hiding this comment.
Thanks @DarkLight1337! Would be nice to make the other suggested change at the same time. It's TMI IMO otherwise
| from vllm.entrypoints.chat_utils import ChatTemplateResolutionError | ||
|
|
||
| try: | ||
| logger.info("Warming up chat template processing...") |
There was a problem hiding this comment.
I think we should also change this one to debug.
|
OK I just merged because it was already green, can open another PR for the other thing... |
|
@DarkLight1337 I opened #37062 for this |
…llm-project#37040) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk> Signed-off-by: Athrael Soju <athrael.soju@gmail.com>
…llm-project#37040) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk> Signed-off-by: Athrael Soju <athrael.soju@gmail.com>
…llm-project#37040) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
…llm-project#37040) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
…llm-project#37040) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
2 participants
Purpose
During chat template warmup, models without a valid chat template raise
ChatTemplateResolutionError, causing some confusion among users as it was logged with a stack trace. This PR catches the specific error and downgrades the log to INFO level.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.