Sherif akoush issue/2621/refactor env var retrieval

components/alibi-detect-server/adserver/cm_model.py

@@ -11,7 +11,8 @@
 from adserver.base import CEModel, ModelResponse
 from adserver.base.storage import download_model
 from seldon_core.flask_utils import SeldonMicroserviceException
-from seldon_core.metrics import DEFAULT_LABELS, NONIMPLEMENTED_MSG
+from seldon_core.metrics import DEFAULT_LABELS
+from seldon_core.env_utils import NONIMPLEMENTED_MSG
 from elasticsearch import Elasticsearch
 from elasticsearch.exceptions import NotFoundError
 

python/requirements-dev.txt

@@ -11,5 +11,7 @@ pillow==8.1.1
 pip-licenses==3.4.0          # fetch licenses
 pytest-cov==2.10.1
 pytest==6.2.2
+pytest-mock==3.6.1
 tenacity==6.3.1              # tenacity - used for smart retrying
+nbqa==0.13.1
 tox<4.0.0

python/seldon_core/env_utils.py

@@ -0,0 +1,35 @@
+"""
+Utilities to deal with Environment variables
+"""
+import json
+import os
+
+ENV_MODEL_NAME = "PREDICTIVE_UNIT_ID"
+ENV_MODEL_IMAGE = "PREDICTIVE_UNIT_IMAGE"
+ENV_SELDON_DEPLOYMENT_NAME = "SELDON_DEPLOYMENT_ID"
+ENV_PREDICTOR_NAME = "PREDICTOR_ID"
+ENV_PREDICTOR_LABELS = "PREDICTOR_LABELS"
+NONIMPLEMENTED_MSG = "NOT_IMPLEMENTED"
+NONIMPLEMENTED_IMAGE_MSG = f"{NONIMPLEMENTED_MSG}:{NONIMPLEMENTED_MSG}"
+
+
+def get_predictior_version(default_str: str = NONIMPLEMENTED_MSG) -> str:
+    return json.loads(os.environ.get(ENV_PREDICTOR_LABELS, "{}")).get(
+        "version", default_str
+    )
+
+
+def get_predictor_name(default_str: str = NONIMPLEMENTED_MSG) -> str:
+    return os.environ.get(ENV_PREDICTOR_NAME, default_str)
+
+
+def get_deployment_name(default_str: str = NONIMPLEMENTED_MSG) -> str:
+    return os.environ.get(ENV_SELDON_DEPLOYMENT_NAME, default_str)
+
+
+def get_model_name(default_str: str = NONIMPLEMENTED_MSG) -> str:
+    return os.environ.get(ENV_MODEL_NAME, default_str)
+
+
+def get_image_name(default_str: str = NONIMPLEMENTED_IMAGE_MSG) -> str:
+    return os.environ.get(ENV_MODEL_IMAGE, default_str)

python/seldon_core/metrics.py

@@ -1,4 +1,3 @@
-import json
 import logging
 import os
 from multiprocessing import Manager
@@ -14,15 +13,15 @@
 )
 from prometheus_client.utils import floatToGoString
 
-logger = logging.getLogger(__name__)
-
-NONIMPLEMENTED_MSG = "NOT_IMPLEMENTED"
+from seldon_core.env_utils import (
+    get_deployment_name,
+    get_image_name,
+    get_model_name,
+    get_predictior_version,
+    get_predictor_name,
+)
 
-ENV_SELDON_DEPLOYMENT_NAME = "SELDON_DEPLOYMENT_ID"
-ENV_MODEL_NAME = "PREDICTIVE_UNIT_ID"
-ENV_MODEL_IMAGE = "PREDICTIVE_UNIT_IMAGE"
-ENV_PREDICTOR_NAME = "PREDICTOR_ID"
-ENV_PREDICTOR_LABELS = "PREDICTOR_LABELS"
+logger = logging.getLogger(__name__)
 
 FEEDBACK_KEY = "seldon_api_model_feedback"
 FEEDBACK_REWARD_KEY = "seldon_api_model_feedback_reward"
@@ -53,22 +52,18 @@ def split_image_tag(tag: str) -> Tuple[str]:
 
 
 # Development placeholder
-image = os.environ.get(ENV_MODEL_IMAGE, f"{NONIMPLEMENTED_MSG}:{NONIMPLEMENTED_MSG}")
+image = get_image_name()
 model_image, model_version = split_image_tag(image)
-predictor_version = json.loads(os.environ.get(ENV_PREDICTOR_LABELS, "{}")).get(
-    "version", f"{NONIMPLEMENTED_MSG}"
-)
+predictor_version = get_predictior_version()
 
 legacy_mode = os.environ.get("SELDON_EXECUTOR_ENABLED", "true").lower() == "false"
 
 DEFAULT_LABELS = {
-    "deployment_name": os.environ.get(
-        ENV_SELDON_DEPLOYMENT_NAME, f"{NONIMPLEMENTED_MSG}"
-    ),
-    "model_name": os.environ.get(ENV_MODEL_NAME, f"{NONIMPLEMENTED_MSG}"),
+    "deployment_name": get_deployment_name(),
+    "model_name": get_model_name(),
     "model_image": model_image,
     "model_version": model_version,
-    "predictor_name": os.environ.get(ENV_PREDICTOR_NAME, f"{NONIMPLEMENTED_MSG}"),
+    "predictor_name": get_predictor_name(),
     "predictor_version": predictor_version,
 }
 

