Merge pull request #6394 from jenshnielsen/remove_parameter_class_att

Gracefully handle that a parameter could shadow a class attr

docs/changes/newsfragments/6394.improved

@@ -0,0 +1 @@
+Improve handling of corner cases where `Instrument.remove_parameter` could raise an exception.

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

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:
         """

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