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

feat(api): return location sequences from commands #17435

Open
wants to merge 9 commits into
base: edge
Choose a base branch
from

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Feb 5, 2025

By adding all the locations of a labware used in a command that establishes or changes the position of a labware, we can free the client from having to accumulate all previous such commands to display a single such command.

These locations are shown as location sequences. These are lists of heterogenous unions keyed by kind. The kinds mostly correspond to the kinds of locations that the engine already uses - they're different types because they have the kind key so you can tell them apart when serialized instead of just via python isinstance checks. All these sequences end in an OnAddressableAreaLocation. This probably needs to be rethought in a subsequent pr, because it makes modules awkward. Labware on a module will have a location sequence containing that module and then an OnAddressableAreaLocation. This probably needs to change to guarantee that sequences end in an OnCutoutLocation or something that identify the addressable area and cutout fixture or something. It's a little awkward that the OT-2 and the flex work differently.

There are some command implementations that are straightforward, and some that are charmingly quirky.

  • loadLabware, reloadLabware, loadLid all just return the location of the newly added labware. Simple, clean, wondrous
  • loadLidStack creates a lid stack object and returns its location, and also creates N lid objects and returns all of their locations, which is done in its own array that is guaranteed to have the same ordering as all the lid ids so we can keep the types of the locations consistent across commands and also avoid altering already-existing result elements
  • moveLabware needs an origin and a destination location, so it has them. In fact, one destination location isn't enough. When we move labware to a trash location, we need to encode both (1) which trash location it was and (2) that it is a trash location. After the command, the position of the labware is OFF_DECK (because it went into the trash), after all. So we get both immediateDestinationLocationSequence (which in this scenario would be the addressable area of the trash) and eventualDestinationLocationSequence (which in this scenario would be off deck). If the two are the same, then the two are the same
  • the stacker store and retrieve commands have info that isn't really actually useful right now, but it will be soon, so let's carve out some space for them. For now the labware just bounces back and forth between the module/off deck and the module/stacker staging slot.

to come out of draft

  • ts bindings
  • stacker store and retrieve
  • [ ] add "what is the old and new state of the locations named in the command" to these values
    not going to do that ^ in this pr just so we can get it merged

These are like offset location sequences, but they involve instance IDs
instead of geometry identifiers. They'll be returned from various commands.
Load labware now tells you the location sequence of the labware it just
loaded.
MoveLabware is a little different than the other implementations.

It needs an origin and a destination sequence, since it's a move -
that's straightforward.

But it also needs to drive a distinction between an immediate
destination: where is the labware dropped off? and an eventual
destination: where does the labware end up? so that the necessary
information to figure out what happens when you move a labware to the
trash is there. We need to identify the trash, and which trash; but we
also need to indicate that the labware is going to get thrown out. So we
have three total locations.
@sfoster1 sfoster1 requested a review from a team as a code owner February 5, 2025 17:24
@sfoster1 sfoster1 marked this pull request as draft February 5, 2025 17:24
Comment on lines 99 to 104
stackLocationSequence: LabwareLocationSequence | None = Field(
None, description="The location sequence for this stack."
)
locationSequences: List[LabwareLocationSequence] | None = Field(
None,
description="The location sequences for the lids just loaded into the stack. These are in the same order as labwareIds.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused between these 2 props. can we maybe clarify it?

Copy link
Contributor

@TamarZanzouri TamarZanzouri Feb 6, 2025

Choose a reason for hiding this comment

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

if one of them is a "physical adapter" maybe it will clarify if we add it to the desc

Comment on lines +107 to +114
eventualDestinationLocationSequence: LabwareLocationSequence | None = Field(
None,
description=(
"The full location in which this labware will eventually reside. This will typically be the same as its "
"immediate destination, but if this labware is going to the trash then this field will be off deck."
),
)
immediateDestinationLocationSequence: LabwareLocationSequence | None = Field(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain the diff between these two props?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is for when you move stuff to the trash. We want to know that it's going to the trash, so we can render it; but it doesn't end up in the trash, it ends up off deck, and we want to know that too. So we have both

def labware_location_is_system(
location: LabwareLocation,
) -> TypeGuard[_SystemLocationType]:
"""Check if a location is the system location."""
Copy link
Contributor

Choose a reason for hiding this comment

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

might have missed this convo but what is a system location?

Copy link
Member Author

Choose a reason for hiding this comment

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

SYSTEM is a new location used for uh I forget. It's like off deck but not literally off deck, I think it's for deleted stuff

labware_location.moduleId
)
try:
self._modules.get_flex_stacker_substate(labware_location.moduleId)
Copy link
Contributor

Choose a reason for hiding this comment

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

are we doing this get just for catching the exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

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

overall looks good! had a few questions/clarifications.

@sfoster1 sfoster1 marked this pull request as ready for review February 6, 2025 22:02
@sfoster1 sfoster1 requested a review from a team as a code owner February 6, 2025 22:16
Copy link

codecov bot commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 25.51%. Comparing base (2b87a7a) to head (494579d).
Report is 19 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #17435      +/-   ##
==========================================
+ Coverage   17.34%   25.51%   +8.16%     
==========================================
  Files        3128     3270     +142     
  Lines      226114   232069    +5955     
  Branches     6884     9323    +2439     
==========================================
+ Hits        39225    59210   +19985     
+ Misses     186889   172842   -14047     
- Partials        0       17      +17     
Flag Coverage Δ
app 3.20% <ø> (?)
g-code-testing 92.43% <ø> (?)
opentrons-ai-client 4.81% <ø> (?)
protocol-designer 17.56% <ø> (+0.22%) ⬆️
shared-data 2.39% <ø> (?)
step-generation 4.08% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
shared-data/command/types/setup.ts 100.00% <ø> (ø)

... and 540 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants