Repository: qiskit-community/qiskit-machine-learning (language: python)

Issue:
Bug/607 fidelity statevector kernel cannot be pickled
<!--
⚠️ If you do not respect this template, your pull request will be closed.
⚠️ Your pull request title should be short detailed and understandable for all.
⚠️ Also, please add it in the CHANGELOG file under Unreleased section.
⚠️ If your pull request fixes an open issue, please link to the issue.

✅ I have added the tests to cover my changes.
✅ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.
-->

### Summary
Fixes #607 where FidelityStatevectorKernel was throwing an error when pickled due to a known pickling error with functools.lru_cache. This has been fixed by adding a self.cache_size attribute and custom __getstate__ and __setstate__ functions used when pickling and unpickling.


### Details and comments
When pickled the lru_cache is set to None in the object dict. When unpicked the cache is re-initiliased with the new cache_size variable and __getstate__ and __setstate__. Unit tests added to check object pickling and unpickling as well as manually using __getstate__ and __setstate__ to initialise a FidelityStatevectorKernel. Passed all other unit tests.
Bug/607 fidelity statevector kernel cannot be pickled
<!--
⚠️ If you do not respect this template, your pull request will be closed.
⚠️ Your pull request title should be short detailed and understandable for all.
⚠️ Also, please add it in the CHANGELOG file under Unreleased section.
⚠️ If your pull request fixes an open issue, please link to the issue.

✅ I have added the tests to cover my changes.
✅ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.
-->

### Summary
Fixes #607 where FidelityStatevectorKernel was throwing an error when pickled due to a known pickling error with functools.lru_cache. This has been fixed by adding a self.cache_size attribute and custom __getstate__ and __setstate__ functions used when pickling and unpickling.


### Details and comments
When pickled the lru_cache is set to None in the object dict. When unpicked the cache is re-initiliased with the new cache_size variable and __getstate__ and __setstate__. Unit tests added to check object pickling and unpickling as well as manually using __getstate__ and __setstate__ to initialise a FidelityStatevectorKernel. Passed all other unit tests.


Discussion / hints:
> Normally if we have a bug that does not show up in the test cases it suggests we need one to test that aspects for better coverage and to make sure it things stay working. I think a test case that pickles and unpickles is needed.

Does that mean a test is needed where we pickle and unpickle all qml objects?
> Does that mean a test is needed where we pickle and unpickle all qml objects?

Seems like a lot of objects! This is much more an edge case I guess due to lru_cache so I would leave it at just a test for this object - makes sure it works in CI over the platforms and continues to do so.
## Pull Request Test Coverage Report for [Build 8098671846](https://coveralls.io/builds/66001365)


### Details

