Skip to content
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): Support simulating PAPIv≥2.14 #13709

Merged
merged 17 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5162235
Minor CommandScraper refactors.
SyntaxColoring Oct 3, 2023
6db5ca0
Refactor CommandScraper for deterministic cleanup.
SyntaxColoring Oct 3, 2023
38eb159
Move ProtocolEngineExecuteError and adapt_protocol_source() to entryp…
SyntaxColoring Oct 6, 2023
3761423
Minor refactor: Add a type alias for the simulate() return value.
SyntaxColoring Oct 10, 2023
bc95903
Minor refactor: Move `contents` variable closer to where it's actuall…
SyntaxColoring Oct 9, 2023
9fa7816
Main event: Rework simulate.py to support simulating PAPIv≥2.14 proto…
SyntaxColoring Oct 10, 2023
b721299
Minor refactor: do .wrapped() in the same place between execute.py an…
SyntaxColoring Oct 10, 2023
f46b77f
Rework `adapt_protocol_source()` to take `Protocol` objects as input.
SyntaxColoring Oct 10, 2023
3516a4f
Minor refactor: Add explanatory comments for bundled_labware NotImple…
SyntaxColoring Oct 10, 2023
0a41b0b
Fixup: Delete noqa C901.
SyntaxColoring Oct 10, 2023
c5cbc27
Minor cleanup to protocol fixture.
SyntaxColoring Oct 11, 2023
3bdd399
Enable APIv≥2.14 simulate tests.
SyntaxColoring Oct 10, 2023
29a001e
Restore bundle-returning functionality.
SyntaxColoring Oct 11, 2023
79b91b4
Add failing test for `simulate()` without a filename.
SyntaxColoring Oct 11, 2023
5c20be0
Fix `Protocol.filename` lossily falling back to "<protocol>", causing…
SyntaxColoring Oct 10, 2023
711059e
Print Protocol Engine errors more nicely, consistent with opentrons_e…
SyntaxColoring Oct 11, 2023
ca8da73
Remove known issue from release notes.
SyntaxColoring Oct 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions api/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Flex touchscreen
- Manage instruments: View information about connected pipettes and the gripper. Attach, detach, or recalibrate instruments.
- Robot settings: Customize the behavior of your Flex, including the LED and touchscreen displays.

Flex features
Flex features

- Analyze and run protocols that use the Flex robot, Flex pipettes, and Flex tip racks.
- Move labware around the deck automatically with the Flex Gripper.
Expand All @@ -48,7 +48,6 @@ Python API features
Some protocols can't be simulated with the `opentrons_simulate` command-line tool:

- JSON protocols created or modified with Protocol Designer v6.0.0 or higher.
- Python protocols specifying an `apiLevel` of 2.14 or higher.

---

Expand Down
125 changes: 21 additions & 104 deletions api/src/opentrons/execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,12 @@
import contextlib
import logging
import os
from pathlib import Path
import sys
import tempfile
from typing import (
TYPE_CHECKING,
BinaryIO,
Callable,
Dict,
Generator,
List,
Optional,
TextIO,
Expand All @@ -35,7 +32,6 @@

from opentrons.hardware_control import (
API as OT2API,
HardwareControlAPI,
ThreadManagedHardware,
ThreadManager,
)
Expand All @@ -59,23 +55,16 @@
Config,
DeckType,
EngineStatus,
ErrorOccurrence as ProtocolEngineErrorOccurrence,
create_protocol_engine,
create_protocol_engine_in_thread,
)
from opentrons.protocol_engine.types import PostRunHardwareState

from opentrons.protocol_reader import ProtocolReader, ProtocolSource
from opentrons.protocol_reader import ProtocolSource

from opentrons.protocol_runner import create_protocol_runner

from .util.entrypoint_util import (
FoundLabware,
find_jupyter_labware,
labware_from_paths,
datafiles_from_paths,
copy_file_like,
)
from .util import entrypoint_util

