-
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
feature(api, robot-server): Allow fixit commands to recover from an error #14908
Conversation
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 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) |
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.
should add explicit handling and 400
errors if this was a non-fixit command that specified a failed command id
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 so far!
api/tests/opentrons/protocol_engine/state/test_command_history.py
Outdated
Show resolved
Hide resolved
dont pick setup when in waiting for recovery
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
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.
Looks really good except for the one sneaky little print
!
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!
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.
A few small things. And I think CI tests are failing, maybe because of the raise
in pickUpTip
. Otherwise, looks great, thanks!
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 CI passes, thank you!
* 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) ...
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!
OT_API_FF_enableErrorRecoveryExperiments=1
/run
and make sure it is inAWAITING_RECOVERY
mode.intent=FIXIT
Review requests
Changes make sense? did I miss anything?
Risk assessment
Medium. changing
add_command
in pe andcreate_command
in router.