From 75d91126ab3f6e90eebd6402c22edc3e929a5ab8 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 22 Nov 2024 13:52:26 -0500 Subject: [PATCH 1/7] Fix file leak in test. --- .../python/tests/pipette/test_mutable_configurations.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/shared-data/python/tests/pipette/test_mutable_configurations.py b/shared-data/python/tests/pipette/test_mutable_configurations.py index 40059f6bfe6..b0a7778ca96 100644 --- a/shared-data/python/tests/pipette/test_mutable_configurations.py +++ b/shared-data/python/tests/pipette/test_mutable_configurations.py @@ -68,9 +68,8 @@ def test_load_old_overrides_regression( "type": "float", "default": 0.1, } - json.dump( - TMPFILE_DATA, open(override_configuration_path / "P20SV222021040709.json", "w") - ) + with open(override_configuration_path / "P20SV222021040709.json", "w") as f: + json.dump(TMPFILE_DATA, f) configs = mutable_configurations.load_with_mutable_configurations( pipette_definition.PipetteModelVersionType( pipette_type=types.PipetteModelType.p20, From f6250a7340fbccd27d931ad5fc7bebf3465be5d3 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 22 Nov 2024 14:42:31 -0500 Subject: [PATCH 2/7] Add quick and dirty tests. --- .../pipette/test_mutable_configurations.py | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/shared-data/python/tests/pipette/test_mutable_configurations.py b/shared-data/python/tests/pipette/test_mutable_configurations.py index b0a7778ca96..b94256f909f 100644 --- a/shared-data/python/tests/pipette/test_mutable_configurations.py +++ b/shared-data/python/tests/pipette/test_mutable_configurations.py @@ -1,4 +1,5 @@ import json +import logging import os from pathlib import Path from typing import Dict, Any, cast, Union, Generator @@ -304,3 +305,152 @@ def test_build_mutable_config_using_old_units() -> None: assert ( types.MutableConfig.build(**old_units_config, name="dropTipSpeed") is not None # type: ignore ) + + +@pytest.mark.parametrize( + ("filename", "type", "channels", "version", "file_contents"), + [ + ( + "P20MV202020121412.json", + types.PipetteModelType.p20, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 0), + '{"model": "p20_multi_v2.0"}', + ), + ( + "P3HSV1318071638.json", + types.PipetteModelType.p300, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(1, 3), + '{"dropTipShake": true, "model": "p300_single_v1.3", "quirks": {"dropTipShake": true}, "top": {"value": 30.0, "default": 19.5, "units": "mm", "type": "float", "min": -20, "max": 30}, "pickUpPresses": {"value": 3.0, "default": 3, "units": "presses", "type": "int", "min": 0, "max": 15}}', + ), + ( + "P3HMV212021040004.json", + types.PipetteModelType.p300, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 1), + '{"needsUnstick": true, "model": "p300_multi_v2.1"}', + ), + ( + "P20SV202020032604.json", + types.PipetteModelType.p20, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 0), + '{"model": "p20_single_v2.0"}', + ), + ( + "P1KSV202019072441.json", + types.PipetteModelType.p1000, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 0), + '{"pickupTipShake": true, "model": "p1000_single_v2.0", "quirks": {"pickupTipShake": true}}', + ), + ( + "P3HMV202021011105.json", + types.PipetteModelType.p300, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 0), + '{"needsUnstick": true, "model": "p300_multi_v2.0"}', + ), + ( + "P20SV202019072527.json", + types.PipetteModelType.p20, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 0), + '{"model": "p20_single_v2.0"}', + ), + ( + "P3HSV202021042602.json", + types.PipetteModelType.p300, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 0), + '{"model": "p300_single_v2.0"}', + ), + ( + "P20SV222021030914.json", + types.PipetteModelType.p20, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 2), + '{"model": "p20_single_v2.2", "quirks": {}, "pickUpPresses": {"value": 3.0, "default": 1, "units": "presses", "type": "int", "min": 0, "max": 15}}', + ), + ( + "P3HMV202020040801.json", + types.PipetteModelType.p300, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 0), + '{"needsUnstick": true, "model": "p300_multi_v2.0", "quirks": {"needsUnstick": true}, "top": {"value": 20.0, "default": 19.5, "units": "mm", "type": "float", "min": -20, "max": 30}, "bottom": {"value": -14.0, "default": -14.5, "units": "mm", "type": "float", "min": -20, "max": 30}, "blowout": {"value": -18.5, "default": -19.0, "units": "mm", "type": "float", "min": -20, "max": 30}, "dropTip": {"value": -20.0, "default": -33.4, "units": "mm", "type": "float", "min": -20, "max": 30}, "pickUpCurrent": {"value": 0.9, "default": 0.8, "units": "amps", "type": "float", "min": 0.1, "max": 2.0}, "pickUpDistance": {"value": 10.0, "default": 11.0, "units": "mm", "type": "float", "min": 0, "max": 10}, "pickUpIncrement": {"value": 0.1, "default": 0.0, "units": "mm", "type": "int", "min": 0, "max": 10}, "pickUpPresses": {"value": 4.0, "default": 1, "units": "presses", "type": "int", "min": 0, "max": 15}, "pickUpSpeed": {"value": 11.0, "default": 10.0, "units": "mm/s", "type": "float", "min": 0.01, "max": 30}, "plungerCurrent": {"value": 1.1, "default": 1.0, "units": "amps", "type": "float", "min": 0.1, "max": 2.0}, "dropTipCurrent": {"value": 1.22, "default": 1.25, "units": "amps", "type": "float", "min": 0.1, "max": 2.0}, "dropTipSpeed": {"value": 8.0, "default": 7.5, "units": "mm/s", "type": "float", "min": 0.01, "max": 30}, "tipLength": {"value": 52.0, "default": 51.0, "units": "mm", "type": "float", "min": 0, "max": 100}}', + ), + ( + "P3HMV212021040002.json", + types.PipetteModelType.p300, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 1), + '{"needsUnstick": true, "model": "p300_multi_v2.1"}', + ), + ( + "P3HMV1318072625.json", + types.PipetteModelType.p300, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(1, 3), + '{"dropTipShake": true, "model": "p300_multi_v1.3", "quirks": {"dropTipShake": true}, "pickUpPresses": {"value": 4.0, "default": 3, "units": "presses", "type": "int", "min": 0, "max": 15}}', + ), + ( + "P50MV1519091757.json", + types.PipetteModelType.p50, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(1, 5), + '{"dropTipShake": true, "doubleDropTip": true, "model": "p50_multi_v1.5", "quirks": {"doubleDropTip": true, "dropTipShake": true}}', + ), + ( + "P3HSV202019072224.json", + types.PipetteModelType.p300, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 0), + '{"model": "p300_single_v2.0", "quirks": {}}', + ), + ( + "P20MV202019112708.json", + types.PipetteModelType.p20, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 0), + '{"model": "p20_multi_v2.0", "quirks": {}}', + ), + ( + "P3HSV202021031503.json", + types.PipetteModelType.p300, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 1), + '{"model": "p300_single_v2.1"}', + ), + ( + "P1KSV202020060206.json", + types.PipetteModelType.p1000, + types.PipetteChannelType.SINGLE_CHANNEL, + types.PipetteVersionType(2, 0), + '{"pickupTipShake": true, "model": "p1000_single_v2.0"}', + ), + ( + "P3HMV202021010906.json", + types.PipetteModelType.p300, + types.PipetteChannelType.EIGHT_CHANNEL, + types.PipetteVersionType(2, 0), + '{"needsUnstick": true, "model": "p300_multi_v2.0"}', + ), + ], +) +def test_loading_does_not_log_warnings( + filename: str, + type: types.PipetteModelType, + channels: types.PipetteChannelType, + version: types.PipetteVersionType, + file_contents: str, + caplog: pytest.LogCaptureFixture, + override_configuration_path: Path, +) -> None: + model = pipette_definition.PipetteModelVersionType(type, channels, version) + (override_configuration_path / filename).write_text(file_contents) + with caplog.at_level(logging.WARNING): + mutable_configurations.load_with_mutable_configurations( + model, override_configuration_path, Path(filename).stem + ) + assert len(caplog.records) == 0 From 98358bb88cbef60b7f446fcd9bf0f89ce7cea360 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 22 Nov 2024 15:16:43 -0500 Subject: [PATCH 3/7] Comments. --- .../python/tests/pipette/test_mutable_configurations.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/shared-data/python/tests/pipette/test_mutable_configurations.py b/shared-data/python/tests/pipette/test_mutable_configurations.py index b94256f909f..38920c473e8 100644 --- a/shared-data/python/tests/pipette/test_mutable_configurations.py +++ b/shared-data/python/tests/pipette/test_mutable_configurations.py @@ -309,6 +309,8 @@ def test_build_mutable_config_using_old_units() -> None: @pytest.mark.parametrize( ("filename", "type", "channels", "version", "file_contents"), + # From https://opentrons.atlassian.net/browse/RQA-3676. + # These could probably be pared down. [ ( "P20MV202020121412.json", @@ -447,6 +449,11 @@ def test_loading_does_not_log_warnings( caplog: pytest.LogCaptureFixture, override_configuration_path: Path, ) -> None: + """Make sure load_with_mutable_configurations() doesn't log any exceptions. + + load_with_mutable_configurations() suppresses and logs internal exceptions to + protect its caller, but those are still bugs, and we still want tests to catch them. + """ model = pipette_definition.PipetteModelVersionType(type, channels, version) (override_configuration_path / filename).write_text(file_contents) with caplog.at_level(logging.WARNING): From efb4f424b2a00d0d7d6a24cc5f9153aac9548aae Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 22 Nov 2024 15:18:58 -0500 Subject: [PATCH 4/7] Seth's fix. --- .../python/opentrons_shared_data/pipette/model_constants.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/shared-data/python/opentrons_shared_data/pipette/model_constants.py b/shared-data/python/opentrons_shared_data/pipette/model_constants.py index 7d34e0e5f6a..8045b3941d3 100644 --- a/shared-data/python/opentrons_shared_data/pipette/model_constants.py +++ b/shared-data/python/opentrons_shared_data/pipette/model_constants.py @@ -95,7 +95,10 @@ "liquid_properties", "default", "supportedTips", - "##EACHTIP##", + # FIX BEFORE MERGE: I do not understand this. Is this right? What is the + # intended difference between ##EACHTIP## and ##EACHTIPTYPE##? Why was + # ##EACHTIP## raising in the first place? + "##EACHTIPTYPE##", "defaultTipLength", ], } From 193f113a2d248a981b0c0fd30903285d20987161 Mon Sep 17 00:00:00 2001 From: Seth Foster Date: Fri, 22 Nov 2024 15:22:22 -0500 Subject: [PATCH 5/7] i think this is it --- .../opentrons_shared_data/pipette/mutable_configurations.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py b/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py index 06b31215b65..fbfcfe6fa72 100644 --- a/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py +++ b/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py @@ -58,6 +58,9 @@ def _do_edit_non_quirk( elif thiskey == "##EACHTIPTYPE##": for key in existing.keys(): _do_edit_non_quirk(new_value, existing[key], restkeys) + elif thiskey == "##EACHTIP##": + for key in existing.keys(): + _do_edit_non_quirk(new_value, existing[key], restkeys) else: _do_edit_non_quirk(new_value, existing[thiskey], restkeys) else: From 721be1631403dba8aa4029a7edf6326b2d66c400 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 22 Nov 2024 15:31:59 -0500 Subject: [PATCH 6/7] noqa --- .../opentrons_shared_data/pipette/mutable_configurations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py b/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py index fbfcfe6fa72..7e1beb5dd35 100644 --- a/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py +++ b/shared-data/python/opentrons_shared_data/pipette/mutable_configurations.py @@ -41,7 +41,7 @@ LIQUID_CLASS = LiquidClasses.default -def _edit_non_quirk( +def _edit_non_quirk( # noqa: C901 mutable_config_key: str, new_mutable_value: MutableConfig, base_dict: Dict[str, Any] ) -> None: def _do_edit_non_quirk( From 3ab56956b441cff756d93d8388d974979cb1dfc8 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 22 Nov 2024 15:32:56 -0500 Subject: [PATCH 7/7] Okay I guess the previous commit means we're reverting the original fix? --- .../python/opentrons_shared_data/pipette/model_constants.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/shared-data/python/opentrons_shared_data/pipette/model_constants.py b/shared-data/python/opentrons_shared_data/pipette/model_constants.py index 8045b3941d3..7d34e0e5f6a 100644 --- a/shared-data/python/opentrons_shared_data/pipette/model_constants.py +++ b/shared-data/python/opentrons_shared_data/pipette/model_constants.py @@ -95,10 +95,7 @@ "liquid_properties", "default", "supportedTips", - # FIX BEFORE MERGE: I do not understand this. Is this right? What is the - # intended difference between ##EACHTIP## and ##EACHTIPTYPE##? Why was - # ##EACHTIP## raising in the first place? - "##EACHTIPTYPE##", + "##EACHTIP##", "defaultTipLength", ], }