Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle that a parameter could shadow a class attr #6394

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/changes/newsfragments/6394.improved
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve handling of corner cases where `Instrument.remove_parameter` could raise an exception.
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 6 additions & 1 deletion src/qcodes/instrument/instrument_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
"""
Expand Down
61 changes: 61 additions & 0 deletions tests/parameter/test_parameter_override.py
Original file line number Diff line number Diff line change
@@ -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):
Expand Down Expand Up @@ -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):
"""
Expand Down Expand Up @@ -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
einsmein marked this conversation as resolved.
Show resolved Hide resolved
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
Loading