-
-
Notifications
You must be signed in to change notification settings - Fork 12.1k
[Frontend] Add --log-error-stack to print stack trace for error response #22960
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
Signed-off-by: Chen Zhang <zhangch99@outlook.com>
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 introduces a --log-error-stack flag, a valuable addition for debugging by enabling stack trace logging for error responses. The implementation is sound, and the new setting is correctly propagated through the various serving classes. My review includes one suggestion to enhance the logging of these stack traces by integrating with the existing logger infrastructure, which will improve consistency and make the logs more manageable.
| import traceback | ||
| exc_type, _, _ = sys.exc_info() | ||
| if exc_type is not None: | ||
| traceback.print_exc() | ||
| else: | ||
| traceback.print_stack() |
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.
Instead of printing directly to stderr using traceback.print_exc() and traceback.print_stack(), it's better to use the configured logger. This ensures that stack traces are formatted and routed according to the application's logging configuration. Using logger.exception() when an exception is present, and logging the formatted stack otherwise, will provide more consistent and manageable error logging.
| import traceback | |
| exc_type, _, _ = sys.exc_info() | |
| if exc_type is not None: | |
| traceback.print_exc() | |
| else: | |
| traceback.print_stack() | |
| import traceback | |
| if sys.exc_info()[0]: | |
| logger.exception("Error will be returned to client: %s", message) | |
| else: | |
| stack = "".join(traceback.format_stack()) | |
| logger.error("Stack trace for error response '%s':\n%s", message, stack) |
|
👋 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 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 🚀 |
Signed-off-by: Chen Zhang <zhangch99@outlook.com>
aarnphm
left a comment
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.
tiny comments, otherwise LGTM.
will CC another few more folks for comments. @DarkLight1337 @hmellor
Signed-off-by: Chen Zhang <zhangch99@outlook.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Chen Zhang <zhangch99@outlook.com>
Signed-off-by: Chen Zhang <zhangch99@outlook.com>
Signed-off-by: Chen Zhang <zhangch99@outlook.com>
…nse (vllm-project#22960) Signed-off-by: Chen Zhang <zhangch99@outlook.com>
…nse (vllm-project#22960) Signed-off-by: Chen Zhang <zhangch99@outlook.com> Signed-off-by: Xiao Yu <xiao.yu@amd.com>
…nse (vllm-project#22960) Signed-off-by: Chen Zhang <zhangch99@outlook.com>
…nse (vllm-project#22960) Signed-off-by: Chen Zhang <zhangch99@outlook.com>
…nse (vllm-project#22960) Signed-off-by: Chen Zhang <zhangch99@outlook.com>
Purpose
It's difficult to locate BadRequestError as we can't see where the error happens from default ouptut. Add a flag to print stack trace during
create_error_responseTest Plan
Test Result
Can print the error stack
(Optional) Documentation Update
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.