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

feature(api, robot-server): Allow fixit commands to recover from an error #14908

Merged
merged 43 commits into from
Apr 22, 2024

Conversation

TamarZanzouri
Copy link
Contributor

@TamarZanzouri TamarZanzouri commented Apr 15, 2024

Overview

closes https://opentrons.atlassian.net/browse/EXEC-285.
Allow FIXIT command intent when waiting for recovery and allow resume from recovery.

Test Plan

Tested on dev server works as expected!

  • set error recovery ff to true OT_API_FF_enableErrorRecoveryExperiments=1
  • upload the following protocol - it should fail:
from opentrons import protocol_api

requirements = {"robotType": "Flex", "apiLevel": "2.17"}


def run(protocol: protocol_api.ProtocolContext) -> None:
    protocol.load_trash_bin("A3")

    tip_rack = protocol.load_labware("opentrons_flex_96_tiprack_50ul", "C2")
    reservoir = protocol.load_labware(
        "opentrons_96_wellplate_200ul_pcr_full_skirt", "C3"
    )
    pipette = protocol.load_instrument(
        "flex_1channel_50", mount="left", tip_racks=[tip_rack]
    )

    for source, dest in zip(reservoir.columns()[0], reservoir.columns()[1]):
        pipette.pick_up_tip()
        pipette.move_to(source.top())
        pipette.move_to(dest.top())
        pipette.drop_tip()
  • GET /run and make sure it is in AWAITING_RECOVERY mode.
  • POST a few commands with intent=FIXIT
  • make sure commands are executed.
  • when done, issue a resumeFromRecovery action. # need to check if this is true

Review requests

Changes make sense? did I miss anything?

Risk assessment

Medium. changing add_command in pe and create_command in router.

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 very good structurally, but I think we want a couple defensive programming enhancements and we should enforce that failed command ids only exist for fixits

@@ -209,7 +223,7 @@ async def create_run_command(
command_create = request_body.data.copy(update={"intent": command_intent})

try:
command = protocol_engine.add_command(command_create)
command = protocol_engine.add_command(command_create, failed_command_id)

except pe_errors.SetupCommandNotAllowedError as e:
raise CommandNotAllowed.from_exc(e).as_error(status.HTTP_409_CONFLICT)
Copy link
Member

Choose a reason for hiding this comment

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

should add explicit handling and 400 errors if this was a non-fixit command that specified a failed command id

Copy link
Contributor

@SyntaxColoring SyntaxColoring 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 so far!

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.31%. Comparing base (8f50b08) to head (364c42d).
Report is 94 commits behind head on edge.

❗ Current head 364c42d differs from pull request most recent head 775bb8a. Consider uploading reports for the commit 775bb8a to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #14908       +/-   ##
===========================================
+ Coverage   67.50%   81.31%   +13.80%     
===========================================
  Files        2521      118     -2403     
  Lines       72090     4029    -68061     
  Branches     9311        0     -9311     
===========================================
- Hits        48666     3276    -45390     
+ Misses      21228      753    -20475     
+ Partials     2196        0     -2196     
Flag Coverage Δ
api ?
app ?
components ?
g-code-testing 92.43% <ø> (ø)
hardware ?
hardware-testing ?
labware-library ?
notify-server ?
ot3-gravimetric-test ?
protocol-designer ?
react-api-client ?
robot-server ?
shared-data 76.11% <ø> (+0.79%) ⬆️
step-generation ?
system-server ?
update-server ?
usb-bridge ?

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

see 2410 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 really good except for the one sneaky little print!

robot-server/robot_server/runs/router/commands_router.py Outdated Show resolved Hide resolved
robot-server/robot_server/runs/router/commands_router.py Outdated Show resolved Hide resolved
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 good to me!

@TamarZanzouri TamarZanzouri changed the title Exec 285 add fixit commands feature(api, robot-server): Allow fixit commands to recover from an error Apr 19, 2024
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

A few small things. And I think CI tests are failing, maybe because of the raise in pickUpTip. Otherwise, looks great, thanks!

shared-data/fixture/schemas/1.json Outdated Show resolved Hide resolved
api/src/opentrons/protocol_engine/commands/command.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_engine/commands/pick_up_tip.py Outdated Show resolved Hide resolved
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

LGTM if CI passes, thank you!

@TamarZanzouri TamarZanzouri merged commit 2d57126 into edge Apr 22, 2024
41 checks passed
@TamarZanzouri TamarZanzouri deleted the EXEC-285-add-fixit-commands branch April 22, 2024 19:44
y3rsh added a commit that referenced this pull request Apr 23, 2024
* edge: (194 commits)
  fix(app): clone run with RTPs from HistoricalProtocolRun (#14959)
  fix(api): Filter out `air_gap()` calls as higher-order commands (#14985)
  fix(app): fix infinitely re-rendering/never rendering firmware success toasts (#14981)
  feat(api): add option to ignore different tip presence states (#14980)
  feat(opentrons-ai-client) add input textbox to container (#14968)
  fix(app): add robotSerialNumber to proceedToRun event (#14976)
  fix(api): remove homing patch fix for right mount when a 96-channel is attached (#14975)
  feat(api-client,app,react-api-client): upload splash logo from desktop app (#14941)
  fix(robot-server): notify /runs when a non-current run is deleted (#14974)
  feature(api, robot-server): Allow fixit commands to recover from an error (#14908)
  feat(hardware-testing): enable multi sensor processing in liquid probe (#14883)
  fix(app): prevent "run again" banner from rendering after navigating away from the current run (#14973)
  refactor(components): refactor roundtab stories (#14956)
  refactor(protocol-designer): assign module slot in createFileWizard instead of modal (#14951)
  fix(app, api-client): fix choose protocol slideout issue (#14949)
  refactor(protocol-designer): tip position modal max values round down (#14972)
  feat(app): add tiprack selection step to quick transfer flow (#14950)
  ci(shared-data): install dependencies in workflow (#14958)
  fix(components): fix icon stories (#14969)
  feat(opentrons-ai-client): introduce react-markdown to chat display component (#14965)
  ...
@shlokamin shlokamin restored the EXEC-285-add-fixit-commands branch April 25, 2024 23:08
@shlokamin shlokamin deleted the EXEC-285-add-fixit-commands branch April 25, 2024 23:10
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.

None yet

3 participants