Support byte string return values from predict()

[deliahu]

Description

1. If the user returns an instance of starlette.response, send that through unmodified
2. If the user returns a string or bytes, send that through to the user, and do not set the media_type header
3. Otherwise attempt to encode as JSON using the default json package, and set the "application/json" media type header

The error message if encoding fails should list the user's three options.

[RobertLucian]

Is it possible to grab this one?

[deliahu]

Absolutely! @vishalbollu and I will discuss tomorrow whether we want to also pass strings through or continue encoding them as json, so we will update you on our thoughts there. But passing the bytes through definitely makes sense since right now that isn't even possible.

[deliahu]

@RobertLucian I spoke with Vishal, and here's what we're thinking:

1. If the user returns an instance of starlette.response, send that through unmodified
2. If the user returns a string or bytes, send that through to the user, and do not set the media_type header
3. Otherwise attempt to encode as JSON using the default json package, and set the "application/json" media type header

The error message if encoding fails should list the user's three options.

We decided to close #910 for now, since using the built-in json makes it easier for users to understand the behavior, and search for error messages.

Does this behavior make sense to you?

[RobertLucian]

@deliahu I just saw your comment here. So, let me iterate what #915 already has:

1. "If the user returns an instance of starlette.response, send that through unmodified"
2. If the user returns a bytes object, send that through to the user, and set media_type to application/octet-stream.
3. "Otherwise attempt to encode as JSON using the default json package, and set the application/json media type header"

And here's an example of the error's message in case the JSON serialization fails:

cortex.lib.exceptions.UserRuntimeException: error: consider passing a bytes object or a custom starlette.response object instead: Object of type 'DataFrame' is not JSON serializable: runtime exception

Out of all these 4 things (the first 3 points and the error message), the only difference is in that string objects are not sent to the user directly and that the media_type is set for bytes objects.

---

Here are my thoughts:

1. Returning a string as-is with Support multiple types of responses in predict() #915 "works" because json.dumps is there. On the other end, the user has to json.loads(response.json()). Do you think returning a string without passing it through json.dumps is necessary? I can do it if asked for. The thinking goes like "well, I still get a string if I json.loads(json.dumps("generic string"))".

2. It might give more clarity to the user on the other end if the media type is set (for bytes [and string]), I'm thinking. I don't have a strong opinion here.

What do you think?

[deliahu]

Yes, I definitely agree with using the application/octet-stream media type, thanks for pointing that out!

Our thinking regarding passing strings through is that the json dumping/loading is unnecessary, since it's already a string (you might as well just read it from the body directly). E.g. currently, if you return "string", the body will be '"string"', and you will need to json.loads() to remove the quotes, which didn't seem to be necessary in the first place. Also, as a user, if I return a string, it may be weird to me to get an encoded string on the other end. Although I'm not fully sold on all this, what do you think?

[deliahu]

@RobertLucian also, I just pushed a few small changes to the docs and the error message, feel free to edit them back or further if you'd like

[RobertLucian]

@deliahu Hmm, I see the logic behind that - eliminating yet another step when it's about strings. I think that makes sense, although, I think it still has to be decoded on the receiving end. I'd give it a text/plain media type. I'm implementing this then.

[deliahu]

@RobertLucian good call on the text/plain media type

This PR looks good to me, I will allow @vishalbollu to do a final pass, weigh in on the error message, and LGTM