feat(api): Limited support for programmatically running PAPIv≥2.14 #12970

SyntaxColoring · 2023-06-23T19:02:05Z

Overview / Changelog

These interfaces now support executing PAPIv≥2.14 protocols:

The opentrons_execute CLI
opentrons.execute.execute()
opentrons.execute.get_protocol_api()

This works towards, but does not close, RSS-268.

Known limitations:

Simulating PAPIv≥2.14 protocols will still raise a NotImplementedError.
Executing or simulating JSONv6+ protocols will still raise a NotImplementedError.
It will not print the run log. (Ticketed as RSS-283.)

Test Plan

Regression testing

New feature testing

Review requests

I hate several things about this.

The _LIVE_PROTOCOL_ENGINE_CONTEXTS global.
_adapt_protocol_source().
create_protocol_engine_in_thread(). This is technically not new—it was added in test(api): Configure ctx fixture as PAPIv2.14 when it's configured as an OT-3 #12567—but now it's being used in production code for the first time.
Letting opentrons.execute drift further apart from opentrons.simulate.

Risk assessment

Medium. This is messy, highly-duplicated, difficult to automatically test, and public-facing.

Much of this code is sensitive to differences between None and {} for bundled_labware, extra_labware, and bundled_data.

Next steps

We should try executing some Flex hardware testing scripts. From @ryanthecoder:

python3 -m hardware_testing.gravimetric --photometric --pipette 1000 --channels 96 --tip 200 --trials 5 this is how we call our scripts. the scripts themselves load a python protocol to fetch which type and location of labware but then just executes python directly talking to the protocol context

After that, we should continue working on RSS-268 until these interfaces are good enough to launch the Flex with.

codecov · 2023-06-23T19:07:07Z

Codecov Report

Merging #12970 (6543c2c) into edge (11fa4eb) will decrease coverage by 0.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge   #12970      +/-   ##
==========================================
- Coverage   72.55%   72.16%   -0.39%     
==========================================
  Files        2390     1565     -825     
  Lines       65903    51711   -14192     
  Branches     7306     3261    -4045     
==========================================
- Hits        47814    37319   -10495     
+ Misses      16344    13876    -2468     
+ Partials     1745      516    -1229

Flag	Coverage Δ
g-code-testing	`96.44% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`

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

Files Changed	Coverage Δ
api/src/opentrons/execute.py	`54.83% <ø> (-6.14%)`	⬇️
api/src/opentrons/protocol_engine/__init__.py	`100.00% <ø> (ø)`
...pentrons/protocol_engine/create_protocol_engine.py	`100.00% <ø> (ø)`
api/src/opentrons/util/entrypoint_util.py	`90.69% <ø> (-0.22%)`	⬇️

... and 825 files with indirect coverage changes

* Fix a confusing assert with an incorrect message. * Comment and refactor test_loop_lifetime() for clarity. * Rename loop_queue to loop_mailbox to reflect what we're using it for.

This promotes this test helper to production code.

We don't really do this often, but it seems nice for keeping the test function names short while still leaving room for testing the other functions in this file.

This promotes this test helper to production code.

…aller wants.

Merging for PR #13074.

sanni-t · 2023-07-11T18:39:22Z

api/src/opentrons/execute.py

-_PYTHON_TOO_NEW_MESSAGE = (
-    "Python protocols with apiLevels higher than 2.13"
-    " cannot currently be executed with"
-    " the opentrons_execute command-line tool,"
-    " the opentrons.execute.execute() function,"
-    " or the opentrons.execute.get_protocol_api() function."
-    " Use a lower apiLevel"
-    " or use the Opentrons App instead."
-)


Maybe we can modify this message to talk about the limitations of the current implementation and log it as a warning instead. Just a thought

Discussed in-person, I think:

We'll raise fine-grained errors when, and only when, you try to do the specific things that the implementation doesn't yet support. I think we should keep them NotImplementedErrors for now, instead of warnings.

sanni-t · 2023-07-11T19:15:57Z

api/src/opentrons/execute.py

-    except protocol_api.ProtocolEngineCoreRequiredError as e:
-        raise NotImplementedError(_PYTHON_TOO_NEW_MESSAGE) from e  # See Jira RCORE-535.
+    else:
+        if bundled_labware is not None:


I don't know the context of bundled labware. Is it there for future implementation/completion or was it a part of some old version that we don't plan on supporting for newer versions? If it's the latter, we should update the error message to indicate that it's not supported anymore.

