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. 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 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: """ diff --git a/tests/parameter/test_parameter_override.py b/tests/parameter/test_parameter_override.py index 22bf286e392..f43059acd54 100644 --- a/tests/parameter/test_parameter_override.py +++ b/tests/parameter/test_parameter_override.py @@ -1,7 +1,13 @@ +import logging +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 +50,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 +119,41 @@ 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, caplog): + 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 + 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 + assert a.some_attr == 1 + with pytest.raises(KeyError, match="some_attr"): + a.remove_parameter("some_attr") + + assert a.some_attr == 1