feat(api): add custom liquid class creator#18416
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #18416 +/- ##
==========================================
- Coverage 25.55% 24.34% -1.21%
==========================================
Files 3254 3100 -154
Lines 277247 262542 -14705
Branches 32256 32465 +209
==========================================
- Hits 70849 63915 -6934
+ Misses 206375 198609 -7766
+ Partials 23 18 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| properties_dict: A dict of transfer properties per tip per pipette | ||
| If you want these transfer properties to be used as a fallback, i.e. | ||
| use them as a default if no transfer properties for the requested pipette | ||
| &/or tip are found, then use the key 'default' for the pipette &/or tip name. |
There was a problem hiding this comment.
Hey, did you say last week that we're not supporting default after all?
There was a problem hiding this comment.
Yep. I had a todo to remove this from docstring. Removed it now
| submerge=SubmergeDict( | ||
| start_position=TipPositionDict( | ||
| position_reference=PositionReference.WELL_BOTTOM, | ||
| position_reference="well-bottom", |
There was a problem hiding this comment.
So you made these a string now?
There wasn't a way to make it take a PositionReference instance when called from TipPositionDict() and a string when called with a raw dict, was there?
There was a problem hiding this comment.
So you made these a string now?
Yep, String literal types
There wasn't a way to make it take a PositionReference instance when called from TipPositionDict() and a string when called with a raw dict, was there?
Not unless we wanted to suppress mypy errors (which is a valid option but I try to avoid it)
| ) -> None: | ||
| """Update the transfer properties for the given pipette and tip combo. | ||
|
|
||
| If an entry does not exist, it will be created. |
There was a problem hiding this comment.
Is this true? If self._by_pipette_setting[pipette_name] didn't exist, in your code on line 102, wouldn't this function just crash instead of creating it?
There was a problem hiding this comment.
Oops you are right. Will fix it
| ) | ||
|
|
||
| def define_liquid_class(self, name: str, version: int = 1) -> LiquidClass: | ||
| def define_liquid_class(self, name: str, version: int) -> LiquidClass: |
There was a problem hiding this comment.
Hm, why did you take out @jbleon95's default of version=1 here?
There was a problem hiding this comment.
I'm passing the default version in the caller for define_liquid_class so there is no reason for this function to assign a default. Besides, mypy doesn't like this. This function was being called in protocol_context.py as define_liquid_class(name) which passes None value to version but version is not marked as Optional so it raises a mypy error. I am a bit surprised that mypy didn't flag this in @jbleon95 's PR because it did on this PR as soon as I merged edge in. Unless something changed in edge after the merge or something messed up during merge into my PR.
| cls, | ||
| name: str, | ||
| display_name: str, | ||
| by_pipette_setting: Dict[str, Dict[str, TransferProperties]], |
There was a problem hiding this comment.
Can we include in the doc-string (or a comment) that this is keyed by pipette name and tiprack and the specific variety of strings we use for those?
|
|
||
| Args: | ||
| name: The name to give to the new liquid class | ||
| properties: A dict of transfer properties per tip per pipette. |
There was a problem hiding this comment.
Same as above, let's describe what pipette name and how the tiprack name should be formatted
| def reshape(cls, data: Any) -> Any: | ||
| """Move any params specified as top-level keys into the 'params' value.""" | ||
| if isinstance(data, dict): | ||
| if None not in (data.get("enable"), data.get("enabled")): |
There was a problem hiding this comment.
Hm, why are we supporting both variants?
There was a problem hiding this comment.
The json schema (and hence pydantic model) defines an enable while the python properties use enabled.
We had switched to using enabled in python to make it more intuitive and readable. For example, water_props.aspirate.delay.enabled is True reads more correctly than water_props.aspirate.delay.enable is True.
| ) | ||
| speed: _GreaterThanZeroNumber = Field( | ||
| ..., description="Touch-tip speed, in millimeters per second." | ||
| ..., alias="speed", description="Touch-tip speed, in millimeters per second." |
There was a problem hiding this comment.
Does this alias do anything?
There was a problem hiding this comment.
Ya, it's needed so that the reshape_glob function can fetch all the field names by alias. If there's no alias specified for a field, fetching the field name returns a None.
| for pipette, by_tiprack_props in properties.items(): | ||
| for tiprack, transfer_props in by_tiprack_props.items(): | ||
| new_tiprack_props[tiprack] = build_transfer_properties( | ||
| transfer_properties=SharedTransferProperties.model_validate( | ||
| transfer_props | ||
| ) | ||
| ) | ||
| by_pipette_setting[pipette] = {tiprack: new_tiprack_props[tiprack]} |
There was a problem hiding this comment.
Er.. made a wrong change in a previous commit- this would totally replace the entire entry for the pipette, even if we only want to update an entry related to a specific tiprack only. Fix coming up
Closes AUTH-839 # Overview - Adds the ability to create a new liquid class based on user-specified properties - Adds the ability to customize an existing liquid class based on user-specified properties ## Changelog - adds liquid class modifiers to enable custom liquid classes - adds the custom liquid class creator API - small changes to 'version' arg typing ## Risk assessment Low. Supplementary stuff. Doesn't modify existing infrastructure. (cherry picked from commit 88da5fa)
Closes AUTH-839 # Overview - Adds the ability to create a new liquid class based on user-specified properties - Adds the ability to customize an existing liquid class based on user-specified properties ## Changelog - adds liquid class modifiers to enable custom liquid classes - adds the custom liquid class creator API - small changes to 'version' arg typing ## Risk assessment Low. Supplementary stuff. Doesn't modify existing infrastructure. (cherry picked from commit 88da5fa)
Closes AUTH-839 # Overview - Adds the ability to create a new liquid class based on user-specified properties - Adds the ability to customize an existing liquid class based on user-specified properties ## Changelog - adds liquid class modifiers to enable custom liquid classes - adds the custom liquid class creator API - small changes to 'version' arg typing ## Risk assessment Low. Supplementary stuff. Doesn't modify existing infrastructure. (cherry picked from commit 88da5fa)
Closes AUTH-839 # Overview - Adds the ability to create a new liquid class based on user-specified properties - Adds the ability to customize an existing liquid class based on user-specified properties ## Changelog - adds liquid class modifiers to enable custom liquid classes - adds the custom liquid class creator API - small changes to 'version' arg typing ## Risk assessment Low. Supplementary stuff. Doesn't modify existing infrastructure. (cherry picked from commit 88da5fa)
Closes AUTH-839 # Overview - Adds the ability to create a new liquid class based on user-specified properties - Adds the ability to customize an existing liquid class based on user-specified properties ## Changelog - adds liquid class modifiers to enable custom liquid classes - adds the custom liquid class creator API - small changes to 'version' arg typing ## Risk assessment Low. Supplementary stuff. Doesn't modify existing infrastructure. (cherry picked from commit 88da5fa)
Closes AUTH-839 # Overview - Adds the ability to create a new liquid class based on user-specified properties - Adds the ability to customize an existing liquid class based on user-specified properties ## Changelog - adds liquid class modifiers to enable custom liquid classes - adds the custom liquid class creator API - small changes to 'version' arg typing ## Risk assessment Low. Supplementary stuff. Doesn't modify existing infrastructure. (cherry picked from commit 88da5fa)
Closes AUTH-839, AUTH-838
Overview
Test Plan and Hands on Testing
Changelog
Review requests
Risk assessment
Low. Supplementary stuff. Doesn't modify existing infrastructure.