-
Notifications
You must be signed in to change notification settings - Fork 258
Adapt to plumpy's greenback async bridge #7206
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
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 |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| import traceback | ||
| from typing import TYPE_CHECKING, AsyncIterator, Awaitable, Dict, Hashable, Optional | ||
|
|
||
| from plumpy import get_or_create_event_loop | ||
|
|
||
| from aiida.orm import AuthInfo | ||
|
|
||
| if TYPE_CHECKING: | ||
|
|
@@ -44,8 +46,8 @@ class TransportQueue: | |
| """ | ||
|
|
||
| def __init__(self, loop: Optional[asyncio.AbstractEventLoop] = None): | ||
| """:param loop: An asyncio event, will use `asyncio.get_event_loop()` if not supplied""" | ||
| self._loop = loop if loop is not None else asyncio.get_event_loop() | ||
| """:param loop: An asyncio event, will use `get_or_create_event_loop()` if not supplied""" | ||
| self._loop = loop if loop else get_or_create_event_loop() | ||
| self._transport_requests: Dict[Hashable, TransportRequest] = {} | ||
|
|
||
| @property | ||
|
|
@@ -67,6 +69,15 @@ async def transport_task(transport_queue, authinfo): | |
| :param authinfo: The authinfo to be used to get transport | ||
| :return: A future that can be yielded to give the transport | ||
| """ | ||
|
|
||
| from plumpy import ensure_portal | ||
|
|
||
| # NOTE: We need to ensure the portal here only because | ||
| # our scheduler has only a sync interface and _get_jobs_from_scheduler is using that | ||
| # if we ever provide a fully async scheduler interface then we can remove this here | ||
|
Collaborator
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. I don't fully understand. We need to call ensure_portal, when we switch to a different task than the task in execute (because this one has an open portal), and when we require a nested sync->async call. So in scheduler we have such a case? But why do we need to open it in transport then? Aren't the two classes decoupled?
Collaborator
Author
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. So when a Once scheduler interface is async, as well we can get rid of the |
||
| # An issue is opened to reference this https://github.com/aiidateam/aiida-core/issues/7222 | ||
| await ensure_portal() | ||
|
|
||
| open_callback_handle = None | ||
| transport_request = self._transport_requests.get(authinfo.pk, None) | ||
|
|
||
|
|
||
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.
@khsrali is this a new requirement or has this been the case even before?
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.
It's new..
We could really not find a better way to do this.
The thing is notebooks have their own running event loop. Since nest_asyncio is dropped, the only way for us to use their loop (since you can have only one running event loop in) is to open a greenback portal.
And that has to be called when the loop has started but before engine calls.
The most practical place to stuff this logic in, was in load_profile.
However, there's a technical issue with that: the greenback portals are only usable when you are back in a the async context. Basically that means either we had to changes it to something like
await load_profile_async()--which defies the efforts of aiida to not expose async syntax to users-- Or to register that call on each cell execution. After many brainstorming we decided to go with the second solution.The interface remains the same
load_profile()but greenback portals become useful from the next execution cell. A minimum "backward incompatible" price that we'll had to pay