[Frontend] Refactor error handling for multiple exceptions in preprocessing #26694
Conversation
…essing Signed-off-by: ApsarasX <[email protected]>
![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 refactors error handling in several preprocessing functions. It standardizes the exception handling by consolidating multiple except blocks into one and improves the user-facing error messages by including the underlying exception cause via e.__cause__. This is a good improvement for consistency and debuggability. My review includes a suggestion to further refine the error message formatting to handle cases where e.__cause__ is None, ensuring cleaner and less confusing error outputs for the user.
| except (ValueError, TypeError) as e: | ||
| logger.exception("Error in preprocessing prompt inputs") | ||
| return self.create_error_response(str(e)) | ||
| return self.create_error_response(f"{e} {e.__cause__}") |
There was a problem hiding this comment.
While including e.__cause__ is a great improvement for error message clarity, it can be None. In that case, the current f-string would append the literal string " None" to the error message, which could be confusing for the end-user. To provide a cleaner error message, it's better to conditionally include the cause only when it exists.
| return self.create_error_response(f"{e} {e.__cause__}") | |
| return self.create_error_response(f"{e}{f' {e.__cause__}' if e.__cause__ else ''}") |
| jinja2.TemplateError) as e: | ||
| logger.exception("Error in preprocessing prompt inputs") | ||
| return self.create_error_response(str(e)) | ||
| return self.create_error_response(f"{e} {e.__cause__}") |
There was a problem hiding this comment.
While including e.__cause__ is a great improvement for error message clarity, it can be None. In that case, the current f-string would append the literal string " None" to the error message, which could be confusing for the end-user. To provide a cleaner error message, it's better to conditionally include the cause only when it exists.
| return self.create_error_response(f"{e} {e.__cause__}") | |
| return self.create_error_response(f"{e}{f' {e.__cause__}' if e.__cause__ else ''}") |
| except (ValueError, TypeError) as e: | ||
| logger.exception("Error in preprocessing prompt inputs") | ||
| return self.create_error_response(str(e)) | ||
| return self.create_error_response(f"{e} {e.__cause__}") |
There was a problem hiding this comment.
While including e.__cause__ is a great improvement for error message clarity, it can be None. In that case, the current f-string would append the literal string " None" to the error message, which could be confusing for the end-user. To provide a cleaner error message, it's better to conditionally include the cause only when it exists.
| return self.create_error_response(f"{e} {e.__cause__}") | |
| return self.create_error_response(f"{e}{f' {e.__cause__}' if e.__cause__ else ''}") |
| except (ValueError, TypeError, jinja2.TemplateError) as e: | ||
| logger.exception("Error in preprocessing prompt inputs") | ||
| return self.create_error_response(str(e)) | ||
| return self.create_error_response(f"{e} {e.__cause__}") |
There was a problem hiding this comment.
While including e.__cause__ is a great improvement for error message clarity, it can be None. In that case, the current f-string would append the literal string " None" to the error message, which could be confusing for the end-user. To provide a cleaner error message, it's better to conditionally include the cause only when it exists.
| return self.create_error_response(f"{e} {e.__cause__}") | |
| return self.create_error_response(f"{e}{f' {e.__cause__}' if e.__cause__ else ''}") |
| except ValueError as e: | ||
| logger.exception("Error in preprocessing prompt inputs") | ||
| return self.create_error_response(str(e)) | ||
| return self.create_error_response(f"{e} {e.__cause__}") |
There was a problem hiding this comment.
While including e.__cause__ is a great improvement for error message clarity, it can be None. In that case, the current f-string would append the literal string " None" to the error message, which could be confusing for the end-user. To provide a cleaner error message, it's better to conditionally include the cause only when it exists.
| return self.create_error_response(f"{e} {e.__cause__}") | |
| return self.create_error_response(f"{e}{f' {e.__cause__}' if e.__cause__ else ''}") |
Signed-off-by: ApsarasX <[email protected]>
1 participant
Purpose
serving_completion.pytest_openai_schema.pypass #17664, changestr(e)tof"{e} {e.__cause__}"in the error handling for preprocessing prompt inputs.Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.