Proto serialization v25 #4333

dstrain115 · 2021-07-19T17:49:07Z

This PR creates a new class called CircuitSerializer that can serialize Circuits into protocol buffers more efficiently than existing mechanisms.

This class is similar to SerializableGateSet and op_(de)serializer, in that it serializes and deserializes cirq.Circuits into Program protocol buffers, but it is about 60% more space efficient and has significant time savings as well.

This serializer works by changing the Gate field and Args dict to a specific message for each allowed gate type. It changes Qubits from a repeated Qubit message, to an index in the constant message.

This PR also adds a FloatArg message which is specific to Arguments where it is known the value will be a float (or a sympy expression).

MichaelBroughton

Looking good. A few minor nits. My biggest concern before merging is that the tests are totally comprehensive. Are we including all the cases with sympy.Symbols that we can in circuit_serializer_test.py ?

MichaelBroughton · 2021-07-20T16:22:19Z

cirq-google/cirq_google/api/v2/program.proto

@@ -34,6 +34,7 @@ message Constant {
  oneof const_value {
    string string_value = 1;
    Circuit circuit_value = 2;
+    Qubit qubit = 3;


Could you please add comments here and in the rest of the file describing these new fields (ideally mentioning that these are new for the v25 and won't nescessarily be populated by the old serializer) ?

MichaelBroughton · 2021-07-20T16:28:35Z

cirq-google/cirq_google/circuit_serializer.py

+        self,
+        gate_set_name: str,
+    ):
+        """Construct the gate set.


Nit: This isn't a gate set ?

MichaelBroughton · 2021-07-20T16:29:01Z

cirq-google/cirq_google/circuit_serializer.py

+            serializers: The OpSerializers to use for serialization.
+                Multiple serializers for a given gate type are allowed and
+                will be checked for a given type in the order specified here.
+                This allows for a given gate type to be serialized into
+                different serialized form depending on the parameters of the
+                gate.
+            deserializers: The OpDeserializers to convert serialized
+                forms of gates or circuits into Operations.


Nit: Args not present.

MichaelBroughton · 2021-07-20T16:38:40Z

cirq-google/cirq_google/circuit_serializer.py

+        else:
+            raise NotImplementedError(f'Unrecognized program type: {type(program)}')


Nit: Convert this to a less nested check.

if not isinstance(program, cirq.Circuit): raise NotImplementedError rest of code.

MichaelBroughton · 2021-07-20T16:45:56Z

cirq-google/cirq_google/circuit_serializer.py

+        msg: Union[None, v2.program_pb2.Operation, v2.program_pb2.CircuitOperation] = None,
+        **kwargs,
+    ) -> Union[v2.program_pb2.Operation, v2.program_pb2.CircuitOperation]:
+        """Disambiguation for operation serialization."""


Nit: no args list or return types.

Removed this function.

MichaelBroughton · 2021-07-20T16:51:15Z

cirq-google/cirq_google/circuit_serializer.py

+        ],
+        **kwargs,
+    ) -> cirq.Operation:
+        """Disambiguation for operation deserialization."""


Nit: missing args and return type

REmoved this function.

MichaelBroughton · 2021-07-20T18:04:53Z

cirq-google/cirq_google/circuit_serializer_test.py

+)
+
+
+OPERATIONS = [


Would it be possible to include symbol cases here and in the CircuitOperations tests ?

Done. CircuitOperations with symbols still pending.

Modified circuit operation test to include symbols.

maffoo · 2021-07-20T18:39:08Z

cirq-google/cirq_google/api/v2/program.proto

+  FloatArg duration_nanos=1;
+}
+
+enum GateType {


Should be able to remove GateType enum since it is replaced by gate_value oneof field.

Removed. This was for side-by-side performance testing and I forgot to remove it.

maffoo · 2021-07-20T18:42:50Z

cirq-google/cirq_google/api/v2/program.proto


  // Map from the argument name to the Argument needed to fully specify
  // the gate.
  map<string, Arg> args = 2;
+  repeated Arg arg_list = 7;


Can also remove arg_list field since gate_value submessages have their own gate-specific args.

maffoo · 2021-07-20T18:48:36Z

cirq-google/cirq_google/arg_func_langs.py

+    msg = v2.program_pb2.FloatArg() if out is None else out
+
+    if isinstance(
+        value, (float, int, sympy.Integer, sympy.Float, sympy.Rational, sympy.NumberSymbol)


Might want to put this type tuple into a module-level constant since it is also used on line 147 below.

maffoo · 2021-07-20T18:50:09Z

cirq-google/cirq_google/arg_func_langs.py

+    which = arg_proto.WhichOneof('arg')
+    if which == 'float_value':
+        result = float(arg_proto.float_value)
+        if math.ceil(result) == math.floor(result):


Could do if math.round(result) == result:

maffoo · 2021-07-20T18:57:40Z

cirq-google/cirq_google/arg_func_langs.py

+    if required_arg_name is not None:
+        raise ValueError(
+            f'{required_arg_name} is missing or has an unrecognized '
+            f'argument type (WhichOneof("arg")={which!r}).'


I'd suggest separating these two cases to give more specific error messages:

if which == 'float_value': ... elif which == 'symbol': ... elif which == 'func': ... elif which is None: if required_arg_name is not None: raise ValueError(f'{required_arg_name} is missing') return None else: raise ValueError(f'unrecognized argument type: WhichOneof("arg")={which!r}')

Good idea, done.

maffoo · 2021-07-21T17:24:30Z

cirq-google/cirq_google/circuit_serializer.py

+            arg_function_language: The `arg_function_language` field from
+                `Program.Language`.
+            use_constants: Whether to use constants in serialization. This is
+                required to be True for serializing CircuitOperations.


What would be the point of allowing this to be false? Could we remove this option and always use the constants table for v2.5?

Removed (for now). My main concern was keeping the function signatures similar to SerializableGateSet, since the next change would be to add an interface that both of them follow, so that I can use both as a "gate_set". I think I can get around this (or deprecate the use_constants in the other version? In any case, I will deal with this in the follow-up PR when I unify the interface.

maffoo · 2021-07-21T17:25:52Z

cirq-google/cirq_google/circuit_serializer.py

+                raw_constants=raw_constants,
+            )
+            if constants is not None:
+                msg.constants.extend(constants)


Could we instead pass msg.constants into _serialize_circuit so that the function can add constants to it directly? Perhaps this could be combined with raw_constants into a helper object that deals with the details of storing constants, since constants and raw_constants must both be present or not.

Passed msg.constants into the file now. Decided not to make a helper object given that raw_constants is the helper object now, and, with all the refactoring, everything that uses these is now private.

maffoo · 2021-07-21T17:31:56Z

cirq-google/cirq_google/circuit_serializer.py

+                return self.serialize_circuit_op(op, msg, **kwargs)
+            raise ValueError(f'Operation is of an unrecognized type: {op!r}')
+
+        if isinstance(msg, v2.program_pb2.Operation):


It feels a bit awkward to have to check at runtime to match types between the proto message and the operation itself. I'd suggest having _serialize_circuit call serialize_gate_op or serialize_circuit_op directly since it already checks whether or not a given op is a CircuitOperation. I suppose this method could stay if you want it to be available to call directly (e.g. for tests), but I think it'd probably be fine to remove.

Also, I'd suggest moving _serialize_circuit up to be immediately after the serialize method; it's a bit hard to follow the flow when that internal method is located down after all the deserialization code.

maffoo · 2021-07-21T17:40:47Z

cirq-google/cirq_google/circuit_serializer.py

+                arg_function_language=arg_function_language,
+            )
+            return msg
+        raise ValueError(f'Cannot serialize op {op!r} of type {type(gate)}')


I would suggest changing this to a chain of elif with a final else that raises the exception, then having a single return at the end. Could also delay adding qubits and tags until after this check of gate types, since it doesn't make sense to do either of those if the gate type itself is not supported.

maffoo · 2021-07-21T17:45:21Z

cirq-google/cirq_google/circuit_serializer.py

+        Returns:
+            The cirq.google.api.v2.CircuitOperation proto.
+        """
+        circuit = getattr(op.untagged, 'circuit', None)


Should fail if circuit is None, so could remove the default here. But I would suggest instead doing an explicit type check:

if not isinstance(op.untagged, cirq.CircuitOperation): raise ValueError(...) circuit = op.untagged.circuit

This should also make the cast below unnecessary since we've already checked the type of op.untagged.

maffoo

A couple of nits, but otherwise LGTM!

maffoo · 2021-07-26T14:33:00Z

cirq-google/cirq_google/api/v2/program.proto

+
+// Representation of cirq.YPowGate
+message YPowGate {
+  FloatArg exponent=1;


nit: add whitespace around = for consistency, in several places in this file.

maffoo · 2021-07-26T14:34:54Z

cirq-google/cirq_google/arg_func_langs.py


 SUPPORTED_SYMPY_OPS = (sympy.Symbol, sympy.Add, sympy.Mul, sympy.Pow)

 # Argument types for gates.
 ARG_LIKE = Union[int, float, List[bool], str, sympy.Symbol, sympy.Add, sympy.Mul]
+FLOAT_ARG_LIKE = Union[float, sympy.Symbol, sympy.Add, sympy.Mul]
+FLOAT_LIKE = (float, int, sympy.Integer, sympy.Float, sympy.Rational, sympy.NumberSymbol)


nit: maybe name this differently to distinguish it from the Unions above, e.g. FLOAT_LIKE_TYPES. Might also be nice to add a comment to indicate why a tuple is needed here, instead of a union.

dstrain115 · 2021-08-03T16:21:41Z

Let me know if there are additional comments. I will be submitting this week.

tonybruguier · 2021-08-26T02:08:56Z

Hello,
Does this PR required a bump the the version of protobuf in requirements.txt? It seems that the file cirq-google/cirq_google/api/v1/program_pb2.py contains code referring to _descriptor._internal_create_key which may be from a newer version?

I'm not certain, but I did some rudimentary tests and it seems that version protobuf==3.17.3 might be compatible.

tonybruguier · 2021-08-26T03:58:12Z

I took the liberty of sending #4473 but I am happy to withdraw if misguided.

It seems PR #4333 used a newer version of the proto syntax, but the requirements have not been updated, so this PR bumps them, As a check, I decided to execute the follow commands: ``` python3 -m pip install protobuf~=3.17.3 python3 -m pip install grpcio-tools~=1.34.1 python3 -m pip install grpcio~=1.34.1 python3 -m pip install mypy-protobuf==1.10 ./dev_tools/build-protos.sh ``` And the generated protos are unchanged, so maybe it's correct?

This PR creates a new class called CircuitSerializer that can serialize Circuits into protocol buffers more efficiently than existing mechanisms. This class is similar to SerializableGateSet and op_(de)serializer, in that it serializes and deserializes cirq.Circuits into Program protocol buffers, but it is about 60% more space efficient and has significant time savings as well. This serializer works by changing the Gate field and Args dict to a specific message for each allowed gate type. It changes Qubits from a repeated Qubit message, to an index in the constant message. This PR also adds a FloatArg message which is specific to Arguments where it is known the value will be a float (or a sympy expression).

…4473) It seems PR quantumlib#4333 used a newer version of the proto syntax, but the requirements have not been updated, so this PR bumps them, As a check, I decided to execute the follow commands: ``` python3 -m pip install protobuf~=3.17.3 python3 -m pip install grpcio-tools~=1.34.1 python3 -m pip install grpcio~=1.34.1 python3 -m pip install mypy-protobuf==1.10 ./dev_tools/build-protos.sh ``` And the generated protos are unchanged, so maybe it's correct?

dstrain115 added 5 commits June 25, 2021 04:34

Initial draft of proto serializer v25

4470889

proto serialization v2.5 both techniques.

9cdb5b8

Add other proto serialization technique

1e22fba

Pass all checks.

2051db6

Add docstrings.

c43ff1d

dstrain115 requested review from cduck, vtomole, wcourtney and a team as code owners July 19, 2021 17:49

dstrain115 requested a review from MichaelBroughton July 19, 2021 17:49

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

dstrain115 added 2 commits July 19, 2021 11:07

Reorder global var

fdfaba6

formatting

714240e

MichaelBroughton requested changes Jul 20, 2021

View reviewed changes

maffoo reviewed Jul 21, 2021

View reviewed changes

dstrain115 added 3 commits July 22, 2021 11:53

Address review comments.

8f49d4a

Add tests for symbols

d7988fd

Fix docstring.

00d8641

balopat assigned maffoo Jul 23, 2021

dstrain115 added 5 commits July 23, 2021 13:35

Address most review comments.

255b5a7

Got rid of a function and some of the internal private function refs

d272a70

Add test circuit operation with symbols.

2320ab2

PAss msg.constants into the functions.

14e8fae

remove unneeded constants

197e487

dstrain115 requested review from maffoo and MichaelBroughton July 24, 2021 13:05

Add a few last proto comments and the deprecation directives.

2a8a927

maffoo approved these changes Jul 26, 2021

View reviewed changes

Fixed formatting and naming of FLOAT_TYPES

be7a83c

Merge branch 'master' into proto_serialization_v25

1450aef

MichaelBroughton approved these changes Aug 4, 2021

View reviewed changes

MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Aug 4, 2021

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

Merge branch 'master' into proto_serialization_v25

b77f943

CirqBot merged commit b1dc973 into quantumlib:master Aug 4, 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 Aug 4, 2021

tonybruguier mentioned this pull request Aug 26, 2021

Bump the proto requirements to match the generated data. #4473

Merged

		else:
		raise NotImplementedError(f'Unrecognized program type: {type(program)}')

		)


		OPERATIONS = [

Proto serialization v25 #4333

Proto serialization v25 #4333

Uh oh!

Conversation

dstrain115 commented Jul 19, 2021

Uh oh!

MichaelBroughton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maffoo left a comment