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

feat(robot-server,api): Add the skeleton for a new complete-recovery run action #14674

Merged
merged 10 commits into from
Mar 18, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Mar 15, 2024

Overview

Closes EXEC-300.

Changelog

  • Add a new action type for POST /runs/{id}/actions: complete-recovery.
  • Connect it to ProtocolEngine and update unit tests along the way.
  • For now, have ProtocolEngine raise a NotImplementedError whenever it encounters it. The actual implementation will be done in future PRs.
  • Update TypeScript bindings.

Test Plan

I've tested with Postman that I can post a complete-recovery action to a run, and it raises the expected NotImplementedError.

Review requests

Do we like the name complete-recovery for this?

Risk assessment

Low.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.33%. Comparing base (c68027d) to head (6c2e32c).
Report is 10 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #14674   +/-   ##
=======================================
  Coverage   67.33%   67.33%           
=======================================
  Files        2485     2485           
  Lines       71389    71389           
  Branches     9032     9032           
=======================================
  Hits        48071    48071           
  Misses      21173    21173           
  Partials     2145     2145           
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

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

Files Coverage Δ
...i/src/opentrons/protocol_engine/actions/actions.py 100.00% <ø> (ø)
...i/src/opentrons/protocol_engine/protocol_engine.py 100.00% <ø> (ø)
...pi/src/opentrons/protocol_engine/state/commands.py 99.39% <ø> (ø)
...i/src/opentrons/protocol_runner/protocol_runner.py 100.00% <ø> (ø)
robot-server/robot_server/runs/action_models.py 100.00% <ø> (ø)
...-server/robot_server/runs/router/actions_router.py 100.00% <ø> (ø)

@SyntaxColoring SyntaxColoring force-pushed the error_recovery_action branch from 0c35a8d to 3b805c9 Compare March 15, 2024 16:31
@SyntaxColoring SyntaxColoring marked this pull request as ready for review March 15, 2024 16:31
@SyntaxColoring SyntaxColoring requested review from a team as code owners March 15, 2024 16:31
@SyntaxColoring SyntaxColoring requested review from mjhuff and a team and removed request for a team March 15, 2024 16:31
@SyntaxColoring SyntaxColoring changed the title feat(robot-server,api): Add skeleton for complete-recovery run action feat(robot-server,api): Add the skeleton for a new complete-recovery run action Mar 15, 2024
@SyntaxColoring SyntaxColoring requested review from a team and TamarZanzouri and removed request for a team March 15, 2024 16:32
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.

modulo the correction on the state hnadling that Derek pointed out, looks good. I'm interested in your reasoning about two things:

  1. why call this complete-recovery as opposed to some resume thing? To me it carries the implication that the engine will be doing something to complete the recover, as opposed to relying on the client to have done something and then naively resuming
  2. in fact now that i'm looking at it, why not have this be PlayAction? That moves us towards a system where we have fewer, more common keywords that do different things based on state - a state machine where logic is biased to "what state am I in".

Like I said, it looks good to merge either way, but I'm interested in hearing your thoughts.

@SyntaxColoring
Copy link
Contributor Author

SyntaxColoring commented Mar 18, 2024

why call this complete-recovery as opposed to some resume thing?

No reasoning. resume-from-recovery does sound clearer to me. I'll switch to that unless anyone has a better suggestion.

in fact now that i'm looking at it, why not have this be PlayAction?

A couple of loose reasons:

  • Depending on how pause-on-open-door shakes out, we might need PlayAction to resume the recovery sequence from a door pause without exiting the recovery sequence.
  • We don't yet have a server-enforced way of preventing clients from stepping on each other's toes, like any kind of remote lock or HTTP conditional request. Given that, having HTTP actions that are only valid in certain contexts is a cheap (but admittedly half-assed) way to decrease the odds of two clients accidentally interacting in a dangerous way.

@SyntaxColoring SyntaxColoring merged commit 935e84d into edge Mar 18, 2024
27 checks passed
@SyntaxColoring SyntaxColoring deleted the error_recovery_action branch March 18, 2024 19:50
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