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

CompilationTargetGateset support in GridDeviceMetadata #5195

Conversation

verult
Copy link
Collaborator

@verult verult commented Apr 4, 2022

Part of #5050

Adds a new property to represent a set of CompilationTargetGatesets, to provide users a convenient way to transform their invalid circuits to a valid one supported by the device, and to allow for a device to potentially include gates from different viable CompilationTargetGatesets (e.g. SYC and SQRT_ISWAP).

Other alternative approaches:

  • Always provide only one CompilationTargetGateset. In cases where multiple CompilationTargetGatesets are valid, the code selects only one of them to surface.
    • How to determine which target gateset to surface?
    • The current solution of allowing a set of CompilationTargetGatesets is simple to implement and gives users a choice (because of different error characteristics for example)
  • Add a utility function in cirq_google but outside the device implementation and GridDeviceMetadata, to avoid increasing the API surface of GridDeviceMetadata. IMO considering CompilationTargetGatesets as a piece of device metadata makes sense as every device could (and should in most cases) have a group of target gatesets. Keeping this in GridDeviceMetadata makes the compilation feature more discoverable.

The JSON diff came out gnarly due to indentations. Basically I added a new GridDeviceMetadata object that contains compilation_target_gatesets.

Part of device refactor (#5050)

cc @dstrain115 @maffoo @MichaelBroughton @mpharrigan @tanujkhattar @wcourtney

@verult verult requested review from a team, vtomole and cduck as code owners April 4, 2022 23:20
@CirqBot CirqBot added the size: M 50< lines changed <250 label Apr 4, 2022
@verult verult force-pushed the cg-device-refactor/griddevicemetadata-gateset-type branch from e1e5306 to 1f36aac Compare April 5, 2022 02:16
@verult verult force-pushed the cg-device-refactor/griddevicemetadata-gateset-type branch 2 times, most recently from a00783e to 620ac3f Compare April 6, 2022 01:18
@tanujkhattar
Copy link
Collaborator

@MichaelBroughton @verult One high level comment: Should this property exist on DeviceMetadata instead of GridDeviceMetadata ? It can return None / empty tuple by default, signifying that device doesn't yet have a compilation target. For most vendor devices, we anyway need to migrate their transformers to the new framework and hence create compilation target gatesets for them so it would make sense to integrate those compilation targets with their devices. Thoughts ?

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.

Looks good overall. Blocking to discuss the comment above on whether we should add this directly to DeviceMetadata.

cirq-core/cirq/devices/grid_device_metadata.py Outdated Show resolved Hide resolved
cirq-core/cirq/devices/grid_device_metadata.py Outdated Show resolved Hide resolved
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.

I take my previous comment back -- I agree we should add the compilation target gateset property to grid device metadata because DeviceMetadata doesn't even have a gateset property on it.

We can merge once the nits are resolved.

@verult verult force-pushed the cg-device-refactor/griddevicemetadata-gateset-type branch 2 times, most recently from f45e9ae to 3bb7336 Compare April 26, 2022 22:33
@verult verult force-pushed the cg-device-refactor/griddevicemetadata-gateset-type branch from 3bb7336 to d3d2252 Compare April 29, 2022 19:31
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.

LGTM after adding original json deserialization test to make sure deserialization without compilation target gatesets property works as expected.

@@ -208,5 +272,18 @@
"row": 10,
"col": 10
}
],
"compilation_target_gatesets": [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't change existing json test. Add a new test instead so that the existing json deserialization works as expected.

Copy link
Collaborator Author

@verult verult May 2, 2022

Choose a reason for hiding this comment

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

We have to at least set compilation_target_gatesets to [], otherwise the test will fail saying the JSON serialization of the instance from the repr file does not match the JSON file. So IIUC we can't deserialization backward compatibility anyway. I could perhaps add an _inward file instead and keep the old entry there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see, my bad. Let's add an _inward file instead.

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 a json_inward which is a copy of the old json, and the corresponding repr_inward.

@verult verult force-pushed the cg-device-refactor/griddevicemetadata-gateset-type branch 2 times, most recently from c9747cf to 7b44d4f Compare May 2, 2022 23:51
@verult verult requested a review from tanujkhattar May 2, 2022 23:52
@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 2, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented May 2, 2022

Automerge cancelled: There are merge conflicts.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 2, 2022
@verult verult force-pushed the cg-device-refactor/griddevicemetadata-gateset-type branch from 7b44d4f to 598d290 Compare May 3, 2022 00:16
@MichaelBroughton MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 3, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 3, 2022
@CirqBot
Copy link
Collaborator

CirqBot commented May 3, 2022

Automerge cancelled: A required status check is not present.

Missing statuses: ['Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)']

@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 3, 2022
@tanujkhattar tanujkhattar merged commit 373bc23 into quantumlib:master May 3, 2022
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants