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

refactor(robot-server): add PE commands to /sessions responses #8006

Merged
merged 4 commits into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/src/opentrons/file_runner/command_queue_worker.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def _next_command(
) -> Optional[str]:
if self._keep_running:
# Will be None if the engine has no commands left.
return self._engine.state_view.commands.get_next_command()
return self._engine.state_view.commands.get_next_queued()
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice.

else:
return None

Expand Down
6 changes: 6 additions & 0 deletions api/src/opentrons/protocol_engine/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from .create_protocol_engine import create_protocol_engine
from .protocol_engine import ProtocolEngine
from .errors import ProtocolEngineError
from .commands import Command, CommandRequest, CommandStatus, CommandType
from .state import State, StateView, LabwareData
from .types import (
DeckLocation,
Expand All @@ -24,6 +25,11 @@
"ProtocolEngine",
# error types
"ProtocolEngineError",
# top level command unions and values
"Command",
"CommandRequest",
"CommandStatus",
"CommandType",
# state interfaces and models
"State",
"StateView",
Expand Down
63 changes: 53 additions & 10 deletions api/src/opentrons/protocol_engine/commands/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,71 @@

from .command import AbstractCommandImpl, BaseCommand, BaseCommandRequest, CommandStatus
from .command_mapper import CommandMapper
from .command_unions import Command, CommandRequest, CommandResult

from .command_unions import Command, CommandRequest, CommandResult, CommandType

from .add_labware_definition import (
AddLabwareDefinition,
AddLabwareDefinitionRequest,
AddLabwareDefinitionData,
AddLabwareDefinitionRequest,
AddLabwareDefinitionResult,
AddLabwareDefinitionCommandType,
)

from .aspirate import (
Aspirate,
AspirateData,
AspirateRequest,
AspirateResult,
AspirateCommandType,
)
from .aspirate import Aspirate, AspirateRequest, AspirateData, AspirateResult
from .dispense import Dispense, DispenseRequest, DispenseData, DispenseResult
from .drop_tip import DropTip, DropTipRequest, DropTipData, DropTipResult

from .dispense import (
Dispense,
DispenseData,
DispenseRequest,
DispenseResult,
DispenseCommandType,
)

from .drop_tip import (
DropTip,
DropTipData,
DropTipRequest,
DropTipResult,
DropTipCommandType,
)

from .load_labware import (
LoadLabware,
LoadLabwareRequest,
LoadLabwareData,
LoadLabwareRequest,
LoadLabwareResult,
LoadLabwareCommandType,
)

from .load_pipette import (
LoadPipette,
LoadPipetteRequest,
LoadPipetteData,
LoadPipetteRequest,
LoadPipetteResult,
LoadPipetteCommandType,
)

from .move_to_well import (
MoveToWell,
MoveToWellRequest,
MoveToWellData,
MoveToWellRequest,
MoveToWellResult,
MoveToWellCommandType,
)

from .pick_up_tip import (
PickUpTip,
PickUpTipData,
PickUpTipRequest,
PickUpTipResult,
PickUpTipCommandType,
)
from .pick_up_tip import PickUpTip, PickUpTipRequest, PickUpTipData, PickUpTipResult


__all__ = [
Expand All @@ -55,6 +89,7 @@
"Command",
"CommandRequest",
"CommandResult",
"CommandType",
# base interfaces
"AbstractCommandImpl",
"BaseCommand",
Expand All @@ -65,39 +100,47 @@
"LoadLabwareRequest",
"LoadLabwareData",
"LoadLabwareResult",
"LoadLabwareCommandType",
# add labware definition command models
"AddLabwareDefinition",
"AddLabwareDefinitionRequest",
"AddLabwareDefinitionData",
"AddLabwareDefinitionResult",
"AddLabwareDefinitionCommandType",
# load pipette command models
"LoadPipette",
"LoadPipetteRequest",
"LoadPipetteData",
"LoadPipetteResult",
"LoadPipetteCommandType",
# move to well command models
"MoveToWell",
"MoveToWellRequest",
"MoveToWellData",
"MoveToWellResult",
"MoveToWellCommandType",
# pick up tip command models
"PickUpTip",
"PickUpTipRequest",
"PickUpTipData",
"PickUpTipResult",
"PickUpTipCommandType",
# drop tip command models
"DropTip",
"DropTipRequest",
"DropTipData",
"DropTipResult",
"DropTipCommandType",
# aspirate command models
"Aspirate",
"AspirateRequest",
"AspirateData",
"AspirateResult",
"AspirateCommandType",
# dispense command models
"Dispense",
"DispenseRequest",
"DispenseData",
"DispenseResult",
"DispenseCommandType",
]
53 changes: 46 additions & 7 deletions api/src/opentrons/protocol_engine/commands/command_unions.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,42 @@
AddLabwareDefinition,
AddLabwareDefinitionRequest,
AddLabwareDefinitionResult,
AddLabwareDefinitionCommandType,
)

