-
Notifications
You must be signed in to change notification settings - Fork 179
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 deck conflict checks for trash bins loaded via addressable areas #14325
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14325 +/- ##
=======================================
Coverage 68.14% 68.14%
=======================================
Files 2512 2512
Lines 71479 71479
Branches 9079 9079
=======================================
Hits 48706 48706
Misses 20670 20670
Partials 2103 2103
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, just a pedantic thing about the public TrashBin
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I quite like how simple this approach turned out to be. But I agree it's going to be tough to use for any other kinds of conflict checks where we need to look at the actual dimensions of the bin, so updating this approach in a follow-up sounds good to me.
# It's important that we don't fetch these IDs from Protocol Engine, and | ||
# use our own bookkeeping instead. If we fetched these IDs from Protocol | ||
# Engine, it would have leaked state from Labware Position Check in the | ||
# same HTTP run. | ||
# | ||
# Wrapping .keys() in list() is just to make Decoy verification easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! We should actually remove/update this comment from this & the other check
calls too. This is talking about labware & module ID duplication from setup-command-based LPC, which is no longer in use. In that way, we should be able to safely fetch the IDs from the engine directly, but I think quite a few entities in the engine-based API are now relying on these in-api labware and module storage so we can't just replace them easily without checking all usages. But it's also not in the scope for this ticket. So maybe just an updated comment explaining 'that this is outdated and we should fetch the IDs from engine now' should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY!
Overview
Closes out RSS-421.
This PR adds the new trash bins, added in 2.16 and loaded via addressable area data, to deck conflict checking in PAPI. Previously all these checks were covered by the labware conflict checking, since the trash containers used to be labware. With the change away from that in the 7.1 release, these checks were lost and protocols that should not pass analysis were.
Now trash bins are taken into account when loading a thermocycler on the Flex (you cannot load a thermocycler when there is a trash in slot A1 and vice versa), and when loading a heater-shaker near the fixed trash on the OT-2. This also opens up potentially adding conflicts for the waste chute, but since none exist currently it is not taken into account.
Test Plan
Verified that the following protocols now fail analysis.
Changelog
TrashBin
class to deck conflict checkReview requests
Risk assessment
Low, this introduces no new functional behavior and re-adds checks that were previously being done in lower API levels.