Fix pickle handling for SingletonGate class #10871
Conversation
This commit fixes an oversight in Qiskit#10314 where the handling of pickle wasn't done correctly. Because the SingletonGate class defines __new__ and based on the parameters to the gate.__class__() call determines whether we get a new mutable copy or a shared singleton immutable instance we need special handling in pickle. By default pickle will call __new__() without any arguments and then rely on __setstate__ to update the state in the new object. This works fine if the original instance was a singleton but in the case of mutable copies this will create a singleton object instead of a mutable copy. To fix this a __reduce__ method is added to ensure arguments get passed to __new__ forcing a mutable object to be created in deserialization. Then a __setstate__ method is defined to correctly update the mutable object post creation.
|
One or more of the the following people are requested to review this:
|
| with io.BytesIO() as fd: | ||
| pickle.dump(gate, fd) | ||
| fd.seek(0) | ||
| copied = pickle.load(fd) |
There was a problem hiding this comment.
Totally unimportant, but pickle allows serialisation to bytes directly, so you can do this as a one-liner if you prefer: copied = pickle.loads(pickle.dumps(gate)).
There was a problem hiding this comment.
I just don't like using pickle.dumps because s implies string to me and it's not a string :D
There was a problem hiding this comment.
maybe it's an initial from a language that reads right-to-left: "DUMP byteS" instead of "DUMP String" lol
There was a problem hiding this comment.
Here's the Python 2 docstring for dumps 🙂 : "Return the pickled representation of the object as a string, instead of writing it to a file."
|
Here is some context for this issue. It was first noticed when the daily run of qiskit-experiments' tests with qiskit's main branch encountered test failures for circuits with conditioned gates (see for example this run). The tests only failed on Linux because on Windows and macOS the transpiler parallelism is disabled. When parallelism is used, objects are passed between processes using pickle, triggering this issue. The issue is a little bit worse than mutable gates becoming instances of the immutable singleton when unpickled. The default unpickling procedure given in the documentation is: def restore(cls, attributes):
obj = cls.__new__(cls)
obj.__dict__.update(attributes)
return objEvery |
|
Defining |
| copied = pickle.load(fd) | ||
| self.assertFalse(copied.mutable) | ||
|
|
||
| def test_mutable_pickle(self): |
There was a problem hiding this comment.
One thing that surprised me with this issue is that it was not caught in this repo before the singleton gate PR was merged. I expect there is a good set of tests for transpiling conditional gates, but likely the tests all transpile a single circuit and don't hit the pickling issues that come up with parallel transpilation. I wonder if the tests added here are enough or if there should be more tests of pickling and unpickling circuits.
There was a problem hiding this comment.
There's actually not a vast amount of testing of conditional gates (with .condition set), because they typically weren't executable on anything but simulators until IBM hardware grew support for midcircuit measurements and more generalised control flow, at which point we began pivoting to IfElseOp.
The bug getting through points to a deficiency in integration testing around conditional gates for sure, and we can add more tests along those lines until Instruction.condition is completely removed in 1.0. Conditionals after transpilation are also harder to write good tests about, because we can't just test for unitary equivalence or anything like that when they're present, so it's easier for stuff to slip through.
|
Right, I was just describing how things work now where there is no |
This commit pivots the pickle interface methods used to implement __getnewargs_ex__ instead of the combination of __reduce__ and __setstate__. Realistically, all we need to do here is pass that we have mutable arguments to new to trigger it to create a separate object, the rest of pickle was working correctly. This makes the interface being used for pickle a lot clearer.
There was a problem hiding this comment.
I think this is okay as it is. I will open an issue about possible testing. I think the tests added here cover the current cases of concern. What I worry about is someone adding a new SingletonGate subclass with an extra slot/attribute not captured in __getnewargs_ex__. It might appear fine in single process tests but then misbehave in user code once it starts getting run through pickle for multiprocess transpiling.
* Fix pickle handling for SingletonGate class This commit fixes an oversight in Qiskit#10314 where the handling of pickle wasn't done correctly. Because the SingletonGate class defines __new__ and based on the parameters to the gate.__class__() call determines whether we get a new mutable copy or a shared singleton immutable instance we need special handling in pickle. By default pickle will call __new__() without any arguments and then rely on __setstate__ to update the state in the new object. This works fine if the original instance was a singleton but in the case of mutable copies this will create a singleton object instead of a mutable copy. To fix this a __reduce__ method is added to ensure arguments get passed to __new__ forcing a mutable object to be created in deserialization. Then a __setstate__ method is defined to correctly update the mutable object post creation. * Use __getnewargs_ex__ insetad of __reduce__ & __setstate__ This commit pivots the pickle interface methods used to implement __getnewargs_ex__ instead of the combination of __reduce__ and __setstate__. Realistically, all we need to do here is pass that we have mutable arguments to new to trigger it to create a separate object, the rest of pickle was working correctly. This makes the interface being used for pickle a lot clearer. * Improve assertion in immutable pickle test
Summary
This commit fixes an oversight in #10314 where the handling of pickle wasn't done correctly. Because the SingletonGate class defines new and based on the parameters to the gate.class() call determines whether we get a new mutable copy or a shared singleton immutable instance we need special handling in pickle. By default pickle will call new() without any arguments and then rely on setstate to update the state in the new object. This works fine if the original instance was a singleton but in the case of mutable copies this will create a singleton object instead of a mutable copy. To fix this a reduce method is added to ensure arguments get passed to new forcing a mutable object to be created in deserialization. Then a setstate method is defined to correctly update the mutable object post creation.
Details and comments