rstudio / vetiver-r

Version, share, deploy, and monitor models
https://rstudio.github.io/vetiver-r/
Other
181 stars 27 forks source link

Consider changing error status to 422 #132

Open juliasilge opened 2 years ago

juliasilge commented 2 years ago

We could change the error that folks see on predicting with bad data from 400 to 422. This would involve doing something like this within vetiver_pr_post():

pr <- plumber::pr_set_error(pr, handler_error_422)

where handler_error_422() would be something like:

handler_error_422 <- function(req, res, err) {
    res$status <- 422
    list(error = "422 - Unprocessable Entity", message = err$message)
}

One issue that I haven't resolved yet is that I think as I have this laid out ⬆️ this will change all errors to 422, even ones that aren't predicting with valid but not endpoint-appropriate JSON. I would need to add more careful checking somehow.

This is motivated by FastAPI returning 422 in the situation where valid JSON is posted but it doesn't match the Pydantic model. @isabelizimm have I stated this correctly? Is this still the case? It do think it is a good idea to harmonize the error status codes between R and Python.

In reading the spec for the 422 code and various discussions, I'm not 100% sure that switching from 400 to 422 is best or right just based on what we're doing. Maybe we can get more feedback 400 vs. 422 for the context of model predictions?

isabelizimm commented 2 years ago

That is correct! 422 is what is used when the data POSTed does not match the Pydantic model. From those discussions, it looks like 422 or 400 are both not incorrect, but I agree we should align the errors if possible (without forcing all errors to be 422/400!). With 422, I think we want to be careful that it's clear the unprocessable entity is at the model level, not necessarily that the API cannot physically process it.

juliasilge commented 1 year ago

Here's a nice outline of how to approach this from @atheriel: https://unconj.ca/blog/structured-errors-in-plumber-apis.html

atheriel commented 1 year ago

You may also be interested in the package I wrote that pushes those ideas further: httpproblems.