-
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
refactor(api, robot-server): removal of automatic drop tip from Flex while maintaining OT2 behavior #14182
refactor(api, robot-server): removal of automatic drop tip from Flex while maintaining OT2 behavior #14182
Conversation
known problems: Test failures on motion planning tests Decision based on robot type yet to be made for ending state, will need to add to somewhere at the start and pass down
… dropping behavior
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.1.0 #14182 +/- ##
=======================================================
- Coverage 70.46% 70.46% -0.01%
=======================================================
Files 2512 2512
Lines 71218 71211 -7
Branches 8974 8974
=======================================================
- Hits 50185 50178 -7
Misses 18837 18837
Partials 2196 2196
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Protocols meeting the description of the test case above were ran on both OT2 and Flex, and the desired behavior was seen. OT2 automatically dropped tips at the end then homed, and Flex simply homed (without the plungers) keeping the tips attached. |
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.
Looks good to me except the commented out code; we should merge it once that's addressed, but I think we should quickly follow up with going back to the original behavior for protocol API versions below 2.16.
drop_tips_after_run=drop_tips_after_run, | ||
post_run_hardware_state=post_run_hardware_state, | ||
) | ||
# self._task_queue.set_post_hardware_run_state( |
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.
commented on purpose?
drop_tips_after_run=drop_tips_after_run, | ||
post_run_hardware_state=post_run_hardware_state, | ||
) | ||
# self._task_queue.set_post_hardware_run_state( |
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.
commented on purpose?
self._task_queue = task_queue or TaskQueue(cleanup_func=protocol_engine.finish) | ||
self._task_queue = ( | ||
task_queue or TaskQueue() | ||
) # cleanup_func=protocol_engine.finish) |
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.
commented out on purpose?
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.
I think this makes sense but I have one question about how this interacts with OT-2 protocols that have fixed trash labware.
@@ -336,6 +336,21 @@ def get_all_cutout_fixtures(self) -> Optional[List[str]]: | |||
for _, cutout_fixture_id in self._state.deck_configuration | |||
] | |||
|
|||
def get_disposal_addressable_areas(self) -> List[str]: |
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.
Is the plan for this to be used in the v7.1.0 launch? If not, I'd say leave it out of the code for now.
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.
Not currently, I had implemented it when I was trying to dynamically choose the trash to utilize, however I ran into the issue that trash is only loaded into the PE store if it is used in a protocol, which means doing a protocol that only consists of a pickup tip and nothing else will result in a protocol engine addressable area list with zero trashes anywhere in it, even a fixed trash. I can remove it for now.
await self._movement_handler.move_to_addressable_area( | ||
pipette_id=pipette_id, | ||
addressable_area_name="fixedTrash", | ||
offset=AddressableOffsetVector(x=0, y=0, z=0), | ||
force_direct=False, | ||
speed=None, | ||
minimum_z_height=None, | ||
) |
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.
How does this work if the OT-2 protocol has a fixed trash labware (definitely PAPIv2.14 and PAPIv2.15, and who knows what happens with PAPIv≤2.13)? I would expect referencing the fixedTrash
addressable area to raise a conflict error, because it can't exist simultaneously with the labware?
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.
Just ran it on an OT2 with 2.16, 2.15, 2.13 and 2.11 protocols that do the test case I mentioned in the PR post. Each of them at the end did an automatic tip drop in the fixedTrash as expected, so it does work even on older API versions. Not too sure there would be a conflict between a fixedTrash addressable area existing in the deck config of an OT2 protocol while a fixedTrash labware exists as well, because the protocol end of things wouldn't really be aware of it if they are running a legacy protocol... right?
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.
Huh, that's surprising.
What I have in mind is: on a Flex, I can't load a labware into slot D3 and use the wasteChute
addressable area. Loading the labware needs D3
, and using the waste chute needs wasteChute
, and those are mutually exclusive.
And I thought that was how this was working for the OT-2, too. The fixed trash labware is "in" "slot 12," which occupies cutout 12, so how is a fixed trash also allowed to occupy cutout 12?
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.
I'm not entirely sure. My understanding is that move to addressable area will move to an area so long as it exists within the deck configuration. I believe the OT2 always has a fixed trash in its deck configuration, so it is a valid call. I don't think we ever explicitly do a "load fixed trash in slot 12" type of command in OT2 protocols, and the labware is added as a created entity to the disposal_locations list in the PAPI automatically, "loaded" by the protocol context. So passing in a reference to the "created" labware or the "created" addressable area both are valid? Might be a weird blindspot in our architecture. Either way as an example, a generic drop_tip call goes to the fixed trash on a 2.13 protocol (it must because nothing else exists), and in the end picking up a tip and stopping a protocol then results in us moving to the fixedTrash addressable area and dropping tips there, so both references must be valid internally. Odd.
if ( | ||
("fixedTrash" in area) | ||
or ("movableTrash" in area) | ||
or ("WasteChute" in area) | ||
): |
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.
Moot if we're removing this, but the addressable areas have a semantic ableToDropTips
field. Let's use that instead of hard-coding these name substrings.
or ("WasteChute" in area) | ||
): | ||
# Append any and all instances/configurations of a MovableTrash or a WasteChute from this deck config | ||
loaded_disposal_locations.append(area) |
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.
Nitpicky Python style thing: we'd typically prefer to write this as a list comprehension instead of as an iterative loop.
loaded_disposal_locations = [
area for area in self._state.loaded_addressable_areas_by_name
if "fixedTrash" in area or "movableTrash" in area or "WasteChute" in area
]
post_run_hardware_state=post_run_hardware_state, | ||
drop_tips_after_run=drop_tips_after_run, |
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.
I think this is okay for this PR if we need to, but it's really looking to me now like we should move post_run_hardware_state
and drop_tips_after_run
into Config
to avoid all of these layers having to know about them.
# await self._cleanup_func( | ||
# error=error, | ||
# drop_tips_after_run=self._drop_tips_after_run, | ||
# post_run_hardware_state=self._post_hardware_run_state, | ||
# ) |
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.
Commented code here and elsewhere—is this left over from debugging?
class CleanupFunc(Callback): | ||
"""Expected cleanup function signature.""" | ||
|
||
def __call__(self, error: Optional[Exception]) -> Any: | ||
def __call__( | ||
self, | ||
error: Optional[Exception], | ||
) -> Any: |
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.
It looks like CleanupFunc
is unused now that set_cleanup_func()
spells out Callable[..., Awaitable[Any]]
itself.
Let's change TaskQueue.set_cleanup_func()
and TaskQueue.__init__()
to keep using CleanupFunc
instead of spelling it out themselves. This error
positional argument is a bit special and it's easy to accidentally omit if every place has to spell it out from scratch (set_cleanup_func()
omitted it, for example).
Overview
Closes RQA-1990 RQA-2022 RSS-252
This will currently remove the automatic drop tip behavior from do_stop_and_recover for Flex protocols. The decision is made in robot-server under the engine store when create_protocol_run is called. For the time being, this is determined based on Robot type. If the robot is a Flex, we set drop_tips_after_run to false, otherwise it is always true. This could be changed to a settings flagged triggered boolean in the future to enable-drop tip behavior on Flex (other changes would be needed covered below related to loaded trashes).
In do_stop_and_recover, if there are still tips attached it will home the gantry and the gripper, but it will not home the plunger. The private _drop_tip function called there now has a changed behavior utilizing move_to_addressable_area. Eventually we will want to update this to pull a dynamic list of loaded trashes, for which I have added a function that returns the list of loaded trash addressable areas within protocol engine. We cannot do this currently, as a trash (and any addressable area) is not loaded to the PE addressable area store unless it is actually used. We will need to change that behavior in the future. What this will eventually allow us to do is automatically drop the tip in a trash can so long as it exists within the deck configuration, regardless of if it is used. For now, only the OT2 supports this functionality (as only the OT2 should automatically drop tips) and is simply provided a hard-coded "fixedTrash" addressable area to move to for tip drop. If a Flex protocol reaches this point (which it should not, except in analysis), for the time being it will log a debug message that states "Flex protocols do not support automatic tip dropping at this time".
Test Plan
Run both OT2 and Flex protocols where the following happens:
Nothing else should occur in the protocol after this.
The expected results are as follows:
FLEX:
The robot should pickup and throw away tips as expected, then pick up the second set of tips and end the protocol. After this, it should home the gantry and the grippers but NOT the plungers. It will then end the protocol and prompt the user, on both the App and ODD that there are still tips attached, and allow them to do a manual drop (if they want) through the tip drop wizard.
OT2:
The robot should pickup and throw away tips as expected, then pick up the second set of tips and go to the trash and throw away the remaining tips automatically.
Risk assessment
On Flex protocols users are now expected to explicitly drop any remaining tips at the end of their protocols. This means that any protocol designed before this refactor will now be stuck with tips at the end if the users designed a protocol that would end with tips still on the pipette at the end.