Qiskit / qiskit

Qiskit is an open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives.
https://www.ibm.com/quantum/qiskit
Apache License 2.0
5.31k stars 2.38k forks source link

Backend based primitives are not serializable via dill #9033

Closed adekusar-drl closed 2 years ago

adekusar-drl commented 2 years ago

Environment

What is happening?

Backend based primitives can't be saved via dill. Reference primitives can be loaded/saved. QuantumInstance is also serializable. This issue comes from Qiskit Machine Learning where quantum models can be saved to a file, then loaded to continue training or for inference on a real hardware.

How can we reproduce the issue?

Run a script

import dill
from qiskit import Aer
from qiskit.primitives import BackendSampler

sampler = BackendSampler(Aer.get_backend("aer_simulator"))
with open("sampler.dill", "wb") as f:
    dill.dump(sampler, f)

with open("sampler.dill", "rb") as f:
    sampler = dill.load(f)

What should happen?

An exception is raised:

python __dill_primitives.py

Traceback (most recent call last):
  File "__dill_primitives.py", line 10, in <module>
    sampler = dill.load(f)
  File ".../envs/dev-qml/lib/site-packages/dill/_dill.py", line 313, in load
    return Unpickler(file, ignore=ignore, **kwds).load()
  File ".../envs/dev-qml/lib/site-packages/dill/_dill.py", line 525, in load
    obj = StockUnpickler.load(self)
TypeError: __new__() missing 1 required positional argument: 'backend'

Any suggestions?

No exceptions must be raised.

ikkoham commented 2 years ago

Adding the following method to BackendSampler should fix this issue.

    def __getnewargs__(self):
        return self._backend,
ikkoham commented 2 years ago

This should be fixed in Terra, but Terra 0.22.1 has been just released...

Workaround

def __getnewargs__(self):
    return self._backend,

BackendSampler.__getnewargs__ = __getnewargs__
jakelishman commented 2 years ago

How come BaseSampler and BaseEstimator are overriding __new__? They don't need to influence the object-creation semantics - it looks to me like everything going on in that method is just instance initialisation, which should be done in __init__. The __new__ override is the underlying cause of the failure to de-pickle here - derived classes from the primitives shouldn't be needing to override the __new__ method to allow themselves to change the __init__ signature.

ikkoham commented 2 years ago

@jakelishman It is legacy and will removed after the deprecation period, but the original introduction reason was a pre-init hook.

jakelishman commented 2 years ago

That still sounds odd, unless the primitives API is asserting that the __init__ methods of subclasses have to have a particular signature, which would be very unusual (and make these BackendSampler etc classes invalid). I'm not sure I understand why that couldn't just have happened at the start of BaseSampler.__init__ - it's the subclass's responsibility to ensure that that base class is initialised correctly (almost invariably by calling super().__init__(...) at some point, for sensible subclasses, and it looks like the primitives would fit this bill).

mtreinish commented 2 years ago

Independent of the mechanics of getting the class usable with pickle, as a general rule Backend objects are not guaranteed to be serializeable, this is part (but not the only reason) of why things like the transpiler do not pass backends around because they have to work in a multiprocessing context which implies they'd be serializable. This is because most backends contain inherently unserializable objects like handles to async remote execution, authorized sessions, compiled modules, etc.

adekusar-drl commented 2 years ago

Once I realized that the backend primitives are not serializable that was a call to re-consider model serialization in QML. I was suspecting that despite now I can serialize a backend instance, it is unnatural and a backend might not be serializable. You just confirmed that. Thanks.