Sherif akoush issue/2621/refactor env var retrieval #3420

sakoush · 2021-07-21T12:22:45Z

Consolidate the logic of getting values from enviornment variables as they were duplicated in two places.

What this PR does / why we need it:

Tidy up code.

Which issue(s) this PR fixes:

Fixes #2621

Special notes for your reviewer:

I have introduced env_utils.py to contain the refactored logic. Otherwise there will be circular dependencies.

Does this PR introduce a user-facing change?:

… separate functions.

seldondev · 2021-07-21T12:23:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign majolo
You can assign the PR to them by writing /assign @majolo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

RafalSkolasinski

Nice! Just a few questions

RafalSkolasinski · 2021-07-21T12:24:09Z

python/seldon_core/env_utils.py

@@ -0,0 +1,35 @@
+"""


Could we just have these in the utils.py? Is there a particular reason to have a separate utils file/

There seems to be some circular dependencies if we do that

seldon_core/metrics.py:16: in <module> from seldon_core.utils import get_image_name, get_model_name, get_deployment_name, get_predictor_name, \ seldon_core/utils.py:16: in <module> from seldon_core.user_model import ( seldon_core/user_model.py:10: in <module> from seldon_core.metrics import SeldonMetrics, validate_metrics

I have not investigated a lot into that though. I think that having a separate module to have all env variables handling is ok. isnt it?

Yeah, it's already an improvement to have them in one place so if there is no obvious solution to circular dependency let's leave it as it is 👍

RafalSkolasinski · 2021-07-21T12:25:15Z

python/seldon_core/metrics.py

-logger = logging.getLogger(__name__)
-
-NONIMPLEMENTED_MSG = "NOT_IMPLEMENTED"
+from seldon_core.env_utils import (


we could just (if we put this into utils)

from seldon_core import utils

and then use utils.get_image_name wherever needed

Though both works and and usually over-import so does not require a change now.

python/seldon_core/utils.py

RafalSkolasinski · 2021-07-21T13:32:08Z

/test integration

RafalSkolasinski · 2021-07-21T13:32:14Z

/test notebooks

adriangonz

Nice one @sakoush ! Just had a look and, other than a small typo, everything looks good 👍

adriangonz · 2021-07-21T14:28:22Z

python/seldon_core/env_utils.py

+NONIMPLEMENTED_IMAGE_MSG = f"{NONIMPLEMENTED_MSG}:{NONIMPLEMENTED_MSG}"
+
+
+def get_predictior_version(default_str: str = NONIMPLEMENTED_MSG) -> str:


typo: get_predictor_version instead of get_predictior_version

adriangonz · 2021-07-21T14:31:56Z

python/seldon_core/utils.py

 def get_request_path():
+    model_name = get_model_name()


Conscious that it falls slightly outside the scope of this PR, but it would make sense to return None rather than NONIMPLEMENTED_MSG?

I think some of these are sometimes used for a key in a dictionary. None cannot be a key as far as I remember.

Actually... it can.

it can but if we read it back it will be "None" and not None so we need to convert anyway

adriangonz · 2021-07-21T14:32:21Z

python/seldon_core/utils.py

@@ -348,6 +352,7 @@ def array_to_list_value(array: np.ndarray, lv: Optional[ListValue] = None) -> Li

    Returns
    -------
+    ListValue protobuf


Nice catch 👍

adriangonz · 2021-07-21T14:33:10Z

python/tests/test_model_microservice.py

@@ -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):


Would it make more sense to mock the environment variables rather than the methods?

Having said that, I just realised that you are testing the environment as well down below so maybe you're right in that it makes more sense to mock only the methods here.

RafalSkolasinski · 2021-07-22T09:37:50Z

/test integration

RafalSkolasinski · 2021-07-22T09:37:55Z

/test notebooks

RafalSkolasinski · 2021-07-22T11:23:01Z

Integration tests

================== 4 failed, 31 passed in 3246.50s (0:54:06) ===================

_______________________ TestPrepack.test_openapi_sklearn _______________________
[gw1] linux -- Python 3.7.10 /opt/conda/bin/python

self = <test_prepackaged_servers.TestPrepack object at 0x7f636d7f1110>
namespace = 'test-openapi-sklearn'

    def test_openapi_sklearn(self, namespace):
        spec = "../../servers/sklearnserver/samples/iris.yaml"
        retry_run(f"kubectl apply -f {spec} -n {namespace}")
        wait_for_status("sklearn", namespace)
>       wait_for_rollout("sklearn", namespace)

test_prepackaged_servers.py:250: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
seldon_e2e_utils.py:161: in wait_for_rollout
    wait_for_deployment(deployment_name, namespace, attempts, sleep)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

deployment_name = 'sklearn-default-0-classifier'
namespace = 'test-openapi-sklearn', attempts = 50, sleep = 5

    def wait_for_deployment(deployment_name, namespace, attempts=50, sleep=5):
        logging.info(f"Waiting for deployment {deployment_name}")
        for _ in range(attempts):
            ret = run(
                f"kubectl rollout status -n {namespace} deploy/{deployment_name}",
                shell=True,
            )
            if ret.returncode == 0:
                logging.info(f"Successfully waited for deployment {deployment_name}")
                break
            logging.warning(f"Unsuccessful wait command but retrying for {deployment_name}")
            time.sleep(sleep)
        assert (
            ret.returncode == 0
>       ), f"Wait for rollout of {deployment_name} failed: non-zero return code"
E       AssertionError: Wait for rollout of sklearn-default-0-classifier failed: non-zero return code

seldon_e2e_utils.py:140: AssertionError

____________________ TestPrepack.test_text_alibi_explainer _____________________
[gw1] linux -- Python 3.7.10 /opt/conda/bin/python

self = <test_prepackaged_servers.TestPrepack object at 0x7f636d674610>
namespace = 'test-text-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)

test_prepackaged_servers.py:207: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
seldon_e2e_utils.py:161: in wait_for_rollout
    wait_for_deployment(deployment_name, namespace, attempts, sleep)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

deployment_name = 'movie-movies-predictor-0-classifier'
namespace = 'test-text-alibi-explainer', attempts = 50, sleep = 5

    def wait_for_deployment(deployment_name, namespace, attempts=50, sleep=5):
        logging.info(f"Waiting for deployment {deployment_name}")
        for _ in range(attempts):
            ret = run(
                f"kubectl rollout status -n {namespace} deploy/{deployment_name}",
                shell=True,
            )
            if ret.returncode == 0:
                logging.info(f"Successfully waited for deployment {deployment_name}")
                break
            logging.warning(f"Unsuccessful wait command but retrying for {deployment_name}")
            time.sleep(sleep)
        assert (
            ret.returncode == 0
>       ), f"Wait for rollout of {deployment_name} failed: non-zero return code"
E       AssertionError: Wait for rollout of movie-movies-predictor-0-classifier failed: non-zero return code

seldon_e2e_utils.py:140: AssertionError

___________________________ TestPrepack.test_sklearn ___________________________
[gw1] linux -- Python 3.7.10 /opt/conda/bin/python

self = <test_prepackaged_servers.TestPrepack object at 0x7f636d6d3f90>
namespace = 'test-sklearn'

    def test_sklearn(self, namespace):
        spec = "../../servers/sklearnserver/samples/iris.yaml"
        retry_run(f"kubectl apply -f {spec} -n {namespace}")
        wait_for_status("sklearn", namespace)
>       wait_for_rollout("sklearn", namespace)

test_prepackaged_servers.py:30: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
seldon_e2e_utils.py:161: in wait_for_rollout
    wait_for_deployment(deployment_name, namespace, attempts, sleep)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

deployment_name = 'sklearn-default-0-classifier', namespace = 'test-sklearn'
attempts = 50, sleep = 5

    def wait_for_deployment(deployment_name, namespace, attempts=50, sleep=5):
        logging.info(f"Waiting for deployment {deployment_name}")
        for _ in range(attempts):
            ret = run(
                f"kubectl rollout status -n {namespace} deploy/{deployment_name}",
                shell=True,
            )
            if ret.returncode == 0:
                logging.info(f"Successfully waited for deployment {deployment_name}")
                break
            logging.warning(f"Unsuccessful wait command but retrying for {deployment_name}")
            time.sleep(sleep)
        assert (
            ret.returncode == 0
>       ), f"Wait for rollout of {deployment_name} failed: non-zero return code"
E       AssertionError: Wait for rollout of sklearn-default-0-classifier failed: non-zero return code

seldon_e2e_utils.py:140: AssertionError

______________________ TestBatchWorker.test_batch_worker _______________________
[gw0] linux -- Python 3.7.10 /opt/conda/bin/python

self = <test_batch_processor.TestBatchWorker object at 0x7f7145b52650>
namespace = 'test-batch-worker'

    def test_batch_worker(self, namespace):
        spec = "../../servers/sklearnserver/samples/iris.yaml"
        retry_run(f"kubectl apply -f {spec} -n {namespace}")
        wait_for_status("sklearn", namespace)
>       wait_for_rollout("sklearn", namespace)

test_batch_processor.py:29: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
seldon_e2e_utils.py:161: in wait_for_rollout
    wait_for_deployment(deployment_name, namespace, attempts, sleep)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

deployment_name = 'sklearn-default-0-classifier'
namespace = 'test-batch-worker', attempts = 50, sleep = 5

    def wait_for_deployment(deployment_name, namespace, attempts=50, sleep=5):
        logging.info(f"Waiting for deployment {deployment_name}")
        for _ in range(attempts):
            ret = run(
                f"kubectl rollout status -n {namespace} deploy/{deployment_name}",
                shell=True,
            )
            if ret.returncode == 0:
                logging.info(f"Successfully waited for deployment {deployment_name}")
                break
            logging.warning(f"Unsuccessful wait command but retrying for {deployment_name}")
            time.sleep(sleep)
        assert (
            ret.returncode == 0
>       ), f"Wait for rollout of {deployment_name} failed: non-zero return code"
E       AssertionError: Wait for rollout of sklearn-default-0-classifier failed: non-zero return code

seldon_e2e_utils.py:140: AssertionError

…koush_issue/2621/refactor_env_var_retrieval

sakoush · 2021-07-27T15:13:54Z

/retest

sakoush · 2021-07-27T15:14:31Z

/test integration

sakoush · 2021-07-27T15:14:41Z

/test notebooks

sakoush · 2021-07-28T09:00:40Z

/test notebooks

RafalSkolasinski · 2021-07-28T09:11:50Z

The failure is on TestNotebooks.test_custom_metrics so most likely flakiness 👍

adriangonz · 2021-07-28T10:39:21Z

Thanks for the heads up @RafalSkolasinski! In that case, is it alright to merge this one?

RafalSkolasinski · 2021-07-28T10:46:27Z

Should be alright but the new tests should finish soon so we can wait a few minutes and see

RafalSkolasinski · 2021-07-28T12:03:16Z

/test notebooks

sakoush added 10 commits July 19, 2021 13:58

Minor docs fixes

c4eb21b

refactor getting model and image name from environment variables into…

10f17d2

… separate functions.

move other environment variables to utils.py

0ad71cb

fix tests after refactor

245efca

tidy up tests

b465820

add some unit tests

07a58ea

refactor unit tests

acd5346

formatting

0c93773

add defaults and tests

4f57127

fix formatting issues

e6c3ceb

seldondev added the size/XL label Jul 21, 2021

sakoush requested review from RafalSkolasinski and adriangonz July 21, 2021 12:22

RafalSkolasinski suggested changes Jul 21, 2021

View reviewed changes

revert changes to license files

c20a204

seldondev added size/L and removed size/XL labels Jul 21, 2021

adriangonz reviewed Jul 21, 2021

View reviewed changes

sakoush added 2 commits July 21, 2021 16:55

fix typo

43e17f2

add docs

c1b1001

RafalSkolasinski mentioned this pull request Jul 23, 2021

sklearn iris model incompatible with latest sklearnserver (1.10.0-dev) #3424

Closed

Merge branch 'master' of github.com:SeldonIO/seldon-core into SherifA…

476a7dd

…koush_issue/2621/refactor_env_var_retrieval

sakoush merged commit 6229ab9 into SeldonIO:master Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sherif akoush issue/2621/refactor env var retrieval #3420

Sherif akoush issue/2621/refactor env var retrieval #3420

sakoush commented Jul 21, 2021

seldondev commented Jul 21, 2021

RafalSkolasinski left a comment

RafalSkolasinski Jul 21, 2021

sakoush Jul 21, 2021

RafalSkolasinski Jul 21, 2021

RafalSkolasinski Jul 21, 2021

RafalSkolasinski commented Jul 21, 2021

RafalSkolasinski commented Jul 21, 2021

adriangonz left a comment

adriangonz Jul 21, 2021

sakoush Jul 21, 2021

adriangonz Jul 21, 2021

RafalSkolasinski Jul 21, 2021 •

edited

Loading

RafalSkolasinski Jul 21, 2021

sakoush Jul 21, 2021

adriangonz Jul 21, 2021

adriangonz Jul 21, 2021

adriangonz Jul 21, 2021

RafalSkolasinski commented Jul 22, 2021

RafalSkolasinski commented Jul 22, 2021

RafalSkolasinski commented Jul 22, 2021

sakoush commented Jul 27, 2021

sakoush commented Jul 27, 2021

sakoush commented Jul 27, 2021

sakoush commented Jul 28, 2021

RafalSkolasinski commented Jul 28, 2021

adriangonz commented Jul 28, 2021

RafalSkolasinski commented Jul 28, 2021

RafalSkolasinski commented Jul 28, 2021

		NONIMPLEMENTED_IMAGE_MSG = f"{NONIMPLEMENTED_MSG}:{NONIMPLEMENTED_MSG}"


		def get_predictior_version(default_str: str = NONIMPLEMENTED_MSG) -> str:

Sherif akoush issue/2621/refactor env var retrieval #3420

Sherif akoush issue/2621/refactor env var retrieval #3420

Conversation

sakoush commented Jul 21, 2021

seldondev commented Jul 21, 2021

RafalSkolasinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalSkolasinski commented Jul 21, 2021

RafalSkolasinski commented Jul 21, 2021

adriangonz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalSkolasinski Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafalSkolasinski commented Jul 22, 2021

RafalSkolasinski commented Jul 22, 2021

RafalSkolasinski commented Jul 22, 2021

sakoush commented Jul 27, 2021

sakoush commented Jul 27, 2021

sakoush commented Jul 27, 2021

sakoush commented Jul 28, 2021

RafalSkolasinski commented Jul 28, 2021

adriangonz commented Jul 28, 2021

RafalSkolasinski commented Jul 28, 2021

RafalSkolasinski commented Jul 28, 2021

RafalSkolasinski Jul 21, 2021 •

edited

Loading