From f3aeef3e5a5cab25b1a41a4db01819fac3653596 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Wed, 28 Aug 2024 13:16:13 +0200 Subject: [PATCH 1/5] Add tests for removing parameters that override class attrs --- tests/parameter/test_parameter_override.py | 55 ++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py index 22bf286e392..78e860bac01 100644 --- a/tests/parameter/test_parameter_override.py +++ b/tests/parameter/test_parameter_override.py @@ -1,7 +1,12 @@ +from typing import TYPE_CHECKING + import pytest from qcodes.instrument import Instrument +if TYPE_CHECKING: + from qcodes.parameters import Parameter + class DummyOverrideInstr(Instrument): def __init__(self, name, **kwargs): @@ -44,6 +49,23 @@ def voltage(self): return self.parameters["voltage"] +class DummyClassAttrInstr(Instrument): + some_attr = 1 + current: "Parameter" + voltage: "Parameter | None" = None + frequency: "Parameter | None" = None + + def __init__(self, name, **kwargs): + """ + We allow overriding a property since this pattern has been seen in the wild + to define an interface for the instrument. + """ + super().__init__(name, **kwargs) + self.voltage = self.add_parameter("voltage", set_cmd=None, get_cmd=None) + self.current = self.add_parameter("current", set_cmd=None, get_cmd=None) + self.add_parameter("frequency", set_cmd=None, get_cmd=None) + + class DummyInstr(Instrument): def __init__(self, name, **kwargs): """ @@ -96,3 +118,36 @@ def test_removed_parameter_from_prop_instrument_works(request): a.remove_parameter("voltage") a.add_parameter("voltage", set_cmd=None, get_cmd=None) a.voltage.set(1) + + +def test_remove_parameter_from_class_attr_works(request): + request.addfinalizer(DummyClassAttrInstr.close_all) + a = DummyClassAttrInstr("my_instr") + + # removing a parameter that is assigned as an attribute + # with a class level type works + assert hasattr(a, "current") + a.remove_parameter("current") + assert not hasattr(a, "current") + + # when we remove a parameter with an attr that shadows a class attr + # we get the class attr after the removal + assert hasattr(a, "voltage") + assert a.voltage is not None + a.remove_parameter("voltage") + assert hasattr(a, "voltage") + assert a.voltage is None + + # modifying a parameter that is not assigned as an attribute + # does not alter the class attribute + assert hasattr(a, "frequency") + assert a.frequency is None + a.remove_parameter("frequency") + assert a.frequency is None + + # removing a classattr raises since it is not a parameter + assert a.some_attr == 1 + with pytest.raises(KeyError, match="some_attr"): + a.remove_parameter("some_attr") + + assert a.some_attr == 1 From 554d54f7a296707000922f856de83115a0bd915d Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Wed, 28 Aug 2024 13:18:18 +0200 Subject: [PATCH 2/5] handle that hasattr returns True for a class attribute but delattr cannot remove it --- src/qcodes/instrument/instrument_base.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qcodes/instrument/instrument_base.py b/src/qcodes/instrument/instrument_base.py index 8a6597c60d9..b7661c19dce 100644 --- a/src/qcodes/instrument/instrument_base.py +++ b/src/qcodes/instrument/instrument_base.py @@ -209,7 +209,12 @@ def remove_parameter(self, name: str) -> None: is_property = isinstance(getattr(self.__class__, name, None), property) if not is_property and hasattr(self, name): - delattr(self, name) + try: + delattr(self, name) + except AttributeError: + self.log.warning( + "Could not remove attribute %s from %s", name, self.full_name + ) def add_function(self, name: str, **kwargs: Any) -> None: """ From 73d164b53b25e7f28c87e661850b866ba5605abf Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Wed, 28 Aug 2024 13:23:45 +0200 Subject: [PATCH 3/5] Silence warning from pytest.asyncio --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 439fa2a2c12..506bef38cc5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -216,7 +216,7 @@ minversion = "7.2" junit_family = "legacy" testpaths = "tests" addopts = "-n auto --dist=loadfile" - +asyncio_default_fixture_loop_scope = "function" markers = "serial" # we ignore warnings From 6d9bf893477850f2179d6df6923f44035c13b3cd Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Wed, 28 Aug 2024 13:29:24 +0200 Subject: [PATCH 4/5] Test that warning is logged --- tests/parameter/test_parameter_override.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py index 78e860bac01..f43059acd54 100644 --- a/tests/parameter/test_parameter_override.py +++ b/tests/parameter/test_parameter_override.py @@ -1,3 +1,4 @@ +import logging from typing import TYPE_CHECKING import pytest @@ -120,7 +121,7 @@ def test_removed_parameter_from_prop_instrument_works(request): a.voltage.set(1) -def test_remove_parameter_from_class_attr_works(request): +def test_remove_parameter_from_class_attr_works(request, caplog): request.addfinalizer(DummyClassAttrInstr.close_all) a = DummyClassAttrInstr("my_instr") @@ -142,7 +143,12 @@ def test_remove_parameter_from_class_attr_works(request): # does not alter the class attribute assert hasattr(a, "frequency") assert a.frequency is None - a.remove_parameter("frequency") + with caplog.at_level(logging.WARNING): + a.remove_parameter("frequency") + assert ( + "Could not remove attribute frequency from my_instr" + in caplog.records[0].message + ) assert a.frequency is None # removing a classattr raises since it is not a parameter From 4c7a07ed597375c14c9c0f708ee31a525abe39d2 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Fri, 6 Sep 2024 10:35:19 +0200 Subject: [PATCH 5/5] Add changelog for 6394 --- docs/changes/newsfragments/6394.improved | 1 + 1 file changed, 1 insertion(+) create mode 100644 docs/changes/newsfragments/6394.improved diff --git a/docs/changes/newsfragments/6394.improved b/docs/changes/newsfragments/6394.improved new file mode 100644 index 00000000000..26fc52b34b4 --- /dev/null +++ b/docs/changes/newsfragments/6394.improved @@ -0,0 +1 @@ +Improve handling of corner cases where `Instrument.remove_parameter` could raise an exception.