Skip to content
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

fix(app,robot-server): Account for failed commands not having a pipetteId #16859

Merged
merged 10 commits into from
Nov 18, 2024
15 changes: 15 additions & 0 deletions api/src/opentrons/protocol_runner/run_orchestrator.py
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,21 @@ def get_nozzle_maps(self) -> Dict[str, NozzleMap]:
"""Get current nozzle maps keyed by pipette id."""
return self._protocol_engine.state_view.tips.get_pipette_nozzle_maps()

def get_tip_attached(self) -> Dict[str, bool]:
"""Get current tip state keyed by pipette id."""

def has_tip_attached(pipette_id: str) -> bool:
return (
self._protocol_engine.state_view.pipettes.get_attached_tip(pipette_id)
is not None
)

pipette_ids = (
pipette.id
for pipette in self._protocol_engine.state_view.pipettes.get_all()
)
return {pipette_id: has_tip_attached(pipette_id) for pipette_id in pipette_ids}

def set_error_recovery_policy(self, policy: ErrorRecoveryPolicy) -> None:
"""Create error recovery policy for the run."""
self._protocol_engine.set_error_recovery_policy(policy)
Expand Down
52 changes: 29 additions & 23 deletions robot-server/robot_server/runs/router/base_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
RunCurrentState,
CommandLinkNoMeta,
NozzleLayoutConfig,
TipState,
)
from ..run_auto_deleter import RunAutoDeleter
from ..run_models import Run, BadRun, RunCreate, RunUpdate
Expand Down Expand Up @@ -591,33 +592,27 @@ async def get_current_state( # noqa: C901
"""
try:
run = run_data_manager.get(run_id=runId)
active_nozzle_maps = run_data_manager.get_nozzle_maps(run_id=runId)

nozzle_layouts = {
pipetteId: ActiveNozzleLayout.construct(
startingNozzle=nozzle_map.starting_nozzle,
activeNozzles=list(nozzle_map.map_store.keys()),
config=NozzleLayoutConfig(nozzle_map.configuration.value.lower()),
)
for pipetteId, nozzle_map in active_nozzle_maps.items()
}

run = run_data_manager.get(run_id=runId)
current_command = run_data_manager.get_current_command(run_id=runId)
last_completed_command = run_data_manager.get_last_completed_command(
run_id=runId
)
except RunNotCurrentError as e:
raise RunStopped(detail=str(e)).as_error(status.HTTP_409_CONFLICT)

links = CurrentStateLinks.construct(
lastCompleted=CommandLinkNoMeta.construct(
id=last_completed_command.command_id,
href=f"/runs/{runId}/commands/{last_completed_command.command_id}",
active_nozzle_maps = run_data_manager.get_nozzle_maps(run_id=runId)
nozzle_layouts = {
pipetteId: ActiveNozzleLayout.construct(
startingNozzle=nozzle_map.starting_nozzle,
activeNozzles=list(nozzle_map.map_store.keys()),
config=NozzleLayoutConfig(nozzle_map.configuration.value.lower()),
)
if last_completed_command is not None
else None
)
for pipetteId, nozzle_map in active_nozzle_maps.items()
}

tip_states = {
pipette_id: TipState.construct(hasTip=has_tip)
for pipette_id, has_tip in run_data_manager.get_tip_attached(
run_id=runId
).items()
}

current_command = run_data_manager.get_current_command(run_id=runId)

estop_engaged = False
place_labware = None
Expand Down Expand Up @@ -672,11 +667,22 @@ async def get_current_state( # noqa: C901
if place_labware:
break

last_completed_command = run_data_manager.get_last_completed_command(run_id=runId)
links = CurrentStateLinks.construct(
lastCompleted=CommandLinkNoMeta.construct(
id=last_completed_command.command_id,
href=f"/runs/{runId}/commands/{last_completed_command.command_id}",
)
if last_completed_command is not None
else None
)

return await PydanticResponse.create(
content=Body.construct(
data=RunCurrentState.construct(
estopEngaged=estop_engaged,
activeNozzleLayouts=nozzle_layouts,
tipStates=tip_states,
placeLabwareState=place_labware,
),
links=links,
Expand Down
7 changes: 7 additions & 0 deletions robot-server/robot_server/runs/run_data_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,13 @@ def get_nozzle_maps(self, run_id: str) -> Dict[str, NozzleMap]:

raise RunNotCurrentError()

def get_tip_attached(self, run_id: str) -> Dict[str, bool]:
"""Get current tip attached states, keyed by pipette id."""
if run_id == self._run_orchestrator_store.current_run_id:
return self._run_orchestrator_store.get_tip_attached()

raise RunNotCurrentError()

def get_all_commands_as_preserialized_list(
self, run_id: str, include_fixit_commands: bool
) -> List[str]:
Expand Down
28 changes: 25 additions & 3 deletions robot-server/robot_server/runs/run_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,16 @@ class ActiveNozzleLayout(BaseModel):
)


class TipState(BaseModel):
"""Information about the tip, if any, currently attached to a pipette."""

hasTip: bool

# todo(mm, 2024-11-15): I think the frontend is currently scraping the commands
# list to figure out where the current tip came from. Extend this class with that
# information so the frontend doesn't have to do that.


class PlaceLabwareState(BaseModel):
"""Details the labware being placed by the gripper."""

Expand All @@ -331,9 +341,21 @@ class PlaceLabwareState(BaseModel):
class RunCurrentState(BaseModel):
"""Current details about a run."""

estopEngaged: bool = Field(..., description="")
activeNozzleLayouts: Dict[str, ActiveNozzleLayout] = Field(...)
placeLabwareState: Optional[PlaceLabwareState] = Field(None)
# todo(mm, 2024-11-15): Having estopEngaged here is a bit of an odd man out because
# it's sensor state that can change on its own at any time, whereas the rest of
# these fields are logical state that changes only when commands are run.
#
# Our current mechanism for anchoring these fields to a specific point in time
# (important for avoiding torn-read problems when a client combines this info with
# info from other endpoints) is `links.currentCommand`, which is based on the idea
# that these fields only change when the current command changes.
#
# We should see if clients can replace this with `GET /robot/control/estopStatus`.
estopEngaged: bool

activeNozzleLayouts: Dict[str, ActiveNozzleLayout]
tipStates: Dict[str, TipState]
placeLabwareState: Optional[PlaceLabwareState]


class CommandLinkNoMeta(BaseModel):
Expand Down
7 changes: 7 additions & 0 deletions robot-server/robot_server/runs/run_orchestrator_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,9 @@ async def clear(self) -> RunResult:
state_summary=run_data, commands=commands, parameters=run_time_parameters
)

# todo(mm, 2024-11-15): Are all of these pass-through methods helpful?
# Can we delete them and make callers just call .run_orchestrator.play(), etc.?

Comment on lines +298 to +300
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TamarZanzouri Is this a good idea, or does this conflict with any future vision you had for the orchestrator stuff?

Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIscussed in-person. I think we're both a little confused about this, but:

RunOrchestratorStore is largely a glorified RunOrchestrator | None variable. And from that perspective, yes, all of these methods are redundant boilerplate, not offering any abstraction. We can replace RunOrchestrator.foo() with RunOrchestrator.run_orchestrator.foo() without losing anything.

There is then a question of, well, why even have RunOrchestratorStore. The answer is that other than all of these wrapper methods, it also provides access to a "default RunOrchestrator" for the "stateless command" endpoints, and it gates that access behind there not already being some other, run-controlled, RunOrchestrator already active. That is a good thing for the server to do, but I wonder if it still makes sense at this layer, now that we have maintenance runs. We might want one higher-level thing mediating between runs, maintenance runs, and stateless commands.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah stateless commands are not a thing we use anymore I'm pretty sure - we replaced them with maintenance runs because it turns out we need state for most stuff we want to do.

def play(self, deck_configuration: Optional[DeckConfigurationType] = None) -> None:
"""Start or resume the run."""
self.run_orchestrator.play(deck_configuration=deck_configuration)
Expand Down Expand Up @@ -331,6 +334,10 @@ def get_nozzle_maps(self) -> Dict[str, NozzleMap]:
"""Get the current nozzle map keyed by pipette id."""
return self.run_orchestrator.get_nozzle_maps()

def get_tip_attached(self) -> Dict[str, bool]:
"""Get current tip state keyed by pipette id."""
return self.run_orchestrator.get_tip_attached()

def get_run_time_parameters(self) -> List[RunTimeParameter]:
"""Parameter definitions defined by protocol, if any. Will always be empty before execution."""
return self.run_orchestrator.get_run_time_parameters()
Expand Down
Loading