* **9** of **9**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.03%**) to **92.636%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/66001365/badge)](https://coveralls.io/builds/66001365) |
| :-- | --: |
| Change from base [Build 8098398840](https://coveralls.io/builds/66000665): |  0.03% |
| Covered Lines: | 1887 |
| Relevant Lines: | 2037 |

---
##### 💛  - [Coveralls](https://coveralls.io)

Added unit testing and edited release notes.
> Thanks very much for addressing this issue! I just have a few minor suggestions.

I appreciate the suggestions! I'm surprised some of them weren't picked up by CI. All the changes were approved.
You'll need to add `from typing import Any` to `fidelity_statevector_kernel.py`, but, unfortunately, I'm not allowed to make that suggestion as it's in an unedited code block.
> `from typing import Any`

Can do, why does it need to be added? - for the dict, got it
> Normally if we have a bug that does not show up in the test cases it suggests we need one to test that aspects for better coverage and to make sure it things stay working. I think a test case that pickles and unpickles is needed.

Does that mean a test is needed where we pickle and unpickle all qml objects?
> Does that mean a test is needed where we pickle and unpickle all qml objects?

Seems like a lot of objects! This is much more an edge case I guess due to lru_cache so I would leave it at just a test for this object - makes sure it works in CI over the platforms and continues to do so.
## Pull Request Test Coverage Report for [Build 8098671846](https://coveralls.io/builds/66001365)


### Details

* **9** of **9**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.03%**) to **92.636%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/66001365/badge)](https://coveralls.io/builds/66001365) |
| :-- | --: |
| Change from base [Build 8098398840](https://coveralls.io/builds/66000665): |  0.03% |
| Covered Lines: | 1887 |
| Relevant Lines: | 2037 |

---
##### 💛  - [Coveralls](https://coveralls.io)

Added unit testing and edited release notes.
> Thanks very much for addressing this issue! I just have a few minor suggestions.

I appreciate the suggestions! I'm surprised some of them weren't picked up by CI. All the changes were approved.
You'll need to add `from typing import Any` to `fidelity_statevector_kernel.py`, but, unfortunately, I'm not allowed to make that suggestion as it's in an unedited code block.
> `from typing import Any`

Can do, why does it need to be added? - for the dict, got it

Fix patch:
diff --git a/qiskit_machine_learning/kernels/fidelity_statevector_kernel.py b/qiskit_machine_learning/kernels/fidelity_statevector_kernel.py
index 7e6ef9720..1a15feef3 100644
--- a/qiskit_machine_learning/kernels/fidelity_statevector_kernel.py
+++ b/qiskit_machine_learning/kernels/fidelity_statevector_kernel.py
@@ -1,6 +1,6 @@
 # This code is part of a Qiskit project.
 #
-# (C) Copyright IBM 2023.
+# (C) Copyright IBM 2023, 2024.
 #
 # This code is licensed under the Apache License, Version 2.0. You may
 # obtain a copy of this license in the LICENSE.txt file in the root directory
@@ -14,7 +14,7 @@
 from __future__ import annotations
 
 from functools import lru_cache
-from typing import Type, TypeVar
+from typing import Type, TypeVar, Any
 
 import numpy as np
 
@@ -97,7 +97,7 @@ def __init__(
         self._auto_clear_cache = auto_clear_cache
         self._shots = shots
         self._enforce_psd = enforce_psd
-
+        self._cache_size = cache_size
         # Create the statevector cache at the instance level.
         self._get_statevector = lru_cache(maxsize=cache_size)(self._get_statevector_)
 
@@ -160,3 +160,12 @@ def clear_cache(self):
         """Clear the statevector cache."""
         # pylint: disable=no-member
         self._get_statevector.cache_clear()
+
+    def __getstate__(self) -> dict[str, Any]:
+        kernel = dict(self.__dict__)
+        kernel["_get_statevector"] = None
+        return kernel
+
+    def __setstate__(self, kernel: dict[str, Any]):
+        self.__dict__ = kernel
+        self._get_statevector = lru_cache(maxsize=self._cache_size)(self._get_statevector_)
diff --git a/releasenotes/notes/fix-fid_statevector_kernel-pickling-b7fa2b13a15ec9c6.yaml b/releasenotes/notes/fix-fid_statevector_kernel-pickling-b7fa2b13a15ec9c6.yaml
new file mode 100644
index 000000000..34c512b83
--- /dev/null
+++ b/releasenotes/notes/fix-fid_statevector_kernel-pickling-b7fa2b13a15ec9c6.yaml
@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Fixed a bug where :class:`.FidelityStatevectorKernel` threw an error when pickled.
\ No newline at end of file


Test patch:
diff --git a/test/kernels/test_fidelity_statevector_kernel.py b/test/kernels/test_fidelity_statevector_kernel.py
index 2c5b81c8a..5366aed91 100644
--- a/test/kernels/test_fidelity_statevector_kernel.py
+++ b/test/kernels/test_fidelity_statevector_kernel.py
@@ -1,6 +1,6 @@
 # This code is part of a Qiskit project.
 #
-# (C) Copyright IBM 2023.
+# (C) Copyright IBM 2023, 2024.
 #
 # This code is licensed under the Apache License, Version 2.0. You may
 # obtain a copy of this license in the LICENSE.txt file in the root directory
@@ -15,6 +15,7 @@
 
 import functools
 import itertools
+import pickle
 import sys
 import unittest
 
@@ -343,6 +344,55 @@ def test_properties(self):
         self.assertEqual(qc, kernel.feature_map)
         self.assertEqual(1, kernel.num_features)
 
+    def test_pickling(self):
+        """Test that the kernel can be pickled correctly and without error."""
+        # Compares original kernel with copies made using pickle module and get & set state directly
+        qc = QuantumCircuit(1)
+        qc.ry(Parameter("w"), 0)
+        kernel1 = FidelityStatevectorKernel(feature_map=qc)
+
+        pickled_obj = pickle.dumps(kernel1)
+        kernel2 = pickle.loads(pickled_obj)
+
+        kernel3 = FidelityStatevectorKernel()
+        kernel3.__setstate__(kernel1.__getstate__())
+
+        with self.subTest("Pickle fail, kernels are not the same type"):
+            self.assertEqual(type(kernel1), type(kernel2))
+
+        with self.subTest("Pickle fail, kernels are not the same type"):
+            self.assertEqual(type(kernel1), type(kernel3))
+
+        with self.subTest("Pickle fail, kernels are not unique objects"):
+            self.assertNotEqual(kernel1, kernel2)
+
+        with self.subTest("Pickle fail, kernels are not unique objects"):
+            self.assertNotEqual(kernel1, kernel3)
+
+        with self.subTest("Pickle fail, caches are not the same type"):
+            self.assertEqual(type(kernel1._get_statevector), type(kernel2._get_statevector))
+
+        with self.subTest("Pickle fail, caches are not the same type"):
+            self.assertEqual(type(kernel1._get_statevector), type(kernel3._get_statevector))
+
+        # Remove cache to check dict properties are otherwise identical.
+        # - caches are never identical as they have different RAM locations.
+        kernel1.__dict__["_get_statevector"] = None
+        kernel2.__dict__["_get_statevector"] = None
+        kernel3.__dict__["_get_statevector"] = None
+
+        # Confirm changes were made.
+        with self.subTest("Pickle fail, caches have not been removed from kernels"):
+            self.assertEqual(kernel1._get_statevector, None)
+            self.assertEqual(kernel2._get_statevector, None)
+            self.assertEqual(kernel3._get_statevector, None)
+
+        with self.subTest("Pickle fail, properties of kernels (bar cache) are not identical"):
+            self.assertEqual(kernel1.__dict__, kernel2.__dict__)
+
+        with self.subTest("Pickle fail, properties of kernels (bar cache) are not identical"):
+            self.assertEqual(kernel1.__dict__, kernel3.__dict__)
+
 
 @ddt
 class TestStatevectorKernelDuplicates(QiskitMachineLearningTestCase):