python/seldon_core/utils.py

@@ -10,6 +10,14 @@
 from google.protobuf.json_format import MessageToDict, ParseDict
 from google.protobuf.struct_pb2 import ListValue
 
+from seldon_core.env_utils import (
+    ENV_MODEL_IMAGE,
+    ENV_MODEL_NAME,
+    NONIMPLEMENTED_IMAGE_MSG,
+    NONIMPLEMENTED_MSG,
+    get_image_name,
+    get_model_name,
+)
 from seldon_core.flask_utils import SeldonMicroserviceException
 from seldon_core.imports_helper import _TF_PRESENT
 from seldon_core.proto import prediction_pb2
@@ -27,19 +35,14 @@
 logger = logging.getLogger(__name__)
 
 
-ENV_MODEL_NAME = "PREDICTIVE_UNIT_ID"
-ENV_MODEL_IMAGE = "PREDICTIVE_UNIT_IMAGE"
-NONIMPLEMENTED_MSG = "NOT_IMPLEMENTED"
-
-model_name = os.environ.get(ENV_MODEL_NAME, f"{NONIMPLEMENTED_MSG}")
-image_name = os.environ.get(
-    ENV_MODEL_IMAGE, f"{NONIMPLEMENTED_MSG}:{NONIMPLEMENTED_MSG}"
-)
-
-
 def get_request_path():
+    model_name = get_model_name()
     if model_name == NONIMPLEMENTED_MSG:
         return {}
+    image_name = get_image_name()
+    assert (
+        image_name != NONIMPLEMENTED_IMAGE_MSG
+    ), f"Both {ENV_MODEL_NAME} and {ENV_MODEL_IMAGE} have to be set"
     return {model_name: image_name}
 
 
@@ -164,6 +167,7 @@ def feedback_to_json(message_proto: prediction_pb2.Feedback) -> Dict:
     ----------
     message_proto
        SeldonMessage proto
+
     Returns
     -------
        JSON Dict
@@ -348,6 +352,7 @@ def array_to_list_value(array: np.ndarray, lv: Optional[ListValue] = None) -> Li
 
     Returns
     -------
