Improve reporting of deserializer validation errors#550
Improve reporting of deserializer validation errors#550dbutenhof wants to merge 2 commits intovllm-project:mainfrom
Conversation
Deserialization of some parameters like `--data` takes place during `run` rather than during option processing. It raises a `ValidationError`, but to the Click CLI infrastructure this is an unexpected exception and causes a full traceback which is not generally useful to the user. This change intercepts `ValidationError` (and for completeness the standard Python `ValueError`) and raises a Click `BadParameter` exception encapsulating the validation error text. This will be reported without traceback. Signed-off-by: David Butenhof <dbutenho@redhat.com>
| ) | ||
| ) | ||
| ) | ||
| except (ValidationError, ValueError) as err: |
There was a problem hiding this comment.
My biggest concern here is we lose debugging information for a large class of errors. ValueError is especially generic and could be emitted in any number of places. Maybe instead we define a custom error type for these runtime argument errors and catch that specific exception here?
There was a problem hiding this comment.
Hmm... yeah, it actually looks like ValueError is used a lot more than I would have expected. And if any of them can occur after "initial startup" we wouldn't want to hide the traceback. So much for a "simple touch", but it was in any case an interesting excursion.
Yeah, we could use another exception for "static startup validation" errors. Perhaps a better solution would be to refactor the deserializers with a validation method that can be called during option parsing rather than let this sort of thing wait until run. And that'll require a lot more thought ...
Summary
I was looking for a "dead simple" problem just to get my feet damp. Issue #205 stood out. (A comment on that issue includes some detailed analysis.)
Deserialization of some parameters like
--datatakes place duringrunrather than during Click option processing. Errors raise a validation exception, but to the Click CLI infrastructure this is an unexpected exception and causes a full traceback which is not generally useful to the user.This change intercepts internal validation error exceptions and raises a Click
BadParameterexception encapsulating the validation error text. This will be reported without traceback.Details
This simply wraps the
asyncio.runwhich starts benchmarking (and runs the deserializers) with a try block to convert the deserializer'sValueErrorintoclick.BadParameterso that Click can generate a better usage message and will suppress the traceback which can obscure the message.Test Plan
This only affects the output of a validation error in benchmark run startup, removing the traceback. While it's not impossible to imagine a
CliRunnertest for this case, the output analysis would be a bit tedious and it didn't seem worthwhile. (Feel free to tell me otherwise! Currently,test_main.pyis rather light on content.)Related Issues
This is "inspired by" #205 but possibly doesn't actually resolve it.
Use of AI
## WRITTEN BY AI ##)