Yeah, I also don't know. Let's ask around and see if we can remove it.

Merging in case PR #13008 affects error handling.

Merging for test and doc improvements in PR #13107.

This was just noise. I think it's okay to not document this explicitly, for now. We don't document other potential reasons why an execute would fail, and it's weird to treat this specially.

SyntaxColoring · 2023-07-20T20:13:04Z

api/src/opentrons/execute.py

+    def to_stderr_string(self) -> str:
+        """Return a string suitable as the stderr output of the `opentrons_execute` CLI.
+
+        This summarizes from the full error details.
+        """
+        # It's unclear what exactly we should extract here.
+        #
+        # First, do we print the first element, or the last, or all of them?
+        #
+        # Second, do we print the .detail? .errorCode? .errorInfo? .wrappedErrors?
+        # By contract, .detail seems like it would be insufficient, but experimentally,
+        # it includes a lot, like:
+        #
+        #     ProtocolEngineError [line 3]: Error 4000 GENERAL_ERROR (ProtocolEngineError):
+        #     UnexpectedProtocolError: Labware "fixture_12_trough" not found with version 1
+        #     in namespace "fixture".
+        return self._error_occurrences[0].detail


@TamarZanzouri @sfoster1 Is there a better thing for me to do here, now that stuff like #13008 is merged?

Here's what I'm trying to solve:

When we execute a protocol through Protocol Engine, we get any kind of fatal error—bugs, hardware errors, Protocol Engine validation errors, non-protocol-related Python mistakes, everything—through Protocol Engine's state_summary.errors. This is a list of ErrorOccurrences.

From that, we need to decide:

How do we propagate that to callers of opentrons.execute.execute()? It needs to be an exception, but what do we put in it?

I currently have the list of ErrorOccurrences as its only member. In stack traces, Python will stringify that as the exception message. This seems...okay, I guess? It's structured and lossless, at least. But it's nonstandard. I guess I'd really prefer if it were a normal Python exception chain all the way down.

What do we print to stderr for callers of the opentrons_execute CLI? Stringifying the ErrorOccurrence, or parts of it, seems plausible, but I don't know the details of how to do that properly.

Fix merge conflicts in test_execute.py.

sanni-t

🙌 Awesome! I know it has a few hacks and still needs to be implemented fully, but this is an amazing start. This ticket is quite a complex puzzle and I think you've approached it quite carefully, covering a lot of edge cases. I might have missed a couple of things while reviewing too (re: it's a complex change), but unit tests and on-robot testing has been successful so I think this is merge worthy!

sanni-t · 2023-07-21T20:51:51Z

api/src/opentrons/execute.py

+
+        # TODO(mm, 2023-06-30): This will home and drop tips at the end, which is not how
+        # things have historically behaved with PAPIv2.13 and older or JSONv5 and older.
+        result = await protocol_runner.run(protocol_source)


Oof, another problem with default/ forced tip dropping.

Yeah. the situation is actually not quite as bad as I thought it was, though. I thought we didn't have a way to clean up an engine without dropping tips. We do: we just have do do ProtocolEngine.finish(drop_tips_and_home=False). The trouble is just at the ProtocolRunner level, which doesn't propagate that functionality up.

sanni-t · 2023-07-21T20:59:37Z

api/src/opentrons/execute.py

+        # opentrons.protocol_api.core.engine, that would incorrectly make
+        # ProtocolContext.is_simulating() return True.


Oo! I did not realize that's how we are checking for simulating engine. Looking at that code, maybe there's scope to reduce redundancy in some configs (virtual pipettes/ modules/ pause ignore) and pooling them all together into an is_simulating check (not for this PR). But I might be forgetting a reason why it was split into multiple flags.

Yeah, it seems like we should either split it even further and have a dedicated flag for something like tell_python_protocol_it_is_simulating, or merge them all. We're in a weird in-between right now.

sanni-t · 2023-07-21T21:11:49Z

api/src/opentrons/execute.py

