Return only JSON responses from KFServer (kubeflow#1918)

* Return only JSON responses

* Update tests

python/kserve/kserve/handlers/http.py

@@ -11,6 +11,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from typing import Any
+
 import tornado.web
 import json
 import pytz
@@ -25,7 +27,28 @@
 from ray.serve.api import RayServeHandle
 
 
-class HTTPHandler(tornado.web.RequestHandler):
+class BaseHandler(tornado.web.RequestHandler):
+    def write_error(self, status_code: int, **kwargs: Any) -> None:
+        """This method is called when there are unhandled tornado.web.HTTPErrors"""
+        self.set_status(status_code)
+
+        reason = "An error occurred"
+
+        exc_info = kwargs.get("exc_info", None)
+        if exc_info is not None:
+            if hasattr(exc_info[1], "reason"):
+                reason = exc_info[1].reason
+
+        self.write({"error": reason})
+
+
+class NotFoundHandler(tornado.web.RequestHandler):
+    def write_error(self, status_code: int, **kwargs: Any) -> None:
+        self.set_status(HTTPStatus.NOT_FOUND)
+        self.write({"error": "invalid path"})
+
+
+class HTTPHandler(BaseHandler):
     def initialize(self, models: KFModelRepository):
         self.models = models  # pylint:disable=attribute-defined-outside-init
 

python/kserve/kserve/kfserver.py

@@ -13,7 +13,6 @@
 
 import argparse
 import logging
-import json
 import inspect
 import sys
 from typing import List, Optional, Dict, Union
@@ -25,7 +24,7 @@
 from tornado import concurrent
 from .utils import utils
 
-from kserve.handlers.http import PredictHandler, ExplainHandler
+from kserve.handlers.http import BaseHandler, NotFoundHandler, PredictHandler, ExplainHandler
 from kserve import KFModel
 from kserve.kfmodel_repository import KFModelRepository
 from ray.serve.api import Deployment, RayServeHandle
@@ -92,7 +91,7 @@ def create_application(self):
              LoadHandler, dict(models=self.registered_models)),
             (r"/v2/repository/models/([a-zA-Z0-9_-]+)/unload",
              UnloadHandler, dict(models=self.registered_models)),
-        ])
+        ], default_handler_class=NotFoundHandler)
 
     def start(self, models: Union[List[KFModel], Dict[str, Deployment]], nest_asyncio: bool = False):
         if isinstance(models, list):
@@ -150,12 +149,12 @@ def register_model(self, model: KFModel):
         logging.info("Registering model: %s", model.name)
 
 
-class LivenessHandler(tornado.web.RequestHandler):  # pylint:disable=too-few-public-methods
+class LivenessHandler(BaseHandler):  # pylint:disable=too-few-public-methods
     def get(self):
-        self.write("Alive")
+        self.write({"status": "alive"})
 
 
-class HealthHandler(tornado.web.RequestHandler):
+class HealthHandler(BaseHandler):
     def initialize(self, models: KFModelRepository):
         self.models = models  # pylint:disable=attribute-defined-outside-init
 
@@ -173,21 +172,21 @@ def get(self, name: str):
                 reason="Model with name %s is not ready." % name
             )
 
-        self.write(json.dumps({
+        self.write({
             "name": model.name,
             "ready": model.ready
-        }))
+        })
 
 
-class ListHandler(tornado.web.RequestHandler):
+class ListHandler(BaseHandler):
     def initialize(self, models: KFModelRepository):
         self.models = models  # pylint:disable=attribute-defined-outside-init
 
     def get(self):
-        self.write(json.dumps([ob.name for ob in self.models.get_models()]))
+        self.write({"models": [ob.name for ob in self.models.get_models()]})
 
 
-class LoadHandler(tornado.web.RequestHandler):
+class LoadHandler(BaseHandler):
     def initialize(self, models: KFModelRepository):  # pylint:disable=attribute-defined-outside-init
         self.models = models
 
@@ -210,13 +209,13 @@ async def post(self, name: str):
                 status_code=503,
                 reason=f"Model with name {name} is not ready."
             )
-        self.write(json.dumps({
+        self.write({
             "name": name,
             "load": True
-        }))
+        })
 
 
-class UnloadHandler(tornado.web.RequestHandler):
+class UnloadHandler(BaseHandler):
     def initialize(self, models: KFModelRepository):  # pylint:disable=attribute-defined-outside-init
         self.models = models
 
