Refactoring handling httpResponse in Java Engine

[lennon310]

With this ressponse:

status = {"code": 400, "reason": "sth goes wrong", "status": "FAILURE", "info": "test in exception"}
return {"data": {"names": ["score"], "ndarray": scores}, "status": status}

from model prediction, the queryREST throws exception at its Spring post method, and the log looks like:

ring-web-5.2.0.RELEASE.jar!/:5.2.0.RELEASE]
        at org.springframework.web.client.ResponseErrorHandler.handleError(ResponseErrorHandler.java:63) ~[spring-web-5.2.0.
RELEASE.jar!/:5.2.0.RELEASE]
        at org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:785) ~[spring-web-5.2.0.RELEASE.jar!
/:5.2.0.RELEASE]
        at org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:743) ~[spring-web-5.2.0.RELEASE.jar!/:5.2
.0.RELEASE]
        at org.springframework.web.client.RestTemplate.execute(RestTemplate.java:717) ~[spring-web-5.2.0.RELEASE.jar!/:5.2.0
.RELEASE]
        at org.springframework.web.client.RestTemplate.postForEntity(RestTemplate.java:470) ~[spring-web-5.2.0.RELEASE.jar!/
:5.2.0.RELEASE]
        at io.seldon.engine.service.InternalPredictionService.queryREST(InternalPredictionService.java:492) ~[classes!/:0.5.
0]
        at io.seldon.engine.service.InternalPredictionService.transformInput(InternalPredictionService.java:290) ~[classes!/
:0.5.0]
        at io.seldon.engine.predictors.PredictiveUnitBean.transformInput(PredictiveUnitBean.java:257) ~[classes!/:0.5.0]
        at io.seldon.engine.predictors.PredictiveUnitBean.getOutputAsync(PredictiveUnitBean.java:134) ~[classes!/:0.5.0]
        at io.seldon.engine.predictors.PredictiveUnitBean$$FastClassBySpringCGLIB$$ddb0d27b.invoke(<generated>) ~[classes!/:
0.5.0]
        at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218) ~[spring-core-5.2.0.RELEASE.jar!/:5.2.0.
RELEASE]
        at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:769) ~[s
pring-aop-5.2.0.RELEASE.jar!/:5.2.0.RELEASE]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163) ~[sprin
g-aop-5.2.0.RELEASE.jar!/:5.2.0.RELEASE]
        at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.proceed(CglibAopProxy.java:747) ~[spring-ao
p-5.2.0.RELEASE.jar!/:5.2.0.RELEASE]
        at org.springframework.aop.interceptor.AsyncExecutionInterceptor.lambda$invoke$0(AsyncExecutionInterceptor.java:115)
 ~[spring-aop-5.2.0.RELEASE.jar!/:5.2.0.RELEASE]
        at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_222]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_222]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_222]
        at java.lang.Thread.run(Thread.java:748) [?:1.8.0_222]

That means the error message parsing is not implemented if it's an non-200 response. We should probably consider moving this exception handling inside this catch block.

Besides, we should decide what status code we want to use to wrap the ApiException into a SeldonMessage. I understand the concern a 200 code doesn't seem like a failure response here, I was setting it to 200 since our service call builder doesn't support deserialization on an non-200 SeldonMessage response.

[ukclivecox]

Any input on this @axsaucedo @adriangonz ?

[axsaucedo]

This is certainly something we should implement - this has been brought up previously (i.e. #939). I think we should try to incorporate this into 1.0 if we can, probably something to tackle after the JSON float/int piece.

[lennon310]

@axsaucedo because this line throws exception directly if the http status code is non-200:

ResponseEntity<String> httpResponse =
            restTemplate.postForEntity(uri, request, String.class);

I was wondering whether there is a way we could still retrieve the response body from there?

[adriangonz]

What status code should we use when the error comes from the inference services?

I agree that 200 is not ideal. However, propagating the error from the prediction microservice has its own caveats as well. In particular, I can think of these two:

• As a user, it would make it more complicated to have a unified way of treating Seldon Core.
• Even with the engine using REST, the prediction microservice could be using gRPC, in which case there wouldn't be an error code.

However, I can see how it could still be useful to differentiate between 5xx and 4xx as a user. And you can also argue that the engine should behave relatively transparently?

What are your thoughts on this @cliveseldon @axsaucedo @lennon310 ?

[lennon310]

I created a PR trying to address the issue of post entity error handling, temporarily leaving the status code as it is since we are still under discussion.

I'm inclined to use 200 because the more important thing for the user is they should get the error message from model service. At the same time @adriangonz I agree it could be useful for an user to differentiate between 5xx and 4xx, and this status code could be set in Python model class as well (as part of prediction depends on the exception caught inside there).

[adriangonz]

@lennon310 now that #1117 is merged, do you consider this solved?

Regarding the status code, I'm inclined to leave it as it is for now, since that's the current documented behaviour.

[lennon310]

@adriangonz I'm trying to create an integration test on its behavior, shall we close the issue after that?

[lennon310]

Added one integration test. Closing this issue.