+    ListValue protobuf
 
     """
     if lv is None:

python/seldon_core/wrapper.py

@@ -8,6 +8,7 @@
 from grpc_reflection.v1alpha import reflection
 
 import seldon_core.seldon_methods
+from seldon_core.env_utils import get_model_name
 from seldon_core.flask_utils import (
     ANNOTATION_GRPC_MAX_MSG_SIZE,
     SeldonMicroserviceException,
@@ -24,7 +25,7 @@
 
 logger = logging.getLogger(__name__)
 
-PRED_UNIT_ID = os.environ.get("PREDICTIVE_UNIT_ID", "0")
+PRED_UNIT_ID = get_model_name("0")
 METRICS_ENDPOINT = os.environ.get("PREDICTIVE_UNIT_METRICS_ENDPOINT", "/metrics")
 PAYLOAD_PASSTHROUGH = getenv_as_bool("PAYLOAD_PASSTHROUGH", default=False)
 

python/tests/test_model_microservice.py

@@ -2,9 +2,9 @@
 import io
 import json
 import logging
-from unittest import mock
 
 import numpy as np
+import pytest
 from google.protobuf import json_format
 from PIL import Image
 
@@ -240,6 +240,22 @@ def send_feedback_grpc(self, request):
         logging.info("Feedback called")
 
 
+@pytest.fixture(name="mock_get_model_name")
+def fixture_get_model_name(mocker):
+    return mocker.patch(
+        "seldon_core.utils.get_model_name", autospec=True, return_value="my-test-model"
+    )
+
+
+@pytest.fixture(name="mock_get_image_name")
+def fixture_get_image_name(mocker):
+    return mocker.patch(
+        "seldon_core.utils.get_image_name",
+        autospec=True,
+        return_value="my-test-model-image",
+    )
+
+
 def test_model_ok():
     user_object = UserObject()
     seldon_metrics = SeldonMetrics()
@@ -311,9 +327,7 @@ def test_model_puid_ok():
     assert j["meta"]["puid"] == "123"
 
 
-@mock.patch("seldon_core.utils.model_name", "my-test-model")
-@mock.patch("seldon_core.utils.image_name", "my-test-model-image")
-def test_requestPath_ok():
+def test_requestPath_ok(mock_get_model_name, mock_get_image_name):
     user_object = UserObject()
     seldon_metrics = SeldonMetrics()
     app = get_rest_microservice(user_object, seldon_metrics)
@@ -327,9 +341,7 @@ def test_requestPath_ok():
     assert j["meta"]["requestPath"] == {"my-test-model": "my-test-model-image"}
 
 
-@mock.patch("seldon_core.utils.model_name", "my-test-model")
-@mock.patch("seldon_core.utils.image_name", "my-test-model-image")
-def test_requestPath_2nd_node_ok():
+def test_requestPath_2nd_node_ok(mock_get_model_name, mock_get_image_name):
     user_object = UserObject()
     seldon_metrics = SeldonMetrics()
     app = get_rest_microservice(user_object, seldon_metrics)
@@ -346,9 +358,7 @@ def test_requestPath_2nd_node_ok():
     }
 
 
-@mock.patch("seldon_core.utils.model_name", "my-test-model")
-@mock.patch("seldon_core.utils.image_name", "my-test-model-image")
-def test_proto_requestPath_ok():
+def test_proto_requestPath_ok(mock_get_model_name, mock_get_image_name):
     user_object = UserObject()
     seldon_metrics = SeldonMetrics()
     app = SeldonModelGRPC(user_object, seldon_metrics)
@@ -366,9 +376,7 @@ def test_proto_requestPath_ok():
     assert j["meta"]["requestPath"] == {"my-test-model": "my-test-model-image"}
 
 
-@mock.patch("seldon_core.utils.model_name", "my-test-model")
-@mock.patch("seldon_core.utils.image_name", "my-test-model-image")
-def test_proto_requestPath_2nd_node_ok():
+def test_proto_requestPath_2nd_node_ok(mock_get_model_name, mock_get_image_name):
     user_object = UserObject()
     seldon_metrics = SeldonMetrics()
     app = SeldonModelGRPC(user_object, seldon_metrics)

python/tests/test_utils.py

@@ -1,4 +1,5 @@
 import base64
+import json
 import logging
 import pickle
 
@@ -8,6 +9,20 @@
 from google.protobuf.struct_pb2 import Value
 
 import seldon_core.utils as scu
+from seldon_core.env_utils import (
+    ENV_MODEL_IMAGE,
+    ENV_MODEL_NAME,
+    ENV_PREDICTOR_LABELS,
+    ENV_PREDICTOR_NAME,
+    ENV_SELDON_DEPLOYMENT_NAME,
+    NONIMPLEMENTED_IMAGE_MSG,
+    NONIMPLEMENTED_MSG,
+    get_deployment_name,
+    get_image_name,
+    get_model_name,
+    get_predictior_version,
+    get_predictor_name,
+)
 from seldon_core.flask_utils import SeldonMicroserviceException
 from seldon_core.imports_helper import _TF_PRESENT
 from seldon_core.proto import prediction_pb2
@@ -469,3 +484,67 @@ def test_getenv_as_bool(monkeypatch, env_val, expected):
 
     value = scu.getenv_as_bool(env_var, default=False)
     assert value == expected
+
+
+class TestEnvironmentVariables:
+    """
+    Tests for getting values from environment variables
+    """
+
+    @pytest.mark.parametrize(
+        "val, expected_val, env_var, getter",
+        [
+            (
+                "DUMMY_VAL_NAME",
+                "DUMMY_VAL_NAME",
+                ENV_SELDON_DEPLOYMENT_NAME,
+                get_deployment_name,
+            ),
+            ("DUMMY_VAL_NAME", "DUMMY_VAL_NAME", ENV_MODEL_NAME, get_model_name),
+            ("DUMMY_VAL_NAME", "DUMMY_VAL_NAME", ENV_MODEL_IMAGE, get_image_name),
+            (
+                "DUMMY_VAL_NAME",
+                "DUMMY_VAL_NAME",
+                ENV_PREDICTOR_NAME,
+                get_predictor_name,
+            ),
+            (
+                json.dumps({"key": "dummy", "version": "2"}),
+                "2",
+                ENV_PREDICTOR_LABELS,
+                get_predictior_version,
+            ),
+        ],
+    )
+    def test_get_deployment_name_ok(
+        self, monkeypatch, val, expected_val, env_var, getter
+    ):
+        monkeypatch.setenv(env_var, val)
+        assert getter() == expected_val
+
+    @pytest.mark.parametrize(
+        "val, getter",
+        [
+            (NONIMPLEMENTED_MSG, get_deployment_name),
+            (NONIMPLEMENTED_MSG, get_model_name),
+            (NONIMPLEMENTED_IMAGE_MSG, get_image_name),
+            (NONIMPLEMENTED_MSG, get_predictor_name),
+            (NONIMPLEMENTED_MSG, get_predictior_version),
+        ],
+    )
+    def test_env_notset_ok(self, val, getter):
+        assert getter() == val
+
+    @pytest.mark.parametrize(
+        "val, getter",
+        [
+            ("0", get_deployment_name),
+            ("0", get_model_name),
+            ("0", get_model_name),
+            ("0", get_image_name),
+            ("0", get_predictor_name),
+            ("0", get_predictior_version),
+        ],
+    )
+    def test_env_notset_with_default_ok(self, val, getter):
+        assert getter(default_str=val) == val