feat(api): command executors can add notes #14652
Conversation
This PR adds an interface for command executors to add notes to the command that they're executing, succeed or fail, and uses that interface to implement an example note warning of aspirate volume rounding. The interface is done by callbacks from the command implementations to the command executor that is executing each command. Implementations, or the functions they call, are responsible for creating the CommandNote instances. The executor just gathers a list of commands and then issues an UpdateCommandAction with the new list. If there are already notes on the command before execution, the new ones are appended. While this doesn't seem necessary right now, we'll want this behavior generically when we have notes also coming from hardware. One thing that's a little annoying about this interface is that the callbacks have to be threaded through all the way to whatever wants to add a note. For instance, in the example aspirate-rounded note, we have to pass the callback all the way into the pipetting executor. An alternative would be to have an interface that can add notes on one of the state instances that can be called from anything that can access the state instance. The thing is, that function won't have the context to know when commands are currently executing or are done executing without having some sort of state machine that only looks at UpdateCommandActions from elsewhere. That code would be pretty ugly. The alternative there would be to issue a new UpdateCommandAction for each note; this would have race condition issues if it provided new values for the notes list. The alternative _there_ would be a new action called like AddCommandNoteAction that carries the attention of adding to the notes list along with it. Of course, that command is not idempotent. This interface works well enough for now that I think we should roll with it, and feel free to change it later. Closes EXEC-291
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14652 +/- ##
=======================================
Coverage 67.29% 67.29%
=======================================
Files 2485 2485
Lines 71496 71496
Branches 9085 9085
=======================================
Hits 48116 48116
Misses 21224 21224
Partials 2156 2156
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Yeah, this fundamental interface looks good to me. I agree that we don't want command executors to add notes by emitting Re the annoyance of having to pass the callback all the way into the pipetting executor: it does seem doable to me to have something "globally" accessible that knows what the current note target is. It can delegate to I agree that this interface is good enough that we should roll with it. |
|
Yeah I honestly don't love the idea of a global callback like that because it gets pretty nonlocal |
|
Spoke privately with Seth about this, I kind of prefer we use |
api/tests/opentrons/protocol_engine/execution/test_pipetting_handler.py
Outdated
Show resolved
Hide resolved
| class CommandNoteTrackerProvider(Protocol): | ||
| """The correct shape for a function that provides a CommandNoteTracker. | ||
|
|
||
| This function will be called by the executor once for each call to execute(). | ||
| It is mostly useful for testing harnesses. | ||
| """ | ||
|
|
||
| def __call__(self) -> CommandNoteTracker: | ||
| """Provide a new CommandNoteTracker.""" | ||
| ... | ||
|
|
||
|
|
||
| class _NoteTracker(CommandNoteTracker): | ||
| def __init__(self) -> None: | ||
| self._notes: List[CommandNote] = [] | ||
|
|
||
| def __call__(self, note: CommandNote) -> None: | ||
| self._notes.append(note) | ||
|
|
||
| def get_notes(self) -> List[CommandNote]: | ||
| return self._notes | ||
|
|
||
|
|
There was a problem hiding this comment.
For simplicity, I'd be down for CommandExecutor to just construct each CommandNoteTracker directly, in a way that can't be overridden by CommandExecutor's unit tests.
The downside is we couldn't test that it instantiates a separate tracker for each command, and I'm fine with that.
There was a problem hiding this comment.
yeah that's a good idea, will reduce the diff
There was a problem hiding this comment.
@SyntaxColoring mind if we put this off actually? the way those tests are currently implemented, we'd have to change a bunch of stuff to get access to the test command constructor arguments
api/src/opentrons/protocol_engine/execution/command_executor.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Max Marrone <[email protected]>
This PR adds an interface for command executors to add notes to the command that they're executing, succeed or fail, and uses that interface to implement an example note warning of aspirate volume rounding. The interface is done by callbacks from the command implementations to the command executor that is executing each command. Implementations, or the functions they call, are responsible for creating the CommandNote instances. The executor just gathers a list of commands and then issues an UpdateCommandAction with the new list. If there are already notes on the command before execution, the new ones are appended. While this doesn't seem necessary right now, we'll want this behavior generically when we have notes also coming from hardware. One thing that's a little annoying about this interface is that the callbacks have to be threaded through all the way to whatever wants to add a note. For instance, in the example aspirate-rounded note, we have to pass the callback all the way into the pipetting executor. An alternative would be to have an interface that can add notes on one of the state instances that can be called from anything that can access the state instance. The thing is, that function won't have the context to know when commands are currently executing or are done executing without having some sort of state machine that only looks at UpdateCommandActions from elsewhere. That code would be pretty ugly. The alternative there would be to issue a new UpdateCommandAction for each note; this would have race condition issues if it provided new values for the notes list. The alternative _there_ would be a new action called like AddCommandNoteAction that carries the attention of adding to the notes list along with it. Of course, that command is not idempotent. This interface works well enough for now that I think we should roll with it, and feel free to change it later. Closes EXEC-291 --------- Co-authored-by: Max Marrone <[email protected]>
This PR adds an interface for command executors to add notes to the command that they're executing, succeed or fail, and uses that interface to implement an example note warning of aspirate volume rounding.
The interface is done by callbacks from the command implementations to the command executor that is executing each command. Implementations, or the functions they call, are responsible for creating the CommandNote instances. The executor just gathers a list of commands and then issues an UpdateCommandAction with the new list. If there are already notes on the command before execution, the new ones are appended. While this doesn't seem necessary right now, we'll want this behavior generically when we have notes also coming from hardware.
One thing that's a little annoying about this interface is that the callbacks have to be threaded through all the way to whatever wants to add a note. For instance, in the example aspirate-rounded note, we have to pass the callback all the way into the pipetting executor. An alternative would be to have an interface that can add notes on one of the state instances that can be called from anything that can access the state instance. The thing is, that function won't have the context to know when commands are currently executing or are done executing without having some sort of state machine that only looks at UpdateCommandActions from elsewhere. That code would be pretty ugly. The alternative there would be to issue a new UpdateCommandAction for each note; this would have race condition issues if it provided new values for the notes list. The alternative there would be a new action called like AddCommandNoteAction that carries the attention of adding to the notes list along with it. Of course, that command is not idempotent.
This interface works well enough for now that I think we should roll with it, and feel free to change it later.
Still todo
testing (once todos are todone)
- [ ] on an executing ot-2, do the samewill test this in followupsCloses EXEC-291