Remove asyncio.run_in_executor() calls and make scheduler cancel() async #9664
Conversation
jonathan-eq
added
the
release-notes:bug-fix
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9664 +/- ##
==========================================
- Coverage 91.73% 91.72% -0.01%
==========================================
Files 426 426
Lines 26518 26526 +8
==========================================
+ Hits 24326 24331 +5
- Misses 2192 2195 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #9664 will not alter performanceComparing Summary
|
|
I was unable to reproduce this with asyncio, but it was possible when using lower level ThreadPoolExecutor. https://superfastpython.com/threadpoolexecutor-shutdown/ |
src/ert/run_models/base_run_model.py
Outdated
| event, | ||
| iteration, | ||
| ) | ||
| if asyncio.get_running_loop().is_running(): |
There was a problem hiding this comment.
Hm, this I think is redundant as this should be always true as this runs inside asyncio.run(...)
I would recommend instead:
loop = asyncio.get_running_loop()
if loop.is_closed():
self.send_snapshot_event(event, iteration)
else:
await asyncio.get_running_loop().run_in_executor(
None,
self.send_snapshot_event,
event,
iteration,
)
|
Maybe it helps if this evaluator function will become async: Currently we call it directly from async context. |
So I think we need to reproduce before anything else, as it is, I am not sure whether this is worse or better. |
jonathan-eq
force-pushed
the
fix-bugs2
branch
2 times, most recently
from
c330a34 to
86787bd
Compare
| @@ -333,8 +333,7 @@ def _signal_cancel(self) -> None: | |||
| """ | |||
| if self._ensemble.cancellable: | |||
| logger.debug("Cancelling current ensemble") | |||
| assert self._loop is not None | |||
| self._loop.run_in_executor(None, self._ensemble.cancel) | |||
There was a problem hiding this comment.
Can we remove self._loop attribute as well?
| iteration, | ||
| ) | ||
|
|
||
| self.send_snapshot_event(event, iteration) |
There was a problem hiding this comment.
Let's test this first on bigpoly with 400 realizations
There was a problem hiding this comment.
Tested with 200 realizations, and there was no difference. I think it should be sufficient
There was a problem hiding this comment.
Tested with 400 realization, and it was the same
jonathan-eq
changed the title
This commit makes the methods async, so we can await them instead fire-and-forgetting them through asyncio.run_coroutine_threadsafe.
jonathan-eq
force-pushed
the
fix-bugs2
branch
from
05f60cd to
4c5af60
Compare
jonathan-eq
changed the title
Issue
Resolves #9307
Approach
The commit in this PR removes the asyncio.run_in_executor() calls, and runs the methods directly instead.
It also makes the signal_cancel, and cancel methods async/await instead of asyncio.run_coroutine_threadsafe().
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')When applicable