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

cirq_google.GridDevice, minus gateset and gate durations #5203

Merged
merged 10 commits into from
May 9, 2022

Conversation

verult
Copy link
Collaborator

@verult verult commented Apr 6, 2022

Part of #5050

  • Implemented qubit set, qubit pairs, and op validation. Gateset and gate duration will be done in a follow-up.
  • Added a device_specification_validation module shell, to be used by the QCS server side to validate the DeviceSpecification proto against things like:
    • Qubit self-loops in qubit pairs
    • Qubit pairs contain qubits not in the valid qubit set.
    • Gate durations contains a gate which is missing in the gateset.

__str__() and _repr_pretty_ are nearly all copied from SerializableDevice.

Part of #5050

Also fixes #5197

Question: Does this definition of device equality make sense, or should it contain device name as well? Implementing equality is convenient for json and repr tests but not sure if it make sense in general.

@dstrain115 @MichaelBroughton

@verult verult requested review from wcourtney, a team, vtomole and cduck as code owners April 6, 2022 01:01
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Apr 6, 2022
@verult verult force-pushed the cg-device-refactor/device branch 3 times, most recently from 3cd7c1b to 53880e6 Compare April 7, 2022 00:21
@MichaelBroughton MichaelBroughton self-assigned this Apr 9, 2022
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

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

Looking really good. Just a few minor tweaks then we should be good to merge.

