`_parse_json_request` should handle dictionaries properly

[zmjjmz]

Hey there,

So I've recently been speccing out a project that would require multiple inputs given to a model. I had a few hiccups getting this to work with a TensorFlow model w/Keras, but I figured out a workaround -- however I don't think this workaround should have to be built by the customer and instead should be handled by default.

Basically, when serve receives a request, it turns it into a tensor proto using json_format.Parse. This means that it cannot handle dictionaries. However, the Sagemaker SDK's predictor will happily take in, serialize, and send off a dictionary from Python. This was a confusing and frustrating difference in API to deal with.

However, it's fairly easy to resolve, as you can simply use tf.make_tensor_proto on each element after being JSON deserialized, as below:

def input_fn(serialized_input, content_type):                                        
    """An input_fn that loads a pickled object"""                                    
    if content_type == "application/json":                                           
        deserialized_input = json.loads(serialized_input)                            
        if isinstance(deserialized_input, dict):                                     
            deserialized_tensorproto = {                                             
                k:tensorflow.make_tensor_proto(v)                                    
                for k, v in deserialized_input.items()                               
            }                                                                        
        else:                                                                        
            deserialized_tensorproto = tensorflow.make_tensor_proto(serialized_input)
        return deserialized_tensorproto                                              
    else:                                                                            
        # Handle other content-types here or raise an Exception                      
        # if the content type is not supported.                                      
        pass                                                                         

Unfortunately this doesn't use json_format, which I'm not sure is a requirement. This input_fn can be used to override the default one and provide the multi-input functionality as done in this demo gist, however it would be great not to have to include this workaround!

[winstonaws]

Agreed that this is a big problem, since we intended the dictionaries to work but clearly they don't. We've also put off fixing this for too long in my personal opinion.

I'd like to work on this, but I won't be able to get to it today or tomorrow. I'll take a look on Friday and give an update then.

[zmjjmz]

Thanks!

[winstonaws]

Took a longer look at this, and agreed that we should add something similar to the code you provided to the container code for deserialization.

I'd also like to take the chance to fix some of the Predictor serialization behavior on the Python SDK side, as it currently accepts a numpy array or a pure python dict/list, but not a dict of numpy arrays, which doesn't make a lot of sense. It'd help to know: what input format were you passing to the predictor? And are there any other sensible input formats you think should be supported?

Actual code to come next week, haha.

[zmjjmz]

So to get this to work I was taking my dict of numpy arrays and doing something like:
predict({k:v.tolist() for k,v in input.iteritems()}) which works but is a bit ugly. I was thinking I'd bring that up as an issue on the SDK repo but I decided it was trivial enough to deal with that I wouldn't worry about it -- although it would be nice to see this behavior better documented.

Not sure if there are any other input formats that should be supported -- for the most part I'm mostly interested in having Sagemaker be a transparent wrapper for Keras models, so if a Keras model's predict method could take it as input then I feel like Sagemaker should...

[winstonaws]

That makes sense. We should at least get it to work for the dict of numpy arrays case. Once the PR linked above gets merged and released, I'll see if I can work on the python SDK side of things, but at the very least that PR should solve the major problem.

[winstonaws]

This should be released now. Didn't get to working on the Python SDK side; if you want you can make a new issue there.

Please reopen if you have further problems along these lines.

[winstonaws]

Python SDK issue opened independently here: aws/sagemaker-python-sdk#394

[zmjjmz]

Thanks!