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.

Sign up for GitHub

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

feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

Merged
merged 10 commits into from
Jan 30, 2025
+140 −51
Merged

feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() #17289

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
24310c3
added InstrumentContext.get_load_name(), updated LiquidClass.get_for()
sanni-t Jan 16, 2025
dc0bfae
Merge branch 'edge' into AUTH-1295-add_get_load_name
sanni-t Jan 29, 2025
2284f95
removed get_load_name, moved the changes into .name as a bug fix
sanni-t Jan 29, 2025
e548bdc
change from 'tipracks' to 'tip_racks'
sanni-t Jan 29, 2025
9181bc5
update tiprack validation logic to account for fixed Flex pip names
sanni-t Jan 30, 2025
06c60c9
Merge branch 'edge' into AUTH-1295-add_get_load_name
sanni-t Jan 30, 2025
9255061
added fix note to internal release notes
sanni-t Jan 30, 2025
09e9765
Update api/src/opentrons/protocol_api/instrument_context.py
sanni-t Jan 30, 2025
c252cc2
Update api/src/opentrons/protocol_api/core/engine/instrument.py
sanni-t Jan 30, 2025
c9b96ec
linting
sanni-t Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Loading
Loading status checks…
change from 'tipracks' to 'tip_racks'
@sanni-t
sanni-t committed Jan 29, 2025
commit e548bdc702b7efad2168b6fe1799962920922c94
12 changes: 6 additions & 6 deletions api/src/opentrons/protocol_api/_liquid.py
View file
Original file line number Diff line number Diff line change
@@ -68,7 +68,7 @@ def display_name(self) -> str:
return self._display_name

def get_for(
self, pipette: Union[str, InstrumentContext], tiprack: Union[str, Labware]
self, pipette: Union[str, InstrumentContext], tip_rack: Union[str, Labware]
) -> TransferProperties:
"""Get liquid class transfer properties for the specified pipette and tip."""
from . import InstrumentContext, Labware
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we just import this at the top of the file (and get rid of the if TYPE_CHECKING bit)?

You are clearly using the symbols InstrumentContext and Labware in your implementation (versus only importing it for the typechecker), so why not treat it as a normal import?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding to this, I can see limited use of getting liquid class properties without having to provide the names, but I'd almost rather that be it's own function which would allow us to remove this internal import. That or we could always flip around the isinstance to check for str instead of InstrumentContext or Labware

Copy link
Member Author

@sanni-t sanni-t Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why can't we just import this at the top of the file (and get rid of the if TYPE_CHECKING bit)?

Because it results in a circular import for InstrumentContext. InstrumentContext already imports from _liquid.

I can see limited use of getting liquid class properties without having to provide the names, but I'd almost rather that be it's own function

Not sure I understand. Can you elaborate?

That or we could always flip around the isinstance to check for str instead of InstrumentContext or Labware

If we want to verify that the args are not of some third, unsupported type then we will have to check if isinstance of InstrumentContext and Labware. Sure we can maybe check that if it's not a string and if there's an error doing pipette.load_name or tiprack.uri, the args are wrong. But it's not as foolproof as doing isinstance checks.

@@ -83,13 +83,13 @@ def get_for(
f" or a pipette name string."
)

if isinstance(tiprack, Labware):
tiprack_uri = tiprack.uri
elif isinstance(tiprack, str):
tiprack_uri = tiprack
if isinstance(tip_rack, Labware):
tiprack_uri = tip_rack.uri
elif isinstance(tip_rack, str):
tiprack_uri = tip_rack
else:
raise ValueError(
f"{tiprack} should either be a tiprack Labware object"
f"{tip_rack} should either be a tiprack Labware object"
f" or a tiprack URI string."
)

3 changes: 1 addition & 2 deletions api/src/opentrons/protocol_api/core/engine/instrument.py
View file
Original file line number Diff line number Diff line change
@@ -997,8 +997,7 @@ def transfer_liquid( # noqa: C901
)
tiprack_uri_for_transfer_props = tip_racks[0][1].get_uri()
transfer_props = liquid_class.get_for(
pipette=self.get_pipette_name(),
tiprack=tiprack_uri_for_transfer_props,
pipette=self.get_pipette_name(), tip_rack=tiprack_uri_for_transfer_props
)
# TODO: use the ID returned by load_liquid_class in command annotations
self.load_liquid_class(
2 changes: 1 addition & 1 deletion api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py
View file
Original file line number Diff line number Diff line change
@@ -36,7 +36,7 @@ def sample_transfer_props(
) -> TransferProperties:
"""Return a mocked out liquid class fixture."""
return LiquidClass.create(maximal_liquid_class_def).get_for(
pipette="flex_1channel_50", tiprack="opentrons_flex_96_tiprack_50ul"
pipette="flex_1channel_50", tip_rack="opentrons_flex_96_tiprack_50ul"
)