feat(api): runtime parameters API for adding and using default parameters in protocols #14668

jbleon95 · 2024-03-15T13:48:46Z

Overview

This PR implements both the current proposed runtime parameter API for adding and defining parameters to your protocol, as well as the means to do so. This only allows usage of the defaults in the protocol, you cannot set it to other values and they are not reported in analysis.

This is accomplished by injecting a new ParameterContext into an add_parameters function that is defined in the protocol, similar to how we inject ProtocolContext into the run function. This contains the user facing API for adding parameters to your protocol.

Once all parameters are parsed and validated (if they fail this they will raise a user friendly error with the correct line number), they are injected into the protocol context under a new context.params object. This will have all the parameters as dynamically set attributes of this class using the set variable name (and as propertys to enforce them being read only) that can then be used in the protocol as needed.

For parameter definitions, both the constraints and the default values are verified to ensure the protocol will not analyze unless parameters have been defined properly.

Test Plan

This protocol is able to analyze on the app and robot, as well as run without any errors.

metadata = {
    'protocolName': 'RTP Test 2 - Default only, all types',
}

requirements = {
    "robotType": "Flex",
    "apiLevel": "2.17"
}


def add_parameters(parameters):
    parameters.add_int(
        display_name="Sample count",
        variable_name="sample_count",
        default=6,
        minimum=1,
        maximum=12,
        description="How many samples to process."
    )
    parameters.add_float(
        display_name="Pipette volume",
        variable_name="volume",
        default=20.0,
        choices=[
            {"display_name": "Low Volume (10.0µL)", "value": 10.0},
            {"display_name": "Medium Volume (20.0µL)", "value": 20.0},
            {"display_name": "High Volume (50.0µL)", "value": 50.0},
        ],
        description="How many microliters to pipette of each sample.",
        unit="µL"
    )
    parameters.add_bool(
        display_name="Dry Run",
        variable_name="dry_run",
        default=False,
        description="Skip aspirate and dispense steps."
    )
    parameters.add_str(
        display_name="Pipette Name",
        variable_name="pipette",
        choices=[
            {"display_name": "Single channel 50µL", "value": "flex_1channel_50"},
            {"display_name": "Eight Channel 50µL", "value": "flex_8channel_50"},
        ],
        default="flex_1channel_50",
        description="What pipette to use during the protocol.",
    )



def run(context):
    trash_bin = context.load_trash_bin('A3')
    tip_rack = context.load_labware('opentrons_flex_96_tiprack_50ul', 'D2')
    source = context.load_labware('opentrons_96_wellplate_200ul_pcr_full_skirt', 'C1')
    destination = context.load_labware('opentrons_96_wellplate_200ul_pcr_full_skirt', 'C3')

    pipette = context.load_instrument(context.params.pipette, mount="left", tip_racks=[tip_rack])

    for i in range(context.params.sample_count):
        source_well = source.wells()[i]
        destination_well = destination.wells()[i]

        pipette.pick_up_tip()
        pipette.move_to(source_well.top())
        if not context.params.dry_run:
            pipette.aspirate(context.params.volume, source_well)
            pipette.dispense(context.params.volume, destination_well)

        pipette.drop_tip()

Changelog

Added ParameterContext class for use in add_parameters special protocol function
ProtocolContext now has a params attribute that holds all the parameters as dynamically set attributes
Added ParameterDefinition class to hold all parameter definition data that will be reported in analysis and passed to the robot server
Added parameter validation when creating definitions

Review requests

General code structure feedback

Risk assessment

Low/medium, this is a brand new API but it is completely encapsulated from the run context. The most dangerous part of this is the dynamically set attributes but I believe we are doing enough checks on variable name and checking existing attributes to prevent the user from doing anything dangerous.

…p_papi_parameter_definitions

codecov · 2024-03-15T13:52:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.34%. Comparing base (3b58363) to head (6eba3d5).
Report is 34 commits behind head on edge.

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #14668      +/-   ##
==========================================
- Coverage   67.34%   67.34%   -0.01%     
==========================================
  Files        2485     2485              
  Lines       71439    71411      -28     
  Branches     9057     9042      -15     
==========================================
- Hits        48114    48095      -19     
+ Misses      21173    21171       -2     
+ Partials     2152     2145       -7

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

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

Files	Coverage Δ
api/src/opentrons/protocol_api/__init__.py	`100.00% <ø> (ø)`
api/src/opentrons/protocol_api/protocol_context.py	`92.74% <ø> (ø)`
...rc/opentrons/protocols/execution/execute_python.py	`89.74% <ø> (+2.78%)`	⬆️

SyntaxColoring

A few suggestions, but LGTM!

SyntaxColoring · 2024-03-18T20:16:46Z

api/src/opentrons/protocol_api/_parameters.py

+            prop = property(
+                fget=lambda s, v=variable_name: Parameters._getparam(s, v)  # type: ignore[misc]
+            )
+            setattr(Parameters, variable_name, prop)


A couple of suggestions for simplifying this.

First, instead of the user-facing API being like this:

def run(context): sample_count = context.params.sample_count

Have you considered making it this?

def run(context): sample_count = context.params["sample_count"]

This gives us namespacing in case we ever need to add more methods like params.get_all().

This matches parameter usage more closely to parameter declaration. In the declaration, the protocol author wrote variable_name="sample_count", with a string literal, so they pass the same string literal here.

This is arguably more straightforward and easier to explain.

Or, taking the context.params.sample_count syntax for granted, an implementation-level suggestion.

Right now, when we initialize the parameter foo="bar", we:

Set a private attribute self._foo = "bar"

Set a public attribute self.foo to a read-only property that returns self._foo

Store "foo": "bar" in the self._values dict

Does 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 the f"_{variable_name}" magic.

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 the fget function of the property.

I think this will be moot if you use self._values as the source of truth, but I've also just realized that in setattr(Parameters, variable_name, prop), it looks like that's mutating the Parameters class, which will leak between protocols?

SyntaxColoring · 2024-03-18T20:18:02Z

api/src/opentrons/protocol_api/parameter_context.py

I would prefix this file name with an underscore unless you want from opentrons.protocol_api import parameter_context to be a thing that protocol authors are allowed to do.

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 add_parameters function.

from opentrons.protocol_api.parameter_context import ParameterContext def add_parameters(parameters: ParameterContext): ...

I would solve the type annotation problem by re-exporting the class so it’s

from opentrons.protocol_api import ParameterContext

api/src/opentrons/protocols/parameters/types.py

SyntaxColoring · 2024-03-18T20:33:41Z

api/src/opentrons/protocols/parameters/validation.py

+    if keyword.iskeyword(variable_name):
+        raise ParameterNameError("Variable name cannot be a reserved Python keyword.")


I'm not sure iskeyword is the right thing to use here; it doesn't seem necessary or sufficient for preventing "bad" variable names.

Some slightly contrived examples:

def is a keyword, ~~but there's no technical reason someone shouldn't be able to name something params.def.~~ It might even be a reasonable thing to name a parameter for a labware definition load name.

Meanwhile, things like __class__ and __eq__ are potentially scarier, but they're not keywords, so they'd pass this check.

This is kind of reaffirming to me that the user-facing syntax should be params["foo"]. It just seems inherently safer than params.foo. It's your call, but if we stick with params.foo, we should put some thought into a threat model.

params.def, using the attribute method of parameter usage, cannot actually be used in code even though it can be set and will raise a SyntaxError when called.

As for the second set of examples, those will be caught by the hasattr check in the Parameters class, so there is no chance of that being overwritten (and I think setattr wouldn't even let you do that in the first place)

SyntaxColoring · 2024-03-18T20:47:24Z

api/src/opentrons/protocols/parameters/validation.py

+UNIT_MAX_LEN = 10
+DISPLAY_NAME_MAX_LEN = 30
+DESCRIPTION_MAX_LEN = 100


Is this to protect against layout problems in the Opentrons App and ODD?

If so: It's up to you, but I think you'll have better results if you leave it to the frontend to truncate or ellipsize overly long names. Here's a contrived display name of len()==15 that will certainly not fit in a space designed to fit "up to 30 characters."

﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽﷽

Or, if this is just to protect against extreme resource usage, I would set the limits to something like 1000.

We've made a deliberate choice not to truncate or ellipsize long names on the frontend to maximize the user experience for lab operators and force protocol authors to choose their names with care. But you bring up a good point with this and I'll bring it up with app&ui on the protocol authorship team.

SyntaxColoring · 2024-03-18T20:53:27Z

api/src/opentrons/protocol_api/protocol_context.py

+    @property
+    def params(self) -> Parameters:
+        return self._params


Might want a @requires_version decorator or a ticket for it, before we forget.

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.

sanni-t

Looks great! I like the dynamic property creation! @SyntaxColoring has some good points that it's not as easily safe as having property getters but we'll do thorough testing and adding safeguards so that there's no scope for misuse.

I think we definitely should put the context.params property behind a requires decorator. Are you planning on doing that in a follow-up?

…ters in protocols (#14668) Implements proposed runtime parameter API for adding and defining parameters and using them within the protocol

jbleon95 added 9 commits March 7, 2024 14:53

first draft of parameter definition

d3e0d47

Merge branch 'edge' of https://github.com/Opentrons/opentrons into rt…

12f39c6

…p_papi_parameter_definitions

first pass at parsing and using parameters in python protocols

dc0386c

refactor to make run_python less complex

27c1c0d

refactor parameter definition to have choices be typeddicts

02aeb87

moving around files

c57fef3

more refactoring of files and locations

7267f83

add more of context api and refactor parameter definition creation

eaf50e1

set parameters as properties to prevent overwriting them in a protocol

dc9cdcd

jbleon95 added 6 commits March 18, 2024 10:13

more validation of string arguments for parameter definitions

7d2df0b

unit test for parameter context

0c27903

refactoring file locations

b6f8a79

unit tests for creating parameter definitions

e57c1a4

fixing up/removing todos

5fd67cc

tests for parameter validation

ec3adda

jbleon95 marked this pull request as ready for review March 18, 2024 19:24

jbleon95 requested a review from a team as a code owner March 18, 2024 19:24

jbleon95 requested a review from sanni-t March 18, 2024 19:25

SyntaxColoring approved these changes Mar 18, 2024

View reviewed changes

changes per code review

7ce24a5

sanni-t approved these changes Mar 19, 2024

View reviewed changes

jbleon95 added 4 commits March 19, 2024 13:38

more changes in response to review

ee05c22

refactor parameter dynamic attribute setting to not set class level

88b674f

add parameters to init

e7c6cef

exclude params from docs for now

6eba3d5

jbleon95 merged commit e5d9260 into edge Mar 19, 2024
22 checks passed

jbleon95 deleted the rtp_papi_parameter_definitions branch March 19, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): runtime parameters API for adding and using default parameters in protocols #14668

feat(api): runtime parameters API for adding and using default parameters in protocols #14668

jbleon95 commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

SyntaxColoring left a comment

SyntaxColoring Mar 18, 2024

jbleon95 Mar 19, 2024

SyntaxColoring Mar 19, 2024

SyntaxColoring Mar 18, 2024

jbleon95 Mar 19, 2024

SyntaxColoring Mar 19, 2024

SyntaxColoring Mar 18, 2024 •

edited

Loading

jbleon95 Mar 19, 2024

SyntaxColoring Mar 18, 2024

jbleon95 Mar 19, 2024

SyntaxColoring Mar 18, 2024

sanni-t Mar 19, 2024

sanni-t left a comment

		if keyword.iskeyword(variable_name):
		raise ParameterNameError("Variable name cannot be a reserved Python keyword.")

feat(api): runtime parameters API for adding and using default parameters in protocols #14668

feat(api): runtime parameters API for adding and using default parameters in protocols #14668

Conversation

jbleon95 commented Mar 15, 2024 • edited Loading

Overview

Test Plan

Changelog

Review requests

Risk assessment

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

SyntaxColoring left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxColoring Mar 18, 2024 • 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

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

jbleon95 commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

SyntaxColoring Mar 18, 2024 •

edited

Loading