Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qiskit session #551

Merged
merged 30 commits into from
Jul 5, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4ff30a5
functionality implemented
austingmhuang May 16, 2024
ccebffc
minor adjustments to tests
austingmhuang May 16, 2024
c5432f1
Merge branch 'new_device_feature_branch' into qiskit_session
austingmhuang Jun 3, 2024
ebf2fb0
[skip-ci] Qiskit Sessions now test many warnings since you can set se…
austingmhuang Jun 3, 2024
a1a2d49
[skip-ci] tests that we are passing on kwargs to Qiskit's session con…
austingmhuang Jun 3, 2024
cc90420
[skip ci] pylint
austingmhuang Jun 4, 2024
2eeba76
Merge branch 'new_device_feature_branch' into qiskit_session
austingmhuang Jun 6, 2024
c1ecbb2
small comments
austingmhuang Jun 6, 2024
08a241d
Generalization of the session options
austingmhuang Jun 7, 2024
fc3adc0
delicious docstrings
austingmhuang Jun 7, 2024
7119c49
[skip ci] tests and clarification
austingmhuang Jun 10, 2024
58e4178
[skip ci] better session options
austingmhuang Jun 10, 2024
5fcb441
comments for clarity
austingmhuang Jun 10, 2024
307f23b
Merge branch 'new_device_feature_branch' into qiskit_session
austingmhuang Jun 13, 2024
88a18c4
changes to the tests & the warning message
austingmhuang Jun 13, 2024
bc3b9a7
docstrings
austingmhuang Jun 13, 2024
c03f162
type error changes
austingmhuang Jun 14, 2024
f4e1c45
docstrings
austingmhuang Jun 19, 2024
5c2bd4c
add qiskit_session to docs
austingmhuang Jun 19, 2024
4d8242e
a little more consistency in comments
austingmhuang Jun 19, 2024
dff7fc8
for docs
austingmhuang Jun 19, 2024
393d13f
fix ci
austingmhuang Jun 19, 2024
af08b68
black
austingmhuang Jun 19, 2024
62914e1
revert
austingmhuang Jun 19, 2024
9aef725
revert
austingmhuang Jun 19, 2024
f7cbf95
Update pennylane_qiskit/qiskit_device2.py
austingmhuang Jun 20, 2024
e8c45d6
Update tests/test_base_device.py
austingmhuang Jun 20, 2024
57a9115
Qiskit Session update
austingmhuang Jun 20, 2024
d8c73b0
Update pennylane_qiskit/qiskit_device2.py
austingmhuang Jul 4, 2024
71a99dc
docstring update
austingmhuang Jul 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions pennylane_qiskit/qiskit_device2.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@

# pylint: disable=protected-access
@contextmanager
def qiskit_session(device):
def qiskit_session(device, **kwargs):
obliviateandsurrender marked this conversation as resolved.
Show resolved Hide resolved
"""A context manager that creates a Qiskit Session and sets it as a session
on the device while the context manager is active. Using the context manager
will ensure the Session closes properly and is removed from the device after
Expand Down Expand Up @@ -98,7 +98,34 @@ def circuit(x):
"""
# Code to acquire session:
existing_session = device._session
session = Session(backend=device.backend)

# When an existing session exists, we want to use its settings unless overwritten
# by settings in the qiskit_session
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still consider the behaviour quite unintuitive. If we want to go for this, can we include information in the docstring about what will happen if the device that is passed already has a session on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think it is kind of unintuitive to do it this way as well.

The best way is probably to just make the docstring more clear that if you initialize qiskit_session you will not be using your existing session set in the device.

The logic is that I think the existing session set in device should only have value in a very specific case: batched circuits. In all other cases, the session is just necessary to access the primitives but doesn't necessarily do anything.

Therefore if a user has gone out of their way to use qiskit_session, they probably want to do some kind of like VQE or QAOA or whatever iterative thing, so they probably want those settings. I'll make it clearer that a User doesn't accidentally think they should set the Session in the device.

if existing_session:
session_args = inspect.signature(Session).parameters
session_options = {arg: getattr(existing_session, "_" + arg) for arg in session_args}
else:
# when an existing session doesn't exist, we create a session with default settings
session_options = {"backend": device.backend, "service": device.service}

for k, v in kwargs.items():
# Options like service and backend should be tied to the settings set on device
if k in session_options and k != "max_time":
austingmhuang marked this conversation as resolved.
Show resolved Hide resolved
warnings.warn(f"Using '{k}' set in device, {getattr(device, k)}", UserWarning)

# Need "_" since `max_time` attribute on Session is `_max_time`
# When there is overlap between the Session options on the device and the session options
# passed in via qiskit_session, we prefer the ones passed in via qiskit_session
elif existing_session and getattr(existing_session, "_" + k):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to worry about setting a default for the getattr here?

Copy link
Contributor Author

@austingmhuang austingmhuang Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. existing_session, if it exists, should always be some Session() which has all those attributes. I suppose if the User themselves wrote their own version of Session (e.g. our mock) this could be a problem, but I don't see why a User would want to do that...? Then again, if Qiskit significantly changes the attributes of Session or adds an attribute without an "_" in front of it this would be a problem. I guess it wouldn't hurt to set a default 🤷 but not sure how much value it provides.

warnings.warn(
f"`{k}` was also set in the Session passed to the device. Using `{k}` '{v}' set in `qiskit_session`.",
UserWarning,
)
session_options[k] = v
austingmhuang marked this conversation as resolved.
Show resolved Hide resolved
else:
session_options[k] = v

session = Session(**session_options)
device._session = session
try:
yield session
Expand Down
101 changes: 98 additions & 3 deletions tests/test_base_device.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

import pennylane as qml
from pennylane.tape.qscript import QuantumScript
from qiskit_ibm_runtime import EstimatorV2 as Estimator
from qiskit_ibm_runtime import EstimatorV2 as Estimator, Session

from qiskit_ibm_runtime.fake_provider import FakeManila, FakeManilaV2
from qiskit_aer import AerSimulator
Expand Down Expand Up @@ -105,8 +105,10 @@ def options(self):
# pylint: disable=too-few-public-methods
class MockSession:
def __init__(self, backend, max_time=None):
self.backend = backend
self.max_time = max_time
self._backend = backend
self._max_time = max_time
self._args = "random" # this is to satisfy a mock
self._kwargs = "random" # this is to satisfy a mock
self.session_id = "123"

def close(self): # This is just to appease a test
Expand Down Expand Up @@ -269,6 +271,99 @@ def test_using_session_context(self, mock_session, initial_session):

assert dev._session == initial_session

def test_using_session_context_options(self):
"""Test that you can set session options using qiskit_session"""
dev = QiskitDevice2(wires=2, backend=backend)

assert dev._session is None

with qiskit_session(dev, max_time=30) as session:
assert dev._session == session
assert dev._session is not None
assert dev._session._max_time == 30

assert dev._session is None

def test_error_when_passing_unexpected_kwarg(self):
"""Test that we accept any keyword argument that the User wants to supply so that if
austingmhuang marked this conversation as resolved.
Show resolved Hide resolved
Qiskit allows for more customization we can automatically accomodate those needs. Right
now there are no such keyword arguments, so an error on Qiskit's side is raised."""

dev = QiskitDevice2(wires=2, backend=backend)

assert dev._session is None

with pytest.raises(
TypeError, match=r"Session.__init__\(\) got an unexpected keyword argument 'any_kwarg'"
):
with qiskit_session(dev, any_kwarg=30) as session:
assert dev._session == session
assert dev._session is not None

assert dev._session is None

def test_no_warning_when_using_initial_session_options(self):
initial_session = Session(backend=backend, max_time=30)
dev = QiskitDevice2(wires=2, backend=backend, session=initial_session)

assert dev._session == initial_session

with qiskit_session(dev) as session:
assert dev._session == session
assert dev._session != initial_session
assert dev._session._max_time == session._max_time
assert dev._session._max_time == initial_session._max_time

assert dev._session == initial_session
assert dev._session._max_time == initial_session._max_time

def test_warnings_when_overriding_session_context_options(self, recorder):
"""Test that warnings are raised when there are is an overlap between the Session options
on the device and the session options passed in via qiskit_session. Also ensures that the
session options passed in from the qiskit_session take precedence except for `backend` or
`service`"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstring could use clarification following updates in the PR

initial_session = Session(backend=backend)
dev = QiskitDevice2(wires=2, backend=backend, session=initial_session)

assert dev._session == initial_session

with pytest.warns(
UserWarning,
match="Using 'backend' set in device",
):
with qiskit_session(dev, max_time=30, backend=FakeManilaV2()) as session:
assert dev._session == session
assert dev._session != initial_session
assert dev._session._backend.name == "aer_simulator"

with pytest.warns(
UserWarning,
match="Using 'service' set in device",
):
with qiskit_session(dev, max_time=30, service="placeholder") as session:
assert dev._session == session
assert dev._session != initial_session
assert dev._session._service != "placeholder"

# device session should be unchanged by qiskit_session
assert dev._session == initial_session

max_time_session = Session(backend=backend, max_time=60)
dev = QiskitDevice2(wires=2, backend=backend, session=max_time_session)

with pytest.warns(
UserWarning,
match="`max_time` was set in the Session passed to the device. Using `max_time` '30' set in `qiskit_session`.",
):
with qiskit_session(dev, max_time=30) as session:
assert dev._session == session
assert dev._session != initial_session
assert dev._session._max_time == 30
assert dev._session._max_time != 60

assert dev._session == max_time_session
assert dev._session._max_time == 60

@pytest.mark.parametrize("initial_session", [None, MockSession(backend)])
def test_update_session(self, initial_session):
"""Test that you can update the session stored on the device"""
Expand Down
Loading