refactor(api): Split UpdateCommandAction #14726
Conversation
These are the easy ones, since they only care about command completions.
executor
legacy
The exact sequence is unclear to me, but it's something like the PAPI thread trying to synthesize a running command while the engine is actually running the initial home. I don't know why this apparent concurrent robot control hasn't caused physical problems with robot motion.
SyntaxColoring
force-pushed
the
split_updatecommandaction
branch
from
c2ccc93 to
b5c54af
Compare
|
After looking at it I think these are good concurrency hazards to fix and I think we can definitely do it especially if we're open to alternative means of fixing things. The home command looks pretty solved, and I think with the alternate intent we can handle the legacy python pretty well. |
Simplify note update logic so it only overwrites, never appends. Now that UpdateCommandAction has been replaced by terminal SucceedCommandAction/FailCommandAction, appending can't happen in practice. Refactor tests to test command notes as part of general success/failure, to save a bunch of test boilerplate.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14726 +/- ##
==========================================
- Coverage 67.19% 67.18% -0.02%
==========================================
Files 2495 2495
Lines 71563 71538 -25
Branches 9076 9076
==========================================
- Hits 48088 48063 -25
Misses 21330 21330
Partials 2145 2145
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This keeps succeeded commands consistent with failed commands.
SyntaxColoring
force-pushed
the
split_updatecommandaction
branch
from
ca56e85 to
e284a7a
Compare
SyntaxColoring
force-pushed
the
split_updatecommandaction
branch
from
e284a7a to
8c6f65d
Compare
| """Mark a given command as succeeded. | ||
|
|
||
| At the time of dispatching this action, the command must be running. | ||
| """ | ||
|
|
||
| command: Command |
There was a problem hiding this comment.
when UpdateCommandAction was the everything-doer, having the command embedded made sense, but now having it in SucceedCommandAction looks a little weirder amongst all its siblings that only reference the id. Does it maybe make sense to have SucceedCommandAction carry notes: List[CommandNote], result: CommandResult, private_result: CommandPrivateResult independently?
There was a problem hiding this comment.
I agree with @sfoster1 we might want to add these to the SucceedCommandAction. I agree UpdateCommandAction had too many responsibilities
There was a problem hiding this comment.
Yep, that makes sense.
I’m pretty sure some of the states stores are reading command.params, too. So to replace that, we’d either need to add params to CompleteCommandAction, or give every state store access to the CommandView so they can retrieve the params themselves.
| """Mark a given command as failed. | ||
|
|
||
| At the time of dispatching this action, the command must be running. | ||
| """ | ||
|
|
||
| command_id: str |
There was a problem hiding this comment.
if we don't want to make SucceedCommandAction carry everything independently, maybe we should make FailCommandAction embed the command too. Both of these actions are going to cause pretty equivalent modifications to the command dataclass anyway.
api/src/opentrons/protocol_engine/execution/command_executor.py
Outdated
Show resolved
Hide resolved
|
|
||
| self._action_dispatcher.dispatch( | ||
| FailCommandAction( | ||
| error=error, | ||
| command_id=command_id, | ||
| error_id=self._model_utils.generate_id(), | ||
| failed_at=self._model_utils.get_timestamp(), | ||
| notes=note_tracker.get_notes(), |
There was a problem hiding this comment.
these notes changes means that command notes from before the beginning of execution will be lost. this might be fine, but we should note (hehe) it somewhere
The idea was to keep it I don’t know if this was a good idea. |
Overview
Closes EXEC-340.
Changelog
Delete the monolithic
UpdateCommandActionand replace it with new, distinctRunCommandActionandSucceedCommandActioncommand transitions. So we end up with this:stateDiagram-v2 [*] --> queued: QueueCommandAction queued --> running: RunCommandAction running --> succeeded: SucceedCommandAction running --> failed: FailCommandActionCompare with the old state diagram, where you could use
UpdateCommandActionto jump around and skip over states.Use
asserts to enforce the preconditions of the state transitions. (For example, you can't run a command when a different one is already running.)Make sure all completed commands have a
noteslist. If there are no notes, make it an empty list. Formerly,noteswould not be present (Nonein Python, omitted in JSON) if there were no notes. Now, it's sort of likecompletedAt, whereNonemeans "hasn't happened yet."This entails a million billion test updates, unfortunately.
This seemed like it would simplify things, but in hindsight, it doesn't make a big difference. I'm happy to undo this if anyone wants.
Update the
LegacyContextPlugin. This was the really annoying part. Before, it emittedUpdateCommandActions to createdsucceededandfailedcommands, skipping overqueuedandrunning. Now, because we're being stricter, it has to explicitly transition each command throughqueuedandrunning.This bumps into a couple of ugly scary things, which I'll describe in separate threads below.
Test plan
Review requests
See my review comments.
This diff got huge, sorry. Most of the files only had trivial changes.
Risk analysis
Medium. This touches some sensitive and messy code having to do with legacy (PAPIv≤2.13, JSONv≤5) protocols. Other than that, the changes are simple.