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

fix(api): fix run cancellation freezing on legacy core #14312

Merged
merged 6 commits into from
Jan 30, 2024

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Jan 11, 2024

Closes RESC-184, RQA-2273

Overview

Successful cancellation of a protocol run using the legacy core relies on making sure that all OT2 robot movements (and delays) happen inside tasks that can be cancelled when a protocol cancel request comes in. But we seem to have missed a few robot movements which were neither wrapped inside cancellable tasks, nor were checking the execution manager status to make sure the robot is okay to be moved.

This PR aims to fix that by putting all such stray direct calls to smoothie inside cancellable tasks.
Right now I've only fixed the direct calls in ot2api.drop_tip() and tested to confirm it fixes the freezing problem. I will be going through rest of the ot2 api to fix any other calls if they exist.
Update: home_plunger also was not being taskified so fixed that as well.

Test Plan

  • Try cancelling a protocol exactly when dropping tip (when plunger moves down) and verify that the cancellation completes and the protocol run finishes.
  • Try cancelling protocol when plunger is homing in the following sample protocol and verify that the cancellation completes and the protocol run finishes.
  • Sample protocol:
metadata = {"apiLevel": "2.11"}

def run(protocol_context):

	p20racks = [protocol_context.load_labware('opentrons_96_tiprack_20ul', '1')]
	p20 = protocol_context.load_instrument('p20_single_gen2', 'right', tip_racks=p20racks)
        dummy_lw = protocol_context.load_labware('opentrons_96_tiprack_20ul', '2')

	p20.pick_up_tip()
	p20.drop_tip()
        p20.move_to(dummy_lw.wells()[0].center())
        p20.home_plunger()

Changelog

  • added the missing decorators to calls to backend:
    • fast homing in drop_tip
    • home_plunger method
  • Also updated type annotations for the wait_for_running decorator implementation!
    • changed to using ParamSpecs and removed relevant typeVars
    • removed the overloads because they don't seem necessary but please check my logic on that

Review requests

  • I went through all calls for backend movement and made sure they all are called in cancellable tasks but would definitely appreciate another pair of eyes checking. Some calls are actually inside nested tasks which shouldn't cause any problems but please raise any concerns if you have. (One such instance has been existing for a while- taskified _move() is called inside taskified home() )
  • Make sure my changes to type annotations are correct.

Risk assessment

Medium. Fixes a bug but also re-arranges a few methods decorated behind wait_for_running. For this reason, we should review and QA this thoroughly

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1624a04) 68.06% compared to head (995d243) 68.09%.
Report is 24 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14312      +/-   ##
==========================================
+ Coverage   68.06%   68.09%   +0.03%     
==========================================
  Files        1621     2503     +882     
  Lines       54710    71589   +16879     
  Branches     4087     9113    +5026     
==========================================
+ Hits        37238    48750   +11512     
- Misses      16817    20711    +3894     
- Partials      655     2128    +1473     
Flag Coverage Δ
app 65.61% <ø> (+26.89%) ⬆️
components 48.85% <ø> (ø)
g-code-testing 96.48% <ø> (ø)
labware-library 41.10% <ø> (ø)
protocol-designer 35.30% <ø> (ø)
step-generation 86.90% <ø> (ø)

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

Files Coverage Δ
api/src/opentrons/hardware_control/api.py 82.38% <ø> (-0.05%) ⬇️
...rc/opentrons/hardware_control/execution_manager.py 89.55% <ø> (-0.86%) ⬇️

... and 885 files with indirect coverage changes

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks great but we might have to dig deep and make sure we're not causing reentrancy issues.

@sanni-t sanni-t force-pushed the api-fix_run_cancellation_freezing branch from ca0f462 to ba2f1ff Compare January 26, 2024 23:18
@sanni-t sanni-t marked this pull request as ready for review January 29, 2024 21:25
@sanni-t sanni-t requested a review from a team as a code owner January 29, 2024 21:25
@sanni-t
Copy link
Member Author

sanni-t commented Jan 30, 2024

Tested the above protocol with API versions 2.11 & 2.15 and verified that the protocol cancels cleanly when:

  • dropping tip
  • homing plunger during home_plunger()
  • homing plungers during home() (to verify nested task cancellation)

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@@ -906,21 +907,21 @@ def engaged_axes(self) -> Dict[Axis, bool]:
return self.get_engaged_axes()

async def disengage_axes(self, which: List[Axis]) -> None:
# Should this have wait_for_running decorator too?
Copy link
Member Author

@sanni-t sanni-t Jan 30, 2024

Choose a reason for hiding this comment

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

Talked with Seth about this and the conclusion is that we should not because disengaging is a quick and harmless action and we shouldn't wait for running for everything without a good reason.

@sanni-t sanni-t merged commit 207964f into edge Jan 30, 2024
44 of 45 checks passed
ncdiehl11 pushed a commit that referenced this pull request Feb 1, 2024
* communicate with smoothie via cancellable tasks during drop_tip movement and home_plunger
@rickwierenga
Copy link

Import ParamSpec from typing breaks compatibility with Python 3.8 and 3.9: https://docs.python.org/3/library/typing.html#typing.ParamSpec. This means OT is now only compatible with Python 3.10.

Can you import it from typing_extensions on 3.8/9 to restore compatibility? I can make a PR if you want.

rickwierenga added a commit to PyLabRobot/pylabrobot that referenced this pull request Mar 7, 2024
rickwierenga added a commit to PyLabRobot/pylabrobot that referenced this pull request Mar 7, 2024
@sanni-t sanni-t deleted the api-fix_run_cancellation_freezing branch July 15, 2024 21:37
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