refactor(api): Monitor PAPI commands, not PE commands #13724
Conversation
SyntaxColoring
added
api
Resolve conflicts in api/src/opentrons/protocol_engine/state/state.py.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.0.1 #13724 +/- ##
====================================================
Coverage 71.26% 71.26%
====================================================
Files 2427 2427
Lines 68300 68300
Branches 8000 8000
====================================================
Hits 48676 48676
Misses 17743 17743
Partials 1881 1881
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
This looks good, local tests all pass as well. One comment to note for the future of this and things related to it, app team seems interested in the notion of "generic annotations" inside PE going forward (so wrapping multiple aspirations under a parent transfer command at the highest level of the log and only having the individual aspirates appear once you looked into the log contents of that transfer, if my understanding is correct). So long as this supports that kind of "readability" change in the future I see no reason why, architecturally, this wouldn't be acceptable.
* Remove broker from ProtocolEngine. This partially reverts commit 24eb3c1 ("fix(api): New protocols print a JSON "run log" from opentrons_execute and opentrons.execute.execute() (#13629)"). * Add broker to ProtocolRunner. * Update execute.py. * Update test_execute.py. * Further deduplicate test_execute.py.
Overview
Closes RSS-320 for Python protocols (but not for JSON protocols). Also moves us towards other things in RSS-318.
Test Plan
No manual tests needed. This is covered by unit tests now, and it's going to be covered by other manual tests as work continues on RSS-318.
If you do test it manually, note that there's at least one unrelated bug that causes the run log text to be wrong: RSS-368.
Changelog
#13629 added a mechanism to monitor the progress of Protocol Engine commands. It turns out that that isn't a great approach for RSS-318.
When
opentrons_simulateandopentrons_executeprint commands, we want them to preserve the structure of the original protocol file: transfers contain aspirates and dispenses, and so on. If we only see a flat sequence of Protocol Engine commands, we lose that structure.Also, we don't have any mechanism right now for converting raw Protocol Engine commands into human-readable strings. That's RSS-368, and we will probably need it for JSON protocols, but we would prefer to not have to deal with it to get
opentrons_simulateto work for Python protocols again.So, instead, we go back to how things worked in our olden RPC times. Instead of a flat sequence of Protocol Engine commands,
opentrons_simulateandopentrons_executewill see a nested sequence of Python Protocol API commands.Specifically, we:
ProtocolEngine.state_update_brokerand friends. This reverts most of fix(api): New protocols print a JSON "run log" from opentrons_execute and opentrons.execute.execute() #13629.ProtocolRunner.broker.opentrons.commandsTypedDicts. This will probably change eventually.Review requests
Is this a good approach, architecturally?
Risk assessment
Low, but there is a tradeoff going forward.
Although these messages give us human readability by default, they are not exhaustive by default.
ProtocolContext,InstrumentContext, andModuleContextneed to explicitly publish activity that they want to print. We will need to make sure they do this for the new commands likemove_labware(). Let's track that as RSS-377.