-
Notifications
You must be signed in to change notification settings - Fork 186
feat(api): add version argument to protocol context get_liquid_class #19408
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
Conversation
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.
Makes sense, thank you!
api/src/opentrons/protocol_api/core/engine/_default_liquid_class_versions.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_api/core/engine/_default_liquid_class_versions.py
Outdated
Show resolved
Hide resolved
@@ -297,7 +297,7 @@ def define_liquid( | |||
"""Define a liquid to load into a well.""" | |||
|
|||
@abstractmethod | |||
def get_liquid_class(self, name: str, version: int) -> LiquidClass: | |||
def get_liquid_class(self, name: str, version: Optional[int]) -> LiquidClass: |
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.
Hm, what's our policy on the core functions vs the public API? Would it be simpler to make the core function always require a version number, and let the public API handle choosing a version number for the user if none is specified?
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 don't think we have a strong policy, there's an argument that can be made that argument validation and resolving happens in the public API, but I also like stashing away the logic for this in the core implementation where it's less immediately visible. The good thing is we can always change this around since it's not user-facing.
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.
Docstring is totally fine, so don't hold this up any more on account of me!
Overview
Addresses part of AUTH-2246
This PR adds a
version
argument to the publicProtocolContext.get_liquid_class
method. It is an optional argument that defaults toNone
. If the version argument is included, we will attempt to load that version of the liquid class, failing if that version does not exist. If the version argument is not filled, we will automatically try and load the latest version for the protocol's API level.This PR adds loading version 2 of
water
,ethanol_80
andglycerol_50
for API version 2.26. This PR does not add those liquid class definitions nor does it bump up the API level. Those two tasks will be covered in future PRs.Test Plan and Hands on Testing
This protocol analyzes properly, and covers both explicitly calling with and without a version. Changing the version to a non-existent one causes an error appear.
Changelog
version
argument toget_liquid_class
get_liquid_class_version
function to determine which liquid class definition version to load when a version is not providedReview requests
Risk assessment
Low, this PR does not introduce the new liquid class versions and does not change the behavior of existing protocols.