-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[BugFix]: properly catch templating error when preprocess input #13976
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
[BugFix]: properly catch templating error when preprocess input #13976
Conversation
|
👋 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 🚀 |
|
Is there a particular reason why we can't just catch all |
c4f4e7c to
85c242f
Compare
|
@DarkLight1337 see this discussion . I originally was catching all the exceptions with |
|
The preprocessor will also raise |
|
@DarkLight1337 from my understanding looking at the code of the runtime_exception_handler currently in place, it seems that the logic is to check if the engine is still running on any If we were to catch all the |
|
@robertgshaw2-redhat can you comment on this as well? Since you also have a PR to improve the error handling, it would be great if they could complement each other. |
Signed-off-by: Guillaume Calmettes <[email protected]>
148da98 to
7266939
Compare
DarkLight1337
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.
He seems busy, I think this PR is reasonable so let's merge this first.
|
Can you merge from main to avoid the CI failures? |
ead82c1 to
d170a1d
Compare
Signed-off-by: Guillaume Calmettes <[email protected]>
d170a1d to
01c3e40
Compare
|
✅ done @DarkLight1337 , waiting for all checks to finish |
…-project#13976) Signed-off-by: Guillaume Calmettes <[email protected]> Signed-off-by: Richard Liu <[email protected]>
…-project#13976) Signed-off-by: Guillaume Calmettes <[email protected]> Signed-off-by: Louis Ulmer <[email protected]>
…-project#13976) Signed-off-by: Guillaume Calmettes <[email protected]>
…-project#13976) Signed-off-by: Guillaume Calmettes <[email protected]> Signed-off-by: Mu Huai <[email protected]>
This is a rework of #13039 following the inputs from @robertgshaw2-redhat (see in particular this comment)
Currently only
ValueErrorexceptions are catched for the preprocessing input step on the frontend openai endpoints.As a result, any error happening in the
preprocess_chatmethod not being aValuError(e.g.: jinja2 templating error, multimodal error) is not catched and result in a500 Internal Server Errorerror, instead of leveraging theself.create_error_responsemethod.This PR adds
TypedErrorandjinja2.TemplateErrorexceptions to the catched exception list, so a proper error is returned.Reproduce the error:
2.a. make a valid openAI query using a feature not supported by the model (e.g.: llama 3.1 doesn't support messages type
image_url)A
500 Internal Server Erroris returned on client side (openai.InternalServerError: Internal Server Error)With this PR
The error is correctly catched and propagated to the client
2.b. another example, make a valid query providing a broken chat template:
Response