feat(api): Do not enqueue json commands on protocol load #14759

TamarZanzouri · 2024-03-29T20:20:43Z

Overview

closes https://opentrons.atlassian.net/browse/EXEC-352.
first step towards fixit commands. do not enqueue json protocol commands.

Test Plan

Tested with Json protocols and Postman:

Make sure loading a protocol and executing it are happening within order.
Make sure get run /commands returning the list properly with successful commands.
Make sure loading a failed protocol should fail the run and fail the command.
Make sure get run /commands for failed runs the list of commands, last command being the failed command.
Fixed e2e test to comply with these changes

Changelog

Do no enqueue commands in PE for Json command upon load.
Execute commands one by one when run get started - same way we do for python protocols.

Review requests

Changes make sense?
GET run /commands will not return the full list of commands if the run did not start - its a change we are doing to make json protocols run like python protocols. are we ok with this?

Risk assessment

Medium. need to do smoke tests for Json protocols and make sure these changes do not affect anything.

codecov · 2024-03-29T20:41:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.19%. Comparing base (0fbb4c7) to head (123f313).
Report is 41 commits behind head on edge.

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #14759      +/-   ##
==========================================
+ Coverage   67.17%   67.19%   +0.02%     
==========================================
  Files        2495     2495              
  Lines       71483    71405      -78     
  Branches     9020     8992      -28     
==========================================
- Hits        48020    47984      -36     
+ Misses      21341    21305      -36     
+ Partials     2122     2116       -6