from .aspirate import Aspirate, AspirateRequest, AspirateResult, AspirateCommandType

from .dispense import Dispense, DispenseRequest, DispenseResult, DispenseCommandType

from .drop_tip import DropTip, DropTipRequest, DropTipResult, DropTipCommandType

from .load_labware import (
LoadLabware,
LoadLabwareRequest,
LoadLabwareResult,
LoadLabwareCommandType,
)

from .load_pipette import (
LoadPipette,
LoadPipetteRequest,
LoadPipetteResult,
LoadPipetteCommandType,
)

from .move_to_well import (
MoveToWell,
MoveToWellRequest,
MoveToWellResult,
MoveToWellCommandType,
)

from .pick_up_tip import (
PickUpTip,
PickUpTipRequest,
PickUpTipResult,
PickUpTipCommandType,
)
from .aspirate import Aspirate, AspirateRequest, AspirateResult
from .dispense import Dispense, DispenseRequest, DispenseResult
from .drop_tip import DropTip, DropTipRequest, DropTipResult
from .load_labware import LoadLabware, LoadLabwareRequest, LoadLabwareResult
from .load_pipette import LoadPipette, LoadPipetteRequest, LoadPipetteResult
from .move_to_well import MoveToWell, MoveToWellRequest, MoveToWellResult
from .pick_up_tip import PickUpTip, PickUpTipRequest, PickUpTipResult

Command = Union[
AddLabwareDefinition,
Expand All @@ -25,6 +53,17 @@
PickUpTip,
]

CommandType = Union[
AddLabwareDefinitionCommandType,
AspirateCommandType,
DispenseCommandType,
DropTipCommandType,
LoadLabwareCommandType,
LoadPipetteCommandType,
MoveToWellCommandType,
PickUpTipCommandType,
]

