[FSTORE-1382] fix for async concurrency issues for sql client #1343

dhananjay-mk · 2024-06-07T09:45:32Z

This PR adds/fixes/changes...

please summarize your changes to the code
and make sure to include all changes to user-facing APIs

JIRA Issue: -

Priority for Review: -

Related PRs: -

How Has This Been Tested?

Unit Tests
Integration Tests
Manual Tests on VM

Checklist For The Assigned Reviewer:

- [ ] Checked if merge conflicts with master exist
- [ ] Checked if stylechecks for Java and Python pass
- [ ] Checked if all docstrings were added and/or updated appropriately
- [ ] Ran spellcheck on docstring
- [ ] Checked if guides & concepts need to be updated
- [ ] Checked if naming conventions for parameters and variables were followed
- [ ] Checked if private methods are properly declared and used
- [ ] Checked if hard-to-understand areas of code are commented
- [ ] Checked if tests are effective
- [ ] Built and deployed changes on dev VM and tested manually
- [x] (Checked if all type annotations were added and/or updated appropriately)

vatj · 2024-06-07T10:52:16Z

python/hsfs/__init__.py

@@ -59,6 +57,3 @@ def get_sdk_info():


 __all__ = ["connection", "disable_usage_logging", "get_sdk_info"]
-# running async code in jupyter throws "RuntimeError: This event loop is already running"
-# with tornado 6. This fixes the issue without downgrade to tornado==4.5.3
-nest_asyncio.apply()


Are we sure that we want to stop providing support for older version of tornado? Or is it enough to move it to the sql engine?

After digging deeper, I realised some of the problems were caused by nest_asyncio as it makes the actual async errors difficult to debug. So I tried to remove its dependency, but its still needed in case of jupyter, otherwise the user has to explicitly call nest_asyncio.apply() in each notebook. It is not needed for python runtimes. So it is needed for below cases:

jupyter -> added check in init_async methods

locust -> added into the locustfile

python/hsfs/core/online_store_sql_engine.py

vatj · 2024-06-07T10:55:11Z

python/hsfs/core/online_store_sql_engine.py

@@ -205,18 +209,30 @@ def _parametrize_prepared_statements(
        return prepared_statements_dict

    def init_async_mysql_connection(self, options=None):
+        def is_runtime_notebook():


Why is this a local function?

moved to the utils class

vatj · 2024-06-07T11:03:51Z

python/hsfs/core/online_store_sql_engine.py

 from typing import Any, Dict, List, Optional, Set, Tuple, Union

+import nest_asyncio


Can you make this an optional import by importing only if is_runtime_notebook?

python/hsfs/core/online_store_sql_engine.py

python/hsfs/util.py

kennethmhc · 2024-06-07T11:44:01Z

python/hsfs/core/online_store_sql_engine.py

-        )
-        self._async_pool = await util.create_async_engine(
-            online_connector, self._external, default_min_size, options=options
+    async def _get_connection_pool(self, default_min_size: int, loop=None) -> None:


loop is not used by any caller

Loop argument is not strictly required as the engine calls its asyncio built-in method, but good to keep it as optional

I am not sure I understand why we would not always pass loop?

kennethmhc · 2024-06-07T11:45:13Z

python/hsfs/util.py

+            loop = asyncio.get_running_loop()
+    except RuntimeError as er:
+        raise RuntimeError(
+            "Event loop is not running. Please provide an event loop to create the engine."


how can user provide the loop?

it gets the loop by the asyncio.get_running_loop so there should be running loop first to call this co-routine. I modified the error message a bit to make it clear. Loop argument is not strictly required but good to keep it as optional

There is no loop argument in init_serving or get_feature_vector

removed loop argument to avoid confusion @kennethmhc

vatj

I would like to understand the optional loop use case. It feels like always having a per serving FV loop is the way to go and keeping it optional is error-prone/not clear to me

vatj · 2024-06-11T07:16:10Z

python/hsfs/core/online_store_sql_engine.py

@@ -698,3 +726,11 @@ def feature_view_api(self) -> feature_view_api.FeatureViewApi:
    @property
    def storage_connector_api(self) -> storage_connector_api.StorageConnectorApi:
        return self._storage_connector_api
+


What about online_connector and connection_pool?

vatj · 2024-06-11T07:18:31Z

python/hsfs/core/online_store_sql_engine.py

-        )
-        self._async_pool = await util.create_async_engine(
-            online_connector, self._external, default_min_size, options=options
+    async def _get_connection_pool(self, default_min_size: int, loop=None) -> None:


I am not sure I understand why we would not always pass loop?

vatj · 2024-06-11T07:19:36Z

python/hsfs/core/online_store_sql_engine.py

+        self._hostname = util.get_host_name() if self._external else None
+
+        if util.is_runtime_notebook():
+            _logger.debug("Running in Jupyter notebook, applying nest_asyncio")


Should this be a warning rather than debug?

I think debug is fine as it is not really something user needs to do

…lclocks#1343) * init-working * add lock changes * move lock to execute method * update lock * pass pool as arg * loop changes and refactoring * make new connection pool for each call * make exec_prep async and add lock * use context manager and refactor init connection * use create task * add semaphore * remove nest_async and use manual loop * make nest asyncio only if jupyter * refactoring and move hostname retrieving to init serving * minor clean * minor cleanup * add unit test * remove return of pool * revert locust changes and refactoring * revert locust changes and refactoring * fix review comments * fix review comments * remove loop method argument --------- Co-authored-by: Victor Jouffrey <[email protected]>

…#1372) * init-working * add lock changes * move lock to execute method * update lock * pass pool as arg * loop changes and refactoring * make new connection pool for each call * make exec_prep async and add lock * use context manager and refactor init connection * use create task * add semaphore * remove nest_async and use manual loop * make nest asyncio only if jupyter * refactoring and move hostname retrieving to init serving * minor clean * minor cleanup * add unit test * remove return of pool * revert locust changes and refactoring * revert locust changes and refactoring * fix review comments * fix review comments * remove loop method argument --------- Co-authored-by: Victor Jouffrey <[email protected]>

dhananjay-mk added 22 commits May 6, 2024 11:53

init-working

0dce95e

add lock changes

6cca141

move lock to execute method

f1afeed

update lock

2e693b3

pass pool as arg

6e78f74

loop changes and refactoring

1cdecee

make new connection pool for each call

d3dd74b

make exec_prep async and add lock

7ccd9ae

use context manager and refactor init connection

d5e6454

use create task

288c1bb

add semaphore

c6c71be

remove nest_async and use manual loop

8f27622

make nest asyncio only if jupyter

dc123a5

merge master

ed23f2d

refactoring and move hostname retrieving to init serving

3f3c64b

minor clean

cb2cbdf

minor cleanup

00e3d2a

add unit test

0089759

remove return of pool

1f3c28c

merge upstream

f1701eb

revert locust changes and refactoring

d4798dc

revert locust changes and refactoring

f8a1f32

dhananjay-mk requested review from kennethmhc and vatj June 7, 2024 09:45

vatj requested changes Jun 7, 2024

View reviewed changes

kennethmhc reviewed Jun 7, 2024

View reviewed changes

dhananjay-mk added 2 commits June 7, 2024 15:12

fix review comments

a9cd199

fix review comments

83c9a46

vatj approved these changes Jun 11, 2024

View reviewed changes

remove loop method argument

ee39c1c

vatj and others added 2 commits June 11, 2024 15:36

Merge branch 'master' into fstore-1382

a7b3c61

Merge remote-tracking branch 'upstream/master' into fstore-1382

800b04a

dhananjay-mk marked this pull request as draft June 11, 2024 15:21

dhananjay-mk marked this pull request as ready for review June 12, 2024 08:16

Merge branch 'master' into fstore-1382

1b675e7

dhananjay-mk merged commit 02291fb into logicalclocks:master Jun 19, 2024
12 checks passed

		from typing import Any, Dict, List, Optional, Set, Tuple, Union

		import nest_asyncio

[FSTORE-1382] fix for async concurrency issues for sql client #1343

[FSTORE-1382] fix for async concurrency issues for sql client #1343

Uh oh!

Conversation

dhananjay-mk commented Jun 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vatj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!