-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(api): runtime parameters API for adding and using default parameters in protocols #14668
Changes from 15 commits
d3e0d47
12f39c6
dc0386c
27c1c0d
02aeb87
c57fef3
7267f83
eaf50e1
dc9cdcd
7d2df0b
0c27903
b6f8a79
e57c1a4
5fd67cc
ec3adda
7ce24a5
ee05c22
88b674f
e7c6cef
6eba3d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
from typing import Dict, Optional, Any | ||
|
||
from opentrons.protocols.parameters.types import AllowedTypes, ParameterNameError | ||
|
||
|
||
class Parameters: | ||
def __init__(self, parameters: Optional[Dict[str, AllowedTypes]] = None) -> None: | ||
self._values: Dict[str, AllowedTypes] = {} | ||
if parameters is not None: | ||
for name, value in parameters.items(): | ||
self._initialize_parameter(name, value) | ||
|
||
def _getparam(self, variable_name: str) -> Any: | ||
return getattr(self, f"_{variable_name}") | ||
|
||
def _initialize_parameter(self, variable_name: str, value: AllowedTypes) -> None: | ||
if not hasattr(self, variable_name) and not hasattr(self, f"_{variable_name}"): | ||
setattr(self, f"_{variable_name}", value) | ||
prop = property( | ||
fget=lambda s, v=variable_name: Parameters._getparam(s, v) # type: ignore[misc] | ||
) | ||
setattr(Parameters, variable_name, prop) | ||
self._values[variable_name] = value | ||
else: | ||
raise ParameterNameError( | ||
f"Cannot use {variable_name} as a variable name, either duplicates another" | ||
f" parameter name, Opentrons reserved function, or Python built-in" | ||
) | ||
|
||
def get_all(self) -> Dict[str, AllowedTypes]: | ||
return self._values |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefix this file name with an underscore unless you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The would be no code execution reason to import the module, but since its the most public facing part of this (as far as the API) I'm alright keeping it without an underscore. Plus it would allow as user to do this pattern of typing the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would solve the type annotation problem by re-exporting the class so it’s
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,169 @@ | ||
"""Parameter context for python protocols.""" | ||
|
||
from typing import List, Optional, Union | ||
|
||
from opentrons.protocols.api_support.types import APIVersion | ||
from opentrons.protocols.parameters import parameter_definition | ||
from opentrons.protocols.parameters.types import ParameterChoices | ||
|
||
from ._parameters import Parameters | ||
|
||
_ParameterDefinitionTypes = Union[ | ||
parameter_definition.ParameterDefinition[int], | ||
parameter_definition.ParameterDefinition[bool], | ||
parameter_definition.ParameterDefinition[float], | ||
parameter_definition.ParameterDefinition[str], | ||
] | ||
|
||
|
||
class ParameterContext: | ||
"""Public context for adding parameters to a protocol.""" | ||
|
||
def __init__(self, api_version: APIVersion) -> None: | ||
"""Initializes a parameter context for user-set parameters.""" | ||
self._api_version = api_version | ||
self._parameters: List[_ParameterDefinitionTypes] = [] | ||
|
||
def add_int( | ||
self, | ||
display_name: str, | ||
variable_name: str, | ||
default: int, | ||
minimum: Optional[int] = None, | ||
maximum: Optional[int] = None, | ||
choices: Optional[List[ParameterChoices]] = None, | ||
description: Optional[str] = None, | ||
unit: Optional[str] = None, | ||
) -> None: | ||
"""Creates an integer parameter, settable within a given range or list of choices. | ||
|
||
Arguments: | ||
display_name: The display name of the int parameter as it would show up on the frontend. | ||
variable_name: The variable name the int parameter will be referred to in the run context. | ||
default: The default value the int parameter will be set to. This will be used in initial analysis. | ||
minimum: The minimum value the int parameter can be set to (inclusive). Mutually exclusive with choices. | ||
maximum: The maximum value the int parameter can be set to (inclusive). Mutually exclusive with choices. | ||
choices: A list of possible choices that this parameter can be set to. | ||
Mutually exclusive with minimum and maximum. | ||
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, | ||
) | ||
) | ||
|
||
def add_float( | ||
self, | ||
display_name: str, | ||
variable_name: str, | ||
default: float, | ||
minimum: Optional[float] = None, | ||
maximum: Optional[float] = None, | ||
choices: Optional[List[ParameterChoices]] = None, | ||
description: Optional[str] = None, | ||
unit: Optional[str] = None, | ||
) -> None: | ||
"""Creates a float parameter, settable within a given range or list of choices. | ||
|
||
Arguments: | ||
display_name: The display name of the float parameter as it would show up on the frontend. | ||
variable_name: The variable name the float parameter will be referred to in the run context. | ||
default: The default value the float parameter will be set to. This will be used in initial analysis. | ||
minimum: The minimum value the float parameter can be set to (inclusive). Mutually exclusive with choices. | ||
maximum: The maximum value the float parameter can be set to (inclusive). Mutually exclusive with choices. | ||
choices: A list of possible choices that this parameter can be set to. | ||
Mutually exclusive with minimum and maximum. | ||
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, | ||
) | ||
) | ||
|
||
def add_bool( | ||
self, | ||
display_name: str, | ||
variable_name: str, | ||
default: bool, | ||
description: Optional[str] = None, | ||
) -> None: | ||
"""Creates a boolean parameter with allowable values of "On" (True) or "Off" (False). | ||
|
||
Arguments: | ||
display_name: The display name of the boolean parameter as it would show up on the frontend. | ||
variable_name: The variable name the boolean parameter will be referred to in the run context. | ||
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, | ||
) | ||
) | ||
|
||
def add_str( | ||
self, | ||
display_name: str, | ||
variable_name: str, | ||
default: str, | ||
choices: Optional[List[ParameterChoices]] = None, | ||
description: Optional[str] = None, | ||
) -> None: | ||
"""Creates a string parameter, settable among given choices. | ||
|
||
Arguments: | ||
display_name: The display name of the string parameter as it would show up on the frontend. | ||
variable_name: The variable name the string parameter will be referred to in the run context. | ||
default: The default value the string parameter will be set to. This will be used in initial analysis. | ||
choices: A list of possible choices that this parameter can be set to. | ||
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, | ||
) | ||
) | ||
|
||
def export_parameters(self) -> Parameters: | ||
"""Exports all parameters into a protocol run usable parameters object. | ||
|
||
:meta private: | ||
|
||
This is intended for Opentrons internal use only and is not a guaranteed API. | ||
""" | ||
return Parameters( | ||
parameters={ | ||
parameter.variable_name: parameter.value | ||
for parameter in self._parameters | ||
} | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,7 @@ | |
MagneticBlockContext, | ||
ModuleContext, | ||
) | ||
from ._parameters import Parameters | ||
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
@@ -167,6 +168,7 @@ def __init__( | |
self._core.load_ot2_fixed_trash_bin() | ||
|
||
self._commands: List[str] = [] | ||
self._params: Parameters = Parameters() | ||
self._unsubscribe_commands: Optional[Callable[[], None]] = None | ||
self.clear_commands() | ||
|
||
|
@@ -215,6 +217,10 @@ def bundled_data(self) -> Dict[str, bytes]: | |
""" | ||
return self._bundled_data | ||
|
||
@property | ||
def params(self) -> Parameters: | ||
return self._params | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might want a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can create a feature flag for now and put it behind that. Replace it with API level check before release. The feature flag will be useful for other backend things too. |
||
|
||
def cleanup(self) -> None: | ||
"""Finalize and clean up the protocol context.""" | ||
if self._unsubscribe_commands: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of suggestions for simplifying this.
First, instead of the user-facing API being like this:
Have you considered making it this?
params.get_all()
.variable_name="sample_count"
, with a string literal, so they pass the same string literal here.Or, taking the
context.params.sample_count
syntax for granted, an implementation-level suggestion.Right now, when we initialize the parameter
foo="bar"
, we:self._foo = "bar"
self.foo
to a read-only property that returnsself._foo
"foo": "bar"
in theself._values
dictDoes this get easier if you treat
self._values
as the single source of truth, and override__getattr__()
(and possibly__setattr__()
) to delegate to it? In other words, instead of dynamically adding attributes, implement dynamic attribute lookup. Then you don't need thef"_{variable_name}"
magic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'm going to leave the syntax the same, but I will take your suggestion to simplify things by using
self._values
as the source of truth for thefget
function of the property.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be moot if you use
self._values
as the source of truth, but I've also just realized that insetattr(Parameters, variable_name, prop)
, it looks like that's mutating theParameters
class, which will leak between protocols?