From 1d52cb2f404c592a15645862b7509bdc05b90dfe Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Tue, 15 Sep 2020 08:36:33 -0400 Subject: [PATCH 01/13] check for in_memory lock store instead of redis when setting number of Sanic workers --- rasa/core/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index 289de4900934..03ae5e8a2b00 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -460,7 +460,7 @@ def _lock_store_is_redis_lock_store( return False # `lock_store` is `None` or `EndpointConfig` - return lock_store is not None and lock_store.type == "redis" + return lock_store is not None and lock_store.type != "in_memory" def number_of_sanic_workers(lock_store: Union[EndpointConfig, LockStore, None]) -> int: From a29752f8b22726c65844538711ae45955fbe2abc Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Tue, 15 Sep 2020 08:38:07 -0400 Subject: [PATCH 02/13] update documentation for disallowing InMemoryLockStore for setting number of Sanic workers --- rasa/core/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index 03ae5e8a2b00..b3288efab88a 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -467,8 +467,7 @@ def number_of_sanic_workers(lock_store: Union[EndpointConfig, LockStore, None]) """Get the number of Sanic workers to use in `app.run()`. If the environment variable constants.ENV_SANIC_WORKERS is set and is not equal to - 1, that value will only be permitted if the used lock store supports shared - resources across multiple workers (e.g. ``RedisLockStore``). + 1, that value will only be permitted if the used lock store is not the InMemoryLockStore. """ def _log_and_get_default_number_of_workers(): From 5ee281cc366176c2358f076a6373993c92d53caa Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Tue, 15 Sep 2020 08:52:15 -0400 Subject: [PATCH 03/13] create 6629 bugfix changelog --- changelog/6629.bugfix.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/6629.bugfix.md diff --git a/changelog/6629.bugfix.md b/changelog/6629.bugfix.md new file mode 100644 index 000000000000..43bc0f8fafbb --- /dev/null +++ b/changelog/6629.bugfix.md @@ -0,0 +1 @@ +Fixed a bug when setting multiple Sanic workers. Previously, if the number was set higher than 1 and you were using a custom lock store, it would reject because of a strict check to use a Redis lock store. From de297f9e36f853989b469ae3127da22a7280de21 Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Mon, 28 Sep 2020 08:34:40 -0400 Subject: [PATCH 04/13] Update function name for check for lock store --- rasa/core/utils.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index b3288efab88a..0b1a906ca8dc 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -32,7 +32,7 @@ # backwards compatibility 1.0.x # noinspection PyUnresolvedReferences -from rasa.core.lock_store import LockStore, RedisLockStore +from rasa.core.lock_store import LockStore, RedisLockStore, InMemoryLockStore from rasa.utils.endpoints import EndpointConfig, read_endpoint_config from sanic import Sanic from sanic.views import CompositionView @@ -450,13 +450,10 @@ def replace_decimals_with_floats(obj: Any) -> Any: return json.loads(json.dumps(obj, cls=DecimalEncoder)) -def _lock_store_is_redis_lock_store( +def _lock_store_is_multi_worker_compatible( lock_store: Union[EndpointConfig, LockStore, None] ) -> bool: - if isinstance(lock_store, RedisLockStore): - return True - - if isinstance(lock_store, LockStore): + if isinstance(lock_store, InMemoryLockStore): return False # `lock_store` is `None` or `EndpointConfig` @@ -495,7 +492,7 @@ def _log_and_get_default_number_of_workers(): ) return _log_and_get_default_number_of_workers() - if _lock_store_is_redis_lock_store(lock_store): + if _lock_store_is_multi_worker_compatible(lock_store): logger.debug(f"Using {env_value} Sanic workers.") return env_value From a539dae61c4fb4b2afc081647ca703e32438c265 Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Mon, 12 Oct 2020 08:39:34 -0400 Subject: [PATCH 05/13] Update documentation for get_number_of_sanic_workers --- rasa/core/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index 0b1a906ca8dc..28cc9745f7db 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -456,6 +456,9 @@ def _lock_store_is_multi_worker_compatible( if isinstance(lock_store, InMemoryLockStore): return False + if isinstance(lock_store, RedisLockStore): + return True + # `lock_store` is `None` or `EndpointConfig` return lock_store is not None and lock_store.type != "in_memory" @@ -464,7 +467,7 @@ def number_of_sanic_workers(lock_store: Union[EndpointConfig, LockStore, None]) """Get the number of Sanic workers to use in `app.run()`. If the environment variable constants.ENV_SANIC_WORKERS is set and is not equal to - 1, that value will only be permitted if the used lock store is not the InMemoryLockStore. + 1, that value will only be permitted if the used lock store is not the `InMemoryLockStore`. """ def _log_and_get_default_number_of_workers(): From c2ab05d0d367987b43bf94c3409600e8b7e207aa Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Mon, 12 Oct 2020 09:02:45 -0400 Subject: [PATCH 06/13] Update tests for custom lock store Sanic workers --- tests/core/test_utils.py | 12 +++++++++--- tests/core/utilities.py | 5 +++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/tests/core/test_utils.py b/tests/core/test_utils.py index 3e9160739003..57f718f7f93d 100644 --- a/tests/core/test_utils.py +++ b/tests/core/test_utils.py @@ -13,6 +13,7 @@ from rasa.core.lock_store import LockStore, RedisLockStore, InMemoryLockStore from rasa.utils.endpoints import EndpointConfig from tests.conftest import write_endpoint_config_to_yaml +from tests.core.utilities import CustomRedisLockStore def test_is_int(): @@ -132,8 +133,10 @@ def test_replace_decimals_with_floats(_input: Any, expected: Any): (5, "in_memory", 1), (2, None, 1), (0, "in_memory", 1), + (3, "tests/core/utilities.CustomRedisLockStore", 3), (3, RedisLockStore(), 3), (2, InMemoryLockStore(), 1), + (3, CustomRedisLockStore(), 3), ], ) def test_get_number_of_sanic_workers( @@ -168,16 +171,19 @@ def test_get_number_of_sanic_workers( (EndpointConfig(type="redis"), True), (RedisLockStore(), True), (EndpointConfig(type="in_memory"), False), - (EndpointConfig(type="random_store"), False), + (EndpointConfig(type="custom_lock_store"), True), (None, False), (InMemoryLockStore(), False), + (CustomRedisLockStore(), True), ], ) -def test_lock_store_is_redis_lock_store( +def test_lock_store_is_multi_worker_compatible( lock_store: Union[EndpointConfig, LockStore, None], expected: bool ): # noinspection PyProtectedMember - assert rasa.core.utils._lock_store_is_redis_lock_store(lock_store) == expected + assert ( + rasa.core.utils._lock_store_is_multi_worker_compatible(lock_store) == expected + ) def test_read_endpoints_from_path(tmp_path: Path): diff --git a/tests/core/utilities.py b/tests/core/utilities.py index cfc17fdea681..0e6daccaae8f 100644 --- a/tests/core/utilities.py +++ b/tests/core/utilities.py @@ -9,6 +9,7 @@ import rasa.shared.utils.io import rasa.utils.io +from rasa.core.lock_store import RedisLockStore from rasa.shared.core.domain import Domain from rasa.shared.core.events import UserUttered, Event from rasa.shared.core.trackers import DialogueStateTracker @@ -85,3 +86,7 @@ def user_uttered( def get_tracker(events: List[Event]) -> DialogueStateTracker: return DialogueStateTracker.from_events("sender", events, [], 20) + + +class CustomRedisLockStore(RedisLockStore): + pass From 609450b58e7b88e16d521bc661c5a1681cb4f6ba Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Thu, 15 Oct 2020 09:00:16 -0400 Subject: [PATCH 07/13] Check that lock store is instance of EndpointConfig before checking type --- rasa/core/utils.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index 28cc9745f7db..311560144617 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -460,7 +460,11 @@ def _lock_store_is_multi_worker_compatible( return True # `lock_store` is `None` or `EndpointConfig` - return lock_store is not None and lock_store.type != "in_memory" + return ( + lock_store is not None + and isinstance(lock_store, EndpointConfig) + and lock_store.type != "in_memory" + ) def number_of_sanic_workers(lock_store: Union[EndpointConfig, LockStore, None]) -> int: From 855009d44c7624dda7aec8fc8d3c8b10d66bb0d5 Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Thu, 15 Oct 2020 09:08:04 -0400 Subject: [PATCH 08/13] Update debug message for setting number of Sanic workers --- rasa/core/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index 311560144617..2132ca4b417a 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -505,6 +505,6 @@ def _log_and_get_default_number_of_workers(): logger.debug( f"Unable to assign desired number of Sanic workers ({env_value}) as " - f"no `RedisLockStore` endpoint configuration has been found." + f"no `RedisLockStore` or custom `LockStore` endpoint configuration has been found." ) return _log_and_get_default_number_of_workers() From 3bd5fffa297135d470d3db7538f0fda60b738765 Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Thu, 22 Oct 2020 08:40:19 -0400 Subject: [PATCH 09/13] update copy for changelog --- changelog/6629.bugfix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/6629.bugfix.md b/changelog/6629.bugfix.md index 43bc0f8fafbb..43063d142356 100644 --- a/changelog/6629.bugfix.md +++ b/changelog/6629.bugfix.md @@ -1 +1 @@ -Fixed a bug when setting multiple Sanic workers. Previously, if the number was set higher than 1 and you were using a custom lock store, it would reject because of a strict check to use a Redis lock store. +Fixed a bug that occurred when setting multiple Sanic workers in combination with a custom [Lock Store](lock-stores.mdx). Previously, if the number was set higher than 1 and you were using a custom lock store, it would reject because of a strict check to use a [Redis Lock Store](lock-stores.mdx#redislockstore). From bbc13cfe57d99098972e46a47be423bde5248bcc Mon Sep 17 00:00:00 2001 From: Joe Riess Date: Thu, 22 Oct 2020 08:41:17 -0400 Subject: [PATCH 10/13] move fixture class into test file --- tests/core/test_utils.py | 7 +++++-- tests/core/utilities.py | 4 ---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/core/test_utils.py b/tests/core/test_utils.py index 57f718f7f93d..14195c412615 100644 --- a/tests/core/test_utils.py +++ b/tests/core/test_utils.py @@ -13,7 +13,10 @@ from rasa.core.lock_store import LockStore, RedisLockStore, InMemoryLockStore from rasa.utils.endpoints import EndpointConfig from tests.conftest import write_endpoint_config_to_yaml -from tests.core.utilities import CustomRedisLockStore + + +class CustomRedisLockStore(RedisLockStore): + pass def test_is_int(): @@ -133,7 +136,7 @@ def test_replace_decimals_with_floats(_input: Any, expected: Any): (5, "in_memory", 1), (2, None, 1), (0, "in_memory", 1), - (3, "tests/core/utilities.CustomRedisLockStore", 3), + (3, "tests/core/test_utils.CustomRedisLockStore", 3), (3, RedisLockStore(), 3), (2, InMemoryLockStore(), 1), (3, CustomRedisLockStore(), 3), diff --git a/tests/core/utilities.py b/tests/core/utilities.py index 0e6daccaae8f..15566d5ad09b 100644 --- a/tests/core/utilities.py +++ b/tests/core/utilities.py @@ -86,7 +86,3 @@ def user_uttered( def get_tracker(events: List[Event]) -> DialogueStateTracker: return DialogueStateTracker.from_events("sender", events, [], 20) - - -class CustomRedisLockStore(RedisLockStore): - pass From cd69e98d61ad32fae44a16bd1107c6a7686e37b8 Mon Sep 17 00:00:00 2001 From: ricwo <20581300+ricwo@users.noreply.github.com> Date: Thu, 22 Oct 2020 15:00:51 +0200 Subject: [PATCH 11/13] Apply suggestions from code review --- tests/core/test_utils.py | 2 +- tests/core/utilities.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/core/test_utils.py b/tests/core/test_utils.py index 14195c412615..e21a60e41ea3 100644 --- a/tests/core/test_utils.py +++ b/tests/core/test_utils.py @@ -16,7 +16,7 @@ class CustomRedisLockStore(RedisLockStore): - pass + """Test class used to test the behavior of custom lock stores.""" def test_is_int(): diff --git a/tests/core/utilities.py b/tests/core/utilities.py index 15566d5ad09b..cfc17fdea681 100644 --- a/tests/core/utilities.py +++ b/tests/core/utilities.py @@ -9,7 +9,6 @@ import rasa.shared.utils.io import rasa.utils.io -from rasa.core.lock_store import RedisLockStore from rasa.shared.core.domain import Domain from rasa.shared.core.events import UserUttered, Event from rasa.shared.core.trackers import DialogueStateTracker From 28f75e034eb58210dc929910097e501ea236d524 Mon Sep 17 00:00:00 2001 From: ricwo <20581300+ricwo@users.noreply.github.com> Date: Thu, 22 Oct 2020 16:36:22 +0200 Subject: [PATCH 12/13] Update rasa/core/utils.py --- rasa/core/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index 2132ca4b417a..fedb4880f353 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -471,7 +471,8 @@ def number_of_sanic_workers(lock_store: Union[EndpointConfig, LockStore, None]) """Get the number of Sanic workers to use in `app.run()`. If the environment variable constants.ENV_SANIC_WORKERS is set and is not equal to - 1, that value will only be permitted if the used lock store is not the `InMemoryLockStore`. + 1, that value will only be permitted if the used lock store is not the + `InMemoryLockStore`. """ def _log_and_get_default_number_of_workers(): From 1c224ef92f7e86e5bfb54c00583f04eb33e42edc Mon Sep 17 00:00:00 2001 From: ricwo <20581300+ricwo@users.noreply.github.com> Date: Thu, 22 Oct 2020 17:33:55 +0200 Subject: [PATCH 13/13] Update rasa/core/utils.py --- rasa/core/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rasa/core/utils.py b/rasa/core/utils.py index fedb4880f353..ad7443925bc3 100644 --- a/rasa/core/utils.py +++ b/rasa/core/utils.py @@ -471,7 +471,7 @@ def number_of_sanic_workers(lock_store: Union[EndpointConfig, LockStore, None]) """Get the number of Sanic workers to use in `app.run()`. If the environment variable constants.ENV_SANIC_WORKERS is set and is not equal to - 1, that value will only be permitted if the used lock store is not the + 1, that value will only be permitted if the used lock store is not the `InMemoryLockStore`. """