CommandRequest = Union[
AddLabwareDefinitionRequest,
AspirateRequest,
Expand Down
2 changes: 1 addition & 1 deletion api/src/opentrons/protocol_engine/protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def add_command(self, request: CommandRequest) -> Command:

async def execute_command_by_id(self, command_id: str) -> Command:
"""Execute a protocol engine command by its identifier."""
queued_command = self.state_view.commands.get_command_by_id(command_id)
queued_command = self.state_view.commands.get(command_id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In usage, state_view.commands.get_command_by_id felt quite excessive. In line with similar feedback in the past, I renamed the methods of the CommandView to remove command, given that they're all accessed via state_view.commands:

before after
commands.get_command_by_id commands.get
commands.get_all_commands commands.get_all
commands.get_next_command commands.get_next_queued

Copy link
Contributor

Choose a reason for hiding this comment

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

This feels much better to me.

If we wanted to go even further, how would we feel about replacing commands.get(id) with commands[id]?

Copy link
Contributor Author

@mcous mcous Jun 29, 2021

Choose a reason for hiding this comment

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

I'm anti-bracket-notation for this interface. Supporting bracket notation would bring in magic (methods) that just aren't needed and don't (IMO) meaningfully improve the DX of this class.

  • It would make testing harder (or at least inconsistent, where we can no longer do a simple function mock)
  • It makes it less obvious that commands.get(id) may be a computed value
    • If, for example, different parts of a given Command were stored in different places, and get pulled them all together
    • This isn't outside the realm of possibility
  • You can already do command_state.commands_by_id[id]


running_command = self._command_mapper.update_command(
command=queued_command,
Expand Down
12 changes: 6 additions & 6 deletions api/src/opentrons/protocol_engine/state/commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from __future__ import annotations
from collections import OrderedDict
from dataclasses import dataclass, replace
from typing import List, Optional, Tuple
from typing import List, Optional

from ..commands import Command, CommandStatus
from ..errors import CommandDoesNotExistError
Expand Down Expand Up @@ -41,23 +41,23 @@ def __init__(self, state: CommandState) -> None:
"""Initialize the view of command state with its underlying data."""
self._state = state

def get_command_by_id(self, command_id: str) -> Command:
def get(self, command_id: str) -> Command:
"""Get a command by its unique identifier."""
try:
return self._state.commands_by_id[command_id]
except KeyError:
raise CommandDoesNotExistError(f"Command {command_id} does not exist")

def get_all_commands(self) -> List[Tuple[str, Command]]:
"""Get a list of all commands in state, paired with their respective IDs.
def get_all(self) -> List[Command]:
"""Get a list of all commands in state.

Entries are returned in the order of first-added command to last-added command.
Replacing a command (to change its status, for example) keeps its place in the
ordering.
"""
return [entry for entry in self._state.commands_by_id.items()]
return list(self._state.commands_by_id.values())

def get_next_command(self) -> Optional[str]:
def get_next_queued(self) -> Optional[str]:
"""Return the next request in line to be executed.

Normally, this corresponds to the earliest-added command that's currently
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def state_view_with_commands(
) -> MagicMock:
"""Create a state view fixture with pending commands."""
pending_commands = commands + [None]
state_view.commands.get_next_command.side_effect = pending_commands
state_view.commands.get_next_queued.side_effect = pending_commands
return state_view


Expand All @@ -54,7 +54,7 @@ async def test_play_no_pending(
state_view: MagicMock,
) -> None:
"""It should not execute any commands."""
state_view.commands.get_next_command.return_value = None
state_view.commands.get_next_queued.return_value = None

subject.play()
await subject.wait_to_be_idle()
Expand Down
28 changes: 12 additions & 16 deletions api/tests/opentrons/protocol_engine/state/test_command_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ def get_command_view(
return CommandView(state=state)


def test_get_command_by_id() -> None:
def test_get_by_id() -> None:
"""It should get a command by ID from state."""
command = create_completed_command(command_id="command-id")
subject = get_command_view(commands_by_id=[("command-id", command)])

assert subject.get_command_by_id("command-id") == command
assert subject.get("command-id") == command


def test_get_command_bad_id() -> None:
Expand All @@ -38,10 +38,10 @@ def test_get_command_bad_id() -> None:
subject = get_command_view(commands_by_id=[("command-id", command)])

with pytest.raises(errors.CommandDoesNotExistError):
subject.get_command_by_id("asdfghjkl")
subject.get("asdfghjkl")


def test_get_all_commands() -> None:
def test_get_all() -> None:
"""It should get all the commands from the state."""
command_1 = create_completed_command(command_id="command-id-1")
command_2 = create_running_command(command_id="command-id-2")
Expand All @@ -55,14 +55,10 @@ def test_get_all_commands() -> None:
]
)

assert subject.get_all_commands() == [
("command-id-1", command_1),
("command-id-2", command_2),
("command-id-3", command_3),
]
assert subject.get_all() == [command_1, command_2, command_3]


def test_get_next_command_returns_first_pending() -> None:
def test_get_next_queued_returns_first_pending() -> None:
"""It should return the first command that's pending."""
pending_command = create_pending_command()
running_command = create_running_command()
Expand All @@ -77,18 +73,18 @@ def test_get_next_command_returns_first_pending() -> None:
]
)

assert subject.get_next_command() == "command-id-3"
assert subject.get_next_queued() == "command-id-3"


def test_get_next_command_returns_none_when_no_pending() -> None:
def test_get_next_queued_returns_none_when_no_pending() -> None:
"""It should return None if there are no pending commands to return."""
running_command = create_running_command(command_id="command-id-1")
completed_command = create_completed_command(command_id="command-id-2")
failed_command = create_failed_command(command_id="command-id-3")

subject = get_command_view()

assert subject.get_next_command() is None
assert subject.get_next_queued() is None

subject = get_command_view(
commands_by_id=[
Expand All @@ -98,10 +94,10 @@ def test_get_next_command_returns_none_when_no_pending() -> None:
]
)

assert subject.get_next_command() is None
assert subject.get_next_queued() is None


def test_get_next_command_returns_none_when_earlier_command_failed() -> None:
def test_get_next_queued_returns_none_when_earlier_command_failed() -> None:
"""It should return None if any prior-added command is failed."""
running_command = create_running_command(command_id="command-id-1")
completed_command = create_completed_command(command_id="command-id-2")
Expand All @@ -117,4 +113,4 @@ def test_get_next_command_returns_none_when_earlier_command_failed() -> None:
]
)

assert subject.get_next_command() is None
assert subject.get_next_queued() is None
6 changes: 3 additions & 3 deletions api/tests/opentrons/protocol_engine/test_protocol_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,9 @@ async def test_execute_command_by_id(
data=data,
)

decoy.when(
state_store.state_view.commands.get_command_by_id("command-id")
).then_return(queued_command)
decoy.when(state_store.state_view.commands.get("command-id")).then_return(
queued_command
)

decoy.when(resources.model_utils.get_timestamp()).then_return(started_at)

Expand Down
Loading