Remove uses of `.device` in cirq-pasqal in preparation for larger circuit.device deprecation #4757

MichaelBroughton · 2021-12-15T01:46:25Z

First devices removal as a part of #4744 .

The high level plan for deprecation is to:

pull all components of the library off of .device and onto ._device to keep existing behavior working.
Deprecate dependent functionality that relies on .device.
After all dependency deprecations are done, deprecate the .device property in cirq-core circuits.py.
After the deprecation cycle is over, delete .device and _device properties from circuit, abstractcircuit, frozencircuit etc.

This PR is tackles cirq-pasqal.

HGSilveri · 2021-12-15T10:36:17Z

Hi @MichaelBroughton ! I'm completely outside of the loop here, so it would be great if you could clarify some things.

If I understand correctly, you plan on dissociating the device from the circuit. Does this imply that circuit creation will no longer be constrained by device limitations, or will there be another mechanism put in place to replace this? If so, will the behaviour be identical? If not, why do you want to get rid of this functionality?

Thanks in advance!

HGSilveri · 2021-12-15T10:40:45Z

cirq-pasqal/cirq_pasqal/pasqal_sampler.py

@@ -21,15 +21,20 @@


 class PasqalSampler(cirq.work.Sampler):
-    def __init__(self, remote_host: str, access_token: str = '') -> None:
+    def __init__(
+        self, remote_host: str, access_token: str = '', device: cirq_pasqal.PasqalDevice = None


We do require a PasqalDevice to be specified. If that's not going to be done in Circuit anymore, device shouldn't be optional and it must be a PasqalDevice instance.

We do require a PasqalDevice to be specified.

I can add the assertion checks at init to make sure it's a pasqal device.

If that's not going to be done in Circuit anymore, device shouldn't be optional and it must be a PasqalDevice instance.

This would make this a breaking change. If this PR were to be merged, both the old and new functionality would still work. Until the deprecation warning is up a user could still attach a device to a circuit or construct a sampler with a device (sampler device taking precedence here). Would you prefer we do a breaking style change and make this argument required ?

I see what you mean. While the option to attach a device to a circuit is still there, this doesn't need to be required. What if you keep it Optional for now, but demand that there is a PasqalDevice associated? If a PasqalDevice is attached to the circuit, this doesn't have to be specified. Otherwise, it is required. Later on, when the option to have a device attached to a circuit is gone, this is turned into a required argument.

Did you consider this option?

I believe so. To me this sounds like what is currently implemented in the code. Re-reading it now I'm having trouble figuring out what we're missing, would you mind adding a little snippet here in python or pseudocode of what changes we should consider to help me understand ?

Sure, see my comment on lines 136 to 142.

MichaelBroughton · 2021-12-15T22:06:01Z

Hi @HGSilveri thanks for the quick response:

If I understand correctly, you plan on dissociating the device from the circuit. Does this imply that circuit creation will no longer be constrained by device limitations, or will there be another mechanism put in place to replace this?

That's right. There will be no new mechanism put in place to meaningfully "attach" a device to a circuit. This does not stop higher level objects like PasqalSampler (or the cirq_google engine emulator) being constructed with devices and using them for circuit validation.

Going forward we expect users to call device.validate_circuit instead of doing the "online style" of circuit construction with validation happening behind the scenes if there is a device attached.

If so, will the behaviour be identical? If not, why do you want to get rid of this functionality?

Looking through the library it was found that there could be a lot of inconsistent and weird behavior in library code that used circuits that might have had devices attached. Two good examples of this are:

In cirq-aqt the run_sample function here relies on the associated device to set the number of qubits used in the state vector simulation. Users relying on len(circuit.all_qubits()) or other functionality could wind up being very confused by this inconsistency when analyzing the returned bitstrings.
In cirq-core if we look at things like optimizers we have some that require gates and qubits to function and others that require circuits (i.e. here and here). If I needed to use both of these methods (or any pair like this) when building/optimizing my circuit I would be ignoring my device constraints for one and considering them with the other while building/optimizing my circuit. In order to ensure my circuit was indeed valid I would likely still wind up having to call device.validate_circuit when I was finished modifying the circuit anyway.

Moving users to just needing to call validate_circuit at their discretion and removing the ability to have different behaviors based on the device attached to the circuit is how we plan to fix these problems. Going forward if a library function is to take device considerations into account it should require a device be explicitly specified.

I copied this section out of the design doc just in case there's access issues.

HGSilveri · 2021-12-16T09:14:20Z

Hi @MichaelBroughton , thanks for the thorough explanation! It's a bit of a shame to see this feature go, but I understand your motives.

I copied this section out of the design doc just in case there's access issues.

Thanks! I do not have access to those, indeed.

dstrain115

Just left two minor comments.

dstrain115 · 2021-12-16T22:56:38Z

cirq-pasqal/cirq_pasqal/pasqal_sampler.py

+        match=lambda args, kwargs: (
+            len(args) >= 2
+            and isinstance(args[1], cirq.AbstractCircuit)
+            and args[1]._device != cirq.UNCONSTRAINED_DEVICE


Can _device be None? I see it set to None above but not to cirq.UNCONSTRAINED_DEVICE. Same with checks below.

The _device on a cirq.Circuit cannot be None. It can either be cirq.UUNCONSTRAINED_DEVICE or a user set one.

The _device property on this sampler is an optional[PasqalDevice] so it can be none.

cirq-pasqal/cirq_pasqal/pasqal_sampler.py

HGSilveri · 2022-01-07T09:40:07Z

cirq-pasqal/cirq_pasqal/pasqal_sampler.py

+        if program._device != cirq.UNCONSTRAINED_DEVICE:
+            assert isinstance(
+                program._device, cirq_pasqal.PasqalDevice
+            ), "Device must inherit from cirq.PasqalDevice."
+            program._device.validate_circuit(program)
+        elif self._device:
+            self._device.validate_circuit(program)


How about this?

Suggested change

if program._device != cirq.UNCONSTRAINED_DEVICE:

assert isinstance(

program._device, cirq_pasqal.PasqalDevice

), "Device must inherit from cirq.PasqalDevice."

program._device.validate_circuit(program)

elif self._device:

self._device.validate_circuit(program)

device = (

program._device if program._device != cirq.UNCONSTRAINED_DEVICE

else self._device

)

assert isinstance(

device, cirq_pasqal.PasqalDevice

), "Device must inherit from cirq.PasqalDevice."

device.validate_circuit(program)

Ahhh I see. The major difference between what is implemented and this is:

It will type check the new self._device.

It will no longer allow a user to do no device verification at all.

This seems fine to me. I will say that generally speaking it's not always a great practice to type check inputs for their expected type in python. This is because now your function boundary which was defined clearly on the types it accepted is now defined on all possible types and works correctly on the desired input types, where it doesn't work is now obscured by this type check that doesn't allow things to crash and provide a full trace. Another way of thinking about this is: this would paradoxically make behavior under violation of the API part of the API.

I'll implement this new way now and if anyone feels strongly we can always revisit in a later PR.

…ghton/Cirq into cirq_pasqal_device_deprecate

HGSilveri

LGTM, thanks!

…cuit.device deprecation (quantumlib#4757) First devices removal as a part of quantumlib#4744 . The high level plan for deprecation is to: 1. pull all components of the library off of `.device` and onto `._device` to keep existing behavior working. 2. Deprecate dependent functionality that relies on `.device`. 3. After all dependency deprecations are done, deprecate the `.device` property in `cirq-core` circuits.py. 4. After the deprecation cycle is over, delete .device and _device properties from circuit, abstractcircuit, frozencircuit etc. This PR is tackles `cirq-pasqal`.

Remove uses of .device in cirq-pasqal.

1752040

MichaelBroughton requested review from dstrain115 and HGSilveri December 15, 2021 01:46

MichaelBroughton requested review from cduck, vtomole and a team as code owners December 15, 2021 01:46

CirqBot added the size: M 50< lines changed <250 label Dec 15, 2021

format circuit.py.

f63befc

HGSilveri requested changes Dec 15, 2021

View reviewed changes

dstrain115 approved these changes Dec 16, 2021

View reviewed changes

MichaelBroughton requested a review from dstrain115 January 5, 2022 02:39

dstrain115 approved these changes Jan 5, 2022

View reviewed changes

cirq-pasqal/cirq_pasqal/pasqal_sampler.py Outdated Show resolved Hide resolved

added assert message.

e518ca1

MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 7, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jan 7, 2022

Merge branch 'master' into cirq_pasqal_device_deprecate

d85244b

MichaelBroughton requested a review from HGSilveri January 7, 2022 01:21

HGSilveri reviewed Jan 7, 2022

View reviewed changes

MichaelBroughton added 2 commits January 7, 2022 12:51

add type checking.

3d24070

Merge branch 'cirq_pasqal_device_deprecate' of github.com:MichaelBrou…

f82487b

…ghton/Cirq into cirq_pasqal_device_deprecate

HGSilveri approved these changes Jan 7, 2022

View reviewed changes

CirqBot merged commit b00907a into quantumlib:master Jan 7, 2022

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jan 7, 2022

MichaelBroughton deleted the cirq_pasqal_device_deprecate branch January 7, 2022 23:44

MichaelBroughton mentioned this pull request Feb 1, 2022

Deprecate device.decompose_operation in cirq-core. #4922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove uses of `.device` in cirq-pasqal in preparation for larger circuit.device deprecation #4757

Remove uses of `.device` in cirq-pasqal in preparation for larger circuit.device deprecation #4757

MichaelBroughton commented Dec 15, 2021 •

edited

Loading

HGSilveri commented Dec 15, 2021

HGSilveri Dec 15, 2021 •

edited

Loading

MichaelBroughton Dec 15, 2021 •

edited

Loading

HGSilveri Dec 16, 2021

HGSilveri Jan 7, 2022

MichaelBroughton Jan 7, 2022

HGSilveri Jan 7, 2022

MichaelBroughton commented Dec 15, 2021 •

edited

Loading

HGSilveri commented Dec 16, 2021

dstrain115 left a comment

dstrain115 Dec 16, 2021

MichaelBroughton Jan 5, 2022

HGSilveri Jan 7, 2022

MichaelBroughton Jan 7, 2022

HGSilveri left a comment

Remove uses of .device in cirq-pasqal in preparation for larger circuit.device deprecation #4757

Remove uses of .device in cirq-pasqal in preparation for larger circuit.device deprecation #4757

Conversation

MichaelBroughton commented Dec 15, 2021 • edited Loading

HGSilveri commented Dec 15, 2021

HGSilveri Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelBroughton Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBroughton commented Dec 15, 2021 • edited Loading

HGSilveri commented Dec 16, 2021

dstrain115 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HGSilveri left a comment

Choose a reason for hiding this comment

Remove uses of `.device` in cirq-pasqal in preparation for larger circuit.device deprecation #4757

Remove uses of `.device` in cirq-pasqal in preparation for larger circuit.device deprecation #4757

MichaelBroughton commented Dec 15, 2021 •

edited

Loading

HGSilveri Dec 15, 2021 •

edited

Loading

MichaelBroughton Dec 15, 2021 •

edited

Loading

MichaelBroughton commented Dec 15, 2021 •

edited

Loading