Comment on lines 172 to 173
return cls(
qubit_pairs,
gateset,
None if gate_durations is None else dict(gate_durations),
all_qubits,
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add this None if gate durations ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dict(None) throws TypeError: 'NoneType' object is not iterable. Alternatively we could also give gate_durations a default value: gate_durations=[]. That might be cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Non-default arguments can't be after default arguments. Will shifting the order of method parameters here break JSON backward compatibility?

Comment on lines 409 to 531
gates_str = f'{self._gates_repr_str}, ' if len(self._gates_repr_str) > 0 else ''
return (
f'cirq.Gateset('
f'{self._gates_repr_str}, '
f'{gates_str}'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why change here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This addresses #5197

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a test to gateset_test.py. Also, If the change is unrelated and is not blocking this PR, then I'd prefer if we can split it out in a separate PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added and moved to #5322



@cirq.value_equality
class GoogleDevice(cirq.Device):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to call it something like UniformGoogleDevice or GoogleGridDevice just to convey the message that this type identifies and works for devices that have uniform gates in all directions etc. Since we are surfacing a GridDeviceMetadata here we want to ensure that the user knows they won't get a device that doesn't have the same gates and connectivity everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. How about just GridDevice? I'm cautious with "uniform" because for example some 2-qubit gates may be missing. The "Google" descriptor is implicit from the package name and was there only because I couldn't think of a better descriptor :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am in favor of GoogleGridDevice or GridDevice.

Comment on lines 73 to 77
https://github.com/quantumlib/Cirq/blob/3969c2d3964cea56df33b329f036ba6810683882/cirq-google/cirq_google/api/v2/device.proto#L13
)
is the main specification for device information surfaced by the Quantum Computing Service.
Thus, this class is should be instantiated using a `DeviceSpecification` proto via the
`from_proto()` class method.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the verbosity here!

Comment on lines 87 to 88
@classmethod
def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the from_proto method preffered over init design-wise ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm good question, I don't have any good reasons other than following the existing pattern in SerializableDevice. We also don't ever use the SerializableDevice constructor anywhere else. @dstrain115 do you happen to know?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The full DeviceSpecification would have to be stored in the repr and json, which could potentially be verbose?

Comment on lines 122 to 129
try:
metadata = cirq.GridDeviceMetadata(
qubit_pairs=qubit_pairs,
gateset=cirq.Gateset(), # TODO(#5050) implement
all_qubits=all_qubits,
)
except ValueError as ve:
raise ValueError("DeviceSpecification is invalid.") from ve
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Do we want to catch the exception here and introduce this dependency on the behavior or GridDeviceMetaData's error behavior ? By extending our API for from_proto here to accomodate errors in downstream code we are kind of bringing the error checking from downstream code upstream here to some extent. Do we want to do that in this case ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good point. It's perhaps better to explicitly call a validation function before doing any parsing, and instead leave this exception here as a last line of defense. Better not to tightly couple with downstream errors. Will update.

Copy link
Collaborator Author

@verult verult left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

Comment on lines 172 to 173
return cls(
qubit_pairs,
gateset,
None if gate_durations is None else dict(gate_durations),
all_qubits,
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dict(None) throws TypeError: 'NoneType' object is not iterable. Alternatively we could also give gate_durations a default value: gate_durations=[]. That might be cleaner.

Comment on lines 409 to 531
gates_str = f'{self._gates_repr_str}, ' if len(self._gates_repr_str) > 0 else ''
return (
f'cirq.Gateset('
f'{self._gates_repr_str}, '
f'{gates_str}'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This addresses #5197

Comment on lines 87 to 88
@classmethod
def from_proto(cls, proto: v2.device_pb2.DeviceSpecification) -> 'GoogleDevice':
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm good question, I don't have any good reasons other than following the existing pattern in SerializableDevice. We also don't ever use the SerializableDevice constructor anywhere else. @dstrain115 do you happen to know?

Comment on lines 122 to 129
try:
metadata = cirq.GridDeviceMetadata(
qubit_pairs=qubit_pairs,
gateset=cirq.Gateset(), # TODO(#5050) implement
all_qubits=all_qubits,
)
except ValueError as ve:
raise ValueError("DeviceSpecification is invalid.") from ve
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good point. It's perhaps better to explicitly call a validation function before doing any parsing, and instead leave this exception here as a last line of defense. Better not to tightly couple with downstream errors. Will update.



@cirq.value_equality
class GoogleDevice(cirq.Device):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. How about just GridDevice? I'm cautious with "uniform" because for example some 2-qubit gates may be missing. The "Google" descriptor is implicit from the package name and was there only because I couldn't think of a better descriptor :)

@verult verult force-pushed the cg-device-refactor/device branch 3 times, most recently from b86ca7b to a14e2df Compare April 27, 2022 18:42
@verult verult requested a review from MichaelBroughton April 27, 2022 18:44
@verult
Copy link
Collaborator Author

verult commented Apr 27, 2022

Addressed comments, and added a validation function which is called at the beginning of from_proto, but also will eventually be called on server side prior to sending the DeviceSpecification.

@verult verult force-pushed the cg-device-refactor/device branch 2 times, most recently from 2edef8f to 5c75527 Compare April 27, 2022 19:58
Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

Looking good! I like the very descriptive docstring blocks within the class.

return cls(
qubit_pairs,
gateset,
None if gate_durations is None else dict(gate_durations),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: Do you think dict(gate_durations) if gate_durations is not None else None is more readable?

Copy link
Collaborator Author

@verult verult Apr 28, 2022

Choose a reason for hiding this comment

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

Keeping the slightly more complicated part up front sgtm, will update



@cirq.value_equality
class GoogleDevice(cirq.Device):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am in favor of GoogleGridDevice or GridDevice.

Example use cases:

* Get an instance of a Google grid device.
>>> device = cirq_google.get_engine().get_processor('weber').get_device()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should create an example that does not require access to the API. This could be a follow-up issue/PR if this is not yet done. For instance, using the virtual engine stuff that we are building.

Copy link
Collaborator Author

@verult verult Apr 28, 2022

Choose a reason for hiding this comment

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

Sounds good. Once the virtual engine work is available I imagine we'd do a docs sweep to update all the examples that access the API. Leaving this as no-op for now.

>>> print(device)

* Determine whether a circuit can be run on the device.
>>> device.validate_circuit(circuit) # Raises an exception if the circuit is invalid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: may be more descriptive to say which exception is raised, e.g. ValueError so that people can put try catch around it easily.


For Google devices, the
[DeviceSpecification proto](
https://github.com/quantumlib/Cirq/blob/3969c2d3964cea56df33b329f036ba6810683882/cirq-google/cirq_google/api/v2/device.proto#L13
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this like to point to master and not a specific commit.


This class only supports `cirq.GridQubit`s and `cirq.NamedQubit`s. If a
`DeviceSpecification.valid_qubits` string is in the form `<int>_<int>`, it is parsed as a
GridQubit. Otherwise it is parsed as a NamedQubit.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting choice. If this is a GridDevice, do you think the qubits should be restricted to GridQubits? If there a use case for named qubits. I would guess that it is likely a mistake if a non-grid qubit id happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sg. This came from the SerializableDevice implementation, but will remove since we don't ever use NamedQubits.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will also add validation for the GridQubit string format.

raise ValueError(f'Qubit pair is not valid on device: {operation.qubits!r}')

def __str__(self) -> str:
# If all qubits are grid qubits, render an appropriate text diagram.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am surprised that the below code for rendering a grid device doesn't exist somewhere already. Is the default insufficient because we do not assume pairwise connectivity between qubits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got this code from SerializableDevice, and there's also something similar for XmonDevice. This GridDevice will eventually replace both of them so I don't think there's a need to define a common function.

cirq.Device doesn't have a default, and I presume it's because yeah connectivity can be more general and would be hard to render. cc @MichaelBroughton

return grid_qubits, spec


def _create_device_spec_qubit_pair_self_loops() -> v2.device_pb2.DeviceSpecification:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: consider adding a docstring for this and the below private functions for improved clarity.

@verult verult requested review from dstrain115 April 28, 2022 20:58
@verult verult force-pushed the cg-device-refactor/device branch from c8743b6 to 9f3afb5 Compare April 29, 2022 00:17
@verult verult changed the title GoogleDevice, minus gateset and gate durations GridDevice, minus gateset and gate durations May 2, 2022
@verult verult force-pushed the cg-device-refactor/device branch from 9f3afb5 to 93c1c5d Compare May 2, 2022 20:40
@verult verult changed the title GridDevice, minus gateset and gate durations cirq_google.GridDevice, minus gateset and gate durations May 2, 2022
Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

Looking really good. Only concern is around proto validation for asymmetric and subset permutation targets.

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
gateset=cirq.Gateset(), # TODO(#5050) implement
all_qubits=all_qubits,
)
except ValueError as ve: # coverage: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get rid of the coverage: ignore here and below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added them because these lines are currently unreachable if everything works correctly, and only act as a last line of defense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test which gives a bad input that triggers this condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The same conditions that would trigger this exception would also cause DeviceSpecification validation to fail, so this is captured by the test test_grid_device_invalid_device_specification. If validation is buggy and this exception here is triggered, the test should get an exception but will fail because there's an error pattern mismatch.

Per @MichaelBroughton 's suggestion I added spec validation logic at the top of from_proto to make it explicit that errors that would've caused GridDeviceMetadata instantiation to fail are due to an invalid DeviceSpecification.

Will update the error match patterns to include "Invalid DeviceSpecification" to make sure that validation error messages are matched instead of the GridDeviceMetadata ones, in case error messages accidentally match up in the future between the two.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it even worth having the ValueError catching if unreachable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so - it would protect against future bugs in validation logic. For example, when GridDeviceMetadata gets an additional constraint but validation isn't updated, it's useful to give users the hint that the error they are getting is due to the DeviceSpecification being invalid rather than a mistake they made.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also think we should remove this check since the code path is unreachable.

We can add an assertion instead if we want to make sure we catch the inconsistencies between validation logic vs new constraints in GridDeviceMetadata but I'm not sure if putting everything under the umbrella of device specification errors is the right thing to do in this case.

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
@tanujkhattar tanujkhattar self-assigned this May 3, 2022
@verult verult requested a review from tanujkhattar May 4, 2022 01:40
… same GridDeviceMetadata as GridDeviceMetadata.repr
@verult
Copy link
Collaborator Author

verult commented May 6, 2022

Ready for final approvals!

Copy link
Collaborator Author

@verult verult left a comment

Choose a reason for hiding this comment

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

My bad, realized I never sent out my comment replies after pushing the code.

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dstrain115 dstrain115 left a comment

Choose a reason for hiding this comment

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

LGTM, just a few minor nits.

cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
cirq-google/cirq_google/devices/grid_device.py Outdated Show resolved Hide resolved
gateset=cirq.Gateset(), # TODO(#5050) implement
all_qubits=all_qubits,
)
except ValueError as ve: # coverage: ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it even worth having the ValueError catching if unreachable?

@verult verult added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 7, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 7, 2022
@verult verult 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 May 7, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 7, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented May 7, 2022

Automerge cancelled: The automerge label was removed.

@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 7, 2022
@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 9, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 9, 2022
@CirqBot CirqBot merged commit 4531a10 into quantumlib:master May 9, 2022
@CirqBot 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 May 9, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…5203)

Part of quantumlib#5050

* Implemented qubit set, qubit pairs, and op validation. Gateset and gate duration will be done in a follow-up.
* Added a `device_specification_validation` module shell, to be used by the QCS server side to validate the DeviceSpecification proto against things like: 
  * Qubit self-loops in qubit pairs
  * Qubit pairs contain qubits not in the valid qubit set.
  * Gate durations contains a gate which is missing in the gateset.

`__str__()` and `_repr_pretty_` are nearly all copied from `SerializableDevice`.

Part of quantumlib#5050

Also fixes quantumlib#5197

**Question:** Does this definition of device equality make sense, or should it contain device name as well? Implementing equality is convenient for json and repr tests but not sure if it make sense in general.

@dstrain115 @MichaelBroughton
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…5203)

Part of quantumlib#5050

* Implemented qubit set, qubit pairs, and op validation. Gateset and gate duration will be done in a follow-up.
* Added a `device_specification_validation` module shell, to be used by the QCS server side to validate the DeviceSpecification proto against things like: 
  * Qubit self-loops in qubit pairs
  * Qubit pairs contain qubits not in the valid qubit set.
  * Gate durations contains a gate which is missing in the gateset.

`__str__()` and `_repr_pretty_` are nearly all copied from `SerializableDevice`.

Part of quantumlib#5050

Also fixes quantumlib#5197

**Question:** Does this definition of device equality make sense, or should it contain device name as well? Implementing equality is convenient for json and repr tests but not sure if it make sense in general.

@dstrain115 @MichaelBroughton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateset: Invalid repr if the gateset is empty
5 participants