+        create_protocol_engine_in_thread(
+            hardware_api=hardware_api.wrapped(),
+            config=_get_protocol_engine_config(),
+            drop_tips_and_home_after=False,


So, am I understanding this correctly that live protocols like those over Jupyter will not drop tips and home at the end of the 'protocol session', but python/json protocol execution will?

Yep, exactly.

…ing.

SyntaxColoring · 2023-07-24T16:35:10Z

~~This PR is causing pipenv run pytest tests/opentrons/test_execute.py to hang on exit when run on Python 3.10. Looking into it.~~

Here's why it was hanging:

A Python thread can be configured to be "daemon" or "non-daemon." The difference is that when the process is shutting down, the Python interpreter won't wait for daemon threads to be joined.

More precisely, the Python interpreter will only start to call our @atexit handlers after all non-daemon threads have joined. This creates a clear deadlock. A non-daemon thread will only exit after our @atexit handler tells it to close, but our @atexit handler will never get a chance to run because there's still a pending thread, so the process will hang on shutdown.

We're seeing this on Python 3.10 and not 3.7 because 3.9 changed ThreadPoolExecutor's threads from daemon to non-daemon. (python/cpython#83993).

My workaround is to ditch ThreadPoolExecutor in favor of a manually-managed Thread so we can set daemon=True. Here's another workaround that's less deeply intrusive, but is more horrifying to me in other ways.

I think the real solution is to let get_protocol_api() users call some kind of .close() method. Then, we don't need those @atexit handlers, and we don't need daemon=True. I agree with this Stack Overflow answer in that the interpreter shutdown phase is not a happy place to be coding. Per prior discussion, I'm not adding that .close() method in this line of PRs. But I view all of this as more reason why we need to do that at some point.

SyntaxColoring · 2023-07-25T15:16:44Z

I have a verbal 👍 from @TamarZanzouri on the above change to fix the shutdown hanging.

…12970)

Notable changes: * Rework `simulate.py` to support PAPIv≥2.14 protocols, via Protocol Engine. This rework mirrors what we did to `execute.py` in #12970. * Rework the `adapt_protocol_source()` helper to cope with protocols for which we don't know the filename. This is necessary to support `opentrons.simulate.simulate()`, whose `filename` argument is optional. Unfortunately, this requires modifications to `opentrons.protocols.parse` to make the filename more pass-through, so `adapt_protocol_source()` can tell when it wasn't provided. Plus various smaller refactors for type safety, deduplication, and consistency.

SyntaxColoring added api Affects the `api` project protocol-engine Ticket related to the Protocol Engine project and associated HTTP APIs labels Jun 23, 2023

SyntaxColoring requested review from andySigler, ryanthecoder and a team June 23, 2023 19:02

SyntaxColoring force-pushed the pe_based_execute branch from f037685 to 19a115b Compare June 28, 2023 14:06

SyntaxColoring added 10 commits June 29, 2023 12:41

Minor improvements to async_context_manager_in_thread() and its tests.

d0b71f8

* Fix a confusing assert with an incorrect message. * Comment and refactor test_loop_lifetime() for clarity. * Rename loop_queue to loop_mailbox to reflect what we're using it for.

Move async_context_manager_in_thread() to opentrons.utils.

4896dea

This promotes this test helper to production code.

Collect async_context_manager_in_thread() tests into a class.

a2683be

We don't really do this often, but it seems nice for keeping the test function names short while still leaving room for testing the other functions in this file.

Move protocol_engine_in_thread() to opentrons.protocol_engine.

91b7b22

This promotes this test helper to production code.

Consistently take HardwareControlAPI interface.

685b801

Consistently take a full Config instead of trying to guess what the c…

6b5abf3

…aller wants.

Add a hacky copy_file_like() helper.

d2745aa

¯\_(ツ)_/¯

ef6fe3e

Fix JSON protocols not being able to use any labware.

b8eca37

Fix Python protocols not including Jupyter labware by default.

2dc868c

SyntaxColoring force-pushed the pe_based_execute branch from 19a115b to 2dc868c Compare July 7, 2023 21:04

SyntaxColoring added 2 commits July 7, 2023 17:26

Minor style fixups and comment clarifications.

81a37f9

Bundled data is supported, actually.

61b6763

SyntaxColoring removed request for ryanthecoder and andySigler July 7, 2023 21:36

This comment was marked as resolved.

Sign in to view

SyntaxColoring changed the title ~~feat(api): Limited support for programmatically running PAPIv2.15~~ feat(api): Limited support for programmatically running PAPIv≥2.14 Jul 7, 2023

SyntaxColoring added papi-v2 Python API V2 robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). labels Jul 7, 2023

SyntaxColoring marked this pull request as ready for review July 8, 2023 00:56

SyntaxColoring requested a review from a team as a code owner July 8, 2023 00:56

