Merge pull request #5341 from jenshnielsen/shutdown_visa

Use weakref finalize to close visa connections

Author: jenshnielsen

docs/changes/newsfragments/5341.improved

@@ -0,0 +1,4 @@
+The `pyvisa.ResourceManager` of a VISA instrument is now exposed
+as `instr.resource_manager`. All VISA instruments will now use `weakref.finalize`
+to close the visa resource on shutdown of the instrument. This should reduce the
+chance that an instrument resource is not cleanup correctly on exit.

pyproject.toml

@@ -243,7 +243,9 @@ markers = "serial"
 filterwarnings = [
     "ignore:The distutils package is deprecated and slated for removal in Python 3.12:DeprecationWarning",
     "ignore:distutils Version classes are deprecated:DeprecationWarning",
-    "ignore:SelectableGroups dict interface is deprecated:DeprecationWarning"
+    "ignore:SelectableGroups dict interface is deprecated:DeprecationWarning",
+    'ignore:Deprecated call to `pkg_resources\.declare_namespace:DeprecationWarning',
+    'ignore:pkg_resources is deprecated as an API',
 ]
 
 [tool.ruff]

qcodes/instrument/visa.py

@@ -5,6 +5,7 @@
 from collections.abc import Sequence
 from importlib.resources import as_file, files
 from typing import Any
+from weakref import finalize
 
 import pyvisa
 import pyvisa.constants as vi_const
@@ -22,6 +23,16 @@
 log = logging.getLogger(__name__)
 
 
+def _close_visa_handle(
+    handle: pyvisa.resources.MessageBasedResource, name: str
+) -> None:
+    log.info(
+        "Closing VISA handle to %s as there are no non weak "
+        "references to the instrument.",
+        name,
+    )
+    handle.close()
+
 class VisaInstrument(Instrument):
 
     """
@@ -100,17 +111,26 @@ def __init__(
                         f"file {pyvisa_sim_file} from module: {module}"
                     )
                 visalib = f"{str(sim_visalib_path)}@sim"
-                visa_handle, visabackend = self._connect_and_handle_error(
-                    address, visalib
-                )
+                (
+                    visa_handle,
+                    visabackend,
+                    resource_manager,
+                ) = self._connect_and_handle_error(address, visalib)
         else:
-            visa_handle, visabackend = self._connect_and_handle_error(address, visalib)
+            visa_handle, visabackend, resource_manager = self._connect_and_handle_error(
+                address, visalib
+            )
+        finalize(self, _close_visa_handle, visa_handle, str(self.name))
 
         self.visabackend: str = visabackend
         self.visa_handle: pyvisa.resources.MessageBasedResource = visa_handle
         """
         The VISA resource used by this instrument.
         """
+        self.resource_manager = resource_manager
+        """
+        The VISA resource manager used by this instrument.
+        """
         self.visalib: str | None = visalib
         self._address = address
 
@@ -122,18 +142,20 @@ def __init__(
 
     def _connect_and_handle_error(
         self, address: str, visalib: str | None
-    ) -> tuple[pyvisa.resources.MessageBasedResource, str]:
+    ) -> tuple[pyvisa.resources.MessageBasedResource, str, pyvisa.ResourceManager]:
         try:
-            visa_handle, visabackend = self._open_resource(address, visalib)
+            visa_handle, visabackend, resource_manager = self._open_resource(
+                address, visalib
+            )
         except Exception as e:
             self.visa_log.exception(f"Could not connect at {address}")
             self.close()
             raise e
-        return visa_handle, visabackend
+        return visa_handle, visabackend, resource_manager
 
     def _open_resource(
         self, address: str, visalib: str | None
-    ) -> tuple[pyvisa.resources.MessageBasedResource, str]:
+    ) -> tuple[pyvisa.resources.MessageBasedResource, str, pyvisa.ResourceManager]:
 
         # in case we're changing the address - close the old handle first
         if getattr(self, "visa_handle", None):
@@ -156,7 +178,7 @@ def _open_resource(
             resource.close()
             raise TypeError("QCoDeS only support MessageBasedResource Visa resources")
 
-        return resource, visabackend
+        return resource, visabackend, resource_manager
 
     def set_address(self, address: str) -> None:
         """
@@ -168,10 +190,13 @@ def set_address(self, address: str) -> None:
                 change the backend for VISA, use the self.visalib attribute
                 (and then call this function).
         """
-        resource, visabackend = self._open_resource(address, self.visalib)
+        resource, visabackend, resource_manager = self._open_resource(
+            address, self.visalib
+        )
         self.visa_handle = resource
         self._address = address
         self.visabackend = visabackend
+        self.resource_manager = resource_manager
 
     def device_clear(self) -> None:
         """Clear the buffers of the device"""
@@ -231,6 +256,13 @@ def close(self) -> None:
         """Disconnect and irreversibly tear down the instrument."""
         if getattr(self, 'visa_handle', None):
             self.visa_handle.close()
+
+        if getattr(self, "visabackend", None) == "sim" and getattr(
+            self, "resource_manager", None
+        ):
+            # work around for https://github.com/QCoDeS/Qcodes/issues/5356 and
+            # https://github.com/pyvisa/pyvisa-sim/issues/82
+            self.resource_manager.close()
         super().close()
 
     def write_raw(self, cmd: str) -> None:

qcodes/tests/test_instrument.py

@@ -4,6 +4,7 @@
 from __future__ import annotations
 
 import contextlib
+import gc
 import io
 import re
 import weakref
@@ -425,6 +426,19 @@ def test_instrument_label(cls, request: FixtureRequest) -> None:
     assert instrument.label == label
 
 
+def test_instrument_without_ref_is_gced():
+    # When there are no active references to an instrument and it is
+    # gced there should be no live references to the instrument
+
+    def use_some_instrument() -> None:
+        _ = Instrument("SomeInstrument")
+        assert list(Instrument._all_instruments.keys()) == ["SomeInstrument"]
+
+    assert len(Instrument._all_instruments) == 0
+    use_some_instrument()
+    gc.collect()
+    assert len(Instrument._all_instruments) == 0
+
 def test_snapshot_and_meta_attrs() -> None:
     """Test snapshot of InstrumentBase contains _meta_attrs attributes"""
     instr = InstrumentBase("instr", label="Label")

qcodes/tests/test_visa.py

@@ -1,3 +1,5 @@
+import gc
+import logging
 import re
 from pathlib import Path
 
@@ -6,7 +8,8 @@
 import pyvisa.constants
 from pytest import FixtureRequest
 
-from qcodes.instrument.visa import VisaInstrument
+from qcodes.instrument import Instrument, VisaInstrument
+from qcodes.instrument_drivers.american_magnetics import AMIModel430
 from qcodes.validators import Numbers
 
 
@@ -19,7 +22,7 @@ def __init__(self, *args, **kwargs):
                            vals=Numbers(-20, 20))
 
     def _open_resource(self, address: str, visalib):
-        return MockVisaHandle(), visalib
+        return MockVisaHandle(), visalib, pyvisa.ResourceManager("@sim")
 
 
 class MockVisaHandle(pyvisa.resources.MessageBasedResource):
@@ -109,6 +112,37 @@ def _make_mock_visa():
         mv.close()
 
 
+def test_visa_gc_closes_connection(caplog) -> None:
+    def use_magnet() -> pyvisa.ResourceManager:
+        x = AMIModel430(
+            "x",
+            address="GPIB::1::INSTR",
+            pyvisa_sim_file="AMI430.yaml",
+            terminator="\n",
+        )
+        assert list(Instrument._all_instruments.keys()) == ["x"]
+        assert len(x.resource_manager.list_opened_resources()) == 1
+        assert x.resource_manager.list_opened_resources() == [x.visa_handle]
+        return x.resource_manager
+
+    # ensure that any unused instruments that have not been gced are gced before running
+    gc.collect()
+    caplog.clear()
+    with caplog.at_level(logging.INFO, logger="qcodes.instrument.visa"):
+        rm = use_magnet()
+        gc.collect()
+    # at this stage the instrument created in use_magnet has gone out of scope
+    # and we have triggered an explicit gc so the weakref.finalize function
+    # has been triggered. We test this
+    # and the instrument should no longer be in the instrument registry
+    assert len(Instrument._all_instruments) == 0
+    assert len(rm.list_opened_resources()) == 0
+    assert (
+        caplog.records[-1].message == "Closing VISA handle to x as there are no non "
+        "weak references to the instrument."
+    )
+
+
 def test_ask_write_local(mock_visa) -> None:
 
     # test normal ask and write behavior