sync rh to all workers in addition to head node #278
Conversation
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
jlewitt1
force-pushed
the
sync-rh-to-all-nodes
branch
4 times, most recently
from
50fd384
to
4542418
Compare
|
|
||
| # Update the cluster config on the cluster | ||
| self.save_config_to_cluster() | ||
| with ProcessPoolExecutor() as executor: |
There was a problem hiding this comment.
@dongreenberg you think this is overkill?
jlewitt1
force-pushed
the
sync-rh-to-all-nodes
branch
2 times, most recently
from
3b15c58
to
13254f1
Compare
jlewitt1
force-pushed
the
sync-rh-to-all-nodes
branch
from
13254f1
to
e2b587f
Compare
| @@ -842,6 +858,25 @@ def disconnect(self): | |||
| if self._rpc_tunnel: | |||
| self._rpc_tunnel.stop() | |||
|
|
|||
| def _sync_across_all_nodes(self, _rh_install_url): | |||
| loop = asyncio.new_event_loop() | |||
| executor = ProcessPoolExecutor() | |||
There was a problem hiding this comment.
Do we need to also call: asyncio.set_event_loop(loop)?
According to the documentation:
If there’s need to set this loop as the event loop for the current context, [set_event_loop()](https://python.readthedocs.io/en/latest/library/asyncio-eventloops.html#asyncio.set_event_loop) must be called explicitly.
If the existing code works, we can probably skip it for now, but it would be good to be aware of it in case of any exceptions or weird behavior that we may face in the future.
There was a problem hiding this comment.
good point, my understanding is that since this loop's usage is confined to this function we don't necessarily need to set it, but if we do then we should also close the loop as you added in your other comment.
Seems like it's the recommended way to do it like this, so I'll update it!
There was a problem hiding this comment.
Looks great now, thanks for updating!
| @@ -703,10 +717,12 @@ def restart_server( | |||
| logger.info(f"Restarting Runhouse API server on {self.name}.") | |||
|
|
|||
| if resync_rh: | |||
| self._sync_runhouse_to_cluster(_install_url=_rh_install_url) | |||
| if not self.address: | |||
There was a problem hiding this comment.
Should we add a comment on what is the logical condition here e.g. in what cases would self.address be None?
| @@ -994,13 +1033,23 @@ def run( | |||
| if not run_name: | |||
| # If not creating a Run then just run the commands via SSH and return | |||
| return self._run_commands_with_ssh( | |||
| commands, cmd_prefix, stream_logs, port_forward, require_outputs | |||
| commands, | |||
There was a problem hiding this comment.
nit: should node be first?
There was a problem hiding this comment.
made it optional since we can always default to using the head node's address, do you think that API should be more explicit?
There was a problem hiding this comment.
Optional sounds great to me, thanks
jlewitt1
force-pushed
the
sync-rh-to-all-nodes
branch
3 times, most recently
from
543f642
to
04ec710
Compare
jlewitt1
force-pushed
the
sync-rh-to-all-nodes
branch
from
04ec710
to
b80308b
Compare
No description provided.