if TYPE_CHECKING:
from opentrons_shared_data.labware.dev_types import (
Expand Down Expand Up @@ -172,7 +161,7 @@ def get_protocol_api(
if extra_labware is None:
extra_labware = {
uri: details.definition
for uri, details in (find_jupyter_labware() or {}).items()
for uri, details in (entrypoint_util.find_jupyter_labware() or {}).items()
}

robot_type = _get_robot_type()
Expand All @@ -191,6 +180,8 @@ def get_protocol_api(
)
else:
if bundled_labware is not None:
# Protocol Engine has a deep assumption that standard labware definitions are always
# implicitly loadable.
raise NotImplementedError(
f"The bundled_labware argument is not currently supported for Python protocols"
f" with apiLevel {ENGINE_CORE_API_VERSION} or newer."
Expand Down Expand Up @@ -363,21 +354,20 @@ def execute( # noqa: C901
stack_logger.propagate = propagate_logs
stack_logger.setLevel(getattr(logging, log_level.upper(), logging.WARNING))

contents = protocol_file.read()

# TODO(mm, 2023-10-02): Switch this truthy check to `is not None`
# to match documented behavior.
# See notes in https://github.com/Opentrons/opentrons/pull/13107
if custom_labware_paths:
extra_labware = labware_from_paths(custom_labware_paths)
extra_labware = entrypoint_util.labware_from_paths(custom_labware_paths)
else:
extra_labware = find_jupyter_labware() or {}
extra_labware = entrypoint_util.find_jupyter_labware() or {}

if custom_data_paths:
extra_data = datafiles_from_paths(custom_data_paths)
extra_data = entrypoint_util.datafiles_from_paths(custom_data_paths)
else:
extra_data = {}

contents = protocol_file.read()
try:
protocol = parse.parse(
contents,
Expand Down Expand Up @@ -422,10 +412,8 @@ def execute( # noqa: C901
)
protocol_file.seek(0)
_run_file_pe(
protocol_file=protocol_file,
protocol_name=protocol_name,
extra_labware=extra_labware,
hardware_api=_get_global_hardware_controller(_get_robot_type()).wrapped(),
protocol=protocol,
hardware_api=_get_global_hardware_controller(_get_robot_type()),
emit_runlog=emit_runlog,
)

Expand Down Expand Up @@ -493,49 +481,15 @@ def main() -> int:
emit_runlog=printer,
)
return 0
except _ProtocolEngineExecuteError as error:
# _ProtocolEngineExecuteError is a wrapper that's meaningless to the CLI user.
except entrypoint_util.ProtocolEngineExecuteError as error:
# This exception is a wrapper that's meaningless to the CLI user.
# Take the actual protocol problem out of it and just print that.
print(error.to_stderr_string(), file=sys.stderr)
return 1
# execute() might raise other exceptions, but we don't have a nice way to print those.
# Just let Python show a traceback.


class _ProtocolEngineExecuteError(Exception):
def __init__(self, errors: List[ProtocolEngineErrorOccurrence]) -> None:
"""Raised when there was any fatal error running a protocol through Protocol Engine.

Protocol Engine reports errors as data, not as exceptions.
But the only way for `execute()` to signal problems to its caller is to raise something.
So we need this class to wrap them.

Params:
errors: The errors that Protocol Engine reported.
"""
# Show the full error details if this is part of a traceback. Don't try to summarize.
super().__init__(errors)
self._error_occurrences = errors

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


def _create_live_context_non_pe(
api_version: APIVersion,
hardware_api: ThreadManagedHardware,
Expand Down Expand Up @@ -640,24 +594,22 @@ def _run_file_non_pe(


def _run_file_pe(
protocol_file: Union[BinaryIO, TextIO],
protocol_name: str,
extra_labware: Dict[str, FoundLabware],
hardware_api: HardwareControlAPI,
protocol: Protocol,
hardware_api: ThreadManagedHardware,
emit_runlog: Optional[_EmitRunlogCallable],
) -> None:
"""Run a protocol file with Protocol Engine."""

async def run(protocol_source: ProtocolSource) -> None:
protocol_engine = await create_protocol_engine(
hardware_api=hardware_api,
hardware_api=hardware_api.wrapped(),
config=_get_protocol_engine_config(),
)

protocol_runner = create_protocol_runner(
protocol_config=protocol_source.config,
protocol_engine=protocol_engine,
hardware_api=hardware_api,
hardware_api=hardware_api.wrapped(),
)

unsubscribe = protocol_runner.broker.subscribe(
Expand All @@ -671,13 +623,11 @@ async def run(protocol_source: ProtocolSource) -> None:
unsubscribe()

if result.state_summary.status != EngineStatus.SUCCEEDED:
raise _ProtocolEngineExecuteError(result.state_summary.errors)
raise entrypoint_util.ProtocolEngineExecuteError(
result.state_summary.errors
)

with _adapt_protocol_source(
protocol_file=protocol_file,
protocol_name=protocol_name,
extra_labware=extra_labware,
) as protocol_source:
with entrypoint_util.adapt_protocol_source(protocol) as protocol_source:
asyncio.run(run(protocol_source))


Expand All @@ -697,39 +647,6 @@ def _get_protocol_engine_config() -> Config:
)


@contextlib.contextmanager
def _adapt_protocol_source(
protocol_file: Union[BinaryIO, TextIO],
protocol_name: str,
extra_labware: Dict[str, FoundLabware],
) -> Generator[ProtocolSource, None, None]:
"""Create a `ProtocolSource` representing input protocol files."""
with tempfile.TemporaryDirectory() as temporary_directory:
# It's not well-defined in our customer-facing interfaces whether the supplied protocol_name
# should be just the filename part, or a path with separators. In case it contains stuff
# like "../", sanitize it to just the filename part so we don't save files somewhere bad.
safe_protocol_name = Path(protocol_name).name

temp_protocol_file = Path(temporary_directory) / safe_protocol_name

# FIXME(mm, 2023-06-26): Copying this file is pure overhead, and it introduces encoding
# hazards. Remove this when we can parse JSONv6+ and PAPIv2.14+ protocols without going
# through the filesystem. https://opentrons.atlassian.net/browse/RSS-281
copy_file_like(source=protocol_file, destination=temp_protocol_file)

custom_labware_files = [labware.path for labware in extra_labware.values()]

protocol_source = asyncio.run(
ProtocolReader().read_saved(
files=[temp_protocol_file] + custom_labware_files,
directory=None,
files_are_prevalidated=False,
)
)

yield protocol_source


def _get_global_hardware_controller(robot_type: RobotType) -> ThreadManagedHardware:
# Build a hardware controller in a worker thread, which is necessary
# because ipython runs its notebook in asyncio but the notebook
Expand Down
9 changes: 9 additions & 0 deletions api/src/opentrons/protocols/execution/execute_python.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,19 @@ def run_python(proto: PythonProtocol, context: ProtocolContext):
# If the protocol is written correctly, it will have defined a function
# like run(context: ProtocolContext). If so, that function is now in the
# current scope.

# TODO(mm, 2023-10-11): This coupling to opentrons.protocols.parse is fragile.
# Can we get the correct filename directly from proto.contents?
if proto.filename and proto.filename.endswith("zip"):
# The ".zip" extension needs to match what opentrons.protocols.parse recognizes as a bundle,
# and the "protocol.ot2.py" fallback needs to match what opentrons.protocol.py sets as the
# AST filename.
filename = "protocol.ot2.py"
else:
# "<protocol>" needs to match what opentrons.protocols.parse sets as the fallback
# AST filename.
filename = proto.filename or "<protocol>"

try:
_runfunc_ok(new_globs.get("run"))
except SyntaxError as se:
Expand Down
15 changes: 10 additions & 5 deletions api/src/opentrons/protocols/parse.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,16 @@ def _parse_python(
extra_labware: Optional[Dict[str, "LabwareDefinition"]] = None,
) -> PythonProtocol:
"""Parse a protocol known or at least suspected to be python"""
filename_checked = filename or "<protocol>"
if filename_checked.endswith(".zip"):
if filename is None:
# The fallback "<protocol>" needs to match what opentrons.protocols.execution.execute_python
# looks for when it extracts tracebacks.
ast_filename = "<protocol>"
elif filename.endswith(".zip"):
# The extension ".zip" and the fallback "protocol.ot2.py" need to match what
# opentrons.protocols.execution.execute_python looks for when it extracts tracebacks.
ast_filename = "protocol.ot2.py"
else:
ast_filename = filename_checked
ast_filename = filename

# todo(mm, 2021-09-13): By default, ast.parse will inherit compiler options
# and future features from this module. This may not be appropriate.
Expand All @@ -244,7 +249,7 @@ def _parse_python(

static_info = _extract_static_python_info(parsed)
protocol = compile(parsed, filename=ast_filename, mode="exec")
version = _get_version(static_info, parsed, filename_checked)
version = _get_version(static_info, parsed, ast_filename)
robot_type = _robot_type_from_static_python_info(static_info)

if version >= APIVersion(2, 0):
Expand All @@ -257,7 +262,7 @@ def _parse_python(

result = PythonProtocol(
text=protocol_contents,
filename=getattr(protocol, "co_filename", "<protocol>"),
filename=filename,
Comment on lines -260 to +265
Copy link
Contributor Author

@SyntaxColoring SyntaxColoring Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Falling back to "<protocol>" at this level was lossy. If we weren't given a filename, we want to preserve that information for the caller, by returning None, so they can choose their own fallback.

contents=protocol,
metadata=static_info.metadata,
api_level=version,
Expand Down
11 changes: 11 additions & 0 deletions api/src/opentrons/protocols/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,22 @@ class StaticPythonInfo:
@dataclass(frozen=True)
class _ProtocolCommon:
text: str

filename: Optional[str]
"""The original name of the main protocol file, if it had a name.

For JSON protocols, this will be the name of the .json file.
For Python protocols, this will be the name of the .py file.
For bundled protocols, this will be the name of the .zip file.

This can be `None` if, for example, we've parsed the protocol from an in-memory text stream.
"""

# TODO(mm, 2023-06-22): Move api_level out of _ProtocolCommon and into PythonProtocol.
# JSON protocols do not have an intrinsic api_level, especially since JSONv6,
# where they are no longer executed via the Python Protocol API.
api_level: "APIVersion"

robot_type: RobotType


Expand Down
Loading
Loading