-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(api): analysis and simulation of OT-2 protocols no longer hang #14433
Conversation
Analyses snapshot test run |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14433 +/- ##
=======================================
Coverage 68.13% 68.13%
=======================================
Files 2522 2522
Lines 72018 72018
Branches 9216 9216
=======================================
Hits 49072 49072
Misses 20757 20757
Partials 2189 2189
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.
LGTM!
if not skip_add_to_engine: | ||
self._engine_client.add_addressable_area(disposal_location.area_name) |
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.
This PR fixes this in the most simple and straightforward way, which is introducing a new, optional boolean argument to append_disposal_location that skips this call.
Hm, okay. Did you get a chance to try this alternative from RSS-464?
- Have
ProtocolContext.__init__()
callProtocolEngine.add_addressable_area()
directly, without going through the SyncClient.
I wouldn't expect that to be much more difficult than this, but I admit I didn't try it.
I'm uncomfortable (as I think you are) with introducing the skip_add_to_engine
complication to a system that's already difficult to reason about (as alluded to in #14358 (review)). I'm fine with merging this to unbreak edge
, since it seems like an implementation detail that we can silently improve in another PR. But how do you feel about doing that improvement in an immediate follow-up? As in, we'd actually do it this week, before picking up any additional work, not just leave it as a # todo
and write a tech debt ticket.
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.
My initial attempts at getting that method to work ran into issues that made me decide to go along with the one this PR implements, but I agree with your assessment that this introduces an additional complication into the system and that with further work and more time there can be a more holistic solution.
I am willing and able to continue to iterate on this this week to make this cleaner, while merging this PR to unbreak edge.
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.
Thanks for jumping on this so quickly. It’s a spicy one.
Overview
Fixes RSS-462 and RQA-2277.
In a previous PR (#14358) we added having trash bins and waste chutes immediately added to the addressable area store when they are loaded in a python protocol. Previously they'd only be added to the engine store when they were first referenced, which could cause confusing line numbers in error messages and didn't allow any deck conflict related checks to rely on them being in the store during analysis.
Unfortunately, the method used to add it to the store relied on the
SyncClient
, and since this was being called in the__init__
of theProtocolContext
for OT-2s for api levels 2.16 and above, this was causing a deadlock since this was being called from the main thread and not the thread running the PAPI protocol.This PR fixes this in the most simple and straightforward way, which is introducing a new, optional boolean argument to
append_disposal_location
that skips this call. This is now used in the call in the__init__
and only there, so Flex trashes and the waste chute will still be added to the engine store immediately. The OT-2 fixed trash will now be added to the store when it is first referenced (either by adrop_tip
,dispense
,blow_out
or amove_to
call), which is how it acted before #14358.This does have the consequence that the OT-2 fixed trash, unlike any other trash bin, will behave slightly differently and inconsistently. In practice, this shouldn't lead to any issues or false positives in analysis. The only checks using the addressable area store for trashes involve the 96 channel pipette, which is Flex only, and all other checks don't rely on the addressable area store.
Test Plan
This empty OT-2 protocol does not hang during analysis or simulation but instead passes/completes.
Changelog
append_disposal_location
to allow skipping adding the area to the engine storeRisk assessment
Low.