refactor(api, robot-server): Refactor ProtocolRunner per protocol type #12343

TamarZanzouri · 2023-03-22T19:46:22Z

Overview

Closes RSS-200.
create an abstract interface for ProtocolRunner and implement per JsonRunner PythonAndLegacyRunner and LiveRunner.

Also fixes a bug that was causing live http runs to finish executing the run after the first command (RSS-214).

Test Plan

Upload python 2.14, json v6, legacy python protocols and make sure you're able to analyze protocols, run LPC and run protocols. Check that:
- all runs are created correctly
- can be started, paused and stopped
- run results are correct
(Optional)Test a LiveRunner: Start an empty run and post protocol-intent commands. Make sure all commands are run successfully. Also test pausing and stopping the protocol.

Changelog

Refactor ProtocolRunner to be an abstract and implement per JsonRunner PythonAndLegacyRunner and LiveRunner.
create_simulating_runner is now constructed per file type.
Change ProtocolAnalyzer to create on the fly the simulating runner instead of injecting it.
Changed tests that were using an empty run.

Review requests

I (Sanniti) have tested the changes on the dev server but it's important to test on a real robot (ot2/ot3 either works) since run flow control actions can't be tested on the dev server.
We have talked about how having a runner that can allow execution of batch commands could be useful for future features. I agree that it would be useful but the way the engine & run is architected currently, it would need a more detailed change to the architecture (more importantly, careful testing) than is strictly necessary for run split. So I have not attempted to do that in this PR. We will be able to make that change in the future when the feature is on the roadmap.
Currently we are setting run_started_at (also in the run response) when there is a play action. with this change we will no longer set this value for setup commands/post commands. this is the only real noticeable change -> might want to check with UI folks. they should use createdAt instead for this case.

Risk assessment

Medium-high. need to make sure nothing is effected from the refactor for all protocol types and for posting commands/runs with no protocol.

…nner

…nto analyze

TamarZanzouri · 2023-03-22T19:51:45Z

api/src/opentrons/protocol_runner/protocol_runner.py

+ protocol_source: Optional[ProtocolSource] = None,
+ ) -> ProtocolRunResult:
+ """Run a given protocol to completion."""
+ # TODO(mc, 2022-01-11): move load to runner creation, remove from `run`


can totally do this for this pr

TamarZanzouri · 2023-03-23T14:48:33Z

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

@@ -37,3 +51,13 @@ stages:
 - id: 'RunActionNotAllowed'
 title: 'Run Action Not Allowed'
 detail: 'Cannot pause a run that is not running.'
+


investigate why this is hanging without the stop

this is hanging on edge too

codecov · 2023-03-23T17:17:37Z

Codecov Report

Merging #12343 (5789c41) into edge (bc70828) will decrease coverage by 0.11%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             edge   #12343      +/-   ##
==========================================
- Coverage   73.81%   73.70%   -0.11%     
==========================================
  Files        2228     2220       -8     
  Lines       61587    61316     -271     
  Branches     6382     6374       -8     
==========================================
- Hits        45461    45194     -267     
+ Misses      14637    14635       -2     
+ Partials     1489     1487       -2

Flag	Coverage Δ
notify-server	`89.13% <ø> (ø)`

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

Impacted Files	Coverage Δ
api/src/opentrons/protocol_runner/__init__.py	`100.00% <ø> (ø)`
...ntrons/protocol_runner/create_simulating_runner.py	`84.61% <ø> (-2.89%)`	⬇️
...i/src/opentrons/protocol_runner/legacy_wrappers.py	`100.00% <ø> (ø)`
...obot-server/robot_server/protocols/dependencies.py	`100.00% <ø> (ø)`
...server/robot_server/protocols/protocol_analyzer.py	`100.00% <ø> (ø)`
robot-server/robot_server/runs/engine_store.py	`96.00% <ø> (-0.08%)`	⬇️
g-code-testing/g_code_parsing/g_code_engine.py	`81.31% <50.00%> (ø)`
...i/src/opentrons/protocol_runner/protocol_runner.py	`100.00% <100.00%> (ø)`

... and 50 files with indirect coverage changes

api/src/opentrons/protocol_runner/protocol_runner.py

api/tests/opentrons/protocol_runner/test_protocol_runner.py

TamarZanzouri · 2023-03-24T19:53:01Z

api/tests/opentrons/protocol_runner/test_protocol_runner.py

+ False, True
+ )
+
+ assert legacy_subject.was_started() is False


this is wrong. need to fix this test to test real cases

SyntaxColoring

Looking great so far!

api/src/opentrons/protocol_runner/protocol_runner.py

Co-authored-by: Max Marrone <[email protected]>

TamarZanzouri · 2023-03-28T20:29:23Z

Tested on an OT-2 with json, python, legacy protocols. tested with live protocol commands. works as expected.

api/src/opentrons/protocol_runner/create_simulating_runner.py

- """Create a ProtocolRunner wired to a simulating HardwareControlAPI.
+async def create_simulating_runner(
+ robot_type: RobotType, protocol_config: ProtocolConfig
+) -> AbstractRunner:


api/src/opentrons/protocol_runner/protocol_runner.py

@@ -27,43 +31,75 @@
 )


-class ProtocolRunResult(NamedTuple):
+class RunnerRunResult(NamedTuple):


api/src/opentrons/protocol_runner/protocol_runner.py

 """An interface to manage and control a protocol run.

- The ProtocolRunner is primarily responsible for feeding a ProtocolEngine
+ The AbstractRunner is primarily responsible for feeding a ProtocolEngine
 with commands and control signals. These commands and signals are
 generated by protocol files, hardware signals, or externally via


api/src/opentrons/protocol_runner/protocol_runner.py

-# TODO(mc, 2022-01-11): this class has become bloated. Split into an abstract
-# interfaces and several concrete implementations per protocol type
-class ProtocolRunner:
+class AbstractRunner(ABC):


sanni-t · 2023-04-03T17:31:10Z

api/src/opentrons/protocol_runner/protocol_runner.py

+ def play(self) -> None: # noqa: D102
+ self._protocol_engine.play()
+
+ def pause(self) -> None: # noqa: D102
+ self._protocol_engine.pause()
+
+ async def stop(self) -> None: # noqa: D102
+ if self.was_started():
+ await self._protocol_engine.stop()
+ else:
+ await self._protocol_engine.finish(
+ drop_tips_and_home=False,
+ set_run_status=False,
+ )


I think we can implement these in the abstract itself.

sanni-t · 2023-04-03T17:35:27Z

api/src/opentrons/protocol_runner/protocol_runner.py

- self._protocol_engine.add_plugin(
- LegacyContextPlugin(broker=broker, equipment_broker=equipment_broker)
+ async def load(self, protocol_source: ProtocolSource) -> None: # noqa: D102
+ raise NotImplementedError(


Raise a more appropriate error

sanni-t · 2023-04-03T17:44:44Z

api/src/opentrons/protocol_runner/create_simulating_runner.py

-async def create_simulating_runner(robot_type: RobotType) -> ProtocolRunner:
- """Create a ProtocolRunner wired to a simulating HardwareControlAPI.
+async def create_simulating_runner(
+ robot_type: RobotType, protocol_config: ProtocolConfig


See if accepting a protocol_type makes more sense here.

Adding a TODO to address later.

api/src/opentrons/protocol_runner/protocol_runner.py

b-cooper

Tested via App branch build, successfully analyzed python and json protocols. ✅

sanni-t

Comments from live review.

sanni-t · 2023-04-11T17:20:13Z

api/src/opentrons/protocol_runner/protocol_runner.py

+ await self._protocol_engine.stop()
+ else:
+ await self._protocol_engine.finish(
+ drop_tips_and_home=False,


Investigate if this should be actually true.

Our current code in edge does tip drop only after a clean protocol run finish and not in runner.stop (run cancellation) so this keeps that behavior as is and is correct.

api/tests/opentrons/protocol_runner/smoke_tests/test_protocol_runner.py

sanni-t · 2023-04-11T17:42:11Z

api/tests/opentrons/protocol_runner/smoke_tests/test_protocol_runner.py

 from opentrons.protocol_runner import create_simulating_runner
+from opentrons.protocols.api_support.types import APIVersion


 # TODO (tz, 6-17-22): API version 3.x in-development.


Check what's going on here. Why's this xfailing?

sanni-t · 2023-04-14T12:22:56Z

api/tests/opentrons/protocol_runner/smoke_tests/test_protocol_runner.py

@@ -1,6 +1,6 @@
-"""Smoke tests for the ProtocolRunner and ProtocolEngine classes.
+"""Smoke tests for the AbstractRunner and ProtocolEngine classes.


There's an integration test for LiveRunner which covers the requirement.

api/tests/opentrons/protocol_runner/test_legacy_command_mapper.py

sanni-t · 2023-04-11T17:47:41Z

api/tests/opentrons/protocol_runner/test_protocol_runner.py

+
+
+@pytest.fixture
+def legacy_subject(


Rename to python_runner_subject?

api/tests/opentrons/protocol_runner/test_protocol_runner.py

robot-server/robot_server/protocols/protocol_analyzer.py

robot-server/robot_server/runs/engine_store.py

sanni-t · 2023-04-11T18:01:39Z

robot-server/robot_server/runs/engine_store.py

 await runner.load(protocol.source)
+ else: # Why does linter not like `else:`..?


I think this is outdated comment.

SyntaxColoring

This looks like a big improvement, thanks @TamarZanzouri and @sanni-t. It's nice that JSON runs don't have to care about dependencies for Python runs anymore, and vice-versa. Looks good to me to merge once comments are resolved.

Here are a bunch of little things, sorry for the spam!

api/tests/opentrons/protocol_runner/smoke_tests/test_legacy_command_mapper.py

api/tests/opentrons/protocol_runner/smoke_tests/test_legacy_custom_labware.py

api/tests/opentrons/protocol_runner/smoke_tests/test_legacy_module_commands.py

api/tests/opentrons/protocol_runner/smoke_tests/test_protocol_runner.py

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

robot-server/tests/runs/test_engine_store.py

api/src/opentrons/protocol_runner/protocol_runner.py

api/tests/opentrons/protocol_runner/test_protocol_runner.py

TamarZanzouri · 2023-04-12T19:59:35Z

api/src/opentrons/protocol_runner/legacy_wrappers.py

@@ -96,6 +96,8 @@ def read(
 )


+# TODO (spp, 2023-04-05): Remove 'legacy' wording since this is the context we are using


TamarZanzouri · 2023-04-12T20:04:03Z

Totally agree with the changes added. Makes sense to move common implementations to the abstract. Thank you for fixing this @sanni-t

- Use `protocol_source.config` where possible - Some docstring corrections - other small misc cleanup in tests Co-authored-by: Max Marrone <[email protected]>

…prepare()

SyntaxColoring

Woohoo! －＝≡ヘ(*・ω・)ノ

jbleon95

Looks good!

TamarZanzouri added 12 commits March 14, 2023 15:08

WIP: created an abstract interface for ProtocolRunner. started JsonRu…

2410782

…nner

WIP create_protocol_runner

290e860

test json_runner and finished logic for json_runner

842a99e

WIP python and legacy runner

f5b5325

python and legacy runner implementation

7de1dae

create_simulating_runner.py

8f30fa7

fixed robot server dependencies.py

c73922c

MaintenanceRunner

33c260a

wip robot-server

b727804

changed create_simulating_runner to monkeypatch

5ecd3cb

removed set_run_started_at and fixed test failing on started_at

037f322

rollback depends for protocol_analyzer.py and moved runner creation i…

d54c1ec

…nto analyze

TamarZanzouri requested review from a team as code owners March 22, 2023 19:46

TamarZanzouri commented Mar 22, 2023

View reviewed changes

reorganize prtocol_runner tests and text fixes

af5aaac

TamarZanzouri commented Mar 23, 2023

View reviewed changes

TamarZanzouri and others added 2 commits March 23, 2023 11:18

removed task_queue from maintenance runner

3a870b0

Merge branch 'edge' into RSS-200-refactor-protocol-runner

4009a37

TamarZanzouri added 3 commits March 23, 2023 13:30

linting

50978e3

fixed failing test in actions router

20d3c3f

fixed maintenance test with task runner

08524fd

TamarZanzouri added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Mar 23, 2023

TamarZanzouri commented Mar 24, 2023

View reviewed changes

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

TamarZanzouri commented Mar 24, 2023

View reviewed changes

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

TamarZanzouri commented Mar 24, 2023

View reviewed changes

api/tests/opentrons/protocol_runner/test_protocol_runner.py Show resolved Hide resolved

TamarZanzouri commented Mar 24, 2023

View reviewed changes

SyntaxColoring reviewed Mar 24, 2023

View reviewed changes

Update api/src/opentrons/protocol_runner/protocol_runner.py

da0fdba

Co-authored-by: Max Marrone <[email protected]>

TamarZanzouri added 4 commits March 28, 2023 10:58

fixed g-code testing

f36be33

g-code testing fix

a81df1d

linting

b647315

linting

e802d65

TamarZanzouri requested a review from SyntaxColoring March 28, 2023 16:05

added pe.play for run live commands

923e00c

added tavern test for posting protocol commands

e1e9ddd

sanni-t reviewed Apr 4, 2023

View reviewed changes

sanni-t added 2 commits April 5, 2023 16:30

addressed PR comments, fixed live runner bug, updated tests

88cae89

updated tests, fixed linter errors

742e292

sanni-t changed the title ~~refactor(api, robot-server): Refactor ProtocolRunner per protocol type and add MaintenanceRunner~~ refactor(api, robot-server): Refactor ProtocolRunner per protocol type Apr 5, 2023

sanni-t requested review from jbleon95 and b-cooper April 5, 2023 23:04

b-cooper reviewed Apr 7, 2023

View reviewed changes

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

sanni-t and others added 2 commits April 7, 2023 12:46

Merge branch 'edge' into RSS-200-refactor-protocol-runner

93be36f

correct docstring

56a1b2f

b-cooper approved these changes Apr 10, 2023

View reviewed changes

sanni-t reviewed Apr 11, 2023

View reviewed changes

SyntaxColoring reviewed Apr 11, 2023

View reviewed changes

TamarZanzouri commented Apr 12, 2023

View reviewed changes

sanni-t and others added 4 commits April 13, 2023 08:05

Apply suggestions from code review

b272e10

- Use `protocol_source.config` where possible - Some docstring corrections - other small misc cleanup in tests Co-authored-by: Max Marrone <[email protected]>

fix runner smoke tests and linter errors, some renaming from PR review

433ca47

update runner typing & type naming, renamed set_task_queue_wait() to …

b6e7385

…prepare()

address all remaining review comments

5789c41

sanni-t requested a review from SyntaxColoring April 14, 2023 22:22

SyntaxColoring approved these changes Apr 14, 2023

View reviewed changes

jbleon95 approved these changes Apr 17, 2023

View reviewed changes

sanni-t merged commit 214f225 into edge Apr 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(api, robot-server): Refactor ProtocolRunner per protocol type #12343

refactor(api, robot-server): Refactor ProtocolRunner per protocol type #12343

TamarZanzouri commented Mar 22, 2023 •

edited by sanni-t

Loading

TamarZanzouri Mar 22, 2023

TamarZanzouri Mar 23, 2023 •

edited

Loading

TamarZanzouri Mar 23, 2023

codecov bot commented Mar 23, 2023 •

edited

Loading

TamarZanzouri Mar 24, 2023

SyntaxColoring left a comment

TamarZanzouri commented Mar 28, 2023

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

sanni-t Apr 3, 2023

sanni-t Apr 3, 2023

sanni-t Apr 3, 2023

sanni-t Apr 12, 2023

b-cooper left a comment

sanni-t left a comment

sanni-t Apr 11, 2023

sanni-t Apr 12, 2023

sanni-t Apr 11, 2023

This comment was marked as resolved.

sanni-t Apr 14, 2023

sanni-t Apr 11, 2023

sanni-t Apr 11, 2023

SyntaxColoring left a comment

TamarZanzouri Apr 12, 2023

TamarZanzouri commented Apr 12, 2023

SyntaxColoring left a comment

jbleon95 left a comment

		@@ -1,6 +1,6 @@
		"""Smoke tests for the ProtocolRunner and ProtocolEngine classes.
		"""Smoke tests for the AbstractRunner and ProtocolEngine classes.

		await runner.load(protocol.source)
		else: # Why does linter not like `else:`..?

		@@ -96,6 +96,8 @@ def read(
		)


		# TODO (spp, 2023-04-05): Remove 'legacy' wording since this is the context we are using

refactor(api, robot-server): Refactor ProtocolRunner per protocol type #12343

refactor(api, robot-server): Refactor ProtocolRunner per protocol type #12343

Conversation

TamarZanzouri commented Mar 22, 2023 • edited by sanni-t Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

Choose a reason for hiding this comment

TamarZanzouri Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

TamarZanzouri commented Mar 28, 2023

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-cooper left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TamarZanzouri commented Apr 12, 2023

SyntaxColoring left a comment

Choose a reason for hiding this comment

jbleon95 left a comment

Choose a reason for hiding this comment

TamarZanzouri commented Mar 22, 2023 •

edited by sanni-t

Loading

TamarZanzouri Mar 23, 2023 •

edited

Loading

codecov bot commented Mar 23, 2023 •

edited

Loading