From bd066efe3ccb2ffc339ed2c3ee0e2f7e6b20cd0e Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 25 Mar 2024 10:22:56 -0400 Subject: [PATCH 01/13] method for setting parameter values --- .../protocol_api/_parameter_context.py | 105 ++++++++++-------- .../protocols/parameters/validation.py | 57 +++++++++- .../protocol_api/test_parameter_context.py | 8 +- 3 files changed, 119 insertions(+), 51 deletions(-) diff --git a/api/src/opentrons/protocol_api/_parameter_context.py b/api/src/opentrons/protocol_api/_parameter_context.py index 6a503f7337a..f7a74c0931b 100644 --- a/api/src/opentrons/protocol_api/_parameter_context.py +++ b/api/src/opentrons/protocol_api/_parameter_context.py @@ -1,9 +1,9 @@ """Parameter context for python protocols.""" -from typing import List, Optional, Union +from typing import List, Optional, Union, Dict from opentrons.protocols.api_support.types import APIVersion -from opentrons.protocols.parameters import parameter_definition +from opentrons.protocols.parameters import parameter_definition, validation from opentrons.protocols.parameters.types import ParameterChoice from ._parameters import Parameters @@ -22,7 +22,7 @@ class ParameterContext: def __init__(self, api_version: APIVersion) -> None: """Initializes a parameter context for user-set parameters.""" self._api_version = api_version - self._parameters: List[_ParameterDefinitionTypes] = [] + self._parameters: Dict[str, _ParameterDefinitionTypes] = {} def add_int( self, @@ -48,18 +48,17 @@ def add_int( description: A description of the parameter as it will show up on the frontend. unit: An optional unit to be appended to the end of the integer as it shown on the frontend. """ - self._parameters.append( - parameter_definition.create_int_parameter( - display_name=display_name, - variable_name=variable_name, - default=default, - minimum=minimum, - maximum=maximum, - choices=choices, - description=description, - unit=unit, - ) + parameter = parameter_definition.create_int_parameter( + display_name=display_name, + variable_name=variable_name, + default=default, + minimum=minimum, + maximum=maximum, + choices=choices, + description=description, + unit=unit, ) + self._parameters[parameter.variable_name] = parameter def add_float( self, @@ -85,18 +84,17 @@ def add_float( description: A description of the parameter as it will show up on the frontend. unit: An optional unit to be appended to the end of the float as it shown on the frontend. """ - self._parameters.append( - parameter_definition.create_float_parameter( - display_name=display_name, - variable_name=variable_name, - default=default, - minimum=minimum, - maximum=maximum, - choices=choices, - description=description, - unit=unit, - ) + parameter = parameter_definition.create_float_parameter( + display_name=display_name, + variable_name=variable_name, + default=default, + minimum=minimum, + maximum=maximum, + choices=choices, + description=description, + unit=unit, ) + self._parameters[parameter.variable_name] = parameter def add_bool( self, @@ -113,18 +111,17 @@ def add_bool( default: The default value the boolean parameter will be set to. This will be used in initial analysis. description: A description of the parameter as it will show up on the frontend. """ - self._parameters.append( - parameter_definition.create_bool_parameter( - display_name=display_name, - variable_name=variable_name, - default=default, - choices=[ - {"display_name": "On", "value": True}, - {"display_name": "Off", "value": False}, - ], - description=description, - ) + parameter = parameter_definition.create_bool_parameter( + display_name=display_name, + variable_name=variable_name, + default=default, + choices=[ + {"display_name": "On", "value": True}, + {"display_name": "Off", "value": False}, + ], + description=description, ) + self._parameters[parameter.variable_name] = parameter def add_str( self, @@ -144,15 +141,33 @@ def add_str( Mutually exclusive with minimum and maximum. description: A description of the parameter as it will show up on the frontend. """ - self._parameters.append( - parameter_definition.create_str_parameter( - display_name=display_name, - variable_name=variable_name, - default=default, - choices=choices, - description=description, - ) + parameter = parameter_definition.create_str_parameter( + display_name=display_name, + variable_name=variable_name, + default=default, + choices=choices, + description=description, ) + self._parameters[parameter.variable_name] = parameter + + def set_parameters( + self, parameter_overrides: Dict[str, Union[float, bool, str]] + ) -> None: + """Sets parameters to values given by client, validating them as well. + + :meta private: + + This is intended for Opentrons internal use only and is not a guaranteed API. + """ + for variable_name, override_value in parameter_overrides.items(): + try: + parameter = self._parameters[variable_name] + except KeyError: + raise ValueError("put some error here") + validated_value = validation.ensure_value_type( + override_value, parameter.parameter_type + ) + parameter.value = validated_value def export_parameters(self) -> Parameters: """Exports all parameters into a protocol run usable parameters object. @@ -164,6 +179,6 @@ def export_parameters(self) -> Parameters: return Parameters( parameters={ parameter.variable_name: parameter.value - for parameter in self._parameters + for parameter in self._parameters.values() } ) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 9b4cae7354e..31edf89580d 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -1,7 +1,8 @@ import keyword -from typing import List, Optional +from typing import List, Optional, Union, Literal from .types import ( + AllowedTypes, ParamType, ParameterChoice, ParameterNameError, @@ -53,6 +54,58 @@ def ensure_unit_string_length(unit: Optional[str]) -> Optional[str]: return unit +def ensure_value_type( + value: Union[float, bool, str], parameter_type: type +) -> AllowedTypes: + """Ensures that the value type coming in from the client matches the given type.""" + validated_value: AllowedTypes + if isinstance(value, float) and parameter_type is int and value.is_integer(): + validated_value = int(value) + else: + validated_value = value + return validated_value + + +def ensure_enum_type(value: AllowedTypes) -> Union[float, str]: + """Ensure that the value that is sent over from the client is the expected type.""" + validated_value: Union[float, bool, str] + if isinstance(value, int): + validated_value = float(value) + elif isinstance(value, bool): + raise ParameterValueError("Cannot send a boolean value as an enum type") + else: + validated_value = value + return validated_value + + +def convert_type_string_for_enum( + parameter_type: type, +) -> Literal["int", "float", "str"]: + """Converts a type object into a string for an enumerated parameter.""" + if parameter_type is int: + return "int" + elif parameter_type is float: + return "float" + elif parameter_type is str: + return "str" + else: + raise ParameterValueError( + f"Cannot resolve parameter type {parameter_type} for an enumerated parameter." + ) + + +def convert_type_string_for_num_param(parameter_type: type) -> Literal["int", "float"]: + """Converts a type object into a string for an enumerated parameter.""" + if parameter_type is int: + return "int" + elif parameter_type is float: + return "float" + else: + raise ParameterValueError( + f"Cannot resolve parameter type {parameter_type} for a number parameter." + ) + + def _validate_choices( minimum: Optional[ParamType], maximum: Optional[ParamType], @@ -116,7 +169,7 @@ def validate_type(value: ParamType, parameter_type: type) -> None: """Validate parameter value is the correct type.""" if not isinstance(value, parameter_type): raise ParameterValueError( - f"Default parameter value has type {type(value)} must match type {parameter_type}." + f"Parameter value has type {type(value)} must match type {parameter_type}." ) diff --git a/api/tests/opentrons/protocol_api/test_parameter_context.py b/api/tests/opentrons/protocol_api/test_parameter_context.py index dd4c6fb8a74..c2d6cdd74be 100644 --- a/api/tests/opentrons/protocol_api/test_parameter_context.py +++ b/api/tests/opentrons/protocol_api/test_parameter_context.py @@ -59,7 +59,7 @@ def test_add_int(decoy: Decoy, subject: ParameterContext) -> None: description="blah blah blah", unit="foot candles", ) - assert param_def in subject._parameters + assert param_def is subject._parameters["xyz"] def test_add_float(decoy: Decoy, subject: ParameterContext) -> None: @@ -87,7 +87,7 @@ def test_add_float(decoy: Decoy, subject: ParameterContext) -> None: description="blah blah blah", unit="lux", ) - assert param_def in subject._parameters + assert param_def is subject._parameters["xyz"] def test_add_bool(decoy: Decoy, subject: ParameterContext) -> None: @@ -111,7 +111,7 @@ def test_add_bool(decoy: Decoy, subject: ParameterContext) -> None: default=False, description="lorem ipsum", ) - assert param_def in subject._parameters + assert param_def is subject._parameters["zxy"] def test_add_string(decoy: Decoy, subject: ParameterContext) -> None: @@ -133,4 +133,4 @@ def test_add_string(decoy: Decoy, subject: ParameterContext) -> None: choices=[{"display_name": "bar", "value": "aaa"}], description="fee foo fum", ) - assert param_def in subject._parameters + assert param_def is subject._parameters["qwerty"] From edcecb0bfc8d987abdfeb15be12989228020142f Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 25 Mar 2024 10:51:25 -0400 Subject: [PATCH 02/13] convert parameters to PE types and fix tests --- .../parameters/parameter_definition.py | 60 +++++++++++++++++++ .../protocol_api/test_parameter_context.py | 12 ++-- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/protocols/parameters/parameter_definition.py b/api/src/opentrons/protocols/parameters/parameter_definition.py index 54c27b1840d..b38ffbde850 100644 --- a/api/src/opentrons/protocols/parameters/parameter_definition.py +++ b/api/src/opentrons/protocols/parameters/parameter_definition.py @@ -10,6 +10,13 @@ ParameterValueError, ) from opentrons.protocols.parameters import validation +from opentrons.protocol_engine.types import ( + RunTimeParameter, + NumberParameter, + BooleanParameter, + EnumParameter, + EnumChoice, +) class ParameterDefinition(Generic[ParamType]): @@ -104,6 +111,59 @@ def variable_name(self) -> str: """The in-protocol variable name of the parameter.""" return self._variable_name + @property + def parameter_type(self) -> type: + """The python type of the parameter.""" + return self._type + + def as_protocol_engine_type(self) -> RunTimeParameter: + """Returns parameter as a Protocol Engine type to send to client.""" + parameter: RunTimeParameter + if self._type is bool: + parameter = BooleanParameter( + type="bool", + displayName=self._display_name, + variableName=self._variable_name, + description=self._description, + value=bool(self._value), + default=bool(self._default), + ) + elif self._choices is not None: + choices = [ + EnumChoice( + displayName=str(choice["display_name"]), + value=validation.ensure_enum_type(choice["value"]), + ) + for choice in self._choices + ] + parameter = EnumParameter( + type=validation.convert_type_string_for_enum(self._type), + displayName=self._display_name, + variableName=self._variable_name, + description=self._description, + choices=choices, + value=validation.ensure_enum_type(self._value), + default=validation.ensure_enum_type(self._default), + ) + elif self._minimum is not None and self._maximum is not None: + parameter = NumberParameter( + type=validation.convert_type_string_for_num_param(self._type), + displayName=self._display_name, + variableName=self._variable_name, + description=self._description, + suffix=self._unit, + min=float(self._minimum), + max=float(self._maximum), + value=float(self._value), + default=float(self._default), + ) + else: + raise ParameterDefinitionError( + f"Cannot resolve parameter {self._display_name} to protocol engine type." + ) + + return parameter + def create_int_parameter( display_name: str, diff --git a/api/tests/opentrons/protocol_api/test_parameter_context.py b/api/tests/opentrons/protocol_api/test_parameter_context.py index c2d6cdd74be..7c90fde27ec 100644 --- a/api/tests/opentrons/protocol_api/test_parameter_context.py +++ b/api/tests/opentrons/protocol_api/test_parameter_context.py @@ -37,6 +37,7 @@ def subject(api_version: APIVersion) -> ParameterContext: def test_add_int(decoy: Decoy, subject: ParameterContext) -> None: """It should create and add an int parameter definition.""" param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + decoy.when(param_def.variable_name).then_return("my cool variable") decoy.when( mock_parameter_definition.create_int_parameter( display_name="abc", @@ -59,12 +60,13 @@ def test_add_int(decoy: Decoy, subject: ParameterContext) -> None: description="blah blah blah", unit="foot candles", ) - assert param_def is subject._parameters["xyz"] + assert param_def is subject._parameters["my cool variable"] def test_add_float(decoy: Decoy, subject: ParameterContext) -> None: """It should create and add a float parameter definition.""" param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + decoy.when(param_def.variable_name).then_return("my cooler variable") decoy.when( mock_parameter_definition.create_float_parameter( display_name="abc", @@ -87,12 +89,13 @@ def test_add_float(decoy: Decoy, subject: ParameterContext) -> None: description="blah blah blah", unit="lux", ) - assert param_def is subject._parameters["xyz"] + assert param_def is subject._parameters["my cooler variable"] def test_add_bool(decoy: Decoy, subject: ParameterContext) -> None: """It should create and add a boolean parameter definition.""" param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + decoy.when(param_def.variable_name).then_return("my coolest variable") decoy.when( mock_parameter_definition.create_bool_parameter( display_name="cba", @@ -111,12 +114,13 @@ def test_add_bool(decoy: Decoy, subject: ParameterContext) -> None: default=False, description="lorem ipsum", ) - assert param_def is subject._parameters["zxy"] + assert param_def is subject._parameters["my coolest variable"] def test_add_string(decoy: Decoy, subject: ParameterContext) -> None: """It should create and add a string parameter definition.""" param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + decoy.when(param_def.variable_name).then_return("my slightly less cool variable") decoy.when( mock_parameter_definition.create_str_parameter( display_name="jkl", @@ -133,4 +137,4 @@ def test_add_string(decoy: Decoy, subject: ParameterContext) -> None: choices=[{"display_name": "bar", "value": "aaa"}], description="fee foo fum", ) - assert param_def is subject._parameters["qwerty"] + assert param_def is subject._parameters["my slightly less cool variable"] From 4e9471de4b3ac3a2cae5453e8bddf84ea6019fc7 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 25 Mar 2024 11:47:11 -0400 Subject: [PATCH 03/13] refactor protocol runner to own parameter context --- .../protocol_runner/legacy_wrappers.py | 6 +++++- .../protocol_runner/protocol_runner.py | 6 ++++++ .../opentrons/protocols/execution/execute.py | 19 ++++++++++++------ .../protocols/execution/execute_python.py | 20 ++++++++++++++----- .../protocol_runner/test_protocol_runner.py | 2 ++ 5 files changed, 41 insertions(+), 12 deletions(-) diff --git a/api/src/opentrons/protocol_runner/legacy_wrappers.py b/api/src/opentrons/protocol_runner/legacy_wrappers.py index c7a4e2852ba..e20dca9d628 100644 --- a/api/src/opentrons/protocol_runner/legacy_wrappers.py +++ b/api/src/opentrons/protocol_runner/legacy_wrappers.py @@ -30,6 +30,7 @@ ModuleContext as LegacyModuleContext, Labware as LegacyLabware, Well as LegacyWell, + ParameterContext, create_protocol_context, ) from opentrons.protocol_api.core.engine import ENGINE_CORE_API_VERSION @@ -172,10 +173,13 @@ class LegacyExecutor: async def execute( protocol: LegacyProtocol, context: LegacyProtocolContext, + parameter_context: ParameterContext, run_time_param_values: Optional[RunTimeParamValuesType], ) -> None: """Execute a PAPIv2 protocol with a given ProtocolContext in a child thread.""" - await to_thread.run_sync(run_protocol, protocol, context, run_time_param_values) + await to_thread.run_sync( + run_protocol, protocol, context, parameter_context, run_time_param_values + ) __all__ = [ diff --git a/api/src/opentrons/protocol_runner/protocol_runner.py b/api/src/opentrons/protocol_runner/protocol_runner.py index d2c67b9cfb3..a8b44fb7eb6 100644 --- a/api/src/opentrons/protocol_runner/protocol_runner.py +++ b/api/src/opentrons/protocol_runner/protocol_runner.py @@ -9,6 +9,7 @@ from opentrons.hardware_control import HardwareControlAPI from opentrons import protocol_reader from opentrons.legacy_broker import LegacyBroker +from opentrons.protocol_api import ParameterContext, MAX_SUPPORTED_VERSION from opentrons.protocol_reader import ( ProtocolSource, JsonProtocolConfig, @@ -150,6 +151,9 @@ def __init__( drop_tips_after_run=drop_tips_after_run, post_run_hardware_state=post_run_hardware_state, ) + # We need to define this here so we can use it throughout this class, but it will be recreated with the proper + # api level version upon a protocol loading. + self._parameter_context = ParameterContext(api_version=MAX_SUPPORTED_VERSION) async def load( self, @@ -171,6 +175,7 @@ async def load( protocol = self._legacy_file_reader.read( protocol_source, labware_definitions, python_parse_mode ) + self._parameter_context = ParameterContext(api_version=protocol.api_level) equipment_broker = None if protocol.api_level < LEGACY_PYTHON_API_VERSION_CUTOFF: @@ -199,6 +204,7 @@ async def load( func=self._legacy_executor.execute, protocol=protocol, context=context, + parameter_context=self._parameter_context, run_time_param_values=run_time_param_values, ) diff --git a/api/src/opentrons/protocols/execution/execute.py b/api/src/opentrons/protocols/execution/execute.py index f49da9160bd..084df9402a6 100644 --- a/api/src/opentrons/protocols/execution/execute.py +++ b/api/src/opentrons/protocols/execution/execute.py @@ -1,7 +1,8 @@ import logging -from typing import Optional, Dict, Union +from typing import Optional -from opentrons.protocol_api import ProtocolContext +from opentrons.protocol_api import ProtocolContext, ParameterContext +from opentrons.protocol_engine.types import RunTimeParamValuesType from opentrons.protocols.execution.execute_python import run_python from opentrons.protocols.execution.json_dispatchers import ( pipette_command_map, @@ -20,17 +21,23 @@ def run_protocol( protocol: Protocol, context: ProtocolContext, - # TODO (spp, 2024-03-20): move RunTimeParamValuesType to a top level types and use here - run_time_param_overrides: Optional[Dict[str, Union[float, bool, str]]] = None, + parameter_context: Optional[ParameterContext] = None, + run_time_param_overrides: Optional[RunTimeParamValuesType] = None, ) -> None: """Run a protocol. :param protocol: The :py:class:`.protocols.types.Protocol` to execute - :param context: The context to use. + :param context: The protocol context to use. + :param parameter_context: The parameter context to use. + :param run_time_param_overrides: Any parameter values that are potentially overriding the defaults """ if isinstance(protocol, PythonProtocol): if protocol.api_level >= APIVersion(2, 0): - run_python(protocol, context) + # If this is None here then we're either running simulate or execute, in any case we don't need to report + # this in analysis which is the reason we'd pass it to this function + if parameter_context is None: + parameter_context = ParameterContext(protocol.api_level) + run_python(protocol, context, parameter_context, run_time_param_overrides) else: raise RuntimeError(f"Unsupported python API version: {protocol.api_level}") else: diff --git a/api/src/opentrons/protocols/execution/execute_python.py b/api/src/opentrons/protocols/execution/execute_python.py index 6deab339fc8..a03aa649040 100644 --- a/api/src/opentrons/protocols/execution/execute_python.py +++ b/api/src/opentrons/protocols/execution/execute_python.py @@ -3,13 +3,14 @@ import logging import traceback import sys -from typing import Any, Dict +from typing import Any, Dict, Optional from opentrons.drivers.smoothie_drivers.errors import SmoothieAlarm from opentrons.protocol_api import ProtocolContext, ParameterContext from opentrons.protocol_api._parameters import Parameters from opentrons.protocols.execution.errors import ExceptionInProtocolError from opentrons.protocols.types import PythonProtocol, MalformedPythonProtocolError +from opentrons.protocol_engine.types import RunTimeParamValuesType from opentrons_shared_data.errors.exceptions import ExecutionCancelledError @@ -65,13 +66,15 @@ def _raise_pretty_protocol_error(exception: Exception, filename: str) -> None: def _parse_and_set_parameters( - protocol: PythonProtocol, new_globs: Dict[Any, Any], filename: str + parameter_context: ParameterContext, + run_time_param_overrides: Optional[RunTimeParamValuesType], + new_globs: Dict[Any, Any], + filename: str, ) -> Parameters: try: _add_parameters_func_ok(new_globs.get("add_parameters")) except SyntaxError as se: raise MalformedPythonProtocolError(str(se)) - parameter_context = ParameterContext(api_version=protocol.api_level) new_globs["__param_context"] = parameter_context try: exec("add_parameters(__param_context)", new_globs) @@ -80,7 +83,12 @@ def _parse_and_set_parameters( return parameter_context.export_parameters() -def run_python(proto: PythonProtocol, context: ProtocolContext): +def run_python( + proto: PythonProtocol, + context: ProtocolContext, + parameter_context: ParameterContext, + run_time_param_overrides: Optional[RunTimeParamValuesType] = None, +) -> None: new_globs: Dict[Any, Any] = {} exec(proto.contents, new_globs) # If the protocol is written correctly, it will have defined a function @@ -100,7 +108,9 @@ def run_python(proto: PythonProtocol, context: ProtocolContext): filename = proto.filename or "" if new_globs.get("add_parameters"): - context._params = _parse_and_set_parameters(proto, new_globs, filename) + context._params = _parse_and_set_parameters( + parameter_context, run_time_param_overrides, new_globs, filename + ) try: _runfunc_ok(new_globs.get("run")) diff --git a/api/tests/opentrons/protocol_runner/test_protocol_runner.py b/api/tests/opentrons/protocol_runner/test_protocol_runner.py index 64034e663bd..7cb13f82a23 100644 --- a/api/tests/opentrons/protocol_runner/test_protocol_runner.py +++ b/api/tests/opentrons/protocol_runner/test_protocol_runner.py @@ -488,6 +488,7 @@ async def test_load_legacy_python( func=legacy_executor.execute, protocol=legacy_protocol, context=legacy_context, + parameter_context=legacy_python_runner_subject._parameter_context, run_time_param_values=None, ), ) @@ -622,6 +623,7 @@ async def test_load_legacy_json( func=legacy_executor.execute, protocol=legacy_protocol, context=legacy_context, + parameter_context=legacy_python_runner_subject._parameter_context, run_time_param_values=None, ), ) From 852ed1edcf8a48345195f04219d87e3228b46e39 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 25 Mar 2024 13:35:49 -0400 Subject: [PATCH 04/13] include parameters in RunResult --- .../protocol_api/_parameter_context.py | 22 ++++++++++++++----- .../protocol_runner/protocol_runner.py | 16 +++++++++++--- .../protocols/execution/execute_python.py | 2 +- .../maintenance_engine_store.py | 2 +- .../robot_server/runs/engine_store.py | 9 +++++++- .../robot_server/runs/run_data_manager.py | 2 +- .../tests/protocols/test_protocol_analyzer.py | 1 + .../tests/runs/test_run_controller.py | 1 + .../tests/runs/test_run_data_manager.py | 8 +++++-- 9 files changed, 48 insertions(+), 15 deletions(-) diff --git a/api/src/opentrons/protocol_api/_parameter_context.py b/api/src/opentrons/protocol_api/_parameter_context.py index f7a74c0931b..d7e7b6bee25 100644 --- a/api/src/opentrons/protocol_api/_parameter_context.py +++ b/api/src/opentrons/protocol_api/_parameter_context.py @@ -4,7 +4,11 @@ from opentrons.protocols.api_support.types import APIVersion from opentrons.protocols.parameters import parameter_definition, validation -from opentrons.protocols.parameters.types import ParameterChoice +from opentrons.protocols.parameters.types import ( + ParameterChoice, + ParameterDefinitionError, +) +from opentrons.protocol_engine.types import RunTimeParameter, RunTimeParamValuesType from ._parameters import Parameters @@ -150,9 +154,7 @@ def add_str( ) self._parameters[parameter.variable_name] = parameter - def set_parameters( - self, parameter_overrides: Dict[str, Union[float, bool, str]] - ) -> None: + def set_parameters(self, parameter_overrides: RunTimeParamValuesType) -> None: """Sets parameters to values given by client, validating them as well. :meta private: @@ -163,13 +165,21 @@ def set_parameters( try: parameter = self._parameters[variable_name] except KeyError: - raise ValueError("put some error here") + raise ParameterDefinitionError( + f"Parameter {variable_name} is not defined as a parameter for this protocol." + ) validated_value = validation.ensure_value_type( override_value, parameter.parameter_type ) parameter.value = validated_value - def export_parameters(self) -> Parameters: + def export_parameters_for_analysis(self) -> List[RunTimeParameter]: + return [ + parameter.as_protocol_engine_type() + for parameter in self._parameters.values() + ] + + def export_parameters_for_protocol(self) -> Parameters: """Exports all parameters into a protocol run usable parameters object. :meta private: diff --git a/api/src/opentrons/protocol_runner/protocol_runner.py b/api/src/opentrons/protocol_runner/protocol_runner.py index a8b44fb7eb6..6667d0982e5 100644 --- a/api/src/opentrons/protocol_runner/protocol_runner.py +++ b/api/src/opentrons/protocol_runner/protocol_runner.py @@ -39,6 +39,7 @@ from ..protocol_engine.types import ( PostRunHardwareState, DeckConfigurationType, + RunTimeParameter, RunTimeParamValuesType, ) @@ -48,6 +49,7 @@ class RunResult(NamedTuple): commands: List[Command] state_summary: StateSummary + parameters: List[RunTimeParameter] class AbstractRunner(ABC): @@ -155,6 +157,11 @@ def __init__( # api level version upon a protocol loading. self._parameter_context = ParameterContext(api_version=MAX_SUPPORTED_VERSION) + @property + def run_time_parameters(self) -> List[RunTimeParameter]: + """Parameter definitions defined by protocol, if any. Will always be empty before execution.""" + return self._parameter_context.export_parameters_for_analysis() + async def load( self, protocol_source: ProtocolSource, @@ -230,7 +237,10 @@ async def run( # noqa: D102 run_data = self._protocol_engine.state_view.get_summary() commands = self._protocol_engine.state_view.commands.get_all() - return RunResult(commands=commands, state_summary=run_data) + parameters = self._parameter_context.export_parameters_for_analysis() + return RunResult( + commands=commands, state_summary=run_data, parameters=parameters + ) class JsonRunner(AbstractRunner): @@ -334,7 +344,7 @@ async def run( # noqa: D102 run_data = self._protocol_engine.state_view.get_summary() commands = self._protocol_engine.state_view.commands.get_all() - return RunResult(commands=commands, state_summary=run_data) + return RunResult(commands=commands, state_summary=run_data, parameters=[]) class LiveRunner(AbstractRunner): @@ -375,7 +385,7 @@ async def run( # noqa: D102 run_data = self._protocol_engine.state_view.get_summary() commands = self._protocol_engine.state_view.commands.get_all() - return RunResult(commands=commands, state_summary=run_data) + return RunResult(commands=commands, state_summary=run_data, parameters=[]) AnyRunner = Union[PythonAndLegacyRunner, JsonRunner, LiveRunner] diff --git a/api/src/opentrons/protocols/execution/execute_python.py b/api/src/opentrons/protocols/execution/execute_python.py index a03aa649040..77954c2b0b5 100644 --- a/api/src/opentrons/protocols/execution/execute_python.py +++ b/api/src/opentrons/protocols/execution/execute_python.py @@ -80,7 +80,7 @@ def _parse_and_set_parameters( exec("add_parameters(__param_context)", new_globs) except Exception as e: _raise_pretty_protocol_error(exception=e, filename=filename) - return parameter_context.export_parameters() + return parameter_context.export_parameters_for_protocol() def run_python( diff --git a/robot-server/robot_server/maintenance_runs/maintenance_engine_store.py b/robot-server/robot_server/maintenance_runs/maintenance_engine_store.py index e7935b44e83..8e42cbf2cae 100644 --- a/robot-server/robot_server/maintenance_runs/maintenance_engine_store.py +++ b/robot-server/robot_server/maintenance_runs/maintenance_engine_store.py @@ -197,4 +197,4 @@ async def clear(self) -> RunResult: commands = state_view.commands.get_all() self._runner_engine_pair = None - return RunResult(state_summary=run_data, commands=commands) + return RunResult(state_summary=run_data, commands=commands, parameters=[]) diff --git a/robot-server/robot_server/runs/engine_store.py b/robot-server/robot_server/runs/engine_store.py index 009f3bb2ecd..7112b311de4 100644 --- a/robot-server/robot_server/runs/engine_store.py +++ b/robot-server/robot_server/runs/engine_store.py @@ -244,6 +244,7 @@ async def clear(self) -> RunResult: """ engine = self.engine state_view = engine.state_view + runner = self.runner if state_view.commands.get_is_okay_to_clear(): await engine.finish( @@ -256,6 +257,12 @@ async def clear(self) -> RunResult: run_data = state_view.get_summary() commands = state_view.commands.get_all() + run_time_parameters = [] + if isinstance(runner, PythonAndLegacyRunner): + run_time_parameters = runner.run_time_parameters + self._runner_engine_pair = None - return RunResult(state_summary=run_data, commands=commands) + return RunResult( + state_summary=run_data, commands=commands, parameters=run_time_parameters + ) diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 0515ee3bc26..92c7d5e12b5 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -294,7 +294,7 @@ async def update(self, run_id: str, current: Optional[bool]) -> Union[Run, BadRu next_current = current if current is False else True if next_current is False: - commands, state_summary = await self._engine_store.clear() + commands, state_summary, parameters = await self._engine_store.clear() run_resource: Union[ RunResource, BadRunResource ] = self._run_store.update_run_state( diff --git a/robot-server/tests/protocols/test_protocol_analyzer.py b/robot-server/tests/protocols/test_protocol_analyzer.py index 6492b815357..94ad467f571 100644 --- a/robot-server/tests/protocols/test_protocol_analyzer.py +++ b/robot-server/tests/protocols/test_protocol_analyzer.py @@ -148,6 +148,7 @@ async def test_analyze( labwareOffsets=[], liquids=[], ), + parameters=[], ) ) diff --git a/robot-server/tests/runs/test_run_controller.py b/robot-server/tests/runs/test_run_controller.py index 8853755e575..5bf5778c486 100644 --- a/robot-server/tests/runs/test_run_controller.py +++ b/robot-server/tests/runs/test_run_controller.py @@ -153,6 +153,7 @@ async def test_create_play_action_to_start( RunResult( commands=protocol_commands, state_summary=engine_state_summary, + parameters=[], ) ) diff --git a/robot-server/tests/runs/test_run_data_manager.py b/robot-server/tests/runs/test_run_data_manager.py index d4bc37ea3d0..92152eb3940 100644 --- a/robot-server/tests/runs/test_run_data_manager.py +++ b/robot-server/tests/runs/test_run_data_manager.py @@ -519,7 +519,9 @@ async def test_update_current( run_id = "hello world" decoy.when(mock_engine_store.current_run_id).then_return(run_id) decoy.when(await mock_engine_store.clear()).then_return( - RunResult(commands=[run_command], state_summary=engine_state_summary) + RunResult( + commands=[run_command], state_summary=engine_state_summary, parameters=[] + ) ) decoy.when( @@ -627,7 +629,9 @@ async def test_create_archives_existing( decoy.when(mock_engine_store.current_run_id).then_return(run_id_old) decoy.when(await mock_engine_store.clear()).then_return( - RunResult(commands=[run_command], state_summary=engine_state_summary) + RunResult( + commands=[run_command], state_summary=engine_state_summary, parameters=[] + ) ) decoy.when( From 7c277ccabccd346fdbf7e4d0b7a51a6c98febbd0 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 25 Mar 2024 15:35:37 -0400 Subject: [PATCH 05/13] report parameters in analysis response --- api/src/opentrons/cli/analyze.py | 4 +--- api/src/opentrons/protocol_runner/protocol_runner.py | 5 +++++ .../protocols/parameters/parameter_definition.py | 1 - .../robot_server/protocols/protocol_analyzer.py | 4 +--- robot-server/robot_server/runs/engine_store.py | 4 +--- .../tests/protocols/test_protocol_analyzer.py | 12 ++++++++++-- 6 files changed, 18 insertions(+), 12 deletions(-) diff --git a/api/src/opentrons/cli/analyze.py b/api/src/opentrons/cli/analyze.py index b2b7d7747a8..a42a4f5f868 100644 --- a/api/src/opentrons/cli/analyze.py +++ b/api/src/opentrons/cli/analyze.py @@ -100,9 +100,7 @@ async def _analyze( ), metadata=protocol_source.metadata, robotType=protocol_source.robot_type, - # TODO(spp, 2024-03-12): update this once protocol reader/ runner can parse - # and report the runTimeParameters - runTimeParameters=[], + runTimeParameters=analysis.parameters, commands=analysis.commands, errors=analysis.state_summary.errors, labware=analysis.state_summary.labware, diff --git a/api/src/opentrons/protocol_runner/protocol_runner.py b/api/src/opentrons/protocol_runner/protocol_runner.py index 6667d0982e5..0703847e0d6 100644 --- a/api/src/opentrons/protocol_runner/protocol_runner.py +++ b/api/src/opentrons/protocol_runner/protocol_runner.py @@ -82,6 +82,11 @@ def broker(self) -> LegacyBroker: """ return self._broker + @property + def run_time_parameters(self) -> List[RunTimeParameter]: + """Parameter definitions defined by protocol, if any. Currently only for python protocols.""" + return [] + def was_started(self) -> bool: """Whether the run has been started. diff --git a/api/src/opentrons/protocols/parameters/parameter_definition.py b/api/src/opentrons/protocols/parameters/parameter_definition.py index b38ffbde850..8e0231d4d5e 100644 --- a/api/src/opentrons/protocols/parameters/parameter_definition.py +++ b/api/src/opentrons/protocols/parameters/parameter_definition.py @@ -121,7 +121,6 @@ def as_protocol_engine_type(self) -> RunTimeParameter: parameter: RunTimeParameter if self._type is bool: parameter = BooleanParameter( - type="bool", displayName=self._display_name, variableName=self._variable_name, description=self._description, diff --git a/robot-server/robot_server/protocols/protocol_analyzer.py b/robot-server/robot_server/protocols/protocol_analyzer.py index b5d208a1f17..1ecee27e1da 100644 --- a/robot-server/robot_server/protocols/protocol_analyzer.py +++ b/robot-server/robot_server/protocols/protocol_analyzer.py @@ -72,9 +72,7 @@ async def analyze( await self._analysis_store.update( analysis_id=analysis_id, robot_type=protocol_resource.source.robot_type, - # TODO(spp, 2024-03-12): update this once protocol reader/ runner can parse - # and report the runTimeParameters - run_time_parameters=[], + run_time_parameters=result.parameters, commands=result.commands, labware=result.state_summary.labware, modules=result.state_summary.modules, diff --git a/robot-server/robot_server/runs/engine_store.py b/robot-server/robot_server/runs/engine_store.py index 7112b311de4..aa5b26d4a77 100644 --- a/robot-server/robot_server/runs/engine_store.py +++ b/robot-server/robot_server/runs/engine_store.py @@ -257,9 +257,7 @@ async def clear(self) -> RunResult: run_data = state_view.get_summary() commands = state_view.commands.get_all() - run_time_parameters = [] - if isinstance(runner, PythonAndLegacyRunner): - run_time_parameters = runner.run_time_parameters + run_time_parameters = runner.run_time_parameters self._runner_engine_pair = None diff --git a/robot-server/tests/protocols/test_protocol_analyzer.py b/robot-server/tests/protocols/test_protocol_analyzer.py index 94ad467f571..c0c63e9e34f 100644 --- a/robot-server/tests/protocols/test_protocol_analyzer.py +++ b/robot-server/tests/protocols/test_protocol_analyzer.py @@ -120,6 +120,14 @@ async def test_analyze( mount=MountType.LEFT, ) + analysis_parameter = pe_types.BooleanParameter( + displayName="Display Name", + variableName="variable_name", + type="bool", + value=False, + default=True, + ) + json_runner = decoy.mock(cls=protocol_runner.JsonRunner) decoy.when( @@ -148,7 +156,7 @@ async def test_analyze( labwareOffsets=[], liquids=[], ), - parameters=[], + parameters=[analysis_parameter], ) ) @@ -162,7 +170,7 @@ async def test_analyze( await analysis_store.update( analysis_id="analysis-id", robot_type=robot_type, - run_time_parameters=[], + run_time_parameters=[analysis_parameter], commands=[analysis_command], labware=[analysis_labware], modules=[], From 069ee15c42dae552eb57d2f5491aae6409689422 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Tue, 26 Mar 2024 10:41:28 -0400 Subject: [PATCH 06/13] add missing docstring --- api/src/opentrons/protocol_api/_parameter_context.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/api/src/opentrons/protocol_api/_parameter_context.py b/api/src/opentrons/protocol_api/_parameter_context.py index d7e7b6bee25..e16273b2a33 100644 --- a/api/src/opentrons/protocol_api/_parameter_context.py +++ b/api/src/opentrons/protocol_api/_parameter_context.py @@ -174,6 +174,12 @@ def set_parameters(self, parameter_overrides: RunTimeParamValuesType) -> None: parameter.value = validated_value def export_parameters_for_analysis(self) -> List[RunTimeParameter]: + """Exports all parameters into a protocol engine models for reporting in analysis. + + :meta private: + + This is intended for Opentrons internal use only and is not a guaranteed API. + """ return [ parameter.as_protocol_engine_type() for parameter in self._parameters.values() From 5e8569f934588151556128b0795c7231957df1a0 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Tue, 26 Mar 2024 11:00:22 -0400 Subject: [PATCH 07/13] unit tests for new validation functions --- .../protocols/parameters/validation.py | 6 +- .../protocols/parameters/test_validation.py | 66 ++++++++++++++++++- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 31edf89580d..140645ae936 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -69,10 +69,10 @@ def ensure_value_type( def ensure_enum_type(value: AllowedTypes) -> Union[float, str]: """Ensure that the value that is sent over from the client is the expected type.""" validated_value: Union[float, bool, str] - if isinstance(value, int): - validated_value = float(value) - elif isinstance(value, bool): + if isinstance(value, bool): raise ParameterValueError("Cannot send a boolean value as an enum type") + elif isinstance(value, int): + validated_value = float(value) else: validated_value = value return validated_value diff --git a/api/tests/opentrons/protocols/parameters/test_validation.py b/api/tests/opentrons/protocols/parameters/test_validation.py index cd82fe173c4..39df25ec810 100644 --- a/api/tests/opentrons/protocols/parameters/test_validation.py +++ b/api/tests/opentrons/protocols/parameters/test_validation.py @@ -1,5 +1,5 @@ import pytest -from typing import Optional, List +from typing import Optional, List, Union from opentrons.protocols.parameters.types import ( AllowedTypes, @@ -129,6 +129,70 @@ def test_validate_options_raises_name_error() -> None: ) +@pytest.mark.parametrize( + ["value", "param_type", "result"], + [ + (1.0, int, 1), + (1.1, int, 1.1), + (2.0, float, 2.0), + (2.2, float, 2.2), + ("3.0", str, "3.0"), + (True, bool, True), + ], +) +def test_ensure_value_type( + value: Union[float, bool, str], param_type: type, result: AllowedTypes +) -> None: + """It should ensure the correct type is there, converting an float to ints.""" + assert result == subject.ensure_value_type(value, param_type) + + +@pytest.mark.parametrize( + ["value", "result"], + [(123, 123.0), (123.4, 123.4), (456.0, 456.0), ("789", "789")], +) +def test_ensure_enum_type(value: AllowedTypes, result: Union[float, str]) -> None: + """It should ensure the value is in the format of a valid enum choice value.""" + assert result == subject.ensure_enum_type(value) + + +def test_ensure_enum_type_raises() -> None: + """It should raise if given a bool for enum value.""" + with pytest.raises(ParameterValueError): + subject.ensure_enum_type(True) + + +@pytest.mark.parametrize( + ["param_type", "result"], + [(int, "int"), (float, "float"), (str, "str")], +) +def test_convert_type_string_for_enum(param_type: type, result: str) -> None: + """It should convert the type into a string for the EnumParameter model.""" + assert result == subject.convert_type_string_for_enum(param_type) + + +def test_convert_type_string_for_enum_raises() -> None: + """It should raise if given a bool to convert to an enum type string.""" + with pytest.raises(ParameterValueError): + subject.convert_type_string_for_enum(bool) + + +@pytest.mark.parametrize(["param_type", "result"], [(int, "int"), (float, "float")]) +def test_convert_type_string_for_num_param(param_type: type, result: str) -> None: + """It should convert the type into a string for the NumberParameter model.""" + assert result == subject.convert_type_string_for_num_param(param_type) + + +@pytest.mark.parametrize( + "param_type", + [str, bool], +) +def test_convert_type_string_for_num_param_raises(param_type: type) -> None: + """It should raise if given a bool or str to convert to an enum type string.""" + with pytest.raises(ParameterValueError): + subject.convert_type_string_for_num_param(param_type) + + @pytest.mark.parametrize( ["default", "minimum", "maximum", "choices", "parameter_type", "error_text"], [ From 4c2ceffd30eea2eb8d311fc0905d116d4ec7ef65 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Wed, 27 Mar 2024 10:51:32 -0400 Subject: [PATCH 08/13] unit tests for parameter definition model functions --- .../parameters/test_parameter_definition.py | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/api/tests/opentrons/protocols/parameters/test_parameter_definition.py b/api/tests/opentrons/protocols/parameters/test_parameter_definition.py index 6e93e54a97c..b2e00f9ecc5 100644 --- a/api/tests/opentrons/protocols/parameters/test_parameter_definition.py +++ b/api/tests/opentrons/protocols/parameters/test_parameter_definition.py @@ -13,6 +13,13 @@ create_str_parameter, ) +from opentrons.protocol_engine.types import ( + NumberParameter, + BooleanParameter, + EnumParameter, + EnumChoice, +) + @pytest.fixture(autouse=True) def _patch_parameter_validation(decoy: Decoy, monkeypatch: pytest.MonkeyPatch) -> None: @@ -237,3 +244,98 @@ def test_str_parameter_default_raises_not_in_allowed_values() -> None: default="waldo", choices=[{"display_name": "where's", "value": "odlaw"}], ) + + +def test_as_protocol_engine_boolean_parameter(decoy: Decoy) -> None: + """It should return a protocol engine BooleanParameter model.""" + decoy.when(mock_validation.ensure_display_name("foo")).then_return("my cool name") + decoy.when(mock_validation.ensure_variable_name("bar")).then_return("my variable") + decoy.when(mock_validation.ensure_description("describe this")).then_return("1 2 3") + + parameter_def = create_bool_parameter( + display_name="foo", + variable_name="bar", + default=False, + choices=[{"display_name": "uhh", "value": False}], + description="describe this", + ) + + assert parameter_def.as_protocol_engine_type() == BooleanParameter( + type="bool", + displayName="my cool name", + variableName="my variable", + description="1 2 3", + value=False, + default=False, + ) + + +def test_as_protocol_engine_enum_parameter(decoy: Decoy) -> None: + """It should return a protocol engine EnumParameter model.""" + decoy.when(mock_validation.ensure_display_name("foo")).then_return("my cool name") + decoy.when(mock_validation.ensure_variable_name("bar")).then_return("my variable") + + parameter_def = create_str_parameter( + display_name="foo", + variable_name="bar", + default="red", + choices=[ + {"display_name": "Lapis lazuli", "value": "blue"}, + {"display_name": "Vermilion", "value": "red"}, + {"display_name": "Celadon", "value": "green"}, + ], + ) + parameter_def.value = "green" + + decoy.when(mock_validation.ensure_enum_type("blue")).then_return("aquamarine") + decoy.when(mock_validation.ensure_enum_type("red")).then_return("crimson") + decoy.when(mock_validation.ensure_enum_type("green")).then_return("forest") + decoy.when(mock_validation.convert_type_string_for_enum(str)).then_return("float") + + assert parameter_def.as_protocol_engine_type() == EnumParameter( + type="float", + displayName="my cool name", + variableName="my variable", + choices=[ + EnumChoice(displayName="Lapis lazuli", value="aquamarine"), + EnumChoice(displayName="Vermilion", value="crimson"), + EnumChoice(displayName="Celadon", value="forest"), + ], + value="forest", + default="crimson", + ) + + +def test_as_protocol_engine_number_parameter(decoy: Decoy) -> None: + """It should return a protocol engine NumberParameter model.""" + decoy.when(mock_validation.ensure_display_name("foo")).then_return("my cool name") + decoy.when(mock_validation.ensure_variable_name("bar")).then_return("my variable") + decoy.when(mock_validation.ensure_description("a b c")).then_return("1 2 3") + decoy.when(mock_validation.ensure_unit_string_length("test")).then_return("microns") + + parameter_def = create_int_parameter( + display_name="foo", + variable_name="bar", + default=42, + minimum=1, + maximum=100, + description="a b c", + unit="test", + ) + + parameter_def.value = 60 + decoy.when(mock_validation.convert_type_string_for_num_param(int)).then_return( + "int" + ) + + assert parameter_def.as_protocol_engine_type() == NumberParameter( + type="int", + displayName="my cool name", + variableName="my variable", + description="1 2 3", + suffix="microns", + min=1.0, + max=100.0, + value=60.0, + default=42.0, + ) From 7314619e2aeb05f83df620ec068f3f7b9a7a918a Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Wed, 27 Mar 2024 12:11:39 -0400 Subject: [PATCH 09/13] unit tests for parameter context --- .../protocol_api/test_parameter_context.py | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/api/tests/opentrons/protocol_api/test_parameter_context.py b/api/tests/opentrons/protocol_api/test_parameter_context.py index 7c90fde27ec..aa8cf0879db 100644 --- a/api/tests/opentrons/protocol_api/test_parameter_context.py +++ b/api/tests/opentrons/protocol_api/test_parameter_context.py @@ -10,7 +10,10 @@ ) from opentrons.protocols.parameters import ( parameter_definition as mock_parameter_definition, + validation as mock_validation, ) +from opentrons.protocol_engine.types import BooleanParameter + from opentrons.protocol_api._parameter_context import ParameterContext @@ -22,6 +25,12 @@ def _mock_parameter_definition_creates( monkeypatch.setattr(mock_parameter_definition, name, decoy.mock(func=func)) +@pytest.fixture(autouse=True) +def _patch_parameter_validation(decoy: Decoy, monkeypatch: pytest.MonkeyPatch) -> None: + for name, func in inspect.getmembers(mock_validation, inspect.isfunction): + monkeypatch.setattr(mock_validation, name, decoy.mock(func=func)) + + @pytest.fixture def api_version() -> APIVersion: """The API version under test.""" @@ -138,3 +147,44 @@ def test_add_string(decoy: Decoy, subject: ParameterContext) -> None: description="fee foo fum", ) assert param_def is subject._parameters["my slightly less cool variable"] + + +def test_set_parameters(decoy: Decoy, subject: ParameterContext) -> None: + """It should set the parameter values.""" + param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + decoy.when(param_def.parameter_type).then_return(bool) + decoy.when(mock_validation.ensure_value_type("bar", bool)).then_return("rhubarb") + subject._parameters["foo"] = param_def + + subject.set_parameters({"foo": "bar"}) + + assert param_def.value == "rhubarb" + + +def test_export_parameters_for_analysis( + decoy: Decoy, subject: ParameterContext +) -> None: + """It should export the parameters as protocol engine types.""" + param_def = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + boolean_param = decoy.mock(cls=BooleanParameter) + decoy.when(param_def.as_protocol_engine_type()).then_return(boolean_param) + subject._parameters["foo"] = param_def + + assert subject.export_parameters_for_analysis() == [boolean_param] + + +def test_export_parameters_for_protocol( + decoy: Decoy, subject: ParameterContext +) -> None: + """It should export the parameters as a Parameters object with the parameters as dynamic attributes.""" + param_def_1 = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + param_def_2 = decoy.mock(cls=mock_parameter_definition.ParameterDefinition) + decoy.when(param_def_1.variable_name).then_return("x") + decoy.when(param_def_1.value).then_return("a") + decoy.when(param_def_2.variable_name).then_return("y") + decoy.when(param_def_2.value).then_return(1.23) + subject._parameters = {"foo": param_def_1, "bar": param_def_2} + + result = subject.export_parameters_for_protocol() + assert result.x == "a" # type: ignore [attr-defined] + assert result.y == 1.23 # type: ignore [attr-defined] From 7634e424d44ef90ea083ff4bf1d2100e7c562710 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Wed, 27 Mar 2024 16:00:01 -0400 Subject: [PATCH 10/13] add test for raising when parameter does not exist --- api/tests/opentrons/protocol_api/test_parameter_context.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/api/tests/opentrons/protocol_api/test_parameter_context.py b/api/tests/opentrons/protocol_api/test_parameter_context.py index aa8cf0879db..8b98ae204ca 100644 --- a/api/tests/opentrons/protocol_api/test_parameter_context.py +++ b/api/tests/opentrons/protocol_api/test_parameter_context.py @@ -12,6 +12,7 @@ parameter_definition as mock_parameter_definition, validation as mock_validation, ) +from opentrons.protocols.parameters.types import ParameterDefinitionError from opentrons.protocol_engine.types import BooleanParameter from opentrons.protocol_api._parameter_context import ParameterContext @@ -161,6 +162,12 @@ def test_set_parameters(decoy: Decoy, subject: ParameterContext) -> None: assert param_def.value == "rhubarb" +def test_set_parameters_raises(decoy: Decoy, subject: ParameterContext) -> None: + """It should raise if the given parameter is not defined.""" + with pytest.raises(ParameterDefinitionError): + subject.set_parameters({"foo": "bar"}) + + def test_export_parameters_for_analysis( decoy: Decoy, subject: ParameterContext ) -> None: From 09511b64441b3d2a5ead145fdb1debd00b26bb84 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Thu, 28 Mar 2024 12:10:03 -0400 Subject: [PATCH 11/13] actually set the parameters like the PR says it does --- api/src/opentrons/protocols/execution/execute_python.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/api/src/opentrons/protocols/execution/execute_python.py b/api/src/opentrons/protocols/execution/execute_python.py index 77954c2b0b5..f33f70d7a4b 100644 --- a/api/src/opentrons/protocols/execution/execute_python.py +++ b/api/src/opentrons/protocols/execution/execute_python.py @@ -78,6 +78,8 @@ def _parse_and_set_parameters( new_globs["__param_context"] = parameter_context try: exec("add_parameters(__param_context)", new_globs) + if run_time_param_overrides is not None: + parameter_context.set_parameters(run_time_param_overrides) except Exception as e: _raise_pretty_protocol_error(exception=e, filename=filename) return parameter_context.export_parameters_for_protocol() From 2bd7c0d67d7aee82a9118e8cf90e6d7f9996c7bc Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Thu, 28 Mar 2024 14:35:34 -0400 Subject: [PATCH 12/13] changes per code review --- .../protocol_runner/legacy_wrappers.py | 2 +- .../protocol_runner/protocol_runner.py | 12 +++++----- .../opentrons/protocols/execution/execute.py | 7 +++++- .../parameters/parameter_definition.py | 6 ++--- .../protocols/parameters/validation.py | 22 +++++++------------ .../parameters/test_parameter_definition.py | 14 +++++------- .../protocols/parameters/test_validation.py | 17 +------------- 7 files changed, 30 insertions(+), 50 deletions(-) diff --git a/api/src/opentrons/protocol_runner/legacy_wrappers.py b/api/src/opentrons/protocol_runner/legacy_wrappers.py index e20dca9d628..9783c877227 100644 --- a/api/src/opentrons/protocol_runner/legacy_wrappers.py +++ b/api/src/opentrons/protocol_runner/legacy_wrappers.py @@ -173,7 +173,7 @@ class LegacyExecutor: async def execute( protocol: LegacyProtocol, context: LegacyProtocolContext, - parameter_context: ParameterContext, + parameter_context: Optional[ParameterContext], run_time_param_values: Optional[RunTimeParamValuesType], ) -> None: """Execute a PAPIv2 protocol with a given ProtocolContext in a child thread.""" diff --git a/api/src/opentrons/protocol_runner/protocol_runner.py b/api/src/opentrons/protocol_runner/protocol_runner.py index 75854d91a75..67ea3d15db4 100644 --- a/api/src/opentrons/protocol_runner/protocol_runner.py +++ b/api/src/opentrons/protocol_runner/protocol_runner.py @@ -9,7 +9,7 @@ from opentrons.hardware_control import HardwareControlAPI from opentrons import protocol_reader from opentrons.legacy_broker import LegacyBroker -from opentrons.protocol_api import ParameterContext, MAX_SUPPORTED_VERSION +from opentrons.protocol_api import ParameterContext from opentrons.protocol_reader import ( ProtocolSource, JsonProtocolConfig, @@ -158,14 +158,14 @@ def __init__( drop_tips_after_run=drop_tips_after_run, post_run_hardware_state=post_run_hardware_state, ) - # We need to define this here so we can use it throughout this class, but it will be recreated with the proper - # api level version upon a protocol loading. - self._parameter_context = ParameterContext(api_version=MAX_SUPPORTED_VERSION) + self._parameter_context: Optional[ParameterContext] = None @property def run_time_parameters(self) -> List[RunTimeParameter]: """Parameter definitions defined by protocol, if any. Will always be empty before execution.""" - return self._parameter_context.export_parameters_for_analysis() + if self._parameter_context is not None: + return self._parameter_context.export_parameters_for_analysis() + return [] async def load( self, @@ -246,7 +246,7 @@ async def run( # noqa: D102 run_data = self._protocol_engine.state_view.get_summary() commands = self._protocol_engine.state_view.commands.get_all() - parameters = self._parameter_context.export_parameters_for_analysis() + parameters = self.run_time_parameters return RunResult( commands=commands, state_summary=run_data, parameters=parameters ) diff --git a/api/src/opentrons/protocols/execution/execute.py b/api/src/opentrons/protocols/execution/execute.py index 084df9402a6..4619e1ae08d 100644 --- a/api/src/opentrons/protocols/execution/execute.py +++ b/api/src/opentrons/protocols/execution/execute.py @@ -37,7 +37,12 @@ def run_protocol( # this in analysis which is the reason we'd pass it to this function if parameter_context is None: parameter_context = ParameterContext(protocol.api_level) - run_python(protocol, context, parameter_context, run_time_param_overrides) + run_python( + proto=protocol, + context=context, + parameter_context=parameter_context, + run_time_param_overrides=run_time_param_overrides, + ) else: raise RuntimeError(f"Unsupported python API version: {protocol.api_level}") else: diff --git a/api/src/opentrons/protocols/parameters/parameter_definition.py b/api/src/opentrons/protocols/parameters/parameter_definition.py index 8e0231d4d5e..2ad5eed3138 100644 --- a/api/src/opentrons/protocols/parameters/parameter_definition.py +++ b/api/src/opentrons/protocols/parameters/parameter_definition.py @@ -131,7 +131,7 @@ def as_protocol_engine_type(self) -> RunTimeParameter: choices = [ EnumChoice( displayName=str(choice["display_name"]), - value=validation.ensure_enum_type(choice["value"]), + value=choice["value"], ) for choice in self._choices ] @@ -141,8 +141,8 @@ def as_protocol_engine_type(self) -> RunTimeParameter: variableName=self._variable_name, description=self._description, choices=choices, - value=validation.ensure_enum_type(self._value), - default=validation.ensure_enum_type(self._default), + value=self._value, + default=self._default, ) elif self._minimum is not None and self._maximum is not None: parameter = NumberParameter( diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 140645ae936..cbb2464ebd0 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -57,7 +57,13 @@ def ensure_unit_string_length(unit: Optional[str]) -> Optional[str]: def ensure_value_type( value: Union[float, bool, str], parameter_type: type ) -> AllowedTypes: - """Ensures that the value type coming in from the client matches the given type.""" + """Ensures that the value type coming in from the client matches the given type. + + This does not guarantee that the value will be the correct type for the given parameter, only that any data coming + in is in the format that we expect. For now, the only transformation it is doing is converting integers represented + as floating points to integers. If something is labelled as an int but is not actually an integer, that will be + caught when it is attempted to be set as the parameter value and will raise the appropriate error there. + """ validated_value: AllowedTypes if isinstance(value, float) and parameter_type is int and value.is_integer(): validated_value = int(value) @@ -66,18 +72,6 @@ def ensure_value_type( return validated_value -def ensure_enum_type(value: AllowedTypes) -> Union[float, str]: - """Ensure that the value that is sent over from the client is the expected type.""" - validated_value: Union[float, bool, str] - if isinstance(value, bool): - raise ParameterValueError("Cannot send a boolean value as an enum type") - elif isinstance(value, int): - validated_value = float(value) - else: - validated_value = value - return validated_value - - def convert_type_string_for_enum( parameter_type: type, ) -> Literal["int", "float", "str"]: @@ -95,7 +89,7 @@ def convert_type_string_for_enum( def convert_type_string_for_num_param(parameter_type: type) -> Literal["int", "float"]: - """Converts a type object into a string for an enumerated parameter.""" + """Converts a type object into a string for a number parameter.""" if parameter_type is int: return "int" elif parameter_type is float: diff --git a/api/tests/opentrons/protocols/parameters/test_parameter_definition.py b/api/tests/opentrons/protocols/parameters/test_parameter_definition.py index b2e00f9ecc5..556ce016672 100644 --- a/api/tests/opentrons/protocols/parameters/test_parameter_definition.py +++ b/api/tests/opentrons/protocols/parameters/test_parameter_definition.py @@ -286,10 +286,6 @@ def test_as_protocol_engine_enum_parameter(decoy: Decoy) -> None: ], ) parameter_def.value = "green" - - decoy.when(mock_validation.ensure_enum_type("blue")).then_return("aquamarine") - decoy.when(mock_validation.ensure_enum_type("red")).then_return("crimson") - decoy.when(mock_validation.ensure_enum_type("green")).then_return("forest") decoy.when(mock_validation.convert_type_string_for_enum(str)).then_return("float") assert parameter_def.as_protocol_engine_type() == EnumParameter( @@ -297,12 +293,12 @@ def test_as_protocol_engine_enum_parameter(decoy: Decoy) -> None: displayName="my cool name", variableName="my variable", choices=[ - EnumChoice(displayName="Lapis lazuli", value="aquamarine"), - EnumChoice(displayName="Vermilion", value="crimson"), - EnumChoice(displayName="Celadon", value="forest"), + EnumChoice(displayName="Lapis lazuli", value="blue"), + EnumChoice(displayName="Vermilion", value="red"), + EnumChoice(displayName="Celadon", value="green"), ], - value="forest", - default="crimson", + value="green", + default="red", ) diff --git a/api/tests/opentrons/protocols/parameters/test_validation.py b/api/tests/opentrons/protocols/parameters/test_validation.py index 39df25ec810..baaa566c565 100644 --- a/api/tests/opentrons/protocols/parameters/test_validation.py +++ b/api/tests/opentrons/protocols/parameters/test_validation.py @@ -143,25 +143,10 @@ def test_validate_options_raises_name_error() -> None: def test_ensure_value_type( value: Union[float, bool, str], param_type: type, result: AllowedTypes ) -> None: - """It should ensure the correct type is there, converting an float to ints.""" + """It should ensure the correct type is there, converting floats to ints.""" assert result == subject.ensure_value_type(value, param_type) -@pytest.mark.parametrize( - ["value", "result"], - [(123, 123.0), (123.4, 123.4), (456.0, 456.0), ("789", "789")], -) -def test_ensure_enum_type(value: AllowedTypes, result: Union[float, str]) -> None: - """It should ensure the value is in the format of a valid enum choice value.""" - assert result == subject.ensure_enum_type(value) - - -def test_ensure_enum_type_raises() -> None: - """It should raise if given a bool for enum value.""" - with pytest.raises(ParameterValueError): - subject.ensure_enum_type(True) - - @pytest.mark.parametrize( ["param_type", "result"], [(int, "int"), (float, "float"), (str, "str")], From 6b5eb3cef060109ef20b4e9b69e8372a089019e3 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Thu, 28 Mar 2024 14:38:18 -0400 Subject: [PATCH 13/13] docstring update I forgot --- api/tests/opentrons/protocols/parameters/test_validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/tests/opentrons/protocols/parameters/test_validation.py b/api/tests/opentrons/protocols/parameters/test_validation.py index baaa566c565..988e203a822 100644 --- a/api/tests/opentrons/protocols/parameters/test_validation.py +++ b/api/tests/opentrons/protocols/parameters/test_validation.py @@ -173,7 +173,7 @@ def test_convert_type_string_for_num_param(param_type: type, result: str) -> Non [str, bool], ) def test_convert_type_string_for_num_param_raises(param_type: type) -> None: - """It should raise if given a bool or str to convert to an enum type string.""" + """It should raise if given a bool or str to convert to a number type string.""" with pytest.raises(ParameterValueError): subject.convert_type_string_for_num_param(param_type)