@@ -228,7 +227,7 @@ def post(self, name: str):
                 status_code=404,
                 reason="Model with name %s does not exist." % name
             )
-        self.write(json.dumps({
+        self.write({
             "name": name,
             "unload": True
-        }))
+        })

python/kserve/test/test_server.py

@@ -10,6 +10,8 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import json
+import re
 
 import avro.io
 import avro.schema
@@ -148,11 +150,18 @@ def app(self):  # pylint: disable=no-self-use
     async def test_liveness(self, http_server_client):
         resp = await http_server_client.fetch('/')
         assert resp.code == 200
+        assert resp.body == b'{"status": "alive"}'
 
     async def test_model(self, http_server_client):
         resp = await http_server_client.fetch('/v1/models/TestModel')
         assert resp.code == 200
 
+    async def test_unknown_model(self, http_server_client):
+        with pytest.raises(HTTPClientError) as err:
+            _ = await http_server_client.fetch('/v1/models/InvalidModel')
+        assert err.value.code == 404
+        assert err.value.response.body == b'{"error": "Model with name InvalidModel does not exist."}'
+
     async def test_predict(self, http_server_client):
         resp = await http_server_client.fetch('/v1/models/TestModel:predict',
                                               method="POST",
@@ -185,7 +194,13 @@ async def test_explain(self, http_server_client):
     async def test_list(self, http_server_client):
         resp = await http_server_client.fetch('/v1/models')
         assert resp.code == 200
-        assert resp.body == b'["TestModel"]'
+        assert resp.body == b'{"models": ["TestModel"]}'
+
+    async def test_unknown_path(self, http_server_client):
+        with pytest.raises(HTTPClientError) as err:
+            _ = await http_server_client.fetch('/unknown_path')
+        assert err.value.code == 404
+        assert err.value.response.body == b'{"error": "invalid path"}'
 
 
 class TestTFHttpServerLoadAndUnLoad:
@@ -289,26 +304,33 @@ async def test_predict_ce_binary_bytes(self, http_server_client):
     async def test_predict_ce_bytes_bad_format_exception(self, http_server_client):
         event = dummy_cloud_event(b'{', set_contenttype=True)
         headers, body = to_binary(event)
-        with pytest.raises(
-            HTTPClientError, match=r".*HTTP 400: Unrecognized request format: "
-                                   r"Expecting property name enclosed in double quotes.*"
-        ):
-            await http_server_client.fetch('/v1/models/TestModel:predict',
-                                           method="POST",
-                                           headers=headers,
-                                           body=body)
+
+        with pytest.raises(HTTPClientError) as err:
+            _ = await http_server_client.fetch('/v1/models/TestModel:predict',
+                                               method="POST",
+                                               headers=headers,
+                                               body=body)
+        assert err.value.code == 400
+
+        error_regex = re.compile("Unrecognized request format: Expecting property name enclosed in double quotes.*")
+        response = json.loads(err.value.response.body)
+        assert error_regex.match(response["error"]) is not None
 
     async def test_predict_ce_bytes_bad_hex_format_exception(self, http_server_client):
         event = dummy_cloud_event(b'0\x80\x80\x06World!\x00\x00', set_contenttype=True)
         headers, body = to_binary(event)
-        with pytest.raises(
-            HTTPClientError, match=r".*HTTP 400: Unrecognized request format: "
-                                   r"'utf-8' codec can't decode byte 0x80 in position 1: invalid start byte.*"
-        ):
-            await http_server_client.fetch('/v1/models/TestModel:predict',
-                                           method="POST",
-                                           headers=headers,
-                                           body=body)
+
+        with pytest.raises(HTTPClientError) as err:
+            _ = await http_server_client.fetch('/v1/models/TestModel:predict',
+                                               method="POST",
+                                               headers=headers,
+                                               body=body)
+        assert err.value.code == 400
+
+        error_regex = re.compile("Unrecognized request format: 'utf-8' codec can't decode byte 0x80 in position 1: "
+                                 "invalid start byte.*")
+        response = json.loads(err.value.response.body)
+        assert error_regex.match(response["error"]) is not None
 
 
 class TestTFHttpServerAvroCloudEvent: