Explainer Gateway Fix for Istio, Ambassador and Python Client (#1668)

* Updated operator constants

* Updated python client to existing changes of explainer

* Updated operator to have aligned requirements on path

* Added explainer test

* fixed explainer typo

* fixed explainer typo

* Reverted addresses

* Added explainer suffix with predictor name

* Updated test

* Updated version it points to

* Added predictor to explainer

* Updated python to reflect notebook

* Updated notebook to include explainerW

* Updated python tests

* Updated python tests

* Updated comment

operator/constants/constants.go

@@ -54,6 +54,6 @@ const (
 
 // Explainers
 const (
-	ExplainerPathSuffix = "/explainer"
+	ExplainerPathSuffix = "-explainer"
 	ExplainerNameSuffix = "-explainer"
 )

operator/controllers/ambassador.go

@@ -80,8 +80,8 @@ func getAmbassadorRestConfig(mlDep *machinelearningv1.SeldonDeployment,
 
 	name := p.Name
 	if isExplainer {
-		name = name + constants.ExplainerNameSuffix
-		serviceNameExternal = serviceNameExternal + constants.ExplainerPathSuffix
+		name = p.Name + constants.ExplainerNameSuffix
+		serviceNameExternal = serviceNameExternal + constants.ExplainerPathSuffix + "/" + p.Name
 	}
 
 	c := AmbassadorConfig{

operator/controllers/ambassador_test.go

@@ -25,7 +25,7 @@ func basicAbassadorTests(t *testing.T, mlDep *machinelearningv1.SeldonDeployment
 	err = yaml.Unmarshal([]byte(parts[0]), &c)
 	g.Expect(err).To(BeNil())
 	if isExplainer {
-		g.Expect(c.Prefix).To(Equal("/seldon/default/mymodel" + constants.ExplainerPathSuffix + "/"))
+		g.Expect(c.Prefix).To(Equal("/seldon/default/mymodel" + constants.ExplainerPathSuffix + "/" + p.Name + "/"))
 	} else {
 		g.Expect(c.Prefix).To(Equal("/seldon/default/mymodel/"))
 	}

operator/controllers/seldondeployment_explainers.go

@@ -224,7 +224,7 @@ func createExplainerIstioResources(pSvcName string, p *machinelearningv1.Predict
 				{
 					Match: []*istio_networking.HTTPMatchRequest{
 						{
-							Uri: &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Prefix{Prefix: "/seldon/" + namespace + "/" + mlDep.Name + "/" + p.Name + constants.ExplainerPathSuffix + "/"}},
+							Uri: &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Prefix{Prefix: "/seldon/" + namespace + "/" + mlDep.GetName() + constants.ExplainerPathSuffix + "/" + p.Name + "/"}},
 						},
 					},
 					Rewrite: &istio_networking.HTTPRewrite{Uri: "/"},
@@ -247,7 +247,7 @@ func createExplainerIstioResources(pSvcName string, p *machinelearningv1.Predict
 						{
 							Uri: &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Prefix{Prefix: "/seldon.protos.Seldon/"}},
 							Headers: map[string]*istio_networking.StringMatch{
-								"seldon":    &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Exact{Exact: mlDep.Name}},
+								"seldon":    &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Exact{Exact: mlDep.GetName()}},
 								"namespace": &istio_networking.StringMatch{MatchType: &istio_networking.StringMatch_Exact{Exact: namespace}},
 							},
 						},

python/seldon_core/seldon_client.py

@@ -211,10 +211,15 @@ def __init__(
         logger.debug("Configuration:" + str(self.config))
 
     def _gather_args(self, **kwargs):
-
-        c2 = {**self.config}
-        c2.update({k: v for k, v in kwargs.items() if v is not None})
-        return c2
+        """
+        Performs a left outer join where kwargs is left and self.config is right
+        which means that the resulting dictionary will only have the variables provided
+        by the parameters available in kwargs, but will be overriden by kwargs if a value
+        that is not None is present.
+        """
+        for k, v in kwargs.items():
+            kwargs[k] = v if kwargs[k] is not None else self.config.get(k, None)
+        return kwargs
 
     def _validate_args(
         self,
@@ -512,11 +517,7 @@ def explain(
         transport: str = None,
         deployment_name: str = None,
         payload_type: str = None,
-        seldon_rest_endpoint: str = None,
-        seldon_grpc_endpoint: str = None,
         gateway_endpoint: str = None,
-        microservice_endpoint: str = None,
-        method: str = None,
         shape: Tuple = (1, 1),
         namespace: str = None,
         data: np.ndarray = None,
@@ -528,6 +529,7 @@ def explain(
         headers: Dict = None,
         http_path: str = None,
         client_return_type: str = None,
+        predictor: str = None,
     ) -> Dict:
         """
 
@@ -573,6 +575,8 @@ def explain(
            Custom http path for predict call to use
         client_return_type
             the return type of all functions can be either dict or proto
+        predictor
+            The name of the predictor to send the explanations to
 
         Returns
         -------
@@ -583,11 +587,7 @@ def explain(
             transport=transport,
             deployment_name=deployment_name,
             payload_type=payload_type,
-            seldon_rest_endpoint=seldon_rest_endpoint,
-            seldon_grpc_endpoint=seldon_grpc_endpoint,
             gateway_endpoint=gateway_endpoint,
-            microservice_endpoint=microservice_endpoint,
-            method=method,
             shape=shape,
             namespace=namespace,
             names=names,
@@ -599,6 +599,7 @@ def explain(
             headers=headers,
             http_path=http_path,
             client_return_type=client_return_type,
+            predictor=predictor,
         )
         self._validate_args(**k)
         if k["gateway"] == "ambassador" or k["gateway"] == "istio":
@@ -1272,7 +1273,7 @@ def get_token(
         token = response.json()["access_token"]
         return token
     else:
-        print("Failed to get token:" + response.text)
+        logger.debug("Failed to get token:" + response.text)
         raise SeldonClientException(response.text)
 
 
@@ -1641,6 +1642,8 @@ def explain_predict_gateway(
     deployment_name: str,
     namespace: str = None,
     gateway_endpoint: str = "localhost:8003",
+    gateway: str = None,
+    transport: str = "rest",
     shape: Tuple[int, int] = (1, 1),
     data: np.ndarray = None,
     headers: Dict = None,
@@ -1654,7 +1657,7 @@ def explain_predict_gateway(
     channel_credentials: SeldonChannelCredentials = None,
     http_path: str = None,
     client_return_type: str = "dict",
-    **kwargs,
+    predictor: str = None,
 ) -> SeldonClientPrediction:
     """
     REST explain request to Gateway Ingress
@@ -1667,6 +1670,10 @@ def explain_predict_gateway(
        k8s namespace of running deployment
     gateway_endpoint
        The host:port of gateway
+    gateway
+       The type of gateway which can be seldon or ambassador/istio
+    transport
+       The type of transport, in this case only rest is supported
     shape
        The shape of the data to send
     data
@@ -1699,6 +1706,9 @@ def explain_predict_gateway(
        A JSON Dict
 
     """
+    if transport != "rest":
+        raise SeldonClientException("Only supported transport is REST for explanations")
+
     if bin_data is not None:
         request = prediction_pb2.SeldonMessage(binData=bin_data)
     elif str_data is not None:
@@ -1724,17 +1734,27 @@ def explain_predict_gateway(
             if not call_credentials.token is None:
                 req_headers["X-Auth-Token"] = call_credentials.token
     if http_path is not None:
-        url = url = (
+        url = (
             scheme
             + "://"
             + gateway_endpoint
             + "/seldon/"
             + namespace
             + "/"
             + deployment_name
+            + "-explainer"
+            + "/"
+            + predictor
             + http_path
         )
+    elif gateway == "seldon":
+        url = scheme + "://" + gateway_endpoint + "/api/v1.0/explain"
     else:
+        if not predictor:
+            raise SeldonClientException(
+                "Predictor parameter must be provided to talk through explainer via gateway"
+            )
+
         if gateway_prefix is None:
             if namespace is None:
                 url = (
@@ -1743,7 +1763,10 @@ def explain_predict_gateway(
                     + gateway_endpoint
                     + "/seldon/"
                     + deployment_name
-                    + "/explainer/api/v1.0/explain"
+                    + "-explainer"
+                    + "/"
+                    + predictor
+                    + "/api/v1.0/explain"
                 )
             else:
                 url = (
@@ -1754,15 +1777,14 @@ def explain_predict_gateway(
                     + namespace
                     + "/"
                     + deployment_name
-                    + "/explainer/api/v1.0/explain"
+                    + "-explainer"
+                    + "/"
+                    + predictor
+                    + "/api/v1.0/explain"
                 )
         else:
             url = (
-                scheme
-                + "://"
-                + gateway_endpoint
-                + gateway_prefix
-                + +"/api/v1.0/explain"
+                scheme + "://" + gateway_endpoint + gateway_prefix + "/api/v1.0/explain"
             )
     verify = True
     cert = None
@@ -1781,7 +1803,6 @@ def explain_predict_gateway(
         url, json=payload, headers=req_headers, verify=verify, cert=cert
     )
     if response_raw.status_code == 200:
-        print(client_return_type)
         if client_return_type == "dict":
             ret_request = payload
             ret_response = response_raw.json()

python/seldon_core/version.py

@@ -1 +1 @@
-__version__ = "1.0.2"
+__version__ = "1.0.3-SNAPSHOT"

python/tests/test_seldon_client.py

@@ -142,9 +142,9 @@ def test_predict_rest_json_data_seldon_return_type(mock_post, mock_token):
 @mock.patch("requests.post", side_effect=mocked_requests_post_success_json_data)
 def test_explain_rest_json_data_ambassador(mock_post):
     sc = SeldonClient(
-        deployment_name="mymodel", gateway="ambassador", client_return_type="dict"
+        deployment_name="mymodel", gateway="ambassador", client_return_type="dict",
     )
-    response = sc.explain(json_data=JSON_TEST_DATA)
+    response = sc.explain(json_data=JSON_TEST_DATA, predictor="default")
     json_response = response.response
     # Currently this doesn't need to convert to JSON due to #1083
     # i.e. json_response = seldon_message_to_json(response.response)
@@ -157,9 +157,9 @@ def test_explain_rest_json_data_ambassador(mock_post):
 @mock.patch("requests.post", side_effect=mocked_requests_post_success_json_data)
 def test_explain_rest_json_data_ambassador_dict_response(mock_post):
     sc = SeldonClient(
-        deployment_name="mymodel", gateway="ambassador", client_return_type="dict"
+        deployment_name="mymodel", gateway="ambassador", client_return_type="dict",
     )
-    response = sc.explain(json_data=JSON_TEST_DATA)
+    response = sc.explain(json_data=JSON_TEST_DATA, predictor="default")
     json_response = response.response
     # Currently this doesn't need to convert to JSON due to #1083
     # i.e. json_response = seldon_message_to_json(response.response)

testing/resources/movies-text-explainer.yaml

@@ -0,0 +1,16 @@
+apiVersion: machinelearning.seldon.io/v1
+kind: SeldonDeployment
+metadata:
+  name: movie
+spec:
+  name: movie
+  predictors:
+  - graph:
+      children: []
+      implementation: SKLEARN_SERVER
+      modelUri: gs://seldon-models/sklearn/moviesentiment
+      name: classifier
+    explainer:
+      type: AnchorText
+    name: movies-predictor
+    replicas: 1

testing/scripts/seldon_e2e_utils.py

@@ -185,6 +185,8 @@ def rest_request(
     data=None,
     dtype="tensor",
     names=None,
+    method="predict",
+    predictor_name="default",
 ):
     try:
         r = rest_request_ambassador(
@@ -196,6 +198,8 @@ def rest_request(
             data=data,
             dtype=dtype,
             names=names,
+            method=method,
+            predictor_name=predictor_name,
         )
         if not r.status_code == 200:
             logging.warning(f"Bad status:{r.status_code}")
@@ -216,6 +220,7 @@ def initial_rest_request(
     data=None,
     dtype="tensor",
     names=None,
+    method="predict",
 ):
     sleeping_times = [1, 5, 10]
     attempt = 0
@@ -231,6 +236,7 @@ def initial_rest_request(
             data=data,
             dtype=dtype,
             names=names,
+            method=method,
         )
 
         if r is None or r.status_code != 200:
@@ -319,6 +325,8 @@ def rest_request_ambassador(
     data=None,
     dtype="tensor",
     names=None,
+    method="predict",
+    predictor_name="default",
 ):
     if data is None:
         shape, arr = create_random_data(data_size, rows)
@@ -336,26 +344,32 @@ def rest_request_ambassador(
     if names is not None:
         payload["data"]["names"] = names
 
-    if namespace is None:
+    if method == "predict":
         response = requests.post(
             "http://"
             + endpoint
             + "/seldon/"
+            + namespace
+            + "/"
             + deployment_name
             + "/api/v0.1/predictions",
             json=payload,
         )
-    else:
+    elif method == "explain":
         response = requests.post(
             "http://"
             + endpoint
             + "/seldon/"
             + namespace
             + "/"
             + deployment_name
-            + "/api/v0.1/predictions",
+            + "-explainer"
+            + "/"
+            + predictor_name
+            + "/api/v0.1/explain",
             json=payload,
         )
+
     return response
 
 

testing/scripts/test_prepackaged_servers.py

@@ -4,6 +4,7 @@
     retry_run,
     create_random_data,
     wait_for_status,
+    rest_request,
 )
 from subprocess import run
 import time
@@ -90,3 +91,27 @@ def test_mlflow(self, namespace):
         assert r.status_code == 200
 
         run(f"kubectl delete -f {spec} -n {namespace}", shell=True)
+
+    # Test prepackaged Text SKLearn Alibi Explainer
+    def test_text_alibi_explainer(self, namespace):
+        spec = "../resources/movies-text-explainer.yaml"
+        retry_run(f"kubectl apply -f {spec} -n {namespace}")
+        wait_for_status("movie", namespace)
+        wait_for_rollout("movie", namespace, expected_deployments=2)
+        time.sleep(1)
+        logging.warning("Initial request")
+        r = initial_rest_request(
+            "movie", namespace, data=["This is test data"], dtype="ndarray"
+        )
+        assert r.status_code == 200
+        e = rest_request(
+            "movie",
+            namespace,
+            data=["This is test data"],
+            dtype="ndarray",
+            method="explain",
+            predictor_name="movies-predictor",
+        )
+        assert e.status_code == 200
+        logging.warning("Success for test_prepack_sklearn")
+        run(f"kubectl delete -f {spec} -n {namespace}", shell=True)