Flag	Coverage Δ
g-code-testing	`92.43% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...i/src/opentrons/protocol_runner/protocol_runner.py	`100.00% <ø> (ø)`

... and 2 files with indirect coverage changes

sfoster1

Looks great to me! Nice work.

robot-server/tests/integration/http_api/runs/test_json_v6_protocol_run.tavern.yaml

api/tests/opentrons/protocol_runner/test_protocol_runner.py

SyntaxColoring · 2024-04-01T15:26:44Z

api/src/opentrons/protocol_runner/protocol_runner.py

+    async def _add_command_and_execute(self) -> None:
+        for command in self._queued_commands:
+            result = await self._protocol_engine.add_and_execute_command(command)
+            if result.error:
+                raise ProtocolCommandFailedError(
+                    original_error=result.error,
+                    message=f"{result.error.errorType}: {result.error.detail}",
+                )
+


Ignoring queued commands for a moment, this doesn't look like it matches the existing implementation based on QueueWorker+get_next_to_execute(). This does not have:

~~Special handling of RunStoppedError to break instead of raising an error~~

~~Yielding to the event loop on each command~~

Pausing on recoverable errors (added in feat(api): Pause run after recoverable errors #14646)

You can see feat(api): Pause when pick_up_tip() errors in a Python protocol #14753 for how I'm doing this for Python protocols. If we're making JSON behave more like Python now, maybe this should work the same way.

Prioritization of "intent": "setup" commands

But according to the tests, this is still working, somehow?

Do we need to handle those things, or are they already handled elsewhere in some way that I'm missing?

Setup commands are added to the queue in PE but are being executed after the initial home commands instead of at the end of the run. adding prioritization will happen in a follow up pr.

Prioritization of "intent": "setup" commands
But according to the tests, this is still working, somehow?

Special handling of RunStoppedError to break instead of raising an error
Pausing on recoverable errors (added in #14646)
You can see #14753 for how I'm doing this for Python protocols. If we're making JSON behave more like Python now, maybe this should work the same way.

Will add logic in this pr

Yielding to the event loop on each command

I do not think we need this bc add_and_execute_command is async and add_command is synch but I will test this to make sure we hare not blocking anything.

I do not think we need this bc add_and_execute_command is async and add_command is synch but I will test this to make sure we hare not blocking anything.

We do need it, unfortunately, for the reasons described in this comment in the old implementation:

opentrons/api/src/opentrons/protocol_engine/execution/queue_worker.py

Lines 80 to 83 in 9322657

await self._command_executor.execute(command_id=command_id)

# Yield to the event loop in case we're executing a long sequence of commands

# that never yields internally. For example, a long sequence of comment commands.

await asyncio.sleep(0)

Unlike JavaScript, Python's await isn't guaranteed to yield to the event loop. It will only do so when it calls something that goes back to the event loop, like network I/O or an asyncio.sleep().

Other than that, that all makes sense, thanks!

I see. I tested this by adding a background task that prints, then time.sleep(3) and was able to see the printing sequence while executing the commands. so this test is not efficient? @SyntaxColoring @sfoster1

I don't think we need the await because, crucially, what we're talking about here is in a different async stack than the lines you linked to @SyntaxColoring . This PR does not replace or even touch the execution queue worker.

The way the engine works, just so we agree, is that there's fundamentally at least three async stacks: the execution stack, which is controlled by the execution queue linked above and owned by the engine, and awaits command implementations; the runner task-queue stack, which is what is changed here and owned by the runner, and (now) dispatches commands via add_and_execute_command; and the stack that calls the runner's primary interface, which is I think owned by the server's run data manager.

The execution stack, as you mention, definitely needs to have an explicit yield in case it's running a bunch of commands that have no yield points... so it does, it's in what you linked above, which is still what's used and which this PR does not touch.

The server stack does not need to have an explicit yield as long as the runner uses the task queue, since if the runner uses a task queue it await self._task_queue.join(), which awaits a future, which is a yield point.

The task queue stack (which is what's changed here) does not need to have an explicit yield if it's using something that eventually uses wait_for because wait_for await (asyncio.Event())s, which is a sync point.

Ah, that makes sense, thanks. I missed that this was still going through the QueueWorker under the hood.

Which means I was also wrong about this:

Special handling of RunStoppedError to break instead of raising an error

Per the documentation of ProtocolEngine.add_and_execute(), if the run is stopped, it actually doesn't raise a RunStoppedError—it returns the command still queued.

@SyntaxColoring
I had a feeling but I wanted to prove that through a test I am trying to add :-)

Which means I was also wrong about this:

@SyntaxColoring I added e2e tests to prove that python and json protocols now behave the same when stopping a protocol mid run

api/src/opentrons/protocol_runner/protocol_runner.py

mjhuff

Disregard my terrifying comments, tested and this does work. App & ODD don't appear to be affected as well!

api/src/opentrons/protocol_runner/protocol_runner.py

sfoster1

Looks excellent!

SyntaxColoring

This looks good to me to merge, thanks. Here are some comments on the tests that we've talked about in Slack.

robot-server/tests/integration/http_api/runs/test_play_stop_papi.tavern.yaml

SyntaxColoring

Nice, TY!

robot-server/tests/integration/http_api/runs/test_play_stop_papi.tavern.yaml

SyntaxColoring · 2024-04-04T16:09:44Z

Oh, and I renamed this from chore: ... to feat: ... because it's a deliberate change in the HTTP API, and we'll have to remember to describe it in the release notes.

…pi.tavern.yaml Co-authored-by: Max Marrone <[email protected]>

# Overview closes https://opentrons.atlassian.net/browse/EXEC-352. first step towards fixit commands. do not enqueue json protocol commands. # Test Plan Tested with Json protocols and Postman: - Make sure loading a protocol and executing it are happening within order. - Make sure get run `/commands` returning the list properly with successful commands. - Make sure loading a failed protocol should fail the run and fail the command. - Make sure get run `/commands` for failed runs the list of commands, last command being the failed command. - Fixed e2e test to comply with these changes # Changelog - Do no enqueue commands in PE for Json command upon load. - Execute commands one by one when run get started - same way we do for python protocols. # Review requests Changes make sense? GET run` /commands` will not return the full list of commands if the run did not start - its a change we are doing to make json protocols run like python protocols. are we ok with this? # Risk assessment Medium. need to do smoke tests for Json protocols and make sure these changes do not affect anything. --------- Co-authored-by: Max Marrone <[email protected]>

TamarZanzouri and others added 9 commits March 26, 2024 12:33

added schema definition for fixtures

07b01f5

initial move commands list into task_queue

e44a81a

moved logic from task queue into runner for json protocols

a073995

reverted task queue changes

ce43819

json runner with Event set and wait. fixed v6 upload test

004e498

reverted task queue changes and use runner._run method instead

e2d4658

Delete shared-data/fixture/schemas/1.json

f87b1db

clean up

3d7c688

Merge branch 'edge' into EXEC-352-do-not-enqueue-json-protocol-commands

23c7d40

TamarZanzouri changed the title ~~Exec 352 do not enqueue json protocol commands~~ chore(api): Do not enqueue json commands on protocol load Mar 29, 2024

TamarZanzouri marked this pull request as ready for review March 29, 2024 20:38

TamarZanzouri requested review from a team as code owners March 29, 2024 20:38

sfoster1 approved these changes Mar 29, 2024

View reviewed changes

SyntaxColoring reviewed Apr 1, 2024

View reviewed changes

api/src/opentrons/protocol_runner/protocol_runner.py Show resolved Hide resolved

mjhuff approved these changes Apr 1, 2024

View reviewed changes

TamarZanzouri added 5 commits April 1, 2024 16:11

fixed failing test from merge

747974c

fixed failing test

bd2afe5

started adding tests WIP

335c10d

added captors and tested add_and_execute_command

2e1c4f4

added test for break on stop

cb83ccf

sfoster1 requested changes Apr 2, 2024

View reviewed changes

api/src/opentrons/protocol_runner/protocol_runner.py Outdated Show resolved Hide resolved

TamarZanzouri added 4 commits April 3, 2024 17:41

added test to not execute commands if run stopped

819d957

fixed logic with test

c5249d3

added tests for papi and json play and stop in the middle of a run

6c812d6

fixed unit test to match logic in e2e test

76aa3e5

TamarZanzouri requested review from sfoster1 and SyntaxColoring April 4, 2024 02:13

sfoster1 approved these changes Apr 4, 2024

View reviewed changes

SyntaxColoring approved these changes Apr 4, 2024

View reviewed changes

robot-server/tests/integration/http_api/runs/test_play_stop_papi.tavern.yaml Show resolved Hide resolved

robot-server/tests/integration/http_api/runs/test_play_stop_papi.tavern.yaml Outdated Show resolved Hide resolved

changed tests to use delay and wait for a running state

d3b0cd3

SyntaxColoring approved these changes Apr 4, 2024

View reviewed changes

robot-server/tests/integration/http_api/runs/test_play_stop_papi.tavern.yaml Outdated Show resolved Hide resolved

SyntaxColoring changed the title ~~chore(api): Do not enqueue json commands on protocol load~~ feat(api): Do not enqueue json commands on protocol load Apr 4, 2024

TamarZanzouri and others added 3 commits April 4, 2024 12:13

Update robot-server/tests/integration/http_api/runs/test_play_stop_pa…

25344bd

…pi.tavern.yaml Co-authored-by: Max Marrone <[email protected]>

removed retry on get run commands

02a6ead

removed retry

123f313

TamarZanzouri merged commit 65885b2 into edge Apr 4, 2024
23 checks passed

TamarZanzouri deleted the EXEC-352-do-not-enqueue-json-protocol-commands branch April 4, 2024 17:47

SyntaxColoring mentioned this pull request Apr 8, 2024

fix(app-testing): snapshot failure capture #14813

Merged

SyntaxColoring mentioned this pull request Apr 30, 2024

chore: release notes for 7.3.0 #15045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Do not enqueue json commands on protocol load #14759

feat(api): Do not enqueue json commands on protocol load #14759

TamarZanzouri commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

sfoster1 left a comment

SyntaxColoring Apr 1, 2024 •

edited

Loading

TamarZanzouri Apr 1, 2024 •

edited

Loading

SyntaxColoring Apr 1, 2024 •

edited

Loading

TamarZanzouri Apr 2, 2024

sfoster1 Apr 2, 2024

SyntaxColoring Apr 2, 2024 •

edited

Loading

SyntaxColoring Apr 2, 2024

TamarZanzouri Apr 2, 2024 •

edited

Loading

TamarZanzouri Apr 4, 2024 •

edited

Loading

mjhuff left a comment •

edited

Loading

sfoster1 left a comment

SyntaxColoring left a comment

SyntaxColoring left a comment

SyntaxColoring commented Apr 4, 2024

	await self._command_executor.execute(command_id=command_id)
	# Yield to the event loop in case we're executing a long sequence of commands
	# that never yields internally. For example, a long sequence of comment commands.
	await asyncio.sleep(0)

feat(api): Do not enqueue json commands on protocol load #14759

feat(api): Do not enqueue json commands on protocol load #14759

Conversation

TamarZanzouri commented Mar 29, 2024 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Mar 29, 2024 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

SyntaxColoring Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

TamarZanzouri Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

TamarZanzouri Apr 2, 2024

Choose a reason for hiding this comment

sfoster1 Apr 2, 2024

Choose a reason for hiding this comment

SyntaxColoring Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

SyntaxColoring Apr 2, 2024

Choose a reason for hiding this comment

TamarZanzouri Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

TamarZanzouri Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

mjhuff left a comment • edited Loading

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

SyntaxColoring commented Apr 4, 2024

TamarZanzouri commented Mar 29, 2024 •

edited

Loading

codecov bot commented Mar 29, 2024 •

edited

Loading

SyntaxColoring Apr 1, 2024 •

edited

Loading

TamarZanzouri Apr 1, 2024 •

edited

Loading

SyntaxColoring Apr 1, 2024 •

edited

Loading

SyntaxColoring Apr 2, 2024 •

edited

Loading

TamarZanzouri Apr 2, 2024 •

edited

Loading

TamarZanzouri Apr 4, 2024 •

edited

Loading

mjhuff left a comment •

edited

Loading