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): add and use inStackerHopperLocation #17535

Merged
merged 4 commits into from
Feb 19, 2025

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Feb 14, 2025

Labware objects that are in the stacker hopper have the new location InStackerHopperLocation, which also identifies the labware Id of the stacker in question.

In this PR, we set the location of labware that get loaded onto the stacker module while static=False to InStackerHopperLocation. However, after the rest of the work in the epic, the only way to get a labware into InStackerHopperLocation is to pass it to stacker/store. It's not a location you can specify in any parameters, which is already true in this PR.

Because we're adding it fresh and thus it isn't already serialized anywhere, we can give the location object a kind member and therefore don't have to have a separate location sequence component.

review requests

does this look like the right data? are we happy with it?

testing

none required, this is well-covered by internal tests and the data that it exposes that changed isn't actually used by anything yet.

@sfoster1 sfoster1 requested a review from CaseyBatten February 14, 2025 20:13
@sfoster1 sfoster1 requested a review from a team as a code owner February 14, 2025 20:13
@sfoster1 sfoster1 requested review from a team as code owners February 14, 2025 20:54
@sfoster1 sfoster1 requested review from koji and removed request for a team February 14, 2025 20:54
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.62%. Comparing base (6ce82af) to head (cf49ac9).
Report is 3 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #17535   +/-   ##
=======================================
  Coverage   63.62%   63.62%           
=======================================
  Files        2838     2838           
  Lines      218019   218019           
  Branches    18107    18107           
=======================================
  Hits       138704   138704           
  Misses      79122    79122           
  Partials      193      193           
Flag Coverage Δ
protocol-designer 18.85% <ø> (ø)
step-generation 4.36% <ø> (ø)

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% <ø> (ø)

Copy link
Contributor

@caila-marashaj caila-marashaj left a comment

Choose a reason for hiding this comment

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

Looks good to me. The way it works is you'll store a labware in a hopper, creating one of these new location types with an eventualDestinationSequence, and then calling retrieve will update the locations of all the labware in the hopper?

@sfoster1
Copy link
Member Author

Looks good to me. The way it works is you'll store a labware in a hopper, creating one of these new location types with an eventualDestinationSequence, and then calling retrieve will update the locations of all the labware in the hopper?

Actually no; the only way you can get this location is storeing a labware there. Rather than loading labware instances into a stacker, you'll configure its labware pool.

Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Looks good overall but I think your tests cases that utilize possibleCutoutFixtureIds are going to start failing if you merge edge in as they do not list all the possibilities? If thats not the case I'll come back and approve.

NotOnDeckLocationSequenceComponent(logicalLocationName=OFF_DECK_LOCATION),
InStackerHopperLocation(moduleId="flex-stacker-id"),
OnCutoutFixtureLocationSequenceComponent(
cutoutId="cutoutB4", possibleCutoutFixtureIds=["flexStackerModuleV1"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this need to be updated to include the following (and in all your other tests that list the potential cutout fixtures)?:
"flexStackerModuleV1WithWasteChuteRightAdapterCovered",
"flexStackerModuleV1WithWasteChuteRightAdapterNoCover",
"flexStackerModuleV1WithMagneticBlockV1"

Copy link
Member Author

Choose a reason for hiding this comment

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

oh because you merged that other PR? i'll merge edge into this and see if it starts failing

This will be used for labware that is currently located in a stacker
hopper as a base, so that OnModuleLocation/OnAddressableAreaLocation can
be reserved for labware on the stacker shuttle.
stacker hoppers. Kept you guessing.

You don't specify this on the input to loadLabware because very soon you
won't be able to load labware into a stacker hopper anymore; once the
rest of the commands are implemented, labware will only get this
location if it is passed to stacker/store.

Closes EXEC-1209
@sfoster1 sfoster1 force-pushed the exec-1209-in-stacker-hopper-location branch from 9da89a9 to 47390f0 Compare February 19, 2025 15:11
@sfoster1
Copy link
Member Author

Looks like it was just the one test @CaseyBatten , fixed up

Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Stupendiferous

@sfoster1 sfoster1 merged commit 70a4ac8 into edge Feb 19, 2025
53 checks passed
@sfoster1 sfoster1 deleted the exec-1209-in-stacker-hopper-location branch February 19, 2025 16:01
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.

3 participants