refactor(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator #15257

TamarZanzouri · 2024-05-23T19:00:43Z

Overview

second phase of run orchestrator. redirect all router level calls via run_orchestrator.

Test Plan

All e2e tests are passing.
through it on a robot and make sure nothing is broken. -> tests this with dev server.
- test json protocols
- test python protocols
- post commands to a run
- post commands with default router

Changelog

added method calls from run_data_manager and routers to engine_store.
all calls get through run_orchestrator to PE and to runners.

Review requests

changes make sense? will something break? do we like this change? should I add more tests?

Risk assessment

Medium. tested with dev server and all tests are passing but still a fundamental change.

codecov · 2024-06-07T21:18:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.43%. Comparing base (1e66d70) to head (20202f1).
Report is 216 commits behind head on edge.

❗ Current head 20202f1 differs from pull request most recent head a1121d1

Please upload reports for the commit a1121d1 to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##             edge   #15257   +/-   ##
=======================================
  Coverage   92.43%   92.43%           
=======================================
  Files          77       77           
  Lines        1283     1283           
=======================================
  Hits         1186     1186           
  Misses         97       97

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

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

api/src/opentrons/protocol_runner/run_orchestrator.py

sfoster1

This looks really good, thanks for taking it on! Let's fix up some docstring copypasta and test it, but the structure looks great!

api/src/opentrons/protocol_runner/run_orchestrator.py

robot-server/robot_server/runs/engine_store.py

Co-authored-by: Seth Foster <[email protected]>

SyntaxColoring

Thank you! This mostly makes sense to me so far and I think we should keep going. A few questions:

SyntaxColoring · 2024-06-10T18:33:29Z

api/src/opentrons/protocol_runner/run_orchestrator.py

+    def prepare(self) -> None:
+        """Prepare live runner for a run."""
+        self._setup_runner.prepare()
+        self._fixit_runner.prepare()
+        self._protocol_live_runner.prepare()


It looks a little questionable that self._setup_runner.prepare() and self._fixit_runner.prepare() are called for live HTTP runs (because that's the only thing calling RunOrchestrator.prepare()), and are not called for JSON or Python protocols. In all cases, the setup runner and fixit runner behave the same, right? So shouldn't they consistently always be .prepare()'d or never be .prepare()'d?

I agree. I will fix it.

api/src/opentrons/protocol_runner/run_orchestrator.py

robot-server/tests/runs/router/test_commands_router.py

robot-server/tests/runs/test_engine_store.py

SyntaxColoring · 2024-06-10T19:16:28Z

Oh and this should probably be refactor instead of chore since it's meaningfully affecting production code.

sfoster1

I agree with @SyntaxColoring about the prepare thing, we should have an explicit reason why we're not doing it, but other than that looks good!

robot-server/tests/runs/test_engine_store.py

…otocol_runners via run orchestrator (#15257)

TamarZanzouri added 30 commits May 22, 2024 16:36

WIP deafult orchestrator

667147d

fixed get_default_orchstrator

47e5f5d

remove runner and engine props from EngineStore WIP

e88982c

play and finish and progress replacing

34b4391

run_data_manager via orchestrator

7bd9f5f

run_data_manger remove direct access to orchestrator

7b29b9a

run_data_manager.py via orchestrator

29813d8

test run_data_manager

c8f1f4b

run_controller.py

9d9979f

light_control_task.py and dependencies.py

13a0cb8

labware_router.py

43d82e8

add command and wait

31e0051

all implemented engine store and above

9531aa7

await add and wait for command

8e06362

fixed failing tests in the commands router

967f78e

run_orchestrator prop

2e00e7d

WIP removed props for engine and runner

4b27221

orchestrator implementations + engine_store redirect

bc8237a

same

bcdfcea

get default orchestrator WIP

a079763

get default orchestrator

98bc42e

fixed tests and removed props from engine_store

9b9f11f

implementation for orchestrator -> engine_store

2ffc0a8

no linting errors!!!

aa156bb

engine_store all tests passing

c598bfa

orchestrator linting

9e8775a

fixing tests

3fb5cba

added tests

a5ddce7

added tests WIP

815c7d9

added tests

83e484b

TamarZanzouri added 4 commits June 6, 2024 15:25

added tests and fixed bugs

1602623

fixed bug with maintenance runs

c18bf98

bug fixes and linting

04c0e1d

fixed but with protocol live runner

20202f1

TamarZanzouri commented Jun 10, 2024

View reviewed changes

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

TamarZanzouri marked this pull request as ready for review June 10, 2024 13:53

TamarZanzouri requested a review from a team as a code owner June 10, 2024 13:53

TamarZanzouri requested review from sfoster1 and SyntaxColoring June 10, 2024 13:53

TamarZanzouri changed the title ~~Exec 418 use orchestrator in router~~ chore(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator Jun 10, 2024

TamarZanzouri commented Jun 10, 2024

View reviewed changes

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

added tests and specific error

8dcc13f

sfoster1 requested changes Jun 10, 2024

View reviewed changes

Update robot-server/robot_server/runs/engine_store.py

2ef261c

Co-authored-by: Seth Foster <[email protected]>

TamarZanzouri requested a review from sfoster1 June 10, 2024 18:00

fixed docsrtings

42dbedd

SyntaxColoring reviewed Jun 10, 2024

View reviewed changes

docstrings

ee5e5a3

sfoster1 approved these changes Jun 10, 2024

View reviewed changes

TamarZanzouri changed the title ~~chore(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator~~ refactor(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator Jun 10, 2024

prepare on init. reverted tests commented out. docstrings

836c3f6

TamarZanzouri requested a review from SyntaxColoring June 10, 2024 20:28

SyntaxColoring approved these changes Jun 10, 2024

View reviewed changes

robot-server/tests/runs/test_engine_store.py Outdated Show resolved Hide resolved

TamarZanzouri added 2 commits June 11, 2024 10:48

get_robot_type

77bf864

robot type import

a1121d1

TamarZanzouri merged commit e7882fd into edge Jun 11, 2024
20 checks passed

TamarZanzouri deleted the EXEC-418-use-orchestrator-in-router branch June 11, 2024 18:29

aaron-kulkarni pushed a commit that referenced this pull request Jun 13, 2024

refactor(api, robot-server): redirect router level calls to PE and pr…

89632ce

…otocol_runners via run orchestrator (#15257)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator #15257

refactor(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator #15257

TamarZanzouri commented May 23, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

sfoster1 left a comment

SyntaxColoring left a comment

SyntaxColoring Jun 10, 2024

TamarZanzouri Jun 10, 2024

SyntaxColoring commented Jun 10, 2024

sfoster1 left a comment

refactor(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator #15257

refactor(api, robot-server): redirect router level calls to PE and protocol_runners via run orchestrator #15257

Conversation

TamarZanzouri commented May 23, 2024 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Jun 7, 2024 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

SyntaxColoring left a comment

Choose a reason for hiding this comment

SyntaxColoring Jun 10, 2024

Choose a reason for hiding this comment

TamarZanzouri Jun 10, 2024

Choose a reason for hiding this comment

SyntaxColoring commented Jun 10, 2024

sfoster1 left a comment

Choose a reason for hiding this comment

TamarZanzouri commented May 23, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading