chore(api, robot-server): Manage runners via run orchestrator #15190
Conversation
| @@ -126,32 +120,32 @@ def __init__( | |||
| self._robot_type = robot_type | |||
| self._deck_type = deck_type | |||
| self._default_engine: Optional[ProtocolEngine] = None | |||
| self._runner_engine_pair: Optional[RunnerEnginePair] = None | |||
| hardware_api.register_callback(_get_estop_listener(self)) | |||
|
|
|||
| @property | |||
| def engine(self) -> ProtocolEngine: | |||
There was a problem hiding this comment.
wanted to change this to a get method instead of a property now that its a property of the orchestrator but bc its a store it dosent really match?
There was a problem hiding this comment.
I think it’s fine to keep it as a property since in theory this will go away in favor of something that returns the orchestrator
| @property | ||
| def runner(self) -> AnyRunner: | ||
| """Get the "current" persisted ProtocolRunner.""" | ||
| return self._protocol_runner or self._setup_runner |
There was a problem hiding this comment.
Will this property's return value need to change intelligently between runners depending on the run state? Like, it returns self._setup_runner while the run is in the setup phase, and self._fixit_runner while error recovery is active, and self._protocol_runner otherwise? Or am I misunderstanding the plan?
There was a problem hiding this comment.
thats not a bad idea. I was thinking about this more like:
if there is a protocol runner always return that, if there is no protocol runner then fixit_runner is not active and we are only able to use setup runner but! this property will probably vanish bc we are going to remove access directly with the runner and let the orchestrator handle the logic. let me know if this makes sense.
There was a problem hiding this comment.
Yeah I think this property goes away forever and the orchestrator itself offers semantically relevant api, like a method called run_setup_command and a method called run_fixit_command
TamarZanzouri
changed the title
TamarZanzouri
force-pushed
the
EXEC-418-run-orchestrator-builder
branch
from
601e5ec
to
b63f97c
Compare
| @property | ||
| def runner(self) -> AnyRunner: | ||
| """Get the "current" persisted ProtocolRunner.""" | ||
| return self._protocol_runner or self._setup_runner |
There was a problem hiding this comment.
Yeah I think this property goes away forever and the orchestrator itself offers semantically relevant api, like a method called run_setup_command and a method called run_fixit_command
| @@ -126,32 +120,32 @@ def __init__( | |||
| self._robot_type = robot_type | |||
| self._deck_type = deck_type | |||
| self._default_engine: Optional[ProtocolEngine] = None | |||
| self._runner_engine_pair: Optional[RunnerEnginePair] = None | |||
| hardware_api.register_callback(_get_estop_listener(self)) | |||
|
|
|||
| @property | |||
| def engine(self) -> ProtocolEngine: | |||
There was a problem hiding this comment.
I think it’s fine to keep it as a property since in theory this will go away in favor of something that returns the orchestrator
| else None | ||
| ) | ||
|
|
||
| # TODO(tz, 2024-5-14): probably dont need this once its all redirected via orchestrator |
There was a problem hiding this comment.
Definitely will - there will need to be a different default_orchestrator that has a different engine and probably only setup and fixit runners
TamarZanzouri
force-pushed
the
EXEC-418-run-orchestrator-builder
branch
from
2f18907
to
91c8a13
Compare
Overview
part of https://opentrons.atlassian.net/browse/EXEC-418.
create RunOrchestrator provider to manage, engine->runners scope.
this tickets is just to introduce the term and redirect calls via the orchestrator.
Test Plan
Changelog
Review requests
do we feel good about these changes?
Risk assessment
low. redirecting calls via orchestrator but structure stayed the same.