From 18da73946e917d21f9c150ef34c212cfb91e9255 Mon Sep 17 00:00:00 2001 From: Sarah Breen Date: Thu, 30 Jan 2025 15:52:47 -0500 Subject: [PATCH 1/4] fix(app): App support for new lid commands and fix lid error boundary trigger (#17386) fix EXEC-1042, RABR-712 --- .../en/protocol_command_text.json | 2 + .../hooks/useFailedLabwareUtils.ts | 2 +- .../CommandText/useCommandTextString/index.ts | 2 + .../utils/commandText/getLoadCommandText.ts | 7 +++ shared-data/command/types/setup.ts | 50 ++++++++++++++++++- .../helpers/getAddressableAreasInProtocol.ts | 8 ++- .../getLoadedLabwareDefinitionsByUri.ts | 6 ++- 7 files changed, 72 insertions(+), 5 deletions(-) diff --git a/app/src/assets/localization/en/protocol_command_text.json b/app/src/assets/localization/en/protocol_command_text.json index 8037b8f2778..fa7484e8c88 100644 --- a/app/src/assets/localization/en/protocol_command_text.json +++ b/app/src/assets/localization/en/protocol_command_text.json @@ -38,6 +38,8 @@ "latching_hs_latch": "Latching labware on Heater-Shaker", "left": "Left", "load_labware_to_display_location": "Load {{labware}} {{display_location}}", + "load_lid": "Loading lid", + "load_lid_stack": "Loading lid stack", "load_liquids_info_protocol_setup": "Load {{liquid}} into {{labware}}", "load_module_protocol_setup": "Load {{module}} in Slot {{slot_name}}", "load_pipette_protocol_setup": "Load {{pipette_name}} in {{mount_name}} Mount", diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/useFailedLabwareUtils.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/useFailedLabwareUtils.ts index c74840ca5b0..c4b4d92a31b 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/useFailedLabwareUtils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/useFailedLabwareUtils.ts @@ -287,7 +287,7 @@ export function getFailedCmdRelevantLabware( const failedLWURI = runRecord?.data.labware.find( labware => labware.id === recentRelevantFailedLabwareCmd?.params.labwareId )?.definitionUri - if (failedLWURI != null) { + if (failedLWURI != null && Object.keys(lwDefsByURI).includes(failedLWURI)) { return { name: getLabwareDisplayName(lwDefsByURI[failedLWURI]), nickname: labwareNickname, diff --git a/components/src/organisms/CommandText/useCommandTextString/index.ts b/components/src/organisms/CommandText/useCommandTextString/index.ts index 53ff89d170b..01754c0e71e 100644 --- a/components/src/organisms/CommandText/useCommandTextString/index.ts +++ b/components/src/organisms/CommandText/useCommandTextString/index.ts @@ -100,6 +100,8 @@ export function useCommandTextString( case 'loadLabware': case 'reloadLabware': + case 'loadLid': + case 'loadLidStack': case 'loadPipette': case 'loadModule': case 'loadLiquid': diff --git a/components/src/organisms/CommandText/useCommandTextString/utils/commandText/getLoadCommandText.ts b/components/src/organisms/CommandText/useCommandTextString/utils/commandText/getLoadCommandText.ts index e875ce989cb..d0355220f29 100644 --- a/components/src/organisms/CommandText/useCommandTextString/utils/commandText/getLoadCommandText.ts +++ b/components/src/organisms/CommandText/useCommandTextString/utils/commandText/getLoadCommandText.ts @@ -73,6 +73,13 @@ export const getLoadCommandText = ({ display_location: displayLocation, }) } + // TODO(sb, 01/29): Add full support for these commands in run log once location refactor is complete + case 'loadLid': { + return t('load_lid') + } + case 'loadLidStack': { + return t('load_lid_stack') + } case 'reloadLabware': { const { labwareId } = command.params const labware = diff --git a/shared-data/command/types/setup.ts b/shared-data/command/types/setup.ts index 554c7706977..7180abfd6f9 100644 --- a/shared-data/command/types/setup.ts +++ b/shared-data/command/types/setup.ts @@ -29,6 +29,24 @@ export interface LoadLabwareRunTimeCommand LoadLabwareCreateCommand { result?: LoadLabwareResult } +export interface LoadLidCreateCommand extends CommonCommandCreateInfo { + commandType: 'loadLid' + params: LoadLidParams +} +export interface LoadLidRunTimeCommand + extends CommonCommandRunTimeInfo, + LoadLidCreateCommand { + result?: LoadLidResult +} +export interface LoadLidStackCreateCommand extends CommonCommandCreateInfo { + commandType: 'loadLidStack' + params: LoadLidStackParams +} +export interface LoadLidStackRunTimeCommand + extends CommonCommandRunTimeInfo, + LoadLidStackCreateCommand { + result?: LoadLidStackResult +} export interface ReloadLabwareCreateCommand extends CommonCommandCreateInfo { commandType: 'reloadLabware' params: { labwareId: string } @@ -89,6 +107,8 @@ export type SetupRunTimeCommand = | LoadModuleRunTimeCommand | LoadLiquidRunTimeCommand | MoveLabwareRunTimeCommand + | LoadLidRunTimeCommand + | LoadLidStackRunTimeCommand export type SetupCreateCommand = | ConfigureNozzleLayoutCreateCommand @@ -98,6 +118,8 @@ export type SetupCreateCommand = | LoadModuleCreateCommand | LoadLiquidCreateCommand | MoveLabwareCreateCommand + | LoadLidCreateCommand + | LoadLidStackCreateCommand export type LabwareLocation = | 'offDeck' @@ -163,7 +185,6 @@ export interface MoveLabwareParams { interface MoveLabwareResult { offsetId: string } - interface LoadModuleParams { moduleId?: string location: ModuleLocation @@ -203,3 +224,30 @@ export interface ConfigureNozzleLayoutParams { pipetteId: string configurationParams: NozzleConfigurationParams } + +interface LoadLidStackParams { + location: LabwareLocation + loadName: string + namespace: string + version: number + quantity: number +} + +interface LoadLidStackResult { + stackLabwareId: string + labwareIds: string[] + definition: LabwareDefinition2 + location: LabwareLocation +} + +interface LoadLidParams { + location: LabwareLocation + loadName: string + namespace: string + version: number +} + +interface LoadLidResult { + labwareId: string + definition: LabwareDefinition2 +} diff --git a/shared-data/js/helpers/getAddressableAreasInProtocol.ts b/shared-data/js/helpers/getAddressableAreasInProtocol.ts index 9be0a547f40..9b972d0accf 100644 --- a/shared-data/js/helpers/getAddressableAreasInProtocol.ts +++ b/shared-data/js/helpers/getAddressableAreasInProtocol.ts @@ -42,7 +42,9 @@ export function getAddressableAreasInProtocol( ) { return [...acc, params.newLocation.addressableAreaName] } else if ( - commandType === 'loadLabware' && + (commandType === 'loadLabware' || + commandType === 'loadLid' || + commandType === 'loadLidStack') && params.location !== 'offDeck' && params.location !== 'systemLocation' && 'slotName' in params.location && @@ -75,7 +77,9 @@ export function getAddressableAreasInProtocol( return [...acc, ...addressableAreaNames] } else if ( - commandType === 'loadLabware' && + (commandType === 'loadLabware' || + commandType === 'loadLid' || + commandType === 'loadLidStack') && params.location !== 'offDeck' && params.location !== 'systemLocation' && 'addressableAreaName' in params.location && diff --git a/shared-data/js/helpers/getLoadedLabwareDefinitionsByUri.ts b/shared-data/js/helpers/getLoadedLabwareDefinitionsByUri.ts index 120dc760d13..4892569a318 100644 --- a/shared-data/js/helpers/getLoadedLabwareDefinitionsByUri.ts +++ b/shared-data/js/helpers/getLoadedLabwareDefinitionsByUri.ts @@ -9,7 +9,11 @@ export function getLoadedLabwareDefinitionsByUri( commands: RunTimeCommand[] ): LabwareDefinitionsByUri { return commands.reduce((acc, command) => { - if (command.commandType === 'loadLabware') { + if ( + command.commandType === 'loadLabware' || + command.commandType === 'loadLid' || + command.commandType === 'loadLidStack' + ) { const labwareDef: LabwareDefinition2 | undefined = command.result?.definition if (labwareDef == null) { From 253c30095342e3e5bdc0ea65dbd17dd1378d37c5 Mon Sep 17 00:00:00 2001 From: Sanniti Pimpley Date: Thu, 30 Jan 2025 16:51:39 -0500 Subject: [PATCH 2/4] fix(api): use smaller of max pipette volume and max tip volume for splitting large transfer volumes (#17387) # Overview Found a bug in the logic that splits large volume transfers into smaller volumes. Bug is that we only check the transfer volume against max pipette volume. So if you use a 50uL pipette with a 200uL tip, everything works correctly, but if you use a 1000uL pipette with 50uL tip, it attempts to pipette upto 1000uL, instead of clipping to the max tip volume of 50uL. This PR fixes that by taking the smaller of pipette volume and tip volume as the max volume a single transfer can take. ## Risk assessment Low. A scope-limited bug fix --- .../protocol_api/core/engine/instrument.py | 10 ++++-- .../core/engine/test_instrument_core.py | 36 +++++++++++++++++++ .../test_transfer_with_liquid_classes.py | 14 ++++---- 3 files changed, 51 insertions(+), 9 deletions(-) diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index 30ace69e63b..d9f0cbdd1d4 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/api/src/opentrons/protocol_api/core/engine/instrument.py @@ -1008,7 +1008,12 @@ def transfer_liquid( # noqa: C901 source_dest_per_volume_step = tx_commons.expand_for_volume_constraints( volumes=[volume for _ in range(len(source))], targets=zip(source, dest), - max_volume=self.get_max_volume(), + max_volume=min( + self.get_max_volume(), + tip_racks[0][1] + .get_well_core("A1") + .get_max_volume(), # Assuming all tips in tiprack are of same volume + ), ) def _drop_tip() -> None: @@ -1175,10 +1180,11 @@ def aspirate_liquid_class( Return: List of liquid and air gap pairs in tip. """ aspirate_props = transfer_properties.aspirate + # TODO (spp, 2025-01-30): check if check_valid_volume_parameters is necessary and is enough. tx_commons.check_valid_volume_parameters( disposal_volume=0, # No disposal volume for 1-to-1 transfer air_gap=aspirate_props.retract.air_gap_by_volume.get_for_volume(volume), - max_volume=self.get_max_volume(), + max_volume=self.get_working_volume(), ) source_loc, source_well = source aspirate_point = ( diff --git a/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py b/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py index 8ac1ffc1dc8..12e658ab7ae 100644 --- a/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py +++ b/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py @@ -1786,6 +1786,42 @@ def test_aspirate_liquid_class( assert result == [LiquidAndAirGapPair(air_gap=222, liquid=111)] +def test_aspirate_liquid_class_raises_for_more_than_max_volume( + decoy: Decoy, + mock_engine_client: EngineClient, + subject: InstrumentCore, + minimal_liquid_class_def2: LiquidClassSchemaV1, + mock_transfer_components_executor: TransferComponentsExecutor, +) -> None: + """It should call aspirate sub-steps execution based on liquid class.""" + source_well = decoy.mock(cls=WellCore) + source_location = Location(Point(1, 2, 3), labware=None) + test_liquid_class = LiquidClass.create(minimal_liquid_class_def2) + test_transfer_properties = test_liquid_class.get_for( + "flex_1channel_50", "opentrons_flex_96_tiprack_50ul" + ) + decoy.when( + mock_engine_client.state.pipettes.get_working_volume("abc123") + ).then_return(100) + decoy.when( + tx_commons.check_valid_volume_parameters( + disposal_volume=0, + air_gap=test_transfer_properties.aspirate.retract.air_gap_by_volume.get_for_volume( + 123 + ), + max_volume=100, + ) + ).then_raise(ValueError("Oh oh!")) + with pytest.raises(ValueError, match="Oh oh!"): + subject.aspirate_liquid_class( + volume=123, + source=(source_location, source_well), + transfer_properties=test_transfer_properties, + transfer_type=TransferType.ONE_TO_ONE, + tip_contents=[], + ) + + def test_dispense_liquid_class( decoy: Decoy, mock_engine_client: EngineClient, diff --git a/api/tests/opentrons/protocol_api_integration/test_transfer_with_liquid_classes.py b/api/tests/opentrons/protocol_api_integration/test_transfer_with_liquid_classes.py index ba3d3facd6a..6fe2474cfba 100644 --- a/api/tests/opentrons/protocol_api_integration/test_transfer_with_liquid_classes.py +++ b/api/tests/opentrons/protocol_api_integration/test_transfer_with_liquid_classes.py @@ -27,8 +27,8 @@ def test_water_transfer_with_volume_more_than_tip_max( tiprack = simulated_protocol_context.load_labware( "opentrons_flex_96_tiprack_50ul", "D1" ) - pipette_50 = simulated_protocol_context.load_instrument( - "flex_1channel_50", mount="left", tip_racks=[tiprack] + pipette_1k = simulated_protocol_context.load_instrument( + "flex_1channel_1000", mount="left", tip_racks=[tiprack] ) nest_plate = simulated_protocol_context.load_labware( "nest_96_wellplate_200ul_flat", "C3" @@ -47,7 +47,7 @@ def test_water_transfer_with_volume_more_than_tip_max( mock_manager = mock.Mock() mock_manager.attach_mock(patched_pick_up_tip, "pick_up_tip") - pipette_50.transfer_liquid( + pipette_1k.transfer_liquid( liquid_class=water, volume=60, source=nest_plate.rows()[0], @@ -58,7 +58,7 @@ def test_water_transfer_with_volume_more_than_tip_max( assert patched_pick_up_tip.call_count == 24 patched_pick_up_tip.reset_mock() - pipette_50.transfer_liquid( + pipette_1k.transfer_liquid( liquid_class=water, volume=100, source=nest_plate.rows()[0], @@ -69,8 +69,8 @@ def test_water_transfer_with_volume_more_than_tip_max( assert patched_pick_up_tip.call_count == 12 patched_pick_up_tip.reset_mock() - pipette_50.pick_up_tip() - pipette_50.transfer_liquid( + pipette_1k.pick_up_tip() + pipette_1k.transfer_liquid( liquid_class=water, volume=50, source=nest_plate.rows()[0], @@ -78,7 +78,7 @@ def test_water_transfer_with_volume_more_than_tip_max( new_tip="never", trash_location=trash, ) - pipette_50.drop_tip() + pipette_1k.drop_tip() assert patched_pick_up_tip.call_count == 1 From a79e873ff55fd2304524e0cf60c25f596f73b82f Mon Sep 17 00:00:00 2001 From: David Chau <46395074+ddcc4@users.noreply.github.com> Date: Thu, 30 Jan 2025 16:57:54 -0500 Subject: [PATCH 3/4] feat(protocol-designer): add python field to commands and timeline (#17383) # Overview This starts the plumbing for Python generation from Protocol Designer. AUTH-1385 We're adding a field for Python code to each command (`CommandsAndWarnings`) and to each entry of the timeline (`CommandsAndRobotState`). And in the reducer, we concatenate the Python commands together, analogously to how we concatenate the JSON commands together. ## Test Plan and Hands on Testing I added unit tests to show that the Python code concatenation works, and that the Python code gets copied from the CommandCreators to the Timeline. The tests also demonstrate that the changes do NOT affect the behavior of existing commands that don't generate Python. I've also done hands-on testing in a private experimental branch that has a more complete implementation of Python generation. ## Review requests My first time making a functional change to PD, let me know if I'm overlooking anything. ## Risk assessment Low. Should not cause any observable change to PD's behavior. --- step-generation/src/__tests__/glue.test.ts | 74 +++++++++++++++++++ step-generation/src/types.ts | 2 + .../src/utils/commandCreatorsTimeline.ts | 1 + .../src/utils/reduceCommandCreators.ts | 7 ++ 4 files changed, 84 insertions(+) diff --git a/step-generation/src/__tests__/glue.test.ts b/step-generation/src/__tests__/glue.test.ts index b5e651d3e16..8524053cb43 100644 --- a/step-generation/src/__tests__/glue.test.ts +++ b/step-generation/src/__tests__/glue.test.ts @@ -93,6 +93,30 @@ const divideCreator: any = ( } } +const pythonHelloWorldCreator: any = ( + params: CountParams, + invariantContext: InvariantContext, + prevState: CountState +) => { + return { + commands: [], + warnings: [], + python: 'print("Hello world")', + } +} + +const pythonGoodbyeWorldCreator: any = ( + params: CountParams, + invariantContext: InvariantContext, + prevState: CountState +) => { + return { + commands: [], + warnings: [], + python: 'print("Goodbye world")', + } +} + function mockNextRobotStateAndWarningsSingleCommand( command: CountCommand, invariantContext: any, @@ -177,6 +201,9 @@ describe('reduceCommandCreators', () => { { command: 'multiply', params: { value: 2 } }, ], warnings: [], + // Note no `python` field here. + // Existing CommandCreators that don't emit Python should behave exactly the same as before. + // This test makes sure we do NOT produce results like `python:'undefined'` or `python:''` or `python:'\n'`. }) }) @@ -226,6 +253,43 @@ describe('reduceCommandCreators', () => { ], }) }) + + it('Python commands are joined together', () => { + const initialState: any = {} + const result: any = reduceCommandCreators( + [ + curryCommandCreator(pythonHelloWorldCreator, {}), + curryCommandCreator(pythonGoodbyeWorldCreator, {}), + ], + invariantContext, + initialState + ) + + expect(result).toEqual({ + commands: [], + warnings: [], + python: 'print("Hello world")\nprint("Goodbye world")', + }) + }) + + it('Python commands mixed with non-Python commands', () => { + const initialState: any = {} + const result: any = reduceCommandCreators( + [ + curryCommandCreator(addCreator, { value: 1 }), + curryCommandCreator(pythonHelloWorldCreator, {}), + ], + invariantContext, + initialState + ) + + expect(result).toEqual({ + commands: [{ command: 'add', params: { value: 1 } }], + warnings: [], + python: 'print("Hello world")', + // should only get 1 line of Python with no stray newlines or `undefined`s. + }) + }) }) describe('commandCreatorsTimeline', () => { @@ -236,6 +300,7 @@ describe('commandCreatorsTimeline', () => { curryCommandCreator(addCreatorWithWarning, { value: 4 }), curryCommandCreator(divideCreator, { value: 0 }), curryCommandCreator(multiplyCreator, { value: 3 }), + curryCommandCreator(pythonHelloWorldCreator, {}), ], invariantContext, initialState @@ -263,6 +328,7 @@ describe('commandCreatorsTimeline', () => { ], }, // no more steps in the timeline, stopped by error + // python output is suppressed too ], }) }) @@ -275,6 +341,7 @@ describe('commandCreatorsTimeline', () => { curryCommandCreator(addCreatorWithWarning, { value: 3 }), curryCommandCreator(multiplyCreator, { value: 2 }), curryCommandCreator(addCreatorWithWarning, { value: 1 }), + curryCommandCreator(pythonHelloWorldCreator, {}), ], invariantContext, initialState @@ -309,6 +376,13 @@ describe('commandCreatorsTimeline', () => { }, ], }, + // Python hello world + { + robotState: { count: 17 }, + commands: [], + warnings: [], + python: 'print("Hello world")', + }, ]) }) }) diff --git a/step-generation/src/types.ts b/step-generation/src/types.ts index 34105529058..95adcbee5a8 100644 --- a/step-generation/src/types.ts +++ b/step-generation/src/types.ts @@ -619,6 +619,7 @@ export interface CommandsAndRobotState { commands: CreateCommand[] robotState: RobotState warnings?: CommandCreatorWarning[] + python?: string } export interface CommandCreatorErrorResponse { @@ -629,6 +630,7 @@ export interface CommandCreatorErrorResponse { export interface CommandsAndWarnings { commands: CreateCommand[] warnings?: CommandCreatorWarning[] + python?: string } export type CommandCreatorResult = | CommandsAndWarnings diff --git a/step-generation/src/utils/commandCreatorsTimeline.ts b/step-generation/src/utils/commandCreatorsTimeline.ts index 878368e6a25..c5728a754ae 100644 --- a/step-generation/src/utils/commandCreatorsTimeline.ts +++ b/step-generation/src/utils/commandCreatorsTimeline.ts @@ -53,6 +53,7 @@ export const commandCreatorsTimeline = ( commands: commandCreatorResult.commands, robotState: nextRobotStateAndWarnings.robotState, warnings: commandCreatorResult.warnings, + python: commandCreatorResult.python, } return { timeline: [...acc.timeline, nextResult], diff --git a/step-generation/src/utils/reduceCommandCreators.ts b/step-generation/src/utils/reduceCommandCreators.ts index 03a5814b46d..68d8c227ccc 100644 --- a/step-generation/src/utils/reduceCommandCreators.ts +++ b/step-generation/src/utils/reduceCommandCreators.ts @@ -13,6 +13,7 @@ interface CCReducerAcc { commands: CreateCommand[] errors: CommandCreatorError[] warnings: CommandCreatorWarning[] + python?: string } export const reduceCommandCreators = ( commandCreators: CurriedCommandCreator[], @@ -36,6 +37,10 @@ export const reduceCommandCreators = ( } } const allCommands = [...prev.commands, ...next.commands] + const allPython = [ + ...(prev.python ? [prev.python] : []), + ...(next.python ? [next.python] : []), + ].join('\n') const updates = getNextRobotStateAndWarnings( next.commands, invariantContext, @@ -50,6 +55,7 @@ export const reduceCommandCreators = ( ...(next.warnings || []), ...updates.warnings, ], + ...(allPython && { python: allPython }), } }, { @@ -69,5 +75,6 @@ export const reduceCommandCreators = ( return { commands: result.commands, warnings: result.warnings, + ...(result.python && { python: result.python }), } } From 3c2379302a3d4c4f24c2e342a22029866a2e68db Mon Sep 17 00:00:00 2001 From: Sanniti Pimpley Date: Thu, 30 Jan 2025 17:18:39 -0500 Subject: [PATCH 4/4] feat(api): fix InstrumentContext.name for Flex and update LiquidClass.get_for() (#17289) Closes AUTH-1295, AUTH-1299 # Overview - fixes `InstrumentContext.name` to fetch the python API load name of flex pipettes - updates `LiquidClass.get_for()` to accept the a loaded instrument object and loaded tiprack object for `pipette` and `tip_rack` args respectively. - changes the tiprack argument name of `get_for()` from `tiprack` to `tip_rack` to be consistent with API naming conventions. ## Risk assessment None --------- Co-authored-by: Max Marrone --- api/release-notes-internal.md | 4 ++ api/src/opentrons/protocol_api/_liquid.py | 39 +++++++++++--- .../protocol_api/core/engine/instrument.py | 52 +++++++++--------- .../protocol_api/instrument_context.py | 5 ++ .../protocols/api_support/instrument.py | 15 +++++- .../core/engine/test_instrument_core.py | 54 +++++++++++++++---- .../test_transfer_components_executor.py | 2 +- .../protocol_api/test_liquid_class.py | 12 +++++ .../protocol_api/test_protocol_context.py | 1 + .../test_liquid_classes.py | 7 +-- 10 files changed, 140 insertions(+), 51 deletions(-) diff --git a/api/release-notes-internal.md b/api/release-notes-internal.md index 7fb93059e15..1186b510eb6 100644 --- a/api/release-notes-internal.md +++ b/api/release-notes-internal.md @@ -11,6 +11,10 @@ This internal release, pulled from the `edge` branch, contains features being de - Python API version bumped to 2.23 - Added liquid classes and new transfer functions +### Bug Fixes In This Release (list in progress): +- Fixed `InstrumentContext.name` so that it returns the correct API-specific names of Flex pipettes. + + ## Internal Release 2.3.0-alpha.2 This internal release, pulled from the `edge` branch, contains features being developed for 8.3.0. It's for internal testing only. diff --git a/api/src/opentrons/protocol_api/_liquid.py b/api/src/opentrons/protocol_api/_liquid.py index 12c9a140ce3..fa979428eb2 100644 --- a/api/src/opentrons/protocol_api/_liquid.py +++ b/api/src/opentrons/protocol_api/_liquid.py @@ -1,7 +1,7 @@ from __future__ import annotations from dataclasses import dataclass -from typing import Optional, Dict +from typing import Optional, Dict, Union, TYPE_CHECKING from opentrons_shared_data.liquid_classes.liquid_class_definition import ( LiquidClassSchemaV1, @@ -12,6 +12,9 @@ build_transfer_properties, ) +if TYPE_CHECKING: + from . import InstrumentContext, Labware + @dataclass(frozen=True) class Liquid: @@ -64,18 +67,42 @@ def name(self) -> str: def display_name(self) -> str: return self._display_name - def get_for(self, pipette: str, tiprack: str) -> TransferProperties: + def get_for( + 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 + + if isinstance(pipette, InstrumentContext): + pipette_name = pipette.name + elif isinstance(pipette, str): + pipette_name = pipette + else: + raise ValueError( + f"{pipette} should either be an InstrumentContext object" + f" or a pipette name string." + ) + + if isinstance(tip_rack, Labware): + tiprack_uri = tip_rack.uri + elif isinstance(tip_rack, str): + tiprack_uri = tip_rack + else: + raise ValueError( + f"{tip_rack} should either be a tiprack Labware object" + f" or a tiprack URI string." + ) + try: - settings_for_pipette = self._by_pipette_setting[pipette] + settings_for_pipette = self._by_pipette_setting[pipette_name] except KeyError: raise ValueError( - f"No properties found for {pipette} in {self._name} liquid class" + f"No properties found for {pipette_name} in {self._name} liquid class" ) try: - transfer_properties = settings_for_pipette[tiprack] + transfer_properties = settings_for_pipette[tiprack_uri] except KeyError: raise ValueError( - f"No properties found for {tiprack} in {self._name} liquid class" + f"No properties found for {tiprack_uri} in {self._name} liquid class" ) return transfer_properties diff --git a/api/src/opentrons/protocol_api/core/engine/instrument.py b/api/src/opentrons/protocol_api/core/engine/instrument.py index d9f0cbdd1d4..d1898e5ccaa 100644 --- a/api/src/opentrons/protocol_api/core/engine/instrument.py +++ b/api/src/opentrons/protocol_api/core/engine/instrument.py @@ -42,7 +42,7 @@ from opentrons.protocol_engine.errors.exceptions import TipNotAttachedError from opentrons.protocol_engine.clients import SyncClient as EngineClient from opentrons.protocols.api_support.definitions import MAX_SUPPORTED_VERSION -from opentrons_shared_data.pipette.types import PipetteNameType, PIPETTE_API_NAMES_MAP +from opentrons_shared_data.pipette.types import PIPETTE_API_NAMES_MAP from opentrons_shared_data.errors.exceptions import ( UnsupportedHardwareCommand, ) @@ -62,6 +62,9 @@ _DISPENSE_VOLUME_VALIDATION_ADDED_IN = APIVersion(2, 17) +_FLEX_PIPETTE_NAMES_FIXED_IN = APIVersion(2, 23) +"""The version after which InstrumentContext.name returns the correct API-specific names of Flex pipettes.""" + class InstrumentCore(AbstractInstrument[WellCore, LabwareCore]): """Instrument API core using a ProtocolEngine. @@ -721,33 +724,29 @@ def get_pipette_name(self) -> str: Will match the load name of the actually loaded pipette, which may differ from the requested load name. - """ - # TODO (tz, 11-23-22): revert this change when merging - # https://opentrons.atlassian.net/browse/RLIQ-251 - pipette = self._engine_client.state.pipettes.get(self._pipette_id) - return ( - pipette.pipetteName.value - if isinstance(pipette.pipetteName, PipetteNameType) - else pipette.pipetteName - ) - def get_load_name(self) -> str: - """Get the pipette's requested API load name. + From API v2.15 to v2.22, this property returned an internal, engine-specific, + name for Flex pipettes (eg, "p50_multi_flex" instead of "flex_8channel_50"). - This is the load name that is specified in the `ProtocolContext.load_instrument()` - method. This name might differ from the engine-specific pipette name. + From API v2.23 onwards, this behavior is fixed so that this property returns + the API-specific names of Flex pipettes. """ + # TODO (tz, 11-23-22): revert this change when merging + # https://opentrons.atlassian.net/browse/RLIQ-251 pipette = self._engine_client.state.pipettes.get(self._pipette_id) - load_name = next( - ( - pip_api_name - for pip_api_name, pip_name in PIPETTE_API_NAMES_MAP.items() - if pip_name == pipette.pipetteName - ), - None, - ) - assert load_name, "Load name not found." - return load_name + if self._protocol_core.api_version < _FLEX_PIPETTE_NAMES_FIXED_IN: + return pipette.pipetteName.value + else: + name = next( + ( + pip_api_name + for pip_api_name, pip_name in PIPETTE_API_NAMES_MAP.items() + if pip_name == pipette.pipetteName + ), + None, + ) + assert name, "Pipette name not found." + return name def get_model(self) -> str: return self._engine_client.state.pipettes.get_model_name(self._pipette_id) @@ -932,7 +931,7 @@ def load_liquid_class( """ liquid_class_record = LiquidClassRecord( liquidClassName=name, - pipetteModel=self.get_load_name(), + pipetteModel=self.get_pipette_name(), tiprack=tiprack_uri, aspirate=transfer_properties.aspirate.as_shared_data_model(), singleDispense=transfer_properties.dispense.as_shared_data_model(), @@ -994,8 +993,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_load_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( diff --git a/api/src/opentrons/protocol_api/instrument_context.py b/api/src/opentrons/protocol_api/instrument_context.py index 6bd93f27970..992aa68c785 100644 --- a/api/src/opentrons/protocol_api/instrument_context.py +++ b/api/src/opentrons/protocol_api/instrument_context.py @@ -64,6 +64,7 @@ _AIR_GAP_TRACKING_ADDED_IN = APIVersion(2, 22) """The version after which air gaps should be implemented with a separate call instead of an aspirate for better liquid volume tracking.""" + AdvancedLiquidHandling = v1_transfer.AdvancedLiquidHandling @@ -2004,6 +2005,10 @@ def trash_container( def name(self) -> str: """ The name string for the pipette (e.g., ``"p300_single"``). + + From API v2.15 to v2.22, this property returned an internal name for Flex pipettes. + From API v2.23 onwards, this behavior is fixed so that this property returns + the Python Protocol API load names of Flex pipettes. """ return self._core.get_pipette_name() diff --git a/api/src/opentrons/protocols/api_support/instrument.py b/api/src/opentrons/protocols/api_support/instrument.py index 3299b8512f9..35625100645 100644 --- a/api/src/opentrons/protocols/api_support/instrument.py +++ b/api/src/opentrons/protocols/api_support/instrument.py @@ -99,7 +99,20 @@ def validate_tiprack( gen_lookup = ( "FLEX" if ("flex" in instr_metadata or "96" in instr_metadata) else "OT2" ) - valid_vols = VALID_PIP_TIPRACK_VOL[gen_lookup][instrument_name.split("_")[0]] + + # TODO (spp, 2025-01-30): do what AA's note above says or at least, + # fetch the 'pip_type' below from the 'model' field in pipette definitions + # so that we don't have to figure it out from pipette names + if instrument_name.split("_")[0] == "flex": + # Flex's API load names have the format 'flex_1channel_1000' + # From API v2.23 on, this is the name returned by InstrumentContext.name + pip_type = "p" + instrument_name.split("_")[2] + else: + # Until API v2.23, InstrumentContext.name returned the engine-specific names + # of Flex pipettes. These names, as well as OT2 pipette names, + # have the format- 'p1000_single_gen2' or 'p1000_single_flex' + pip_type = instrument_name.split("_")[0] + valid_vols = VALID_PIP_TIPRACK_VOL[gen_lookup][pip_type] if tiprack_vol not in valid_vols: log.warning( f"The pipette {instrument_name} and its tip rack {tip_rack.load_name}" diff --git a/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py b/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py index 12e658ab7ae..c7e5fa904e0 100644 --- a/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py +++ b/api/tests/opentrons/protocol_api/core/engine/test_instrument_core.py @@ -187,32 +187,60 @@ def test_pipette_id(subject: InstrumentCore) -> None: assert subject.pipette_id == "abc123" -def test_get_pipette_name( - decoy: Decoy, mock_engine_client: EngineClient, subject: InstrumentCore +@pytest.mark.parametrize( + "version", + [ + APIVersion(2, 15), + APIVersion(2, 17), + APIVersion(2, 20), + APIVersion(2, 22), + ], +) +def test_get_pipette_name_old( + decoy: Decoy, + mock_engine_client: EngineClient, + mock_protocol_core: ProtocolCore, + subject: InstrumentCore, + version: APIVersion, ) -> None: """It should get the pipette's load name.""" + decoy.when(mock_protocol_core.api_version).then_return(version) decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return( LoadedPipette.model_construct(pipetteName=PipetteNameType.P300_SINGLE) # type: ignore[call-arg] ) - - result = subject.get_pipette_name() - - assert result == "p300_single" + assert subject.get_pipette_name() == "p300_single" + decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return( + LoadedPipette.model_construct(pipetteName=PipetteNameType.P1000_96) # type: ignore[call-arg] + ) + assert subject.get_pipette_name() == "p1000_96" + decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return( + LoadedPipette.model_construct(pipetteName=PipetteNameType.P50_SINGLE_FLEX) # type: ignore[call-arg] + ) + assert subject.get_pipette_name() == "p50_single_flex" -def test_get_pipette_load_name( - decoy: Decoy, mock_engine_client: EngineClient, subject: InstrumentCore +@pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 23))) +def test_get_pipette_name_new( + decoy: Decoy, + mock_engine_client: EngineClient, + mock_protocol_core: ProtocolCore, + subject: InstrumentCore, + version: APIVersion, ) -> None: """It should get the pipette's API-specific load name.""" + decoy.when(mock_protocol_core.api_version).then_return(version) decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return( LoadedPipette.model_construct(pipetteName=PipetteNameType.P300_SINGLE) # type: ignore[call-arg] ) - assert subject.get_load_name() == "p300_single" - + assert subject.get_pipette_name() == "p300_single" decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return( LoadedPipette.model_construct(pipetteName=PipetteNameType.P1000_96) # type: ignore[call-arg] ) - assert subject.get_load_name() == "flex_96channel_1000" + assert subject.get_pipette_name() == "flex_96channel_1000" + decoy.when(mock_engine_client.state.pipettes.get("abc123")).then_return( + LoadedPipette.model_construct(pipetteName=PipetteNameType.P50_SINGLE_FLEX) # type: ignore[call-arg] + ) + assert subject.get_pipette_name() == "flex_1channel_50" def test_get_mount( @@ -1671,11 +1699,14 @@ def test_liquid_probe_with_recovery( ) +@pytest.mark.parametrize("version", versions_at_or_above(APIVersion(2, 23))) def test_load_liquid_class( decoy: Decoy, mock_engine_client: EngineClient, + mock_protocol_core: ProtocolCore, subject: InstrumentCore, minimal_liquid_class_def2: LiquidClassSchemaV1, + version: APIVersion, ) -> None: """It should send the load liquid class command to the engine.""" sample_aspirate_data = minimal_liquid_class_def2.byPipette[0].byTipType[0].aspirate @@ -1686,6 +1717,7 @@ def test_load_liquid_class( minimal_liquid_class_def2.byPipette[0].byTipType[0].multiDispense ) + decoy.when(mock_protocol_core.api_version).then_return(version) test_liq_class = decoy.mock(cls=LiquidClass) test_transfer_props = decoy.mock(cls=TransferProperties) diff --git a/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py b/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py index 4dadf5b503b..b58b873523c 100644 --- a/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py +++ b/api/tests/opentrons/protocol_api/core/engine/test_transfer_components_executor.py @@ -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" ) diff --git a/api/tests/opentrons/protocol_api/test_liquid_class.py b/api/tests/opentrons/protocol_api/test_liquid_class.py index 7118080eda0..47404c2b480 100644 --- a/api/tests/opentrons/protocol_api/test_liquid_class.py +++ b/api/tests/opentrons/protocol_api/test_liquid_class.py @@ -1,10 +1,12 @@ """Tests for LiquidClass methods.""" import pytest +from decoy import Decoy from opentrons_shared_data.liquid_classes.liquid_class_definition import ( LiquidClassSchemaV1, ) from opentrons.protocol_api import LiquidClass +from opentrons.protocol_api import InstrumentContext, Labware def test_create_liquid_class( @@ -17,6 +19,7 @@ def test_create_liquid_class( def test_get_for_pipette_and_tip( + decoy: Decoy, minimal_liquid_class_def2: LiquidClassSchemaV1, ) -> None: """It should get the properties for the specified pipette and tip.""" @@ -26,6 +29,15 @@ def test_get_for_pipette_and_tip( 10.0: 40.0, 20.0: 30.0, } + mock_instrument = decoy.mock(cls=InstrumentContext) + mock_tiprack = decoy.mock(cls=Labware) + decoy.when(mock_instrument.name).then_return("flex_1channel_50") + decoy.when(mock_tiprack.uri).then_return("opentrons_flex_96_tiprack_50ul") + result_2 = liq_class.get_for(mock_instrument, mock_tiprack) + assert result_2.aspirate.flow_rate_by_volume.as_dict() == { + 10.0: 40.0, + 20.0: 30.0, + } def test_get_for_raises_for_incorrect_pipette_or_tip( diff --git a/api/tests/opentrons/protocol_api/test_protocol_context.py b/api/tests/opentrons/protocol_api/test_protocol_context.py index ebe6734a539..4536003094a 100644 --- a/api/tests/opentrons/protocol_api/test_protocol_context.py +++ b/api/tests/opentrons/protocol_api/test_protocol_context.py @@ -283,6 +283,7 @@ def test_load_instrument( ).then_return(mock_instrument_core) decoy.when(mock_instrument_core.get_pipette_name()).then_return("Gandalf the Grey") + decoy.when(mock_instrument_core.get_model()).then_return("wizard") decoy.when(mock_core.get_disposal_locations()).then_raise( NoTrashDefinedError("No trash!") ) diff --git a/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py b/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py index 83b53f01e1a..cf7791a271a 100644 --- a/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py +++ b/api/tests/opentrons/protocol_api_integration/test_liquid_classes.py @@ -13,7 +13,7 @@ def test_liquid_class_creation_and_property_fetching( ) -> None: """It should create the liquid class and provide access to its properties.""" pipette_load_name = "flex_8channel_50" - simulated_protocol_context.load_instrument(pipette_load_name, mount="left") + p50 = simulated_protocol_context.load_instrument(pipette_load_name, mount="left") tiprack = simulated_protocol_context.load_labware( "opentrons_flex_96_tiprack_50ul", "D1" ) @@ -24,10 +24,7 @@ def test_liquid_class_creation_and_property_fetching( # TODO (spp, 2024-10-17): update this to fetch pipette load name from instrument context assert ( - water.get_for( - pipette_load_name, tiprack.uri - ).dispense.flow_rate_by_volume.get_for_volume(1) - == 50 + water.get_for(p50, tiprack).dispense.flow_rate_by_volume.get_for_volume(1) == 50 ) assert water.get_for(pipette_load_name, tiprack.uri).aspirate.submerge.speed == 100