From 4a962723860bb4c3b4ab9826254ade61dba1c1a7 Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Mon, 2 May 2022 17:36:43 -0700 Subject: [PATCH 1/7] Implement poller and in-memory cache --- eppo_client/__init__.py | 69 ++++++++++++++++++++ eppo_client/client.py | 20 +++++- eppo_client/configuration_requestor.py | 38 +++++++++-- eppo_client/configuration_store.py | 29 +++++++++ eppo_client/constants.py | 9 +++ eppo_client/http_client.py | 63 ++++++++++++++++++ eppo_client/poller.py | 33 ++++++++++ eppo_client/read_write_lock.py | 42 ++++++++++++ requirements-test.txt | 3 +- requirements.txt | 4 +- test/client_test.py | 88 +++++++++++++------------- test/configuration_store_test.py | 21 ++++++ test/poller_test.py | 21 ++++++ 13 files changed, 388 insertions(+), 52 deletions(-) create mode 100644 eppo_client/configuration_store.py create mode 100644 eppo_client/constants.py create mode 100644 eppo_client/http_client.py create mode 100644 eppo_client/poller.py create mode 100644 eppo_client/read_write_lock.py create mode 100644 test/configuration_store_test.py create mode 100644 test/poller_test.py diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index e69de29..ca46d90 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -0,0 +1,69 @@ +from typing import Optional +from eppo_client.client import EppoClient +from eppo_client.config import Config +from eppo_client.configuration_requestor import ExperimentConfigurationRequestor +from eppo_client.configuration_store import ConfigurationStore +from eppo_client.constants import CACHE_TTL_SECONDS, MAX_CACHE_ENTRIES +from eppo_client.http_client import HttpClient, SdkParams +from eppo_client.read_write_lock import ReadWriteLock + +with open("VERSION.txt") as version_file: + __version__ = version_file.read().strip() + +__client: Optional[EppoClient] = None +__lock = ReadWriteLock() + + +def init(config: Config) -> EppoClient: + """Initializes a global Eppo client instance + + This method should be called once on application startup. + If invoked more than once, it will re-initialize the global client instance. + Use the :func:`eppo_client.get_instance()` method to access the client instance. + + :param config: client configuration containing the API Key + :type config: Config + """ + config._validate() + sdk_params = SdkParams( + apiKey=config.api_key, sdkName="python", sdkVersion=__version__ + ) + http_client = HttpClient(base_url=config.base_url, sdk_params=sdk_params) + config_store = ConfigurationStore( + max_size=MAX_CACHE_ENTRIES, ttl_seconds=CACHE_TTL_SECONDS + ) + config_requestor = ExperimentConfigurationRequestor( + http_client=http_client, config_store=config_store + ) + global __client + global __lock + try: + __lock.acquire_write() + if __client: + # if a client was already initialized, stop the background processes of the old client + __client._shutdown() + __client = EppoClient(config_requestor=config_requestor) + return __client + finally: + __lock.release_write() + + +def get_instance() -> EppoClient: + """Used to access an initialized client instance + + Use this method to get a client instance for assigning variants. + This method may only be called after invocation of :func:`eppo_client.init()`, otherwise it throws an exception. + + :return: a shared client instance + :rtype: EppoClient + """ + try: + global __client + global __lock + __lock.acquire_read() + if __client: + return __client + else: + raise Exception("init() must be called before get_instance()") + finally: + __lock.release_read() diff --git a/eppo_client/client.py b/eppo_client/client.py index cc3ef4a..8857d4b 100644 --- a/eppo_client/client.py +++ b/eppo_client/client.py @@ -3,13 +3,21 @@ ExperimentConfigurationDto, ExperimentConfigurationRequestor, ) +from eppo_client.constants import POLL_INTERVAL_MILLIS, POLL_JITTER_MILLIS +from eppo_client.poller import Poller from eppo_client.shard import get_shard, is_in_shard_range from eppo_client.validation import validate_not_blank class EppoClient: - def __init__(self, config_requestor: ExperimentConfigurationRequestor) -> None: + def __init__(self, config_requestor: ExperimentConfigurationRequestor): self.__config_requestor = config_requestor + self.__poller = Poller( + interval_millis=POLL_INTERVAL_MILLIS, + jitter_millis=POLL_JITTER_MILLIS, + callback=config_requestor.fetch_and_store_configurations, + ) + self.__poller.start() def assign(self, subject: str, experiment_key: str) -> Optional[str]: """Maps a subject to a variation for a given experiment @@ -24,7 +32,7 @@ def assign(self, subject: str, experiment_key: str) -> Optional[str]: if ( experiment_config is None or not experiment_config.enabled - or not self.is_in_experiment_sample( + or not self._is_in_experiment_sample( subject, experiment_key, experiment_config ) ): @@ -42,7 +50,13 @@ def assign(self, subject: str, experiment_key: str) -> Optional[str]: None, ) - def is_in_experiment_sample( + def _shutdown(self): + """Stops all background processes used by the client + Do not use the client after calling this method. + """ + self.__poller.stop() + + def _is_in_experiment_sample( self, subject: str, experiment_key: str, diff --git a/eppo_client/configuration_requestor.py b/eppo_client/configuration_requestor.py index 5aa2686..aa55425 100644 --- a/eppo_client/configuration_requestor.py +++ b/eppo_client/configuration_requestor.py @@ -1,8 +1,13 @@ -from typing import List, Optional +import logging +from typing import Dict, List, Optional, cast from eppo_client.base_model import SdkBaseModel +from eppo_client.configuration_store import ConfigurationStore +from eppo_client.http_client import HttpClient, HttpRequestError from eppo_client.shard import ShardRange +logger = logging.getLogger(__name__) + class VariationDto(SdkBaseModel): name: str @@ -14,12 +19,37 @@ class ExperimentConfigurationDto(SdkBaseModel): percent_exposure: float enabled: bool variations: List[VariationDto] - name: str + name: Optional[str] + + +RAC_ENDPOINT = "/randomized_assignment/config" class ExperimentConfigurationRequestor: + def __init__( + self, + http_client: HttpClient, + config_store: ConfigurationStore[ExperimentConfigurationDto], + ): + self.__http_client = http_client + self.__config_store = config_store + def get_configuration( self, experiment_key: str ) -> Optional[ExperimentConfigurationDto]: - # TODO: implement this method - return None + if self.__http_client.is_unauthorized(): + raise ValueError("Unauthorized: please check your API key") + return self.__config_store.get_configuration(experiment_key) + + def fetch_and_store_configurations(self) -> Dict[str, ExperimentConfigurationDto]: + try: + configs = cast(dict, self.__http_client.get(RAC_ENDPOINT)) + for exp_key, exp_config in configs.get("experiments", {}).items(): + configs[exp_key] = ExperimentConfigurationDto(**exp_config) + self.__config_store.set_configurations(configs) + except HttpRequestError as e: + logger.error("Error retrieving assignment configurations: " + str(e)) + if e.is_recoverable(): + return {} + else: + raise e # caught by the polling task; causes assignment polling to stop diff --git a/eppo_client/configuration_store.py b/eppo_client/configuration_store.py new file mode 100644 index 0000000..6cb81b5 --- /dev/null +++ b/eppo_client/configuration_store.py @@ -0,0 +1,29 @@ +from typing import Dict, Optional, TypeVar, Generic +from cachetools import TTLCache + +from eppo_client.read_write_lock import ReadWriteLock + +T = TypeVar("T") + + +class ConfigurationStore(Generic[T]): + def __init__(self, ttl_seconds: float, max_size: int): + self.__cache = TTLCache(maxsize=max_size, ttl=ttl_seconds) + self.__lock = ReadWriteLock() + + def get_configuration(self, key: str) -> Optional[T]: + try: + self.__lock.acquire_read() + return self.__cache[key] + except KeyError: + return None # key does not exist + finally: + self.__lock.release_read() + + def set_configurations(self, configs: Dict[str, T]): + try: + self.__lock.acquire_write() + for key, config in configs.items(): + self.__cache[key] = config + finally: + self.__lock.release_write() diff --git a/eppo_client/constants.py b/eppo_client/constants.py new file mode 100644 index 0000000..6315a91 --- /dev/null +++ b/eppo_client/constants.py @@ -0,0 +1,9 @@ +# configuration cache +MAX_CACHE_ENTRIES = 1000 # arbitrary; the caching library requires a max limit +CACHE_TTL_SECONDS = 15 * 60 + +# poller +SECOND_MILLIS = 1000 +MINUTE_MILLIS = 60 * SECOND_MILLIS +POLL_JITTER_MILLIS = 30 * SECOND_MILLIS +POLL_INTERVAL_MILLIS = 5 * MINUTE_MILLIS diff --git a/eppo_client/http_client.py b/eppo_client/http_client.py new file mode 100644 index 0000000..a37a2b9 --- /dev/null +++ b/eppo_client/http_client.py @@ -0,0 +1,63 @@ +from typing import Any +from pydantic.dataclasses import dataclass +from requests.adapters import HTTPAdapter, Retry +from http import HTTPStatus + +import requests + +from eppo_client.base_model import SdkBaseModel + + +class SdkParams(SdkBaseModel): + # attributes are camelCase because that's what the backend endpoint expects + apiKey: str + sdkName: str + sdkVersion: str + + +class HttpRequestError(Exception): + def __init__(self, message: str, status_code: int): + super().__init__(message) + self.status_code = status_code + + def is_recoverable(self) -> bool: + if self.status_code >= 400 and self.status_code < 500: + return ( + self.status_code == HTTPStatus.TOO_MANY_REQUESTS + or self.status_code == HTTPStatus.REQUEST_TIMEOUT + ) + return True + + +REQUEST_TIMEOUT_SECONDS = 2 +# Retry reference: https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#module-urllib3.util.retry +# This applies only to failed DNS lookups and connection timeouts, never to requests where data has made it to the server. +MAX_RETRIES = Retry(total=3, backoff_factor=1) + + +class HttpClient: + def __init__(self, base_url: str, sdk_params: SdkParams): + self.__base_url = base_url + self.__sdk_params = sdk_params + self.__session = requests.Session() + self.__session.mount("https://", HTTPAdapter(max_retries=MAX_RETRIES)) + self.__is_unauthorized = False + + def is_unauthorized(self) -> bool: + return self.__is_unauthorized + + def get(self, resource: str) -> Any: + response = self.__session.get( + self.__base_url + resource, + params=self.__sdk_params.dict(), + timeout=REQUEST_TIMEOUT_SECONDS, + ) + self.__is_unauthorized = response.status_code == HTTPStatus.UNAUTHORIZED + if response.status_code == HTTPStatus.OK.value: + return response.json() + raise HttpRequestError( + "HTTP {} error while requesting resource {}".format( + response.status_code, resource + ), + status_code=response.status_code, + ) diff --git a/eppo_client/poller.py b/eppo_client/poller.py new file mode 100644 index 0000000..88b5686 --- /dev/null +++ b/eppo_client/poller.py @@ -0,0 +1,33 @@ +import logging +from multiprocessing import Event +from random import randrange +from threading import Thread +from typing import Callable + +logger = logging.getLogger(__name__) + + +class Poller(Thread): + def __init__(self, interval_millis: int, jitter_millis: int, callback: Callable): + self.__jitter_millis = jitter_millis + self.__interval = interval_millis + self.__callback = callback + self.__stop_event = Event() + super().__init__(daemon=True) + + def stop(self): + self.__stop_event.set() + + def run(self): + is_stopped = self.__stop_event.is_set() + while not is_stopped: + try: + self.__callback() + except Exception as e: + logger.error("Unexpected error running poll task: " + str(e)) + break + is_stopped = self._wait_for_interval() + + def _wait_for_interval(self): + interval_with_jitter = self.__interval - randrange(0, self.__jitter_millis) + self.__stop_event.wait(interval_with_jitter / 1000) diff --git a/eppo_client/read_write_lock.py b/eppo_client/read_write_lock.py new file mode 100644 index 0000000..7f0c5fe --- /dev/null +++ b/eppo_client/read_write_lock.py @@ -0,0 +1,42 @@ +import threading + +# Copied from: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch06s04.html + + +class ReadWriteLock: + """A lock object that allows many simultaneous "read locks", but + only one "write lock." """ + + def __init__(self): + self._read_ready = threading.Condition(threading.Lock()) + self._readers = 0 + + def acquire_read(self): + """Acquire a read lock. Blocks only if a thread has + acquired the write lock.""" + self._read_ready.acquire() + try: + self._readers += 1 + finally: + self._read_ready.release() + + def release_read(self): + """Release a read lock.""" + self._read_ready.acquire() + try: + self._readers -= 1 + if not self._readers: + self._read_ready.notifyAll() + finally: + self._read_ready.release() + + def acquire_write(self): + """Acquire a write lock. Blocks until there are no + acquired read or write locks.""" + self._read_ready.acquire() + while self._readers > 0: + self._read_ready.wait() + + def release_write(self): + """Release a write lock.""" + self._read_ready.release() diff --git a/requirements-test.txt b/requirements-test.txt index 51ee95f..0a5b952 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,4 +1,5 @@ tox pytest mypy -google-cloud-storage \ No newline at end of file +google-cloud-storage +httpretty \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 59cc1e9..2c40773 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1 +1,3 @@ -pydantic \ No newline at end of file +pydantic +requests +cachetools diff --git a/test/client_test.py b/test/client_test.py index 8b2486e..0432755 100644 --- a/test/client_test.py +++ b/test/client_test.py @@ -1,49 +1,63 @@ import json import os -from typing import List, Optional from unittest.mock import patch +import httpretty import pytest -from eppo_client.base_model import SdkBaseModel from eppo_client.client import EppoClient +from eppo_client.config import Config from eppo_client.configuration_requestor import ( ExperimentConfigurationDto, - ExperimentConfigurationRequestor, VariationDto, ) from eppo_client.shard import ShardRange - -mock_api_key = "mock-api-key" - - -class AssignmentTestCase(SdkBaseModel): - experiment: str - percent_exposure: float - variations: List[VariationDto] - subjects: List[str] - expected_assignments: List[Optional[str]] - +from eppo_client import init, get_instance test_data = [] for file_name in [file for file in os.listdir("test/test-data/assignment")]: with open("test/test-data/assignment/{}".format(file_name)) as test_case_json: test_case_dict = json.load(test_case_json) - variations = [ - VariationDto(**variation_dict) - for variation_dict in test_case_dict["variations"] - ] - test_case_dict["variations"] = variations - test_data.append(AssignmentTestCase(**test_case_dict)) + test_data.append(test_case_dict) +exp_configs = dict() +for experiment_test in test_data: + experiment_name = experiment_test["experiment"] + exp_configs[experiment_name] = { + "subjectShards": 10000, + "enabled": True, + "variations": experiment_test["variations"], + "name": experiment_name, + "percentExposure": experiment_test["percentExposure"], + } -def test_assign_blank_experiment(): - client = EppoClient(config_requestor=ExperimentConfigurationRequestor()) +MOCK_BASE_URL = "http://localhost:4000/api" + + +@pytest.fixture(scope="session", autouse=True) +def init_fixture(): + httpretty.enable() + config_response_json = json.dumps({"experiments": exp_configs}) + httpretty.register_uri( + httpretty.GET, + MOCK_BASE_URL + "/randomized_assignment/config", + body=config_response_json, + ) + client = init(Config(base_url=MOCK_BASE_URL, api_key="dummy")) + yield + client._shutdown() + httpretty.disable() + + +@patch("eppo_client.configuration_requestor.ExperimentConfigurationRequestor") +def test_assign_blank_experiment(mock_config_requestor): + client = EppoClient(config_requestor=mock_config_requestor) with pytest.raises(Exception) as exc_info: client.assign("subject-1", "") assert exc_info.value.args[0] == "Invalid value for experiment_key: cannot be blank" -def test_assign_blank_subject(): - client = EppoClient(config_requestor=ExperimentConfigurationRequestor()) +@patch("eppo_client.configuration_requestor.ExperimentConfigurationRequestor") +def test_assign_blank_subject(mock_config_requestor): + client = EppoClient(config_requestor=mock_config_requestor) with pytest.raises(Exception) as exc_info: client.assign("", "experiment-1") assert exc_info.value.args[0] == "Invalid value for subject: cannot be blank" @@ -66,22 +80,10 @@ def test_assign_subject_not_in_sample(mock_config_requestor): @pytest.mark.parametrize("test_case", test_data) def test_assign_subject_in_sample(test_case): - print("---- Test case for {} Experiment".format(test_case.experiment)) - with patch( - "eppo_client.configuration_requestor.ExperimentConfigurationRequestor" - ) as mock_config_requestor: - mock_config_requestor.get_configuration.return_value = ( - ExperimentConfigurationDto( - subjectShards=10000, - percentExposure=test_case.percent_exposure, - enabled=True, - variations=test_case.variations, - name=test_case.experiment, - ) - ) - client = EppoClient(config_requestor=mock_config_requestor) - assignments = [ - client.assign(subject, test_case.experiment) - for subject in test_case.subjects - ] - assert assignments == test_case.expected_assignments + print("---- Test case for {} Experiment".format(test_case["experiment"])) + client = get_instance() + assignments = [ + client.assign(subject, test_case["experiment"]) + for subject in test_case["subjects"] + ] + assert assignments == test_case["expectedAssignments"] diff --git a/test/configuration_store_test.py b/test/configuration_store_test.py new file mode 100644 index 0000000..fc1027e --- /dev/null +++ b/test/configuration_store_test.py @@ -0,0 +1,21 @@ +from eppo_client.configuration_requestor import ExperimentConfigurationDto +from eppo_client.configuration_store import ConfigurationStore + +test_exp = ExperimentConfigurationDto( + subject_shards=1000, + percent_exposure=1, + enabled=True, + variations=[], + name="randomization_algo", +) + +store = ConfigurationStore(ttl_seconds=100, max_size=1) +store.set_configurations({"randomization_algo": test_exp}) + + +def test_get_configuration_unknown_key(): + assert store.get_configuration("unknown_exp") == None + + +def test_get_configuration_known_key(): + assert store.get_configuration("randomization_algo") == test_exp diff --git a/test/poller_test.py b/test/poller_test.py new file mode 100644 index 0000000..3bc681e --- /dev/null +++ b/test/poller_test.py @@ -0,0 +1,21 @@ +from time import sleep +from unittest.mock import Mock +from eppo_client.poller import Poller + + +def test_invokes_callback_until_stopped(): + callback = Mock(return_value=None) + task = Poller(interval_millis=10, jitter_millis=1, callback=callback) + task.start() + sleep(0.099) + task.stop() + assert callback.call_count == 10 + + +def test_stops_polling_if_unexpected_error(): + callback = Mock(side_effect=Exception("bad request")) + task = Poller(interval_millis=10, jitter_millis=1, callback=callback) + task.start() + sleep(0.099) + task.stop() + assert callback.call_count == 1 From 794f9910e2c1ef2031a0ea48e1d355597bb50e1b Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Wed, 4 May 2022 08:15:26 -0700 Subject: [PATCH 2/7] fix linter errors --- eppo_client/__init__.py | 7 +++++-- eppo_client/configuration_requestor.py | 7 +++++-- eppo_client/configuration_store.py | 2 +- eppo_client/http_client.py | 4 ++-- requirements.txt | 2 ++ test/client_test.py | 2 +- test/configuration_store_test.py | 6 ++++-- 7 files changed, 20 insertions(+), 10 deletions(-) diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index ca46d90..2aba028 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -1,7 +1,10 @@ from typing import Optional from eppo_client.client import EppoClient from eppo_client.config import Config -from eppo_client.configuration_requestor import ExperimentConfigurationRequestor +from eppo_client.configuration_requestor import ( + ExperimentConfigurationDto, + ExperimentConfigurationRequestor, +) from eppo_client.configuration_store import ConfigurationStore from eppo_client.constants import CACHE_TTL_SECONDS, MAX_CACHE_ENTRIES from eppo_client.http_client import HttpClient, SdkParams @@ -29,7 +32,7 @@ def init(config: Config) -> EppoClient: apiKey=config.api_key, sdkName="python", sdkVersion=__version__ ) http_client = HttpClient(base_url=config.base_url, sdk_params=sdk_params) - config_store = ConfigurationStore( + config_store: ConfigurationStore[ExperimentConfigurationDto] = ConfigurationStore( max_size=MAX_CACHE_ENTRIES, ttl_seconds=CACHE_TTL_SECONDS ) config_requestor = ExperimentConfigurationRequestor( diff --git a/eppo_client/configuration_requestor.py b/eppo_client/configuration_requestor.py index aa55425..9030153 100644 --- a/eppo_client/configuration_requestor.py +++ b/eppo_client/configuration_requestor.py @@ -43,10 +43,13 @@ def get_configuration( def fetch_and_store_configurations(self) -> Dict[str, ExperimentConfigurationDto]: try: - configs = cast(dict, self.__http_client.get(RAC_ENDPOINT)) - for exp_key, exp_config in configs.get("experiments", {}).items(): + configs = cast( + dict, self.__http_client.get(RAC_ENDPOINT).get("experiments", {}) + ) + for exp_key, exp_config in configs.items(): configs[exp_key] = ExperimentConfigurationDto(**exp_config) self.__config_store.set_configurations(configs) + return configs except HttpRequestError as e: logger.error("Error retrieving assignment configurations: " + str(e)) if e.is_recoverable(): diff --git a/eppo_client/configuration_store.py b/eppo_client/configuration_store.py index 6cb81b5..fba0c96 100644 --- a/eppo_client/configuration_store.py +++ b/eppo_client/configuration_store.py @@ -8,7 +8,7 @@ class ConfigurationStore(Generic[T]): def __init__(self, ttl_seconds: float, max_size: int): - self.__cache = TTLCache(maxsize=max_size, ttl=ttl_seconds) + self.__cache: TTLCache = TTLCache(maxsize=max_size, ttl=ttl_seconds) self.__lock = ReadWriteLock() def get_configuration(self, key: str) -> Optional[T]: diff --git a/eppo_client/http_client.py b/eppo_client/http_client.py index a37a2b9..4f32b10 100644 --- a/eppo_client/http_client.py +++ b/eppo_client/http_client.py @@ -1,5 +1,4 @@ from typing import Any -from pydantic.dataclasses import dataclass from requests.adapters import HTTPAdapter, Retry from http import HTTPStatus @@ -31,7 +30,8 @@ def is_recoverable(self) -> bool: REQUEST_TIMEOUT_SECONDS = 2 # Retry reference: https://urllib3.readthedocs.io/en/latest/reference/urllib3.util.html#module-urllib3.util.retry -# This applies only to failed DNS lookups and connection timeouts, never to requests where data has made it to the server. +# This applies only to failed DNS lookups and connection timeouts, +# never to requests where data has made it to the server. MAX_RETRIES = Retry(total=3, backoff_factor=1) diff --git a/requirements.txt b/requirements.txt index 2c40773..4d3989d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,5 @@ pydantic requests cachetools +types-cachetools +types-requests diff --git a/test/client_test.py b/test/client_test.py index 0432755..d6974c6 100644 --- a/test/client_test.py +++ b/test/client_test.py @@ -1,7 +1,7 @@ import json import os from unittest.mock import patch -import httpretty +import httpretty # type: ignore import pytest from eppo_client.client import EppoClient from eppo_client.config import Config diff --git a/test/configuration_store_test.py b/test/configuration_store_test.py index fc1027e..4b32893 100644 --- a/test/configuration_store_test.py +++ b/test/configuration_store_test.py @@ -9,12 +9,14 @@ name="randomization_algo", ) -store = ConfigurationStore(ttl_seconds=100, max_size=1) +store: ConfigurationStore[ExperimentConfigurationDto] = ConfigurationStore( + ttl_seconds=100, max_size=1 +) store.set_configurations({"randomization_algo": test_exp}) def test_get_configuration_unknown_key(): - assert store.get_configuration("unknown_exp") == None + assert store.get_configuration("unknown_exp") is None def test_get_configuration_known_key(): From 0271e30f0523cd2e034ffb0f5fa1f20cf680fb44 Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Wed, 4 May 2022 10:01:26 -0700 Subject: [PATCH 3/7] adjust error handling of http client --- eppo_client/http_client.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/eppo_client/http_client.py b/eppo_client/http_client.py index 4f32b10..046aec5 100644 --- a/eppo_client/http_client.py +++ b/eppo_client/http_client.py @@ -1,4 +1,5 @@ from typing import Any +from requests.exceptions import Timeout from requests.adapters import HTTPAdapter, Retry from http import HTTPStatus @@ -16,8 +17,8 @@ class SdkParams(SdkBaseModel): class HttpRequestError(Exception): def __init__(self, message: str, status_code: int): - super().__init__(message) self.status_code = status_code + super().__init__(message) def is_recoverable(self) -> bool: if self.status_code >= 400 and self.status_code < 500: @@ -47,17 +48,21 @@ def is_unauthorized(self) -> bool: return self.__is_unauthorized def get(self, resource: str) -> Any: - response = self.__session.get( - self.__base_url + resource, - params=self.__sdk_params.dict(), - timeout=REQUEST_TIMEOUT_SECONDS, - ) - self.__is_unauthorized = response.status_code == HTTPStatus.UNAUTHORIZED - if response.status_code == HTTPStatus.OK.value: + try: + response = self.__session.get( + self.__base_url + resource, + params=self.__sdk_params.dict(), + timeout=REQUEST_TIMEOUT_SECONDS, + ) + self.__is_unauthorized = response.status_code == HTTPStatus.UNAUTHORIZED + if response.status_code != HTTPStatus.OK: + raise self._get_http_error(response.status_code, resource) return response.json() - raise HttpRequestError( - "HTTP {} error while requesting resource {}".format( - response.status_code, resource - ), - status_code=response.status_code, + except Timeout: + raise self._get_http_error(HTTPStatus.REQUEST_TIMEOUT, resource) + + def _get_http_error(self, status_code: int, resource: str) -> HttpRequestError: + return HttpRequestError( + "HTTP {} error while requesting resource {}".format(status_code, resource), + status_code=status_code, ) From e28862d305a3773bae3ec1685dc2a6a97cf7e1e3 Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Wed, 4 May 2022 11:28:52 -0700 Subject: [PATCH 4/7] include license in manifest.in --- MANIFEST.in | 3 ++- setup.cfg | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/MANIFEST.in b/MANIFEST.in index d27ff90..5d7f09f 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1,4 +1,5 @@ include requirements.txt include README.md include requirements-test.txt -include VERSION.txt \ No newline at end of file +include VERSION.txt +include LICENSE \ No newline at end of file diff --git a/setup.cfg b/setup.cfg index 833d48d..6b405c8 100644 --- a/setup.cfg +++ b/setup.cfg @@ -11,7 +11,7 @@ project_urls = Bug Tracker = https://github.com/Eppo-exp/python-sdk/issues classifiers = Programming Language :: Python :: 3 - License :: OSI Approved :: Apache Software License + License :: OSI Approved :: MIT License Operating System :: OS Independent [options] From 782bf56ec6a7aa17626ab9553cca71dd7bc3eddd Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Wed, 4 May 2022 13:31:03 -0700 Subject: [PATCH 5/7] load version from source code instead of file --- MANIFEST.in | 1 - VERSION.txt | 1 - eppo_client/__init__.py | 3 +-- setup.cfg | 14 +++++++------- 4 files changed, 8 insertions(+), 11 deletions(-) delete mode 100644 VERSION.txt diff --git a/MANIFEST.in b/MANIFEST.in index 5d7f09f..dd3d5f3 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -1,5 +1,4 @@ include requirements.txt include README.md include requirements-test.txt -include VERSION.txt include LICENSE \ No newline at end of file diff --git a/VERSION.txt b/VERSION.txt deleted file mode 100644 index 8a9ecc2..0000000 --- a/VERSION.txt +++ /dev/null @@ -1 +0,0 @@ -0.0.1 \ No newline at end of file diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index 2aba028..a36526e 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -10,8 +10,7 @@ from eppo_client.http_client import HttpClient, SdkParams from eppo_client.read_write_lock import ReadWriteLock -with open("VERSION.txt") as version_file: - __version__ = version_file.read().strip() +__version__ = "0.0.1" __client: Optional[EppoClient] = None __lock = ReadWriteLock() diff --git a/setup.cfg b/setup.cfg index 6b405c8..100a484 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,6 +1,6 @@ [metadata] name = eppo-server-sdk -version = file: VERSION.txt +version = attr: eppo_client.__version__ author = Eppo author_email = eppo-team@geteppo.com description = Eppo SDK for Python @@ -15,10 +15,10 @@ classifiers = Operating System :: OS Independent [options] -package_dir = - = eppo_client -packages = find: +packages = eppo_client python_requires = >=3.6 - -[options.packages.find] -where = eppo_client \ No newline at end of file +include_package_data=True +install_requires = + pydantic + requests + cachetools From 0d83c07986d865988175b837ec4748d4bd2f4922 Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Wed, 4 May 2022 15:18:11 -0700 Subject: [PATCH 6/7] fix polling bug --- eppo_client/__init__.py | 4 ++-- eppo_client/poller.py | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index a36526e..fccb0eb 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -59,9 +59,9 @@ def get_instance() -> EppoClient: :return: a shared client instance :rtype: EppoClient """ + global __client + global __lock try: - global __client - global __lock __lock.acquire_read() if __client: return __client diff --git a/eppo_client/poller.py b/eppo_client/poller.py index 88b5686..267ae6a 100644 --- a/eppo_client/poller.py +++ b/eppo_client/poller.py @@ -7,26 +7,31 @@ logger = logging.getLogger(__name__) -class Poller(Thread): +class Poller: def __init__(self, interval_millis: int, jitter_millis: int, callback: Callable): self.__jitter_millis = jitter_millis self.__interval = interval_millis - self.__callback = callback self.__stop_event = Event() - super().__init__(daemon=True) + self.__callback = callback + self.__thread = Thread(target=self.poll, daemon=True) + + def start(self): + self.__thread.start() def stop(self): self.__stop_event.set() - def run(self): - is_stopped = self.__stop_event.is_set() - while not is_stopped: + def is_stopped(self): + return self.__stop_event.is_set() + + def poll(self): + while not self.is_stopped(): try: self.__callback() except Exception as e: logger.error("Unexpected error running poll task: " + str(e)) break - is_stopped = self._wait_for_interval() + self._wait_for_interval() def _wait_for_interval(self): interval_with_jitter = self.__interval - randrange(0, self.__jitter_millis) From 91184311a418d8486cab0919aa15d63c5b4cc1ff Mon Sep 17 00:00:00 2001 From: Peter Loomis Date: Thu, 5 May 2022 15:23:43 -0700 Subject: [PATCH 7/7] use LRU cache instead of TTL cache --- eppo_client/__init__.py | 4 ++-- eppo_client/configuration_store.py | 6 +++--- eppo_client/constants.py | 1 - test/client_test.py | 2 ++ test/configuration_store_test.py | 20 ++++++++++++++++++-- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/eppo_client/__init__.py b/eppo_client/__init__.py index fccb0eb..5aced67 100644 --- a/eppo_client/__init__.py +++ b/eppo_client/__init__.py @@ -6,7 +6,7 @@ ExperimentConfigurationRequestor, ) from eppo_client.configuration_store import ConfigurationStore -from eppo_client.constants import CACHE_TTL_SECONDS, MAX_CACHE_ENTRIES +from eppo_client.constants import MAX_CACHE_ENTRIES from eppo_client.http_client import HttpClient, SdkParams from eppo_client.read_write_lock import ReadWriteLock @@ -32,7 +32,7 @@ def init(config: Config) -> EppoClient: ) http_client = HttpClient(base_url=config.base_url, sdk_params=sdk_params) config_store: ConfigurationStore[ExperimentConfigurationDto] = ConfigurationStore( - max_size=MAX_CACHE_ENTRIES, ttl_seconds=CACHE_TTL_SECONDS + max_size=MAX_CACHE_ENTRIES ) config_requestor = ExperimentConfigurationRequestor( http_client=http_client, config_store=config_store diff --git a/eppo_client/configuration_store.py b/eppo_client/configuration_store.py index fba0c96..f9c57f4 100644 --- a/eppo_client/configuration_store.py +++ b/eppo_client/configuration_store.py @@ -1,5 +1,5 @@ from typing import Dict, Optional, TypeVar, Generic -from cachetools import TTLCache +from cachetools import LRUCache from eppo_client.read_write_lock import ReadWriteLock @@ -7,8 +7,8 @@ class ConfigurationStore(Generic[T]): - def __init__(self, ttl_seconds: float, max_size: int): - self.__cache: TTLCache = TTLCache(maxsize=max_size, ttl=ttl_seconds) + def __init__(self, max_size: int): + self.__cache: LRUCache = LRUCache(maxsize=max_size) self.__lock = ReadWriteLock() def get_configuration(self, key: str) -> Optional[T]: diff --git a/eppo_client/constants.py b/eppo_client/constants.py index 6315a91..ed6806b 100644 --- a/eppo_client/constants.py +++ b/eppo_client/constants.py @@ -1,6 +1,5 @@ # configuration cache MAX_CACHE_ENTRIES = 1000 # arbitrary; the caching library requires a max limit -CACHE_TTL_SECONDS = 15 * 60 # poller SECOND_MILLIS = 1000 diff --git a/test/client_test.py b/test/client_test.py index d6974c6..a376732 100644 --- a/test/client_test.py +++ b/test/client_test.py @@ -1,5 +1,6 @@ import json import os +from time import sleep from unittest.mock import patch import httpretty # type: ignore import pytest @@ -42,6 +43,7 @@ def init_fixture(): body=config_response_json, ) client = init(Config(base_url=MOCK_BASE_URL, api_key="dummy")) + sleep(0.1) # wait for initialization yield client._shutdown() httpretty.disable() diff --git a/test/configuration_store_test.py b/test/configuration_store_test.py index 4b32893..7538856 100644 --- a/test/configuration_store_test.py +++ b/test/configuration_store_test.py @@ -9,15 +9,31 @@ name="randomization_algo", ) +TEST_MAX_SIZE = 10 + store: ConfigurationStore[ExperimentConfigurationDto] = ConfigurationStore( - ttl_seconds=100, max_size=1 + max_size=TEST_MAX_SIZE ) -store.set_configurations({"randomization_algo": test_exp}) def test_get_configuration_unknown_key(): + store.set_configurations({"randomization_algo": test_exp}) assert store.get_configuration("unknown_exp") is None def test_get_configuration_known_key(): + store.set_configurations({"randomization_algo": test_exp}) assert store.get_configuration("randomization_algo") == test_exp + + +def test_evicts_old_entries_when_max_size_exceeded(): + store.set_configurations({"item_to_be_evicted": test_exp}) + assert store.get_configuration("item_to_be_evicted") == test_exp + configs = {} + for i in range(0, TEST_MAX_SIZE): + configs["test-entry-{}".format(i)] = test_exp + store.set_configurations(configs) + assert store.get_configuration("item_to_be_evicted") is None + assert ( + store.get_configuration("test-entry-{}".format(TEST_MAX_SIZE - 1)) == test_exp + )