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(api,robot-server,app): use labwareURI instead of labwareID for placeLabwareState and unsafe/placeLabware command. #16719

Merged
merged 4 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from all 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-client/src/runs/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ export interface NozzleLayoutValues {
}

export interface PlaceLabwareState {
labwareId: string
labwareURI: string
location: OnDeckLabwareLocation
shouldPlaceDown: boolean
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from typing import TYPE_CHECKING, Optional, Type
from typing_extensions import Literal

from opentrons.calibration_storage.helpers import details_from_uri
from opentrons.hardware_control.types import Axis, OT3Mount
from opentrons.motion_planning.waypoints import get_gripper_labware_placement_waypoints
from opentrons.protocol_engine.errors.exceptions import (
Expand All @@ -13,7 +14,12 @@
)
from opentrons.types import Point

from ...types import DeckSlotLocation, ModuleModel, OnDeckLabwareLocation
from ...types import (
DeckSlotLocation,
LoadedLabware,
ModuleModel,
OnDeckLabwareLocation,
)
from ..command import AbstractCommandImpl, BaseCommand, BaseCommandCreate, SuccessData
from ...errors.error_occurrence import ErrorOccurrence
from ...resources import ensure_ot3_hardware
Expand All @@ -32,7 +38,7 @@
class UnsafePlaceLabwareParams(BaseModel):
"""Payload required for an UnsafePlaceLabware command."""

labwareId: str = Field(..., description="The id of the labware to place.")
labwareURI: str = Field(..., description="Labware URI for labware.")
Comment on lines -35 to +41
Copy link
Contributor

@SyntaxColoring SyntaxColoring Nov 7, 2024

Choose a reason for hiding this comment

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

loadLabware splits this out into separate loadName/namespace/version fields, so I might have matched that here for consistency.

Don't feel obligated to do that in this PR, though. If we care, we should be able to change this later because we only use this command in maintenance runs, so there are fewer persistence layer compatibility concerns.

location: OnDeckLabwareLocation = Field(
..., description="Where to place the labware."
)
Expand Down Expand Up @@ -71,8 +77,8 @@ async def execute(
is pressed, get into error recovery, etc).

Unlike the `moveLabware` command, where you pick a source and destination
location, this command takes the labwareId to be moved and location to
move it to.
location, this command takes the labwareURI of the labware to be moved
and location to move it to.

"""
ot3api = ensure_ot3_hardware(self._hardware_api)
Expand All @@ -84,10 +90,35 @@ async def execute(
"Cannot place labware when gripper is not gripping."
)

labware_id = params.labwareId
# Allow propagation of LabwareNotLoadedError.
definition_uri = self._state_view.labware.get(labware_id).definitionUri
location = self._state_view.geometry.ensure_valid_gripper_location(
params.location,
)

# TODO: We need a way to create temporary labware for moving around,
# the labware should get deleted once its used.
details = details_from_uri(params.labwareURI)
labware = await self._equipment.load_labware(
load_name=details.load_name,
namespace=details.namespace,
version=details.version,
location=location,
labware_id=None,
)

self._state_view.labware._state.definitions_by_uri[
params.labwareURI
] = labware.definition
self._state_view.labware._state.labware_by_id[
labware.labware_id
] = LoadedLabware.construct(
id=labware.labware_id,
location=location,
loadName=labware.definition.parameters.loadName,
definitionUri=params.labwareURI,
offsetId=labware.offsetId,
)
Comment on lines +97 to +119
Copy link
Contributor

@SyntaxColoring SyntaxColoring Nov 7, 2024

Choose a reason for hiding this comment

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

For the benefit of other reviewers, and to answer your review question:

Modifying self._state_view.labware._state directly from a command implementation like this is, in most cases, not acceptable, as you called out. But we're going to replace this code imminently as part of RQA-3471, so this code makes sense to bridge the gap until then. I wouldn't sweat it, if it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with that point, this is a temporary patch for the existing architecture which will be replaced with the changes Max mentioned.


labware_id = labware.labware_id
# todo(mm, 2024-11-06): This is only correct in the special case of an
# absorbance reader lid. Its definition currently puts the offsets for *itself*
# in the property that's normally meant for offsets for its *children.*
Expand All @@ -109,10 +140,6 @@ async def execute(
params.location.slotName.id
)

location = self._state_view.geometry.ensure_valid_gripper_location(
params.location,
)

# This is an absorbance reader, move the lid to its dock (staging area).
if isinstance(location, DeckSlotLocation):
module = self._state_view.modules.get_by_slot(location.slotName)
Expand All @@ -122,7 +149,7 @@ async def execute(
)

new_offset_id = self._equipment.find_applicable_labware_offset_id(
labware_definition_uri=definition_uri,
labware_definition_uri=params.labwareURI,
labware_location=location,
)

Expand Down
6 changes: 3 additions & 3 deletions app/src/resources/modules/hooks/usePlacePlateReaderLid.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export function usePlacePlateReaderLid(
const location = placeLabware.location
const loadModuleCommand = buildLoadModuleCommand(location as ModuleLocation)
const placeLabwareCommand = buildPlaceLabwareCommand(
placeLabware.labwareId as string,
placeLabware.labwareURI as string,
location
)
commandsToExecute = [loadModuleCommand, placeLabwareCommand]
Expand Down Expand Up @@ -72,11 +72,11 @@ const buildLoadModuleCommand = (location: ModuleLocation): CreateCommand => {
}

const buildPlaceLabwareCommand = (
labwareId: string,
labwareURI: string,
location: OnDeckLabwareLocation
): CreateCommand => {
return {
commandType: 'unsafe/placeLabware' as const,
params: { labwareId, location },
params: { labwareURI, location },
}
}
21 changes: 14 additions & 7 deletions robot-server/robot_server/runs/router/base_router.py
Original file line number Diff line number Diff line change
Expand Up @@ -602,6 +602,7 @@ async def get_current_state( # noqa: C901
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
Expand Down Expand Up @@ -636,11 +637,14 @@ async def get_current_state( # noqa: C901
if isinstance(command, MoveLabware):
location = command.params.newLocation
if isinstance(location, DeckSlotLocation):
place_labware = PlaceLabwareState(
location=location,
labwareId=command.params.labwareId,
shouldPlaceDown=False,
)
for labware in run.labware:
if labware.id == command.params.labwareId:
place_labware = PlaceLabwareState(
location=location,
labwareURI=labware.definitionUri,
shouldPlaceDown=False,
)
break
# Handle absorbance reader lid
elif isinstance(command, (OpenLid, CloseLid)):
for mod in run.modules:
Expand All @@ -655,10 +659,13 @@ async def get_current_state( # noqa: C901
and hw_mod.serial_number == mod.serialNumber
):
location = mod.location
labware_id = f"{mod.model}Lid{location.slotName}"
# TODO: Not the best location for this, we should
# remove this once we are no longer defining the plate reader lid
# as a labware.
labware_uri = "opentrons/opentrons_flex_lid_absorbance_plate_reader_module/1"
vegano1 marked this conversation as resolved.
Show resolved Hide resolved
place_labware = PlaceLabwareState(
location=location,
labwareId=labware_id,
labwareURI=labware_uri,
shouldPlaceDown=estop_engaged,
)
break
Expand Down
2 changes: 1 addition & 1 deletion robot-server/robot_server/runs/run_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ class ActiveNozzleLayout(BaseModel):
class PlaceLabwareState(BaseModel):
"""Details the labware being placed by the gripper."""

labwareId: str = Field(..., description="The ID of the labware to place.")
labwareURI: str = Field(..., description="The URI of the labware to place.")
location: OnDeckLabwareLocation = Field(
..., description="The location the labware should be in."
)
Expand Down
8 changes: 4 additions & 4 deletions shared-data/command/schemas/10.json
Original file line number Diff line number Diff line change
Expand Up @@ -4749,9 +4749,9 @@
"description": "Payload required for an UnsafePlaceLabware command.",
"type": "object",
"properties": {
"labwareId": {
"title": "Labwareid",
"description": "The id of the labware to place.",
"labwareURI": {
"title": "Labwareuri",
"description": "Labware URI for labware.",
"type": "string"
},
"location": {
Expand All @@ -4773,7 +4773,7 @@
]
}
},
"required": ["labwareId", "location"]
"required": ["labwareURI", "location"]
},
"UnsafePlaceLabwareCreate": {
"title": "UnsafePlaceLabwareCreate",
Expand Down
2 changes: 1 addition & 1 deletion shared-data/command/types/unsafe.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export interface UnsafeUngripLabwareRunTimeCommand
result?: any
}
export interface UnsafePlaceLabwareParams {
labwareId: string
labwareURI: string
location: OnDeckLabwareLocation
}
export interface UnsafePlaceLabwareCreateCommand
Expand Down
Loading