refactor(robot-server): retrieve state of previous runs #8676
Conversation
mcous
added
the
robot-svcs
Codecov Report
@@ Coverage Diff @@
## edge #8676 +/- ##
==========================================
+ Coverage 74.76% 74.77% +0.01%
==========================================
Files 1731 1731
Lines 46511 46538 +27
Branches 4666 4666
==========================================
+ Hits 34775 34800 +25
- Misses 10932 10934 +2
Partials 804 804
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mcous
changed the title
| if not self.engine.state_view.commands.get_is_stopped(): | ||
| raise EngineConflictError("Current engine is not stopped.") |
There was a problem hiding this comment.
If I'm reading this right, if a run stops due to an internal error or failed Protocol Engine command, the client will still need to issue a stop action on it before attempting to create a new run. Is that correct?
There was a problem hiding this comment.
The runner always stops the engine at the conclusion of the protocol file run, even if that run bails out early due to error. So this shouldn't be a problem
| except EngineMissingError: | ||
| pass | ||
| else: | ||
| if not engine_state.commands.get_is_stopped(): |
There was a problem hiding this comment.
See prior comment about whether .get_is_stopped() is the right thing to check here.
| current: bool = Field( | ||
| ..., | ||
| description="Whether this Run is currently controlling the robot", | ||
| ) |
There was a problem hiding this comment.
How about:
Whether this run is currently occupying the robot.
"Currently occupying the robot" means either:
- This run is actively making the robot move.
- This run is pending physical setup or teardown, with the user's help.
At any given time, there's at most one current run.
For me, an important part of the mental model of a run is that it doesn't represent robot control. It's represents some fuzzier, broader notion of what the user "is doing with" the robot. Still including robot control, but now also including physically setting up and tearing down the deck, and reviewing the run results.
Also, my understanding from App+UI is that they're relying on HTTP calibration sessions (under /sessions) being usable while a run is current, but not "active." In that case, I wouldn't say it's the run that's "currently controlling the robot."
There was a problem hiding this comment.
For me, an important part of the mental model of a run is that it doesn't represent robot control. It's represents some fuzzier, broader notion of what the user "is doing with" the robot.
I don't think that distinction is worth the extra reading. If the run is both "current" and "finished", it still has "control" over the robot insofar as it contains the most accurate state representation available of what the robot physically looks like. No other run is allowed to move the robot, so it's still in "currently in control"
Also, my understanding from App+UI is that they're relying on HTTP calibration sessions (under /sessions) being usable while a run is current, but not "active." In that case, I wouldn't say it's the run that's "currently controlling the robot."
For the 5.0 release, those API's should be considered private, deprecated, and pending removal.
At any given time, there's at most one current run.
Good call, this is important to call out
|
Merged for speed and because I think I've addressed all comments. Fast following to address #8594, can address anything outstanding there |
Overview
This PR allows a new run to be created after a previous run has stopped. It adds the
currentflag to the Run model, but leaves the rest of #8594 alone for a future PR (adding entry tolinks, allowingcurrent: falseto be patched).Closes #8470
Changelog
Review requests
I've updated the Postman collection to hit
/runsinstead of/sessionsHappy path:
Things to ensure:
current: falseRisk assessment
Low. Feature flagged, on it's way to a beta release for full validation