Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #99: "RuntimeError: This event loop is already running" in colab and notebook #100

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions python/_restclient/hydrotools/_restclient/_restclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,11 @@ def headers(self) -> dict:

def close(self) -> None:
""" Release aiohttp.ClientSession """
# Session never instantiated, thus cannot be closed
session = getattr(self, "_session", None)
if session is None:
return

if not self._session.closed:
if not self._loop.is_closed():
self._add_to_loop(self._session.close())
Expand Down
19 changes: 18 additions & 1 deletion python/_restclient/hydrotools/_restclient/async_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,24 @@ def __init__(

def _add_to_loop(self, coro: Coroutine):
""" Add coro to event loop via run_until_complete """
return self._loop.run_until_complete(coro)
try:
return self._loop.run_until_complete(coro)

except RuntimeError as e:
try:
# `RuntimeError: This event loop is already running` thrown by jupyter notebook
# See hydrotools #99 for context and notebook #3397 for detail
import nest_asyncio

nest_asyncio.apply()

return self._loop.run_until_complete(coro)
except ModuleNotFoundError:
error_message = (
"nest_asycnio package not found. Install using `pip install nest_asycnio`.\n"
"See https://github.com/NOAA-OWP/hydrotools/issues/99 for more detail."
)
raise ModuleNotFoundError(error_message) from e

def _wrap_func_in_coro(self, func: Callable, *args, **kwargs):
""" Create partial func; wrap and call partial in coro; return coro """
Expand Down
2 changes: 1 addition & 1 deletion python/_restclient/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
SUBPACKAGE_SLUG = f"{NAMESPACE_PACKAGE_NAME}.{SUBPACKAGE_NAME}"

# Subpackage version
VERSION = "3.0.1"
VERSION = "3.0.2"

# Package author information
AUTHOR = "Austin Raney"
Expand Down
20 changes: 20 additions & 0 deletions python/_restclient/tests/test_restclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,3 +150,23 @@ def test_build_url(loop):

assert client.build_url(base_url) == base_url
assert client.build_url(base_url, query_params) == f"{base_url}?key=value"


def test_restclient_nest_asyncio_ModuleNotFoundError(loop):
"""Test for #99"""
import asyncio
import warnings

async def test():
await asyncio.sleep(0.01)

with warnings.catch_warnings():
# ignore coroutine not awaited warning for output sake.
# This is not the purpose of this test
warnings.simplefilter("ignore", category=RuntimeWarning)
with pytest.raises(ModuleNotFoundError):
# implicitly verify that `nest_asyncio` is not installed
# this test will need to change if `nest_asyncio` becomes a requirement
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to elaborate in this comment a bit more concerning the implications of nest_asyncio becoming a requirement? I assume this test will fail is it's already installed. Would that require us to remove this test or write a new test? If nest_asyncio became a requirement, would that remove the need for the test? If so, why not just make it a requirement now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are correct -- if nest_asyncio is already installed that test will fail. My thinking was to treat this dependency differently since it isn't typically required. I agree with you about the fragility of this test, I need to rethink how to test this bug fix.

RestClient(enable_cache=False)

loop.run_until_complete(test())