feat(api): add liquid class in PAPI #16506
Conversation
|
|
||
|
|
||
| class LiquidClassDefinitionDoesNotExist(Exception): | ||
| """Specified liquid class' definition does not exist.""" |
There was a problem hiding this comment.
I don't think this has to be possessive.
| """Specified liquid class' definition does not exist.""" | |
| """Specified liquid class definition does not exist.""" |
| # Calling this often will degrade protocol execution performance. | ||
| liquid_class_def = liquid_classes.load_definition(name) | ||
| self._defined_liquid_class_defs_by_name[name] = liquid_class_def | ||
| except KeyError: |
There was a problem hiding this comment.
Shouldnt this be LiquidClassDefinitionDoesNotExist and not KeyError?
| def get_for(self, pipette: str, tiprack: str) -> TransferProperties: | ||
| """Get liquid class transfer properties for the specified pipette and tip.""" | ||
| settings_for_pipette: Sequence[ByPipetteSetting] = list( | ||
| filter( |
There was a problem hiding this comment.
This is more of a style preference, but I much prefer having list comprehensions here rather than lambda expressions. Not only are they more efficient (no list call and function calls), I find them easier to read.
I feel like feature flags, with the 'internal use' warning, set clear expectations that this code is not built for external use and can/will change without notice or version change. Do you think it's not a good idea? |
I think it's fine. I just think it can be hard to do if one of the things you want it to control is like, "added a new entry to an enum", but for this it def works |
AUTH-835, AUTH-836 # Overview Adds `LiquidClass` class and `ProtocolContext.define_liquid_class()` to PAPI. Also adds an `allowLiquidClasses` feature flag that will need to be set to True in order to use the new API method. ## Changelog - added `LiquidClass` dataclass to PAPI - added `ProtocolContext.define_liquid_class` to define a liquid class in the protocol - added `allowLiquidClasses` internal feature flag Some small schema changes: - changed `liquidName` -> `liquidClassName` and made it a `safestring` - added a `displayName` - changed `tipType` -> tiprack Misc shared data changes: - added `liquid_classes.load_definition()` - added validation tests for liquid class definitions
AUTH-835, AUTH-836
Overview
Adds
LiquidClassclass andProtocolContext.define_liquid_class()to PAPI. Also adds anallowLiquidClassesfeature flag that will need to be set to True in order to use the new API method.Test Plan and Hands on Testing
I think the integration and unit tests in the PR are good enough for this work. This PR doesn't add the ability to actually use the liquid class in any way other than fetching values of the class so there isn't anything to be tested on a robot.
Changelog
LiquidClassdataclass to PAPIProtocolContext.define_liquid_classto define a liquid class in the protocolallowLiquidClassesinternal feature flagSome small schema changes:
liquidName->liquidClassNameand made it asafestringdisplayNametipType-> tiprackMisc shared data changes:
liquid_classes.load_definition()Review requests
LiquidClassstructure make sense?byTipType. ButbyTiprackTypealso didn't feel correct. So looking for suggestions for improvement or reassurance that it's fine the way it is.Risk assessment
Low. Doesn't affect anything existing, and is behind a feature flag.