add delayed container start #131
Conversation
This is the case for containers created by a MirrorContainerProcessManager
This was a weird bug which saved the state at the time of the delayed start. Instead we need to serializer the agent_creator function at the creation of the agent process in create_agent_process
maurerle
force-pushed
the
add_mp_tests
branch
from
7fd0562 to
5efe0c3
Compare
maurerle
force-pushed
the
add_mp_tests
branch
2 times, most recently
from
ac4f8bb to
2925aa0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #131 +/- ##
===============================================
+ Coverage 88.65% 88.92% +0.27%
===============================================
Files 22 22
Lines 2407 2448 +41
===============================================
+ Hits 2134 2177 +43
+ Misses 273 271 -2 ☔ View full report in Codecov by Sentry. |
adds a test which checks this for a multiprocessing container
maurerle
force-pushed
the
add_mp_tests
branch
from
2925aa0 to
9cf064a
Compare
mango/container/mp.py
Outdated
| @@ -444,6 +449,21 @@ def _find_sp_queue(self, aid): | |||
| raise ValueError(f"The aid '{aid}' does not exist in any subprocess.") | |||
|
|
|||
| def create_agent_process(self, agent_creator, container, mirror_container_creator): | |||
There was a problem hiding this comment.
Regarding the design, it makes sense that delayed starting agent processes is an additional feature. I do not feel the prior approach was bad. Being able to start the process at will before starting the container makes sense as it is closer to the normal agent lifecycle. So I would suggest to introduce a new method for delayed agent process start (or a flag?). This also would prevent breaking the API.
mango/container/mp.py
Outdated
| ) in self._agent_process_init_list: | ||
| await self.create_internal_agent_process( | ||
| agent_creator, container, mirror_container_creator | ||
| ) |
There was a problem hiding this comment.
should be using asyncio.gather instead to decrease runtime
There was a problem hiding this comment.
gather did not work somehow, as the ProcessHandle is unhashable..
Using a poor men's version of gather now instead
maurerle
force-pushed
the
add_mp_tests
branch
from
9492e09 to
d32bc40
Compare
maurerle
force-pushed
the
add_mp_tests
branch
from
d32bc40 to
a080e76
Compare
|
It probably helps to only review the changes since yesterday's review: |
This solves having sync creation of
as_agent_process.fixes #129
This is based on top of the described wanted behavior in: maurerle@eaccade
Dill has to be dumped on the creation of the mirror agent function and not later.
Otherwise this leads to very unexpected behavior (storing the state of the variables at the time of dumping, which is then the same for all delayed agent_creators, leading to duplicate agent ids)
Furthermore, it was missing to call
on_readyin mirror containers, which is fixed as well through this PR