From 88fcdf93921cbb0bbfd73968a533d32c6d45fdde Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Wed, 15 Nov 2023 15:59:00 +0000 Subject: [PATCH 1/2] Fix `Channel.__hash__` in multiprocessing contexts Storing an explicit hash key is fragile in cases that a channel might be created in a different process to where it might be compared or the hash used, because the hash seeding can vary depending on how the new interpreter process was created, especially if it's not done by `fork`. In this case, transmitting the stored `_hash` over pickle meant that a `DriveChannel(0)` created in the main process of a macOS runner could compare equal to a `DriveChannel(0)` created in a separate process (standard start method `spawn`) and pickled over the wire to the main process, but have different hashes, violating the Python data model. Instead, we can just use the standard Python behaviour of creating the hash on demand when requested; this should typically be preferred unless absolutely necessary for critical performance reasons, because it will generally fail safe. --- qiskit/pulse/channels.py | 3 +-- .../notes/fix-pulse-channel-hash-549a8fb5d8738c4d.yaml | 6 ++++++ 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 releasenotes/notes/fix-pulse-channel-hash-549a8fb5d8738c4d.yaml diff --git a/qiskit/pulse/channels.py b/qiskit/pulse/channels.py index 687b837700d3..e3da9b923ca4 100644 --- a/qiskit/pulse/channels.py +++ b/qiskit/pulse/channels.py @@ -96,7 +96,6 @@ def __init__(self, index: int): """ self._validate_index(index) self._index = index - self._hash = hash((self.__class__.__name__, self._index)) @property def index(self) -> Union[int, ParameterExpression]: @@ -156,7 +155,7 @@ def __eq__(self, other: "Channel") -> bool: return type(self) is type(other) and self._index == other._index def __hash__(self): - return self._hash + return hash((type(self), self._index)) class PulseChannel(Channel, metaclass=ABCMeta): diff --git a/releasenotes/notes/fix-pulse-channel-hash-549a8fb5d8738c4d.yaml b/releasenotes/notes/fix-pulse-channel-hash-549a8fb5d8738c4d.yaml new file mode 100644 index 000000000000..19d022572963 --- /dev/null +++ b/releasenotes/notes/fix-pulse-channel-hash-549a8fb5d8738c4d.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fixed the :func:`hash` of Qiskit Pulse ``Channel`` objects (such as :class:`.DriveChannel`) in + cases where the channel was transferred from one Python process to another that used a different + hash seed. From 14d7f892b208f3b5239cff3110bfd7a2a0f19eaa Mon Sep 17 00:00:00 2001 From: Jake Lishman Date: Wed, 15 Nov 2023 17:25:09 +0000 Subject: [PATCH 2/2] Fix `hash` and equality in other pulse objects This removes all caching of items' `hash`es. This practice is quite fraught in multiprocessing contexts, and should only be done when it is absolutely performance critical. In a couple of cases, the pulse objects were using the cached `hash` as the main component of their `__eq__` methods, which is not correct; it's totally valid to have hash collisions without implying that two objects are equal. --- qiskit/pulse/instructions/instruction.py | 5 +-- qiskit/pulse/model/frames.py | 43 ++++++++++-------------- qiskit/pulse/model/mixed_frames.py | 3 +- qiskit/pulse/model/pulse_target.py | 14 ++++---- 4 files changed, 26 insertions(+), 39 deletions(-) diff --git a/qiskit/pulse/instructions/instruction.py b/qiskit/pulse/instructions/instruction.py index 7c6af5788e8b..559b4fb38dc7 100644 --- a/qiskit/pulse/instructions/instruction.py +++ b/qiskit/pulse/instructions/instruction.py @@ -53,7 +53,6 @@ def __init__( """ self._operands = operands self._name = name - self._hash = None self._validate() def _validate(self): @@ -301,9 +300,7 @@ def __eq__(self, other: "Instruction") -> bool: return isinstance(other, type(self)) and self.operands == other.operands def __hash__(self) -> int: - if self._hash is None: - self._hash = hash((type(self), self.operands, self.name)) - return self._hash + return hash((type(self), self.operands, self.name)) def __add__(self, other): """Return a new schedule with `other` inserted within `self` at `start_time`. diff --git a/qiskit/pulse/model/frames.py b/qiskit/pulse/model/frames.py index ae63fe3c7948..803573301320 100644 --- a/qiskit/pulse/model/frames.py +++ b/qiskit/pulse/model/frames.py @@ -33,28 +33,6 @@ class Frame(ABC): The default initial phase for every frame is 0. """ - def __init__(self, identifier): - """Create ``Frame``. - - Args: - identifier: A unique identifier used to hash the Frame. - """ - self._hash = hash((type(self), identifier)) - - def __eq__(self, other: "Frame") -> bool: - """Return True iff self and other are equal, specifically, iff they have the same type and hash. - - Args: - other: The frame to compare to this one. - - Returns: - True iff equal. - """ - return type(self) is type(other) and self._hash == other._hash - - def __hash__(self) -> int: - return self._hash - class GenericFrame(Frame): """Pulse module GenericFrame. @@ -74,7 +52,6 @@ def __init__(self, name: str): name: A unique identifier used to identify the frame. """ self._name = name - super().__init__(name) @property def name(self) -> str: @@ -84,6 +61,12 @@ def name(self) -> str: def __repr__(self) -> str: return f"GenericFrame({self._name})" + def __eq__(self, other): + return type(self) is type(other) and self._name == other._name + + def __hash__(self): + return hash((type(self), self._name)) + class QubitFrame(Frame): """A frame associated with the driving of a qubit. @@ -102,7 +85,6 @@ def __init__(self, index: int): """ self._validate_index(index) self._index = index - super().__init__("QubitFrame" + str(index)) @property def index(self) -> int: @@ -122,6 +104,12 @@ def _validate_index(self, index) -> None: def __repr__(self) -> str: return f"QubitFrame({self._index})" + def __eq__(self, other): + return type(self) is type(other) and self._index == other._index + + def __hash__(self): + return hash((type(self), self._index)) + class MeasurementFrame(Frame): """A frame associated with the measurement of a qubit. @@ -141,7 +129,6 @@ def __init__(self, index: int): """ self._validate_index(index) self._index = index - super().__init__("MeasurementFrame" + str(index)) @property def index(self) -> int: @@ -160,3 +147,9 @@ def _validate_index(self, index) -> None: def __repr__(self) -> str: return f"MeasurementFrame({self._index})" + + def __eq__(self, other): + return type(self) is type(other) and self._index == other._index + + def __hash__(self): + return hash((type(self), self._index)) diff --git a/qiskit/pulse/model/mixed_frames.py b/qiskit/pulse/model/mixed_frames.py index 50ff13c47a44..454cdbf0d2c5 100644 --- a/qiskit/pulse/model/mixed_frames.py +++ b/qiskit/pulse/model/mixed_frames.py @@ -47,7 +47,6 @@ def __init__(self, pulse_target: PulseTarget, frame: Frame): """ self._pulse_target = pulse_target self._frame = frame - self._hash = hash((self._pulse_target, self._frame, type(self))) @property def pulse_target(self) -> PulseTarget: @@ -75,4 +74,4 @@ def __eq__(self, other: "MixedFrame") -> bool: return self._pulse_target == other._pulse_target and self._frame == other._frame def __hash__(self) -> int: - return self._hash + return hash((self._pulse_target, self._frame, type(self))) diff --git a/qiskit/pulse/model/pulse_target.py b/qiskit/pulse/model/pulse_target.py index 69894780df27..bb9702ccfad4 100644 --- a/qiskit/pulse/model/pulse_target.py +++ b/qiskit/pulse/model/pulse_target.py @@ -59,7 +59,6 @@ def __init__(self, name: str): name: A string identifying the port. """ self._name = name - self._hash = hash((name, type(self))) @property def name(self) -> str: @@ -78,12 +77,12 @@ def __eq__(self, other: "Port") -> bool: """ return type(self) is type(other) and self._name == other._name + def __hash__(self) -> int: + return hash((self._name, type(self))) + def __repr__(self) -> str: return f"Port({self._name})" - def __hash__(self) -> int: - return self._hash - class LogicalElement(PulseTarget, ABC): """Base class of logical elements. @@ -104,7 +103,6 @@ def __init__(self, index: Tuple[int, ...]): """ self._validate_index(index) self._index = index - self._hash = hash((index, type(self))) @property def index(self) -> Tuple[int, ...]: @@ -132,13 +130,13 @@ def __eq__(self, other: "LogicalElement") -> bool: """ return type(self) is type(other) and self._index == other._index + def __hash__(self) -> int: + return hash((self._index, type(self))) + def __repr__(self) -> str: ind_str = str(self._index) if len(self._index) > 1 else f"({self._index[0]})" return type(self).__name__ + ind_str - def __hash__(self) -> int: - return self._hash - class Qubit(LogicalElement): """Qubit logical element.