Skip to content

Commit

Permalink
Fix Channel.__hash__ in multiprocessing contexts (#11251)
Browse files Browse the repository at this point in the history
* 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.

* 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.
  • Loading branch information
jakelishman authored Nov 15, 2023
1 parent 28154e6 commit 3c1a87c
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 41 deletions.
3 changes: 1 addition & 2 deletions qiskit/pulse/channels.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Expand Down Expand Up @@ -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):
Expand Down
5 changes: 1 addition & 4 deletions qiskit/pulse/instructions/instruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ def __init__(
"""
self._operands = operands
self._name = name
self._hash = None
self._validate()

def _validate(self):
Expand Down Expand Up @@ -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`.
Expand Down
43 changes: 18 additions & 25 deletions qiskit/pulse/model/frames.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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:
Expand All @@ -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.
Expand All @@ -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:
Expand All @@ -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.
Expand All @@ -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:
Expand All @@ -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))
3 changes: 1 addition & 2 deletions qiskit/pulse/model/mixed_frames.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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)))
14 changes: 6 additions & 8 deletions qiskit/pulse/model/pulse_target.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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.
Expand All @@ -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, ...]:
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

0 comments on commit 3c1a87c

Please sign in to comment.