(De-)serialization of CircuitOperations #3923

95-martin-orion · 2021-03-17T19:44:14Z

Follow up for #3891. Part of #3634.

This PR adds the CircuitOp(De)serializer classes and updates existing behavior to handle them appropriately, including:

Addition of Op(De)serializer superclasses for operation (de-)serializers
Backwards-compatible conversion of GateOp(De)serializer fields to non-public, with public properties
Updates and deprecations for names made awkward by this change (e.g. gate_type -> internal_type)

Changes not included in this PR:

Add CircuitOperation (de-)serializers to the default QCS gateset

95-martin-orion · 2021-03-17T19:48:31Z

TODO in this PR: replace newly-deprecated fields in tests and add deprecation tests for these fields.

95-martin-orion · 2021-03-17T20:28:46Z

TODO in this PR: replace newly-deprecated fields in tests and add deprecation tests for these fields.

This is now done for tests caught by check/pytest-changed-files. May still hit some bumps if other uses of these fields are floating around the repo somewhere.

95-martin-orion · 2021-03-30T17:09:34Z

I realized while looking at the internal tools that the current proto format for moments (ops + circuit ops) will cause downstream tools to silently ignore circuit operations in circuits. One possible workaround is to append an invalid 'flag' operation to moments with circuit operations to cause a failure; tools with circuit operation support would then need to identify and ignore these 'flag' operations.

95-martin-orion · 2021-04-05T16:23:21Z

Pinging @dstrain115 and @maffoo for review.

dstrain115 · 2021-04-07T02:08:42Z

cirq/google/devices/known_devices_test.py

+    assert (
+        str(circuitop_proto)
+        == """\
+valid_gate_sets {


One thing that we should think about is whether a device specification should specify that the device supports circuit operations or not. (Or should we assume they all support circuit operations?)

This may have to be in a later PR, since it involves further proto changes, but we should give it some thought and maybe add an issue about it.

Apart from the transitional phase where we're still bringing support online, I think assuming all devices support circuit operations is valid, since in the worst case you can simply decompose the circuit operation.

TODO(95-martin-orion): test creating a device spec with CircuitOperation to identify necessary changes / ensure that this is well-behaved.

@dstrain115: added test_sycamore_circuitop_device to provide this coverage, and it failed. Updated serializable_device to resolve the issue.

cirq/google/op_deserializer.py

dstrain115 · 2021-04-07T03:19:21Z

cirq/google/op_deserializer.py

+    def serialized_id(self) -> str:
+        """Returns the string identifier for the resulting serialized object.
+
+        This value should reflect the internal_type of the serializer.


I am not sure I understand what this sentence means. What does "should reflect the internal_type" imply?
Same comment for deserializer.

I must have misunderstood the IDs when typing this up (plus a copy-paste error in the deserializer comment). Updated to (hopefully) more accurately represent the behavior.

cirq/google/op_serializer.py

dstrain115 · 2021-04-07T03:33:54Z

cirq/google/op_serializer.py

+
+        msg = msg or v2.program_pb2.CircuitOperation()
+        try:
+            msg.circuit_constant_index = raw_constants.index(op.circuit)


Any idea if this will be a performance concern? It seems we are doing a O(n) search of comparisons of potentially big circuits here. I am not sure how fast comparing two circuits would be. Maybe this is not a huge concern since a circuit would likely not have too many circuitsops, but this does seem to be a potential performance issue.

Do you think it is worth making raw_constants a dictionary instead?

That's a legitimate concern, although I would hope proper use of CircuitOperations would limit the size of these circuits somewhat. Since op.circuit is guaranteed to be a hashable FrozenCircuit, I'll change this to a constant-to-index map.

Switched to a map. Since this broke the symmetry between raw_constants in the serializer and deserializer, I went ahead with the rename to deserialized_constants in the deserializer as well.

dstrain115 · 2021-04-07T03:34:38Z

cirq/google/op_serializer_test.py

+    raw_constants = [default_circuit()]
+
+    repetition_spec = v2.program_pb2.RepetitionSpecification()
+    repetition_spec.repetition_count = 1


I would also test different repetition specs.

Done. Also changed the serializer code, since the old behavior failed this new test.

cirq/google/op_serializer_test.py

review-notebook-app · 2021-04-20T22:27:35Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

95-martin-orion · 2021-04-20T22:37:21Z

Apologies for the review spam, everyone - had some troubles merging master into this PR. It should be resolved now - feel free to unsubscribe from this if it's still spamming you.

95-martin-orion · 2021-05-07T14:35:28Z

Pinging @dstrain115 for re-review.

dstrain115

Mostly doc comments and then LGTM.

dstrain115 · 2021-05-19T01:55:52Z

cirq-google/cirq_google/op_deserializer.py

@@ -31,6 +36,46 @@
    import cirq


+class OpDeserializer(abc.ABC):
+    """Generic supertype for op deserializers."""


Maybe put "operation deserializers" to avoid using an abbreviation in the docstring.

dstrain115 · 2021-05-19T01:58:00Z

cirq-google/cirq_google/op_deserializer.py

+        """
+
+    @property  # type: ignore
+    @deprecated(deadline='v0.12', fix='Use serialized_id instead.')


Why does this need to be deprecated if it is a new class? Is this because OpDeserializer is itself deprecating a class?

You have a point - I needed it here to get the tests working initially, but now it can be safely relocated to the GateOpDeserializer.

Applied this change and updated all deprecation deadlines to v0.13, since we are now in v0.11.

dstrain115 · 2021-05-19T01:58:58Z

cirq-google/cirq_google/op_deserializer.py

+    def serialized_id(self) -> str:
+        """Returns the string identifier for the accepted serialized objects.
+
+        This ID denotes the serialization format this deserializer consumes.


I can't understand what this docstring means. Maybe an example or concrete statement would help?

dstrain115 · 2021-05-19T02:00:51Z

cirq-google/cirq_google/op_deserializer.py

+        constants: List[v2.program_pb2.Constant] = None,
+        deserialized_constants: List[Any] = None,
+    ) -> 'cirq.CircuitOperation':
+        """Turns a cirq.google.api.v2.CircuitOperation proto into a CircuitOperation."""


Add an Args section or at least point out that constants should have been previously parsed.

Copied the docstring down from the superclass method and added changes as necessary. Same for the from_proto method of the GateOpDeserializer.

dstrain115 · 2021-05-19T02:02:25Z

cirq-google/cirq_google/op_deserializer_test.py

@@ -362,3 +329,167 @@ def test_token_with_references():

    with pytest.raises(ValueError, match='Proto has references to constants table'):
        deserializer.from_proto(serialized)
+
+
+DEFAULT_TOKEN = 'test_tag'


Should this go at the top of the file?

dstrain115 · 2021-05-19T02:03:30Z

cirq-google/cirq_google/op_serializer.py

@@ -30,6 +32,78 @@
 Gate = TypeVar('Gate', bound=ops.Gate)


+class OpSerializer(abc.ABC):
+    """Generic supertype for op serializers."""


Same here, recommend using operation. It might be good to explain each of these abstract classes further, if possible as well.

dstrain115 · 2021-05-19T02:04:53Z

cirq-google/cirq_google/op_serializer_test.py

@@ -392,3 +406,139 @@ def test_token_serialization_with_constant_reference(constants, expected_index,
        GateWithAttribute(0.125)(q).with_tags(tag), constants=constants
    )
    assert constants == expected_constants
+
+
+DEFAULT_TOKEN = 'test_tag'


Put at top of file?

dstrain115 · 2021-05-19T02:05:49Z

cirq-google/cirq_google/serializable_gate_set.py

-        Returns:
-            A dictionary corresponds to the cirq_google.api.v2.Operation proto.
+                Returns:
+        <<<<<<< HEAD:cirq/google/serializable_gate_set.py


Merge conflict

dstrain115 · 2021-05-19T02:06:18Z

cirq-google/cirq_google/serializable_gate_set.py

-                cirq_google.api.v2.Operation proto.
+                Args:
+                    operation_proto: A dictionary representing a
+        <<<<<<< HEAD:cirq/google/serializable_gate_set.py


Another merge conflict.

95-martin-orion

Review comments addressed, PTAL.

95-martin-orion · 2021-05-19T14:10:09Z

cirq-google/cirq_google/op_deserializer.py

@@ -31,6 +36,46 @@
    import cirq


+class OpDeserializer(abc.ABC):
+    """Generic supertype for op deserializers."""


95-martin-orion · 2021-05-19T14:23:17Z

cirq-google/cirq_google/op_deserializer.py

+    def serialized_id(self) -> str:
+        """Returns the string identifier for the accepted serialized objects.
+
+        This ID denotes the serialization format this deserializer consumes.


95-martin-orion · 2021-05-19T14:43:53Z

cirq-google/cirq_google/op_deserializer_test.py

@@ -362,3 +329,167 @@ def test_token_with_references():

    with pytest.raises(ValueError, match='Proto has references to constants table'):
        deserializer.from_proto(serialized)
+
+
+DEFAULT_TOKEN = 'test_tag'


95-martin-orion · 2021-05-19T14:44:08Z

cirq-google/cirq_google/op_serializer_test.py

@@ -392,3 +406,139 @@ def test_token_serialization_with_constant_reference(constants, expected_index,
        GateWithAttribute(0.125)(q).with_tags(tag), constants=constants
    )
    assert constants == expected_constants
+
+
+DEFAULT_TOKEN = 'test_tag'


95-martin-orion · 2021-05-19T14:46:26Z

cirq-google/cirq_google/serializable_gate_set.py

-        Returns:
-            A dictionary corresponds to the cirq_google.api.v2.Operation proto.
+                Returns:
+        <<<<<<< HEAD:cirq/google/serializable_gate_set.py


95-martin-orion · 2021-05-19T14:46:37Z

cirq-google/cirq_google/serializable_gate_set.py

-                cirq_google.api.v2.Operation proto.
+                Args:
+                    operation_proto: A dictionary representing a
+        <<<<<<< HEAD:cirq/google/serializable_gate_set.py


95-martin-orion · 2021-05-19T14:52:02Z

cirq-google/cirq_google/op_deserializer.py

+        """
+
+    @property  # type: ignore
+    @deprecated(deadline='v0.12', fix='Use serialized_id instead.')


You have a point - I needed it here to get the tests working initially, but now it can be safely relocated to the GateOpDeserializer.

Applied this change and updated all deprecation deadlines to v0.13, since we are now in v0.11.

95-martin-orion · 2021-05-19T15:03:25Z

cirq-google/cirq_google/op_deserializer.py

+        constants: List[v2.program_pb2.Constant] = None,
+        deserialized_constants: List[Any] = None,
+    ) -> 'cirq.CircuitOperation':
+        """Turns a cirq.google.api.v2.CircuitOperation proto into a CircuitOperation."""


Copied the docstring down from the superclass method and added changes as necessary. Same for the from_proto method of the GateOpDeserializer.

95-martin-orion · 2021-05-19T15:11:35Z

cirq-google/cirq_google/op_serializer.py

@@ -30,6 +32,78 @@
 Gate = TypeVar('Gate', bound=ops.Gate)


+class OpSerializer(abc.ABC):
+    """Generic supertype for op serializers."""


Follow up for quantumlib#3891. Part of quantumlib#3634. This PR adds the `CircuitOp(De)serializer` classes and updates existing behavior to handle them appropriately, including: - Addition of `Op(De)serializer` superclasses for operation (de-)serializers - Backwards-compatible conversion of `GateOp(De)serializer` fields to non-public, with public properties - Updates and deprecations for names made awkward by this change (e.g. `gate_type` -> `internal_type`) Changes **not** included in this PR: - Add `CircuitOperation` (de-)serializers to the default QCS gateset

95-martin-orion added 11 commits March 17, 2021 10:47

First pass, still needs tests.

185e95e

Clean up TODOs

c80d982

Basic (de-)serialization tests.

1ee4b30

Update gate_sets_test.

fdaa216

Deserializer test coverage

4aa8ccf

CircuitOpSerializer test coverage

14ca867

SerializableGateSet test coverage

de2b042

Known devices test coverage

fded376

Formatting

7f3a269

Pick up changes in proto.

1877a71

Rewind gate_sets_test format.

8b2ed14

95-martin-orion requested review from maffoo and dstrain115 March 17, 2021 19:44

95-martin-orion requested review from cduck, vtomole, wcourtney and a team as code owners March 17, 2021 19:44

95-martin-orion mentioned this pull request Mar 17, 2021

CircuitOperation proto serialization 95-martin-orion/Cirq#1

Closed

google-cla bot added the cla: yes Makes googlebot stop complaining. label Mar 17, 2021

Avoid deprecated fields

b98219d

95-martin-orion added 2 commits March 22, 2021 15:21

Merge remote-tracking branch 'upstream/master' into cirq-proto-serial-2

c3f5215

Update cirq.google refs

69b7168

dstrain115 reviewed Apr 7, 2021

View reviewed changes

95-martin-orion added 3 commits April 12, 2021 09:43

Round out tests and docs

138441b

Merge branch 'master' into cirq-proto-serial-2

f6020d4

Use map for raw_constants in serializer

4759ff2

95-martin-orion requested review from HGSilveri, ma5x, mrwojtek, nakardo and pschindler as code owners April 20, 2021 22:27

Merge branch 'master' into cirq-proto-serial-2

a37dccb

95-martin-orion force-pushed the cirq-proto-serial-2 branch from ba6b592 to a37dccb Compare April 20, 2021 22:35

95-martin-orion removed request for dabacon, nakardo, gmauricio, ColemanCollins, mrwojtek, pschindler, HGSilveri and ma5x April 20, 2021 22:35

dstrain115 approved these changes May 19, 2021

View reviewed changes

Docstring and deprecation fixes

b7ab8d6

95-martin-orion commented May 19, 2021

View reviewed changes

Merge branch 'master' into cirq-proto-serial-2

c40d061

95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label May 24, 2021

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label May 24, 2021

Merge branch 'master' into cirq-proto-serial-2

233fe88

CirqBot merged commit 71285d2 into quantumlib:master May 24, 2021

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 24, 2021

(De-)serialization of CircuitOperations #3923

(De-)serialization of CircuitOperations #3923

Conversation

95-martin-orion commented Mar 17, 2021

95-martin-orion commented Mar 17, 2021 • edited

95-martin-orion commented Mar 17, 2021

95-martin-orion commented Mar 30, 2021

95-martin-orion commented Apr 5, 2021

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

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

Choose a reason for hiding this comment

review-notebook-app bot commented Apr 20, 2021

95-martin-orion commented Apr 20, 2021

95-martin-orion commented May 7, 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

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

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

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

95-martin-orion 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

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

95-martin-orion commented Mar 17, 2021 •

edited