-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(api): New protocols print a JSON "run log" from opentrons_execute and opentrons.execute.execute() #13629
fix(api): New protocols print a JSON "run log" from opentrons_execute and opentrons.execute.execute() #13629
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.0.1 #13629 +/- ##
=======================================================
- Coverage 71.29% 71.29% -0.01%
=======================================================
Files 2427 2427
Lines 68229 68226 -3
Branches 7968 7968
=======================================================
- Hits 48647 48644 -3
Misses 17715 17715
Partials 1867 1867
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ab8131c
to
d0b7d86
Compare
@property | ||
def state_update_broker(self) -> ReadOnlyBroker[None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things I do not like about this:
- All of the footguns described in the docstring.
- It's callback-based, which can be spaghetti.
Things I like about this:
- Making the mechanism this generic—as opposed to something more specific like
wait_for_commands()
—lets us build more logic outside ofProtocolEngine
. This is really nice for curbingProtocolEngine
complexity. - This is is roughly compatible with my experiments in notifications for the HTTP API. I'm expecting various internal classes like
ProtocolEngine
andAnalysisStore
to grow interfaces that let callers monitor them for changes in a coarse-grained way. Then, higher-level endpoints filter those notifications down and return only what the client actually cares about. - Despite callbacks being ugly, they are suited to the actual requirements of RSS-238. The ultimate user-facing interface is callback-based. If we want async generators later, it's easier to build those atop callbacks than to do it the other way around.
9d3e5d6
to
c85888b
Compare
10aa56d
to
8801356
Compare
Do test shenanigans to get this to work with Decoy.
…ine.command_monitor.
This makes the unit tests for `monitor_commands()` a little bit less horrifying because we don't have to do tricks to capture the callback. We can just supply it a real broker and feed it messages the real way. Also, add docstrings to the `ProtocolEngine` interfaces.
8801356
to
1a8c1f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I think this is the most straightforward but sensible way to add this feature to PE-run CLI.
last_running_id: typing.Optional[str] = None | ||
|
||
def handle_state_update(_message_from_broker: None) -> None: | ||
nonlocal last_running_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is new to me :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...Python. 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jeremy. I like this approach. clean and straightforward. nice work!
… and opentrons.execute.execute() (#13629)
* 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.
* 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-283.
When running PAPIv≥2.14 or JSONv≥6 protocols,
opentrons_execute
will now output a "run log" to stdout like this:And the same objects are exposed by
opentrons.execute.execute()
'semit_runlog
callback.Test Plan
python3 -m opentrons.execute path_to_some_protocol.py
with a protocol that specifies"apiLevel": "2.15"
.opentrons_execute path_to_some_protocol.py
with a protocol that specifies"apiLevel": "2.14"
or above.Changelog
Add a new helper to allow monitoring a
ProtocolEngine
for ongoing commands.This works by subscribing to every state update and seeing if the currently running command has changed since the last one. If it has, we emit a "command running" or "command no longer running" event. Conceptually, that event structure matches the "before" and "after" messages that the Python Protocol API has long published.
Wire that helper up to the
opentrons_execute
CLI and theopentrons.execute.execute()
function.opentrons.execute.execute()
has a quirky interface that we need to preserve for now, so this takes some shoehorning. For this ticket, I'm not concerned about how the protocol's activity gets printed—just that it gets printed somehow. So I'm doing that by reporting each command as a "comment" where the message is the command's JSON text. There's obviously a lot of room for improvement here. RSS-320 tracks making these messages more nicely human-readable. And if there's a use case for it, we could also go the other way, officially exposing the command in a machine-readable way.Review requests
Risk assessment
Low. This is tricky to test, but it's also fairly nonintrusive.