-
Notifications
You must be signed in to change notification settings - Fork 5
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
Make validation errors less verbose #546
Conversation
So cool!! |
e15abed
to
01015c6
Compare
I don't understand the problem with the documentation test failing. It has hardly anything to do with this PR, so I included a simple workaround, and we should look into it in another PR. Ready for review! |
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.
raise HTTPException(HTTP_400_BAD_REQUEST, detail=str(e)) | ||
if isinstance(e, ValidationError): | ||
raise |
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.
I put the threshold in the config page to -50%, and got these surprising warnings:
I only expected the last one.
That happens with all Union
types. The postprocessors
is a list of Union[TemperatureScaling, ThresholdConfig, CustomObject]
, so Pydantic tries to parse
{
"class_name": "azimuth.utils.ml.postprocessing.Thresholding",
"args": [],
"kwargs": {"threshold": -0.5},
"remote": null,
"threshold": -0.5
}
into those 3 classes, and gives you the reason why each one is failing. It can't be a TemperatureScaling
since class_name
is incompatible, it can't be a CustomObject
since the field threshold
doesn't exist, and finally, it can't be a ThresholdConfig
because the value of threshold
is below 0.
I was not able to figure out what the order of the errors means, so I didn't feel confident enough to assume that the last one is always the relevant one, for example. Also, you really could make different errors in the config, and getting all the errors back at once is pretty neat. So, we'll have to play more with it and come up with ideas on how to improve.
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.
Another example in the config would be use_cuda: Union[Literal["auto"], bool]
.
If we try to PATCH
the config with
{"use_cuda": "potato"}
we get
AzimuthConfig['use_cuda']: unexpected value; permitted: 'auto'
AzimuthConfig['use_cuda']: value could not be parsed to a boolean
which makes sense.
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.
Thank you for the thorough explanation!!
Description:
Here is an example of a response to a
PATCH
/config
with an invalid config:And here is an extreme example with 4 errors (
a
,b
,c
andd
) in aGET
/dataset_splits/c/utterances?outcome=a&outcome=b&sort=d&pipeline_index=0
Checklist:
You should check all boxes before the PR is ready. If a box does not apply, check it to acknowledge it.
ran
pre-commit run --all-files
at the end.our users.
README
files and our wiki for any big design decisions, if relevant.