-
Notifications
You must be signed in to change notification settings - Fork 7
🥅 Handle template errors #20
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
🥅 Handle template errors #20
Conversation
Signed-off-by: Evaline Ju <[email protected]>
Signed-off-by: Evaline Ju <[email protected]>
Signed-off-by: Evaline Ju <[email protected]>
| # but the error message may not be directly user-comprehensible | ||
| # This has to be caught first because jinja UndefinedError are TemplateError | ||
| chat_response = ErrorResponse( | ||
| message="Template error. Please check request.", |
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.
Does the message in this case not usable to be passed to the user?
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.
Yes - an example is example 2 of the PR description e.g. 'dict object' has no attribute 'prompt' when prompt is not an end-user-facing parameter. I think considerations could be made to see if some more of these can be given as feedback to be caught as exceptions in the default template [depending on the model].
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.
hmm. true. but that information might still be helpful for debugging. So can we log the raw error message while we switch it with better error message here ?
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.
sure, we can just fall through to the general TemplateError then
Signed-off-by: Evaline Ju <[email protected]>
c9a90f6 to
722c6a5
Compare
gkumbhat
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.
Looks good to me!
* 🥅✅ Handle jinja template errors * 🥅 Use bad request for template errors * 🎨 Unused exception variable * 🥅 Consolidate on template error Signed-off-by: Evaline Ju <[email protected]> --------- Signed-off-by: Evaline Ju <[email protected]>
Catch template errors so that the server does not just
Internal Server Erroron fixable requests. The assumption here is that since the (chat) template can be overwritten, errors can be considered bad requests.Here, some particular error cases are addressed -
TemplateErrorare returned fromraise_exceptionin jinja templates.UndefinedErrorare a sub-class ofTemplateError.UndefinedErrormay not give easily user-fixable messages, but these are propagated anyway, since it was decided that such improvements can be made at the template (or default model chat template) level.Testing
Example error calls with a granite guardian model server
Example 1 - wrong "role" ("moo" is not an expected "role" for the default risk
harm)Before this update, the call would
Internal Server Error. In logs one could seejinja2.exceptions.TemplateError: [role, risk] combination is incorrect. Now{"object":"error","message":"[role, risk] combination is incorrect","type":"BadRequestError","param":null,"code":400}%Example 2 - wrong number of messages for the relevance risk
Before this update, the call would
Internal Server Error. In logs one could seejinja2.exceptions.UndefinedError: 'dict object' has no attribute 'prompt'. Now{"object":"error","message":"'dict object' has no attribute 'prompt'","type":"BadRequestError","param":null,"code":400}%Closes: #14