SyntaxColoring added 2 commits July 10, 2023 15:52

Fix confusing error message.

56f2c76

Merge branch 'edge' into pe_based_execute

9693d85

Merging for PR #13074.

SyntaxColoring added 8 commits July 11, 2023 14:52

Fix: Don't forget to add custom labware to PAPIv2.14+ live contexts.

fa47468

Fix ^D hanging in a REPL after get_protocol_api("2.15").

d6ee2fb

Simplify the copy_file_like() hack by trying less.

e9c59d8

Ugh.

8754ec7

Test fixup: Write encoding should match read encoding.

fcc62c1

Test fixup: Use dummy binary data that's more obviously dummy & binary.

6729d49

Test fixup: Rename test file to match source.

8d6139b

Test fixup: Remove unused param.

50a74b2

sanni-t reviewed Jul 13, 2023

View reviewed changes

Merge branch 'edge' into pe_based_execute

433e9d2

Merging in case PR #13008 affects error handling.

SyntaxColoring mentioned this pull request Jul 15, 2023

docs(api): Clean up custom/extra labware in opentrons.simulate and opentrons.execute #13107

Merged

SyntaxColoring added 4 commits July 18, 2023 15:17

Merge branch 'edge' into pe_based_execute

d49dbe8

Merging for test and doc improvements in PR #13107.

Enable Protocol Engine in test parametrization.

8fe755c

Undo ":raises Exception:" docs addition.

1b35fbf

This was just noise. I think it's okay to not document this explicitly, for now. We don't document other potential reasons why an execute would fail, and it's weird to treat this specially.

Fix errors not being reported.

372c456

SyntaxColoring commented Jul 20, 2023

View reviewed changes

SyntaxColoring added 2 commits July 20, 2023 16:56

Give up even harder on encodings, I guess.

913d4de

Merge branch 'edge' into pe_based_execute

91e6bd3

Fix merge conflicts in test_execute.py.

sanni-t approved these changes Jul 21, 2023

View reviewed changes

Don't test with emoji that can't be encoded by Windows' default encod…

9b25d8e

…ing.

SyntaxColoring added 2 commits July 24, 2023 20:36

Perhaps.

4369f9b

Explain yoself.

6543c2c

SyntaxColoring merged commit 63f94d9 into edge Jul 25, 2023

SyntaxColoring deleted the pe_based_execute branch July 25, 2023 15:33

caila-marashaj pushed a commit that referenced this pull request Jul 26, 2023

feat(api): Limited support for programmatically running PAPIv≥2.14 (#…

d78d500

…12970)

SyntaxColoring mentioned this pull request Oct 10, 2023

fix(api): Support simulating PAPIv≥2.14 #13709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Limited support for programmatically running PAPIv≥2.14 #12970

feat(api): Limited support for programmatically running PAPIv≥2.14 #12970

SyntaxColoring commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

This comment was marked as resolved.

sanni-t Jul 11, 2023

SyntaxColoring Jul 14, 2023

sanni-t Jul 11, 2023

SyntaxColoring Jul 13, 2023

SyntaxColoring Jul 20, 2023

sanni-t left a comment

sanni-t Jul 21, 2023

SyntaxColoring Jul 24, 2023 •

edited

Loading

sanni-t Jul 21, 2023

SyntaxColoring Jul 24, 2023

sanni-t Jul 21, 2023

SyntaxColoring Jul 24, 2023

SyntaxColoring commented Jul 24, 2023 •

edited

Loading

SyntaxColoring commented Jul 25, 2023

		# opentrons.protocol_api.core.engine, that would incorrectly make
		# ProtocolContext.is_simulating() return True.

feat(api): Limited support for programmatically running PAPIv≥2.14 #12970

feat(api): Limited support for programmatically running PAPIv≥2.14 #12970

Conversation

SyntaxColoring commented Jun 23, 2023 • edited Loading

Overview / Changelog

Test Plan

Regression testing

New feature testing

Review requests

Risk assessment

Next steps

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this 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

SyntaxColoring Jul 24, 2023 • edited Loading

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

Choose a reason for hiding this comment

SyntaxColoring commented Jul 24, 2023 • edited Loading

SyntaxColoring commented Jul 25, 2023

SyntaxColoring commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

SyntaxColoring Jul 24, 2023 •

edited

Loading

SyntaxColoring commented Jul 24, 2023 •

edited

Loading