-
Notifications
You must be signed in to change notification settings - Fork 17
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
♻️ Make Process.run
async
#272
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
import traceback | ||
from enum import Enum | ||
from types import TracebackType | ||
from typing import TYPE_CHECKING, Any, Callable, Optional, Tuple, Type, Union, cast | ||
from typing import TYPE_CHECKING, Any, Awaitable, Callable, Optional, Tuple, Type, Union, cast | ||
|
||
import yaml | ||
from yaml.loader import Loader | ||
|
@@ -19,7 +19,7 @@ | |
from .base import state_machine | ||
from .lang import NULL | ||
from .persistence import auto_persist | ||
from .utils import SAVED_STATE_TYPE | ||
from .utils import SAVED_STATE_TYPE, ensure_coroutine | ||
|
||
__all__ = [ | ||
'Continue', | ||
|
@@ -195,10 +195,16 @@ class Running(State): | |
_running: bool = False | ||
_run_handle = None | ||
|
||
def __init__(self, process: 'Process', run_fn: Callable[..., Any], *args: Any, **kwargs: Any) -> None: | ||
def __init__( | ||
self, process: 'Process', run_fn: Callable[..., Union[Awaitable[Any], Any]], *args: Any, **kwargs: Any | ||
) -> None: | ||
super().__init__(process) | ||
assert run_fn is not None | ||
self.run_fn = run_fn | ||
self.run_fn = ensure_coroutine(run_fn) | ||
# We wrap `run_fn` to a coroutine so we can apply await on it, | ||
# even it if it was not a coroutine in the first place. | ||
# This allows the same usage of async and non-async function | ||
# with the await syntax while not changing the program logic. | ||
self.args = args | ||
self.kwargs = kwargs | ||
self._run_handle = None | ||
|
@@ -211,7 +217,7 @@ def save_instance_state(self, out_state: SAVED_STATE_TYPE, save_context: persist | |
|
||
def load_instance_state(self, saved_state: SAVED_STATE_TYPE, load_context: persistence.LoadSaveContext) -> None: | ||
super().load_instance_state(saved_state, load_context) | ||
self.run_fn = getattr(self.process, saved_state[self.RUN_FN]) | ||
self.run_fn = ensure_coroutine(getattr(self.process, saved_state[self.RUN_FN])) | ||
if self.COMMAND in saved_state: | ||
self._command = persistence.Savable.load(saved_state[self.COMMAND], load_context) # type: ignore | ||
|
||
|
@@ -225,7 +231,7 @@ async def execute(self) -> State: # type: ignore | |
try: | ||
try: | ||
self._running = True | ||
result = self.run_fn(*self.args, **self.kwargs) | ||
result = await self.run_fn(*self.args, **self.kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe the better way to do this is using https://docs.python.org/3/library/inspect.html#inspect.isawaitable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is mentioned in the blog post that @chrisjsewell mentioned in https://github.com/aiidateam/plumpy/pull/272/files#r1257558025 with
At least this is my understanding why we do this. (Also would add something like this to _run_task in class Process) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @agoscinski , I've added your comment. |
||
finally: | ||
self._running = False | ||
except Interruption: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am quite worried about this. I think this run_fn will become the
continue_fn
when it is recover from the Waiting state, which means all suchxx_fn
should be coroutines along the way. I need to take a close look to see how this change will make things different. Will do it next week.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisjsewell suggested to do
await_me_maybe
https://github.com/aiidateam/plumpy/pull/272/files#r1257558025 that would avoid this but I did not read about any technical reason do it. The blog post he links is only arguing for it cause of cleanness of the code. I assumed wrapping a blocking function is like writingasync
to the function, it then is just executed like a blocking function when used with await. SoThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, the blog post mentions the "maybe await" pattern is mostly for async framework that can support the downstream app can write block function. If the operation is block function, then it is run in block manner.
From aiida-core point of view, this never happened, since the
continue_fn
is never set in theaiida-core
Waiting class. Thedef run
is used to create the initialCreated
state and used to transfer the aiida Processinto its own
Waiting
state(s).