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): prevent protocols from passing with mistyped property setting for liquid classes #17258

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

jbleon95
Copy link
Contributor

@jbleon95 jbleon95 commented Jan 13, 2025

Overview

This PR addresses an issue found while testing liquid classes. The API for changing individual liquid class properties uses the @property.setter decorator to allow easy one line changes of properties while validating the values that the properties are being set to. A consequence of this is because Python allows dynamic setting of variables, a typo (e.g putting water_p50_props.aspirate.mix.enable = True instead of water_p50_props.aspirate.mix.enabled = True) did not cause the protocol to fail analysis. This means that a multi-hour protocol could not behave as expected due to a typo somewhere in the protocol.

Since this would be a huge pain point for customers, the liquid class properties now use the slots=True argument of dataclass to automatically generate the __slots__ attribute, preventing instances from being assigned new variables while giving a small, though negligible, performance boost.

Test Plan and Hands on Testing

This protocol now fails analysis with a AttributeError [line 16]: 'MixProperties' object has no attribute 'enable' error. Fixing line 16 to be enabled causes the protocol to correctly pass.

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

def run(protocol_context):
	tiprack1 = protocol_context.load_labware("opentrons_flex_96_tiprack_50ul", "C1")
	trash = protocol_context.load_trash_bin('A3')
	pipette_50 = protocol_context.load_instrument("flex_1channel_1000", "right", tip_racks=[tiprack1])

	nest_plate = protocol_context.load_labware("nest_96_wellplate_100ul_pcr_full_skirt", "D3")
	arma_plate = protocol_context.load_labware("armadillo_96_wellplate_200ul_pcr_full_skirt", "D2")

	water_class = protocol_context.define_liquid_class("water")
	water_p50_props = water_class.get_for("flex_1channel_1000", "opentrons_flex_96_tiprack_50ul")
	water_p50_props.aspirate.mix.enable = True  # This is a typo

Changelog

  • Disallow setting non-existent properties for liquid class property dataclasses

Risk assessment

Low

@jbleon95 jbleon95 requested review from sanni-t and ddcc4 January 13, 2025 20:21
@jbleon95 jbleon95 requested a review from a team as a code owner January 13, 2025 20:21
@ddcc4
Copy link
Contributor

ddcc4 commented Jan 13, 2025

Hey, if your goal is to define a class where no new fields can be created dynamically, can you look into using slots?

(I haven't had a chance to dig through it to see if it would work for our use case, but that's what slots are for.)

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

Nice. Thank you both!

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Awesome!

@jbleon95 jbleon95 merged commit 7a48c12 